threads / patch / 21566

patchDefine $PERL_PATH in test-lib.sh

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

## tl;dr

18 messages between Nov 10, 2009 and Nov 17, 2009. Diffs are folded; open one to read it.

replies: 17people: 5as markdown or json

Philippe Bruhat (BooK)· Nov 10, 2009, 10:46 UTC · lore

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) <book@cpan.org>
---
 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(-)
Show changes to 4 files +6 −4

t/t9400-git-cvsserver-server.sh, t/t9401-git-cvsserver-crlf.sh, t/t9700-perl-git.sh, t/test-lib.sh

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
Jeff King· Nov 10, 2009, 12:23 UTC · re: Philippe Bruhat (BooK) · lore

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

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.

Show 11 quoted lines
> --- 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

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?
Show changes to Makefile +1 −0
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)· Nov 10, 2009, 13:33 UTC · re: Jeff King · lore

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

On Tue, Nov 10, 2009 at 07:23:15AM -0500, Jeff King wrote:
Show 6 quoted lines
> 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)

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.
Show 5 quoted lines
> 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))
Junio C Hamano· Nov 15, 2009, 09:12 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 7 quoted lines
> 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"?

Show 12 quoted lines
> 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)· Nov 16, 2009, 23:48 UTC · re: Junio C Hamano · lore

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

On Sun, Nov 15, 2009 at 01:12:37AM -0800, Junio C Hamano wrote:
Show 13 quoted lines
> Jeff King <peff@peff.net> 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"?
>

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))
Philippe Bruhat (BooK)· Nov 16, 2009, 23:53 UTC · re: Philippe Bruhat (BooK) · lore

[PATCH] Make sure $PERL_PATH is defined when the test suite is run.

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 <peff@peff.net>.
Signed-off-by: Philippe Bruhat (BooK) <book@cpan.org>
---
 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(-)
Show changes to 4 files +5 −4

Makefile, t/t9400-git-cvsserver-server.sh, t/t9401-git-cvsserver-crlf.sh, t/t9700-perl-git.sh

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
Junio C Hamano· Nov 17, 2009, 00:10 UTC · re: Philippe Bruhat (BooK) · lore

Re: [PATCH] Make sure $PERL_PATH is defined when the test suite is run.

"Philippe Bruhat (BooK)" <book@cpan.org> writes:
Show 13 quoted lines
> 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
>  }
Shouldn't these $PERL_PATH all be quoted inside double-quotes?
Philippe Bruhat (BooK)· Nov 17, 2009, 00:17 UTC · re: Junio C Hamano · lore

Re: [PATCH] Make sure $PERL_PATH is defined when the test suite is run.

On Mon, Nov 16, 2009 at 04:10:13PM -0800, Junio C Hamano wrote:
Show 17 quoted lines
> "Philippe Bruhat (BooK)" <book@cpan.org> writes:
> 
> > 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
> >  }
> 
> Shouldn't these $PERL_PATH all be quoted inside double-quotes?

I have no idea. I assume it's to protect against paths with a space in them, so yes, probably.

Amending my patch and sending again.
-- 
 Philippe Bruhat (BooK)

 When you double-cross a friend, you triple-cross yourself.
                                     (Moral from Groo The Wanderer #8 (Epic))
Philippe Bruhat (BooK)· Nov 17, 2009, 00:20 UTC · re: Junio C Hamano · lore

[PATCH] Make sure $PERL_PATH is defined when the test suite is run.

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 <peff@peff.net>.
Signed-off-by: Philippe Bruhat (BooK) <book@cpan.org>
---
 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(-)
Show changes to 4 files +5 −4

Makefile, t/t9400-git-cvsserver-server.sh, t/t9401-git-cvsserver-crlf.sh, t/t9700-perl-git.sh

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..c2ec3cb 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..40637d6 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..96a2e55 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
Johannes Sixt· Nov 17, 2009, 08:30 UTC · re: Philippe Bruhat (BooK) · lore

Re: [PATCH] Make sure $PERL_PATH is defined when the test suite is run.

Philippe Bruhat (BooK) schrieb:
Show 7 quoted lines
> --- 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))'\' >$@
Make it: ... >>$@
>  	@echo TAR=\''$(subst ','\'',$(subst ','\'',$(TAR)))'\' >>$@
>  	@echo NO_CURL=\''$(subst ','\'',$(subst ','\'',$(NO_CURL)))'\' >>$@
>  	@echo NO_PERL=\''$(subst ','\'',$(subst ','\'',$(NO_PERL)))'\' >>$@
>  test_external_without_stderr \
>      'Perl API' \
> -    perl "$TEST_DIRECTORY"/t9700/test.pl
> +    $PERL_PATH "$TEST_DIRECTORY"/t9700/test.pl
This one needs the double-quotes as well.
-- Hannes
Philippe Bruhat (BooK)· Nov 17, 2009, 08:35 UTC · re: Johannes Sixt · lore

Re: [PATCH] Make sure $PERL_PATH is defined when the test suite is run.

On Tue, Nov 17, 2009 at 09:30:17AM +0100, Johannes Sixt wrote:
Show 10 quoted lines
> Philippe Bruhat (BooK) schrieb:
> > --- 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))'\' >$@
> 
> Make it: ... >>$@
This proves late commits needs many extra pair of eyes. :-)
Show 10 quoted lines
> >  	@echo TAR=\''$(subst ','\'',$(subst ','\'',$(TAR)))'\' >>$@
> >  	@echo NO_CURL=\''$(subst ','\'',$(subst ','\'',$(NO_CURL)))'\' >>$@
> >  	@echo NO_PERL=\''$(subst ','\'',$(subst ','\'',$(NO_PERL)))'\' >>$@
> 
> >  test_external_without_stderr \
> >      'Perl API' \
> > -    perl "$TEST_DIRECTORY"/t9700/test.pl
> > +    $PERL_PATH "$TEST_DIRECTORY"/t9700/test.pl
> 
> This one needs the double-quotes as well.
Thanks. Sending again. (sorry for the noise)
-- 
 Philippe Bruhat (BooK)

 "Did I err?"      (Groo, in too many issues to count - ...and *YES* he did!)
Junio C Hamano· Nov 17, 2009, 18:25 UTC · re: Philippe Bruhat (BooK) · lore

Re: [PATCH] Make sure $PERL_PATH is defined when the test suite is run.

"Philippe Bruhat (BooK)" <book@cpan.org> writes:
Show 7 quoted lines
>> Make it: ... >>$@
>
> This proves late commits needs many extra pair of eyes. :-)
> ...
>> This one needs the double-quotes as well.
>
> Thanks. Sending again. (sorry for the noise)
Thanks, I also missed them. Will re-queue.
Philippe Bruhat (BooK)· Nov 17, 2009, 08:42 UTC · re: Johannes Sixt · lore

[PATCH] Make sure $PERL_PATH is defined when the test suite is run.

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 <peff@peff.net>.
Signed-off-by: Philippe Bruhat (BooK) <book@cpan.org>
---
 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(-)
Show changes to 4 files +5 −4

Makefile, t/t9400-git-cvsserver-server.sh, t/t9401-git-cvsserver-crlf.sh, t/t9700-perl-git.sh

diff --git a/Makefile b/Makefile
index 35f5294..287c7fc 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..c2ec3cb 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..40637d6 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..8686086 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
Johannes Sixt· Nov 10, 2009, 12:26 UTC · re: Philippe Bruhat (BooK) · lore

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

Philippe Bruhat (BooK) schrieb:
Show 10 quoted lines
> 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 || {

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

Show 9 quoted lines
> 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
Wouldn't
   ... && export PERL_PATH=perl
be a safer fall-back?
-- Hannes
Philippe Bruhat (BooK)· Nov 10, 2009, 13:34 UTC · re: Johannes Sixt · lore

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

On Tue, Nov 10, 2009 at 01:26:53PM +0100, Johannes Sixt wrote:
Show 8 quoted lines
> >  
> > +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))
Junio C Hamano· Nov 10, 2009, 20:17 UTC · re: Philippe Bruhat (BooK) · lore

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

"Philippe Bruhat (BooK)" <book@cpan.org> writes:
Show 12 quoted lines
> On Tue, Nov 10, 2009 at 01:26:53PM +0100, Johannes Sixt wrote:
>> >  
>> > +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.

Philippe Bruhat (BooK)· Nov 11, 2009, 08:40 UTC · re: Junio C Hamano · lore

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

On Tue, Nov 10, 2009 at 12:17:26PM -0800, Junio C Hamano wrote:
Show 19 quoted lines
> "Philippe Bruhat (BooK)" <book@cpan.org> writes:
> 
> > On Tue, Nov 10, 2009 at 01:26:53PM +0100, Johannes Sixt wrote:
> >> >  
> >> > +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))
Jeff King· Nov 11, 2009, 08:43 UTC · re: Philippe Bruhat (BooK) · lore

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

On Wed, Nov 11, 2009 at 09:40:14AM +0100, Philippe Bruhat (BooK) wrote:
Show 9 quoted lines
> > 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

← back to recent threads