[PATCH] Define $PERL_PATH in test-lib.sh

The main Makefile defines PERL_PATH as the perl to use in the shebang
line of git*.perl commands. This ensures this will be the perl used
to run the tests (in case another perl appears in $PATH before the one
defined in $PERL_PATH)

Signed-off-by: Philippe Bruhat (BooK) <redacted>
---
 t/t9400-git-cvsserver-server.sh |    2 +-
 t/t9401-git-cvsserver-crlf.sh   |    2 +-
 t/t9700-perl-git.sh             |    4 ++--
 t/test-lib.sh                   |    2 ++
 4 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/t/t9400-git-cvsserver-server.sh b/t/t9400-git-cvsserver-server.sh
index 64f947d..dc710f8 100755
--- a/t/t9400-git-cvsserver-server.sh
+++ b/t/t9400-git-cvsserver-server.sh

@@ -20,7 +20,7 @@ then
     say 'skipping git-cvsserver tests, cvs not found'
     test_done
 fi
-perl -e 'use DBI; use DBD::SQLite' >/dev/null 2>&1 || {
+$PERL_PATH -e 'use DBI; use DBD::SQLite' >/dev/null 2>&1 || {
     say 'skipping git-cvsserver tests, Perl SQLite interface unavailable'
     test_done
 }

diff --git a/t/t9401-git-cvsserver-crlf.sh b/t/t9401-git-cvsserver-crlf.sh
index aca40c1..c9e3dba 100755
--- a/t/t9401-git-cvsserver-crlf.sh
+++ b/t/t9401-git-cvsserver-crlf.sh

@@ -57,7 +57,7 @@ then
     say 'skipping git-cvsserver tests, perl not available'
     test_done
 fi
-perl -e 'use DBI; use DBD::SQLite' >/dev/null 2>&1 || {
+$PERL_PATH -e 'use DBI; use DBD::SQLite' >/dev/null 2>&1 || {
     say 'skipping git-cvsserver tests, Perl SQLite interface unavailable'
     test_done
 }

diff --git a/t/t9700-perl-git.sh b/t/t9700-perl-git.sh
index 4eb7d3f..3354e18 100755
--- a/t/t9700-perl-git.sh
+++ b/t/t9700-perl-git.sh

@@ -11,7 +11,7 @@ if ! test_have_prereq PERL; then
 	test_done
 fi
 
-perl -MTest::More -e 0 2>/dev/null || {
+$PERL_PATH -MTest::More -e 0 2>/dev/null || {
 	say "Perl Test::More unavailable, skipping test"
 	test_done
 }

@@ -48,6 +48,6 @@ test_expect_success \
 
 test_external_without_stderr \
     'Perl API' \
-    perl "$TEST_DIRECTORY"/t9700/test.pl
+    $PERL_PATH "$TEST_DIRECTORY"/t9700/test.pl
 
 test_done

diff --git a/t/test-lib.sh b/t/test-lib.sh
index f2ca536..54dd4d5 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh

@@ -730,6 +730,8 @@ esac
 
 test -z "$NO_PERL" && test_set_prereq PERL
 
+test -z "$NO_PERL" && test -z "$PERL_PATH" && export PERL_PATH=/usr/bin/perl
+
 # test whether the filesystem supports symbolic links
 ln -s x y 2>/dev/null && test -h y 2>/dev/null && test_set_prereq SYMLINKS
 rm -f y

-- 
1.6.0.3.517.g759a

On Tue, Nov 10, 2009 at 11:46:51AM +0100, Philippe Bruhat (BooK) wrote:

The main Makefile defines PERL_PATH as the perl to use in the shebang
line of git*.perl commands. This ensures this will be the perl used
to run the tests (in case another perl appears in $PATH before the one
defined in $PERL_PATH)

I think this "the perl used to run the tests" needs to be clarified in
the commit message.  There are really three ways we use perl in the
tests:

  1. To run to the git-* scripts themselves.

  2. To run a test snippet of perl as if we were a git-* script.

  3. To run random perl helper functions.

We already use PERL_PATH for (1). I don't think there is much point in
worrying about (3). If the perl in your PATH is so broken that it can't
be used for simple helpers, then you should fix your PATH.

Your patch seems to just fix (2), which I think is sane. But I wanted to
note it, because when I read your commit message, I wasn't sure which
you were doing.

Will this work if I just have PERL_PATH in my config.mak in the root
directory? Should we be adding PERL_PATH to the generated
GIT-BUILD-OPTIONS file in the root, which gets sourced by test-lib?

Something like the following (completely untested) patch?

diff --git a/Makefile b/Makefile
index a10a60c..b9a8145 100644
--- a/Makefile
+++ b/Makefile

@@ -1643,6 +1643,7 @@ GIT-CFLAGS: .FORCE-GIT-CFLAGS
 # and the first level quoting from the shell that runs "echo".
 GIT-BUILD-OPTIONS: .FORCE-GIT-BUILD-OPTIONS
 	@echo SHELL_PATH=\''$(subst ','\'',$(SHELL_PATH_SQ))'\' >$@
+	@echo PERL_PATH=\''$(subst ','\'',$(PERL_PATH_SQ))'\' >$@
 	@echo TAR=\''$(subst ','\'',$(subst ','\'',$(TAR)))'\' >>$@
 	@echo NO_CURL=\''$(subst ','\'',$(subst ','\'',$(NO_CURL)))'\' >>$@
 	@echo NO_PERL=\''$(subst ','\'',$(subst ','\'',$(NO_PERL)))'\' >>$@

Philippe Bruhat (BooK) schrieb:

Shouldn't this be "$PERL_PATH", i.e., double-quoted? (Ditto in the other
cases that you replaced.)

Wouldn't

   ... && export PERL_PATH=perl

be a safer fall-back?

-- Hannes

On Tue, Nov 10, 2009 at 07:23:15AM -0500, Jeff King wrote:

On Tue, Nov 10, 2009 at 11:46:51AM +0100, Philippe Bruhat (BooK) wrote:

quoted

The main Makefile defines PERL_PATH as the perl to use in the shebang
line of git*.perl commands. This ensures this will be the perl used
to run the tests (in case another perl appears in $PATH before the one
defined in $PERL_PATH)

PERL_PATH is a variable in the Makefile that, if not defined is set up
to /usr/bin/perl.

It is used to set the shebang line in the git-* perl scripts.

I think this "the perl used to run the tests" needs to be clarified in
the commit message.  There are really three ways we use perl in the
tests:

  1. To run to the git-* scripts themselves.

Yes, this PERL_PATH.

  2. To run a test snippet of perl as if we were a git-* script.

Actually, my goal was to run Makefile.PL with the proper perl (see below).

  3. To run random perl helper functions.

I didn't replace those ones, because any perl is good enough to do some
perl -i.bak -pe 's/foo/bar/' processing.

We already use PERL_PATH for (1). I don't think there is much point in
worrying about (3). If the perl in your PATH is so broken that it can't
be used for simple helpers, then you should fix your PATH.

The Perl in my PATH doesn't have Error.pm installed, but /usr/bin/perl
has it. When the Makefile.PL is run by /usr/bin/perl, the private-Error.pm
module is not copied in blib. The git-* perl scripts are using the perl
in PERL_PATH, and therefore can't load Error.pm (not in blib during make test),
causing test breakage.

This is the original reason for this patch. I fixed the general (2) case,
deliberately avoiding to replace every call to perl (3).

Your patch seems to just fix (2), which I think is sane. But I wanted to
note it, because when I read your commit message, I wasn't sure which
you were doing.

Yes.

-- 
 Philippe Bruhat (BooK)

 Trust only in incompetence. You will never be disappointed.
                                    (Moral from Groo The Wanderer #16 (Epic))

On Tue, Nov 10, 2009 at 01:26:53PM +0100, Johannes Sixt wrote:

quoted

 
+test -z "$NO_PERL" && test -z "$PERL_PATH" && export PERL_PATH=/usr/bin/perl

Wouldn't

   ... && export PERL_PATH=perl

be a safer fall-back?

/usr/bin/perl is the value used in the top-level Makefile.
I used this for consistency.

-- 
 Philippe Bruhat (BooK)

 The greatest monster of them all is ignorance.
                             (Moral to Pal'n Drumm Story in Groo #89 (Epic))

"Philippe Bruhat (BooK)" [off-list ref] writes:

On Tue, Nov 10, 2009 at 01:26:53PM +0100, Johannes Sixt wrote:

quoted

quoted

 
+test -z "$NO_PERL" && test -z "$PERL_PATH" && export PERL_PATH=/usr/bin/perl

Wouldn't

   ... && export PERL_PATH=perl

be a safer fall-back?

/usr/bin/perl is the value used in the top-level Makefile.
I used this for consistency.

Hmm, but that means two separate definitions in ./Makefile and
t/test-lib.sh must be kept in sync forever, and there is not even a
comment next to the line that requires such care in your patch to help
people who might want to change these lines in the future.

On Tue, Nov 10, 2009 at 12:17:26PM -0800, Junio C Hamano wrote:

"Philippe Bruhat (BooK)" [off-list ref] writes:

quoted

On Tue, Nov 10, 2009 at 01:26:53PM +0100, Johannes Sixt wrote:

quoted

quoted

 
+test -z "$NO_PERL" && test -z "$PERL_PATH" && export PERL_PATH=/usr/bin/perl

Wouldn't

   ... && export PERL_PATH=perl

be a safer fall-back?

/usr/bin/perl is the value used in the top-level Makefile.
I used this for consistency.

Hmm, but that means two separate definitions in ./Makefile and
t/test-lib.sh must be kept in sync forever, and there is not even a
comment next to the line that requires such care in your patch to help
people who might want to change these lines in the future.

Is there a way to obtain whatever value was computed in the Makefile,
or should I just add a comment in all-caps saying "keep this in sync
with the default value in the top level Makefile"? (and a more detailed
commit message)

-- 
 Philippe Bruhat (BooK)

 Freedom is not an individual effort. Yours comes only when you grant others
 theirs.                             (Moral from Groo The Wanderer #5 (Epic))

On Wed, Nov 11, 2009 at 09:40:14AM +0100, Philippe Bruhat (BooK) wrote:

quoted

Hmm, but that means two separate definitions in ./Makefile and
t/test-lib.sh must be kept in sync forever, and there is not even a
comment next to the line that requires such care in your patch to help
people who might want to change these lines in the future.

Is there a way to obtain whatever value was computed in the Makefile,
or should I just add a comment in all-caps saying "keep this in sync
with the default value in the top level Makefile"? (and a more detailed
commit message)

Yes. Did you miss the second half of my other message?

  http://article.gmane.org/gmane.comp.version-control.git/132561

-Peff

Jeff King [off-list ref] writes:

On Tue, Nov 10, 2009 at 11:46:51AM +0100, Philippe Bruhat (BooK) wrote:
(snip)
Will this work if I just have PERL_PATH in my config.mak in the root
directory? Should we be adding PERL_PATH to the generated
GIT-BUILD-OPTIONS file in the root, which gets sourced by test-lib?

Something like the following (completely untested) patch?

Philippe, could you please help getting this topic unstuck with a "it
works" or "it doesn't and here is a better solution"?

On Sun, Nov 15, 2009 at 01:12:37AM -0800, Junio C Hamano wrote:

Jeff King [off-list ref] writes:

quoted

On Tue, Nov 10, 2009 at 11:46:51AM +0100, Philippe Bruhat (BooK) wrote:
(snip)
Will this work if I just have PERL_PATH in my config.mak in the root
directory? Should we be adding PERL_PATH to the generated
GIT-BUILD-OPTIONS file in the root, which gets sourced by test-lib?

Something like the following (completely untested) patch?

Philippe, could you please help getting this topic unstuck with a "it
works" or "it doesn't and here is a better solution"?

I took Jeff's patch the main Makefile, removed my patch to test-lib.sh,
and it worked. That is to say, the test suite failed on the perl tests
when the first perl in the PATH was my local perl without Error.pm
installed. With the changes, the test suite passed, even with my local
perl first in the PATH.

Patch with a reworked commit message follows.

-- 
 Philippe Bruhat (BooK)

 The truly stupid always find a way to create disaster.
                                                (Moral from Groo #10 (Image))

Some test scripts run Perl scripts as if they were git-* scripts, and
thus need to use the same perl that will be put in the shebang line of
git*.perl commands. $PERL_PATH therefore needs to be used instead of
a bare "perl".

The tests can fail if another perl is found in $PATH before the one
defined in $PERL_PATH.

Example test failure caused by this: the perl defined in $PERL_PATH has
Error.pm installed, and therefore the Git.pm's Makefile.PL doesn't install
the private copy. The perl from $PATH doesn't have Error.pm installed, and
all git*.perl scripts invoked during the test will fail loading Error.pm.

Makefile patch by Jeff King [off-list ref].

Signed-off-by: Philippe Bruhat (BooK) <redacted>
---
 Makefile                        |    1 +
 t/t9400-git-cvsserver-server.sh |    2 +-
 t/t9401-git-cvsserver-crlf.sh   |    2 +-
 t/t9700-perl-git.sh             |    4 ++--
 4 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/Makefile b/Makefile
index 35f5294..8e8f981 100644
--- a/Makefile
+++ b/Makefile

@@ -1633,6 +1633,7 @@ GIT-CFLAGS: .FORCE-GIT-CFLAGS
 # and the first level quoting from the shell that runs "echo".
 GIT-BUILD-OPTIONS: .FORCE-GIT-BUILD-OPTIONS
 	@echo SHELL_PATH=\''$(subst ','\'',$(SHELL_PATH_SQ))'\' >$@
+	@echo PERL_PATH=\''$(subst ','\'',$(PERL_PATH_SQ))'\' >$@
 	@echo TAR=\''$(subst ','\'',$(subst ','\'',$(TAR)))'\' >>$@
 	@echo NO_CURL=\''$(subst ','\'',$(subst ','\'',$(NO_CURL)))'\' >>$@
 	@echo NO_PERL=\''$(subst ','\'',$(subst ','\'',$(NO_PERL)))'\' >>$@

diff --git a/t/t9400-git-cvsserver-server.sh b/t/t9400-git-cvsserver-server.sh
index 64f947d..dc710f8 100755
--- a/t/t9400-git-cvsserver-server.sh
+++ b/t/t9400-git-cvsserver-server.sh

@@ -20,7 +20,7 @@ then
     say 'skipping git-cvsserver tests, cvs not found'
     test_done
 fi
-perl -e 'use DBI; use DBD::SQLite' >/dev/null 2>&1 || {
+$PERL_PATH -e 'use DBI; use DBD::SQLite' >/dev/null 2>&1 || {
     say 'skipping git-cvsserver tests, Perl SQLite interface unavailable'
     test_done
 }

diff --git a/t/t9401-git-cvsserver-crlf.sh b/t/t9401-git-cvsserver-crlf.sh
index aca40c1..c9e3dba 100755
--- a/t/t9401-git-cvsserver-crlf.sh
+++ b/t/t9401-git-cvsserver-crlf.sh

@@ -57,7 +57,7 @@ then
     say 'skipping git-cvsserver tests, perl not available'
     test_done
 fi
-perl -e 'use DBI; use DBD::SQLite' >/dev/null 2>&1 || {
+$PERL_PATH -e 'use DBI; use DBD::SQLite' >/dev/null 2>&1 || {
     say 'skipping git-cvsserver tests, Perl SQLite interface unavailable'
     test_done
 }

diff --git a/t/t9700-perl-git.sh b/t/t9700-perl-git.sh
index 4eb7d3f..3354e18 100755
--- a/t/t9700-perl-git.sh
+++ b/t/t9700-perl-git.sh

@@ -11,7 +11,7 @@ if ! test_have_prereq PERL; then
 	test_done
 fi
 
-perl -MTest::More -e 0 2>/dev/null || {
+$PERL_PATH -MTest::More -e 0 2>/dev/null || {
 	say "Perl Test::More unavailable, skipping test"
 	test_done
 }

@@ -48,6 +48,6 @@ test_expect_success \
 
 test_external_without_stderr \
     'Perl API' \
-    perl "$TEST_DIRECTORY"/t9700/test.pl
+    $PERL_PATH "$TEST_DIRECTORY"/t9700/test.pl
 
 test_done

-- 
1.6.0.3.517.g759a