[PATCH 0/3] running tests as fakeroot

[PATCH 0/3] running tests as fakeroot

[Jonathan Nieder]

Hi,

The Debian build system makes it very tempting to build as
fakeroot: basically, one has a choice of

  debian/rules build
  fakeroot debian/rules binary

or just

  fakeroot debian/rules binary

to build a .deb from the current sources.  I hadn’t succumbed
to temptation until today; but now that I have, I want to fix it.
I’ll discuss whether this is a good idea more in patch 3.

Patches 1 and 2 are a bit simpler.  Patch 1 allows tests to have
multiple prerequisites, which seems like a good idea generally;
patch 2 adds a missing POSIXPERM prerequisite to an existing test.

I hope you like the patches.  Sorry for the huge cc list; I did a
quick search to find people interested one way or another, and
aparently there are many.  I am sending patches 1 and 2 only to the
git list to avoid making the problem worse.

Thoughts?
Jonathan Nieder (3):
  test-lib: tests can have multiple prerequisites
  t1004 (read-tree): the unremovable symlink test requires POSIXPERM
  Permit tests to be run as a (fake) root user

 t/t1004-read-tree-m-u-wf.sh |    2 +-
 t/test-lib.sh               |   22 ++++++++++++++--------
 2 files changed, 15 insertions(+), 9 deletions(-)

[PATCH 1/3] test-lib: tests can have multiple prerequisites

[Jonathan Nieder]

Treat the prereq argument to test_expect_success and
test_expect_failure as a space-separated list of prerequisites.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Alas, I didn’t notice the thread with the almost identical patch [1]
before writing this one.

Since I am painting it, I prefer this way.  I find it intuitive and
can’t really see where the fuss about using some other coding style
came from.  So I would be very happy if someone who does understand
the fuss could come up with a comment to add to the commit message
about this. :)

 t/test-lib.sh |   17 ++++++++++-------
 1 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index c582964..a53b6cf 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -315,12 +315,15 @@ test_set_prereq () {
 satisfied=" "
 
 test_have_prereq () {
-	case $satisfied in
-	*" $1 "*)
-		: yes, have it ;;
-	*)
-		! : nope ;;
-	esac
+	for prereq_to_test
+	do
+		case $satisfied in
+		*" $prereq_to_test "*)
+			: yes, have it ;;
+		*)
+			return 1 ;;
+		esac
+	done
 }
 
 # You are not expected to call test_ok_ and test_failure_ directly, use
@@ -370,7 +373,7 @@ test_skip () {
 		esac
 	done
 	if test -z "$to_skip" && test -n "$prereq" &&
-	   ! test_have_prereq "$prereq"
+	   ! test_have_prereq $prereq
 	then
 		to_skip=t
 	fi
-- 
1.7.1.rc1

Re: [PATCH 1/3] test-lib: tests can have multiple prerequisites

[Jonathan Nieder]

Jonathan Nieder wrote:

> Treat the prereq argument to test_expect_success and
> test_expect_failure as a space-separated list of prerequisites.
> 
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
> Alas, I didn’t notice the thread with the almost identical patch [1]
> before writing this one.
> 
> Since I am painting it, I prefer this way.  I find it intuitive and
> can’t really see where the fuss about using some other coding style
> came from.

Whoops, forgot to link.  Maybe this would be a good entry point:

[1] http://thread.gmane.org/gmane.comp.version-control.git/116729/focus=118595

Sorry about that.
Jonathan

Re: [PATCH 1/3] test-lib: tests can have multiple prerequisites

[Jeff King]

On Wed, Apr 21, 2010 at 08:38:06AM -0500, Jonathan Nieder wrote:

> Alas, I didn’t notice the thread with the almost identical patch [1]
> before writing this one.
> 
> Since I am painting it, I prefer this way.  I find it intuitive and
> can’t really see where the fuss about using some other coding style
> came from.  So I would be very happy if someone who does understand
> the fuss could come up with a comment to add to the commit message
> about this. :)

I liked commas better, but since you are doing the work of writing the
patches, I will let it go. :)

I do wonder if it might be less error-prone to have:

  test_expect_success PREREQ1 PREREQ2 'desc' 'test'

instead of

  test_expect_success 'PREREQ1 PREREQ2' 'desc' 'test'

which should just be something like

  while test $# -gt 2; do
    test_have_prereq "$1" || skip=t
    shift
  done

But if you think that's dumb, just ignore it. It is really not worth
spending that much time on.

-Peff

Re: [PATCH 1/3] test-lib: tests can have multiple prerequisites

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I do wonder if it might be less error-prone to have:
>
>   test_expect_success PREREQ1 PREREQ2 'desc' 'test'
>
> instead of
>
>   test_expect_success 'PREREQ1 PREREQ2' 'desc' 'test'

Spot what is wrong with this:

        test_expect_success make sure we have the repo '
                test -d .git
        '

I think "we may or may not have a single prereq word" would be less error
prone.  (you could make sure "test_have_prereq" gets no lowercase letter
to mitigate the issue, though).

I tend to like the comma myself, but quoted, space separated list is fine
by me.

Re: [PATCH 1/3] test-lib: tests can have multiple prerequisites

[Jeff King]

On Mon, Apr 26, 2010 at 05:06:55PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > I do wonder if it might be less error-prone to have:
> >
> >   test_expect_success PREREQ1 PREREQ2 'desc' 'test'
> >
> > instead of
> >
> >   test_expect_success 'PREREQ1 PREREQ2' 'desc' 'test'
> 
> Spot what is wrong with this:
> 
>         test_expect_success make sure we have the repo '
>                 test -d .git
>         '

To me it's easy to spot, but there is nothing wrong with it that the
shell will spot, so that is a downside. Whatever we go with, it would
perhaps be less error prone to set "NOPREREQ1" when we don't have
PREREQ1, and check that either $1 or NO$1 is set when checking
prerequisites. But that is probably just over-engineering. This is not
an error we have seen a lot of, and it is not that hard to spot with
code review.

-Peff

[PATCH 2/3] t1004 (read-tree): the unremovable symlink test requires POSIXPERM

[Jonathan Nieder]

Now that it is possible to express this, declare the test from
8a785dc (Add tests to catch problems with un-unlinkable symlinks,
2008-03-18) to require both POSIXPERM and SYMLINKS.

This test would fail on Windows on filesystems that do support
symbolic links and in other situations where symlinks are usable but
Unix-style permissions are not.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
It seems that an almost identical patch was submitted while the
prerequisite mechanism was in its infancy.

http://thread.gmane.org/gmane.comp.version-control.git/116729/focus=118385

 t/t1004-read-tree-m-u-wf.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/t/t1004-read-tree-m-u-wf.sh b/t/t1004-read-tree-m-u-wf.sh
index f19b4a2..62854a7 100755
--- a/t/t1004-read-tree-m-u-wf.sh
+++ b/t/t1004-read-tree-m-u-wf.sh
@@ -177,7 +177,7 @@ test_expect_success SYMLINKS 'funny symlink in work tree' '
 
 '
 
-test_expect_success SYMLINKS 'funny symlink in work tree, un-unlink-able' '
+test_expect_success 'POSIXPERM SYMLINKS' 'funny symlink in work tree, un-unlink-able' '
 
 	rm -fr a b &&
 	git reset --hard &&
-- 
1.7.1.rc1

[PATCH 3/3] Permit tests to be run as a (fake) root user

[Jonathan Nieder]

Skip tests that require sane behavior wrt permissions if the current
uid is 0.

So now if you run

  fakeroot sh -c '
	make all test install install-doc prefix=/usr/local \
		DESTDIR=wherever >packaged-git.log 2>&1 &&
	cd wherever &&
	tar -cf - . |
	gzip
  ' >packaged-git.tar.gz

then this should successfully build a binary tarball for git, running
some tests while at it.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 Goals
 -----
 Before the POSIXPERM mechanism existed (which makes this change too
 easy to avoid considering), I think many people wanted to discourage
 running tests as root to avoid hosing people’s machines.  Which makes
 some sense.
 
 But it is possible and may even be valuable to run tests in a
 sandbox. [1] [8] If that were the only reason to run tests as root, I
 would suggest requiring some particular environment variable to be
 set as a safety.
 
 In Debian, I think it might be policy to allow building as “root”.  In
 this context, that almost always happens through some sort of ptrace or
 LD_PRELOAD hack like fakeroot.  If a user knows to, it is easy to fall
 back to building as an unprivileged user, but really I think a user
 should not have to know to. [2] [3]  If some non-POSIXPERM test fails
 when run as (fake) root, that is something that would be nice to
 know. [4]
 
 Which prerequisite?
 -------------------
 It is possible to use autoconf-style tests to check for each
 capability we would like to lack. [5]  Certainly, POSIXPERM does not
 actually describe the exact set of abilities one gains by running as
 an unprivileged user. [6]  But unsetting POSIXPERM does successfully
 convey a simple truth: if you run tests as root, you have lost the ability
 to pay attention to permissions and keep your sanity. [7]
 
 [1] http://thread.gmane.org/gmane.comp.version-control.git/86885/focus=87664
 [2] http://thread.gmane.org/gmane.comp.version-control.git/140128
 [3] http://thread.gmane.org/gmane.comp.version-control.git/17904/focus=17910
 [4] http://thread.gmane.org/gmane.comp.version-control.git/75561/focus=75568
 [5] http://thread.gmane.org/gmane.comp.version-control.git/18667
 [6] http://thread.gmane.org/gmane.comp.version-control.git/121502/focus=121531
 [7] http://thread.gmane.org/gmane.comp.version-control.git/116729/focus=118385
 [8] http://thread.gmane.org/gmane.comp.version-control.git/52654/focus=52689
     Holy cow, that Solaris bug is still not fixed [9].  Maybe it
     would be worthwhile to make an unlink() wrapper in compat/ to
     avoid new uses leaving users on such operating systems in danger.
 [9] http://bugs.opensolaris.org/view_bug.do?bug_id=6581318

Thanks for reading.  I hope the story was not too dull.

 t/test-lib.sh |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index a53b6cf..0f51757 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -782,7 +782,10 @@ case $(uname -s) in
 	# exec does not inherit the PID
 	;;
 *)
-	test_set_prereq POSIXPERM
+	if test $(id -u) != 0
+	then
+		test_set_prereq POSIXPERM
+	fi
 	test_set_prereq BSLASHPSPEC
 	test_set_prereq EXECKEEPSPID
 	;;
-- 
1.7.1.rc1