patch, 3 partsrunning tests as fakeroot

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(-)

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(-)

Jonathan Nieder wrote:

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

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

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

Jeff King <peff@peff.net> writes:

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.

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

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

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(-)

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(-)