threads / patch / 37998

patcht0090: mark add-interactive test with PERL prerequisite

Subject: [PATCH] t0090: mark add-interactive test with PERL prerequisite

## tl;dr

11 messages between Nov 18, 2014 and Nov 18, 2014. Diffs are folded; open one to read it.

replies: 10people: 3as markdown or json

Jeff King· Nov 18, 2014, 17:22 UTC · lore

The add-interactive system is built in perl. If you build with NO_PERL, running "git commit --interactive" will exit with an error and the test will fail.

Signed-off-by: Jeff King <peff@peff.net>
---
Noticed by Michael while working around gitweb failures by setting
NO_PERL. :)

It didn't reproduce for me in my existing build directory, presumably because I had an old git-add--interactive build product lying around. But running the tests in a clean clone with NO_PERL set reproduces easily.

 t/t0090-cache-tree.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to t/t0090-cache-tree.sh +1 −1
diff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh
index 158cf4f..067f4c6 100755
--- a/t/t0090-cache-tree.sh
+++ b/t/t0090-cache-tree.sh
@@ -131,7 +131,7 @@ test_expect_success 'second commit has cache-tree' '
 	test_cache_tree
 '
 
-test_expect_success 'commit --interactive gives cache-tree on partial commit' '
+test_expect_success PERL 'commit --interactive gives cache-tree on partial commit' '
 	cat <<-\EOT >foo.c &&
 	int foo()
 	{

-- 
2.1.2.596.g7379948
Jeff King· Nov 18, 2014, 17:29 UTC · re: Jeff King · lore

[PATCH] t960[34]: mark cvsimport tests as requiring perl

Git-cvsimport is written in perl, which understandably causes the tests to fail if you build with NO_PERL (which will avoid building cvsimport at all). The earlier cvsimport tests in t9600-t9602 are all marked with a PERL prerequisite, but these ones are not.

The one in t9603 was likely not noticed because it is an expected failure anyway.

The ones in t9604 have been around for a long time, but it is likely that the combination of NO_PERL and having cvsps installed is rare enough that nobody noticed.

Signed-off-by: Jeff King <peff@peff.net>
---
It would probably make sense to have these scripts just
skip_all if NO_PERL is set, but I opted to follow the pattern
set by t9600, etc. If somebody feels like spending time refactoring the
cvsimport test harness, be my guest.
 t/t9603-cvsimport-patchsets.sh  | 2 +-
 t/t9604-cvsimport-timestamps.sh | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)
Show changes to 2 files +3 −3

t/t9603-cvsimport-patchsets.sh, t/t9604-cvsimport-timestamps.sh

diff --git a/t/t9603-cvsimport-patchsets.sh b/t/t9603-cvsimport-patchsets.sh
index 52034c8..c4c3c49 100755
--- a/t/t9603-cvsimport-patchsets.sh
+++ b/t/t9603-cvsimport-patchsets.sh
@@ -16,7 +16,7 @@ test_description='git cvsimport testing for correct patchset estimation'
 
 setup_cvs_test_repository t9603
 
-test_expect_failure 'import with criss cross times on revisions' '
+test_expect_failure PERL 'import with criss cross times on revisions' '
 
     git cvsimport -p"-x" -C module-git module &&
     (cd module-git &&
diff --git a/t/t9604-cvsimport-timestamps.sh b/t/t9604-cvsimport-timestamps.sh
index 1fd5142..a4b3db2 100755
--- a/t/t9604-cvsimport-timestamps.sh
+++ b/t/t9604-cvsimport-timestamps.sh
@@ -5,7 +5,7 @@ test_description='git cvsimport timestamps'
 
 setup_cvs_test_repository t9604
 
-test_expect_success 'check timestamps are UTC (TZ=CST6CDT)' '
+test_expect_success PERL 'check timestamps are UTC (TZ=CST6CDT)' '
 
 	TZ=CST6CDT git cvsimport -p"-x" -C module-1 module &&
 	git cvsimport -p"-x" -C module-1 module &&
@@ -34,7 +34,7 @@ test_expect_success 'check timestamps are UTC (TZ=CST6CDT)' '
 	test_cmp actual-1 expect-1
 '
 
-test_expect_success 'check timestamps with author-specific timezones' '
+test_expect_success PERL 'check timestamps with author-specific timezones' '
 
 	cat >cvs-authors <<-EOF &&
 	user1=User One <user1@domain.org>

-- 
2.1.2.596.g7379948
Jonathan Nieder· Nov 18, 2014, 18:56 UTC · re: Jeff King · lore

Re: [PATCH] t960[34]: mark cvsimport tests as requiring perl

Jeff King wrote:
> It would probably make sense to have these scripts just
> skip_all if NO_PERL is set, but I opted to follow the pattern
> set by t9600, etc. If somebody feels like spending time refactoring the
> cvsimport test harness, be my guest.

Wouldn't it be a matter of the following, plus (optionally) dropping the existing PERL prerequisites on cvs tests?

-- >8 --
Subject: test: cvsimport requires perl

Git-cvsimport is written in perl, which understandably causes the tests to fail if you build with NO_PERL (which will avoid building cvsimport at all). The earlier cvsimport tests in t9600-t9602 are all marked with a PERL prerequisite, but t9603 and 9604 are not.

The one in t9603 was likely not noticed because it is an expected failure.

The ones in t9604 have been around for a long time, but it is likely that the combination of NO_PERL and having cvsps installed is rare enough that nobody noticed.

Reported-by: Jeff King <peff@peff.net>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 t/lib-cvs.sh | 6 ++++++
 1 file changed, 6 insertions(+)
Show changes to t/lib-cvs.sh +6 −0
diff --git a/t/lib-cvs.sh b/t/lib-cvs.sh
index 9b2bcfb..b75df11 100644
--- a/t/lib-cvs.sh
+++ b/t/lib-cvs.sh
@@ -10,6 +10,12 @@ then
 	test_done
 fi
 
+if ! test_have_prereq PERL
+then
+	skip_all='skipping cvsimport tests, perl not available'
+	test_done
+fi
+
 CVS="cvs -f"
 export CVS
 

-- 
2.1.0.rc2.206.gedb03e5
Jeff King· Nov 18, 2014, 19:15 UTC · re: Jonathan Nieder · lore

Re: [PATCH] t960[34]: mark cvsimport tests as requiring perl

On Tue, Nov 18, 2014 at 10:56:22AM -0800, Jonathan Nieder wrote:
Show 11 quoted lines
> Jeff King wrote:
> 
> > It would probably make sense to have these scripts just
> > skip_all if NO_PERL is set, but I opted to follow the pattern
> > set by t9600, etc. If somebody feels like spending time refactoring the
> > cvsimport test harness, be my guest.
> 
> Wouldn't it be a matter of the following, plus (optionally) dropping
> the existing PERL prerequisites on cvs tests?
> [...]
>  t/lib-cvs.sh | 6 ++++++

Yeah, I think so. I was worried that lib-cvs was used by the other CVS tests (like t9200, and t940x), but it seems to be cvsimport-specific. If you do go this route (and that is fine with me), maybe it is worth changing the filename to make that more clear.

-Peff
Jeff King· Nov 18, 2014, 17:43 UTC · re: Jeff King · lore

Re: [PATCH] t0090: mark add-interactive test with PERL prerequisite

On Tue, Nov 18, 2014 at 12:22:31PM -0500, Jeff King wrote:
Show 13 quoted lines
> The add-interactive system is built in perl. If you build
> with NO_PERL, running "git commit --interactive" will exit
> with an error and the test will fail.
> 
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> Noticed by Michael while working around gitweb failures by setting
> NO_PERL. :)
> 
> It didn't reproduce for me in my existing build directory, presumably
> because I had an old git-add--interactive build product lying around.
> But running the tests in a clean clone with NO_PERL set reproduces
> easily.

I don't think fixing this is really a high priority. You can't ever be free of odd interactions with previous build artifacts. After all, you might have a built "git-foo" from a previous version of git (or one from the future, or even an alternate reality from another branch), and our Makefile should not have to know about every previous version you may have built in the path. That is what "git clean" is for (or just using a clean build directory).

But fixing this one in particular is pretty easy (and we _do_ know about the wrongly-built file; our dependencies are just incomplete):

-- >8 --
Subject: Makefile: have perl scripts depend on NO_PERL setting

If NO_PERL is not set, our perl scripts are built as usual. If it is set, then we build "dummy" versions that tell you git was built without perl support and exit gracefully.

However, if you switch to NO_PERL in a directory with existing build artifacts, we do not notice that the files need rebuilt. We see only that they are newer than the "unimplemented.sh" wrapper and assume they are done. So doing:

  make
  make NO_PERL=Nope

would result in a git-add--interactive script that uses perl (and running the test suite would make use of it).

Instead, we should trigger a rebuild of the perl scripts anytime NO_PERL changes.

Signed-off-by: Jeff King <peff@peff.net>
---
 Makefile | 3 +++
 1 file changed, 3 insertions(+)
Show changes to Makefile +3 −0
diff --git a/Makefile b/Makefile
index 827006b..0fa02ff 100644
--- a/Makefile
+++ b/Makefile
@@ -1676,6 +1676,9 @@ git.res: git.rc GIT-VERSION-FILE
 	  $(join -DMAJOR= -DMINOR=, $(wordlist 1,2,$(subst -, ,$(subst ., ,$(GIT_VERSION))))) \
 	  -DGIT_VERSION="\\\"$(GIT_VERSION)\\\"" $< -o $@
 
+# This makes sure we depend on the NO_PERL setting itself.
+$(patsubst %.perl,%,$(SCRIPT_PERL)): GIT-BUILD-OPTIONS
+
 ifndef NO_PERL
 $(patsubst %.perl,%,$(SCRIPT_PERL)): perl/perl.mak
 

-- 
2.1.2.596.g7379948
Jonathan Nieder· Nov 18, 2014, 18:38 UTC · re: Jeff King · lore

Re: [PATCH] t0090: mark add-interactive test with PERL prerequisite

Jeff King wrote:
> Subject: Makefile: have perl scripts depend on NO_PERL setting
[...]
> ---
>  Makefile | 3 +++
>  1 file changed, 3 insertions(+)
Gah.  Good catch.
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
[...]
Show 11 quoted lines
> --- a/Makefile
> +++ b/Makefile
> @@ -1676,6 +1676,9 @@ git.res: git.rc GIT-VERSION-FILE
>  	  $(join -DMAJOR= -DMINOR=, $(wordlist 1,2,$(subst -, ,$(subst ., ,$(GIT_VERSION))))) \
>  	  -DGIT_VERSION="\\\"$(GIT_VERSION)\\\"" $< -o $@
>  
> +# This makes sure we depend on the NO_PERL setting itself.
> +$(patsubst %.perl,%,$(SCRIPT_PERL)): GIT-BUILD-OPTIONS
> +
>  ifndef NO_PERL
>  $(patsubst %.perl,%,$(SCRIPT_PERL)): perl/perl.mak

Why do these repeat the 'patsubst ...' expression instead of using SCRIPT_PERL_GEN, by the way?

-- >8 --
Subject: Makefile: simplify by using SCRIPT_{PERL,SH}_GEN macros

SCRIPT_PERL_GEN is defined as $(patsubst %.perl,%,$(SCRIPT_PERL)) for use in targets like build-perl-script used by makefiles in subdirectories that override SCRIPT_PERL (see v1.8.2-rc0~17^2, "git-remote-mediawiki: use toplevel's Makefile", 2013-02-08).

The same expression is used in the rules that actually write the generated perl scripts, and since this rules were introduced before SCRIPT_PERL_GEN, they use the longhand instead of that macro. Use the macro to make reading easier.

Likewise for SCRIPT_SH_GEN. The Python rules already got the same simplification in v1.8.4-rc0~162^2~8 (2013-05-24).

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 Makefile | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)
Show changes to Makefile +5 −5
diff --git a/Makefile b/Makefile
index 0fa02ff..8f980e0 100644
--- a/Makefile
+++ b/Makefile
@@ -1662,7 +1662,7 @@ GIT-SCRIPT-DEFINES: FORCE
             fi
 
 
-$(patsubst %.sh,%,$(SCRIPT_SH)) : % : %.sh GIT-SCRIPT-DEFINES
+$(SCRIPT_SH_GEN) : % : %.sh GIT-SCRIPT-DEFINES
 	$(QUIET_GEN)$(cmd_munge_script) && \
 	chmod +x $@+ && \
 	mv $@+ $@
@@ -1677,10 +1677,10 @@ git.res: git.rc GIT-VERSION-FILE
 	  -DGIT_VERSION="\\\"$(GIT_VERSION)\\\"" $< -o $@
 
 # This makes sure we depend on the NO_PERL setting itself.
-$(patsubst %.perl,%,$(SCRIPT_PERL)): GIT-BUILD-OPTIONS
+$(SCRIPT_PERL_GEN): GIT-BUILD-OPTIONS
 
 ifndef NO_PERL
-$(patsubst %.perl,%,$(SCRIPT_PERL)): perl/perl.mak
+$(SCRIPT_PERL_GEN): perl/perl.mak
 
 perl/perl.mak: perl/PM.stamp
 
@@ -1693,7 +1693,7 @@ perl/perl.mak: GIT-CFLAGS GIT-PREFIX perl/Makefile perl/Makefile.PL
 	$(QUIET_SUBDIR0)perl $(QUIET_SUBDIR1) PERL_PATH='$(PERL_PATH_SQ)' prefix='$(prefix_SQ)' $(@F)
 
 PERL_DEFINES = $(PERL_PATH_SQ):$(PERLLIB_EXTRA_SQ)
-$(patsubst %.perl,%,$(SCRIPT_PERL)): % : %.perl perl/perl.mak GIT-PERL-DEFINES GIT-VERSION-FILE
+$(SCRIPT_PERL_GEN): % : %.perl perl/perl.mak GIT-PERL-DEFINES GIT-VERSION-FILE
 	$(QUIET_GEN)$(RM) $@ $@+ && \
 	INSTLIBDIR=`MAKEFLAGS= $(MAKE) -C perl -s --no-print-directory instlibdir` && \
 	INSTLIBDIR_EXTRA='$(PERLLIB_EXTRA_SQ)' && \
@@ -1727,7 +1727,7 @@ git-instaweb: git-instaweb.sh gitweb GIT-SCRIPT-DEFINES
 	chmod +x $@+ && \
 	mv $@+ $@
 else # NO_PERL
-$(patsubst %.perl,%,$(SCRIPT_PERL)) git-instaweb: % : unimplemented.sh
+$(SCRIPT_PERL_GEN) git-instaweb: % : unimplemented.sh
 	$(QUIET_GEN)$(RM) $@ $@+ && \
 	sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
 	    -e 's|@@REASON@@|NO_PERL=$(NO_PERL)|g' \

-- 
2.1.0.rc2.206.gedb03e5
Jonathan Nieder· Nov 18, 2014, 18:43 UTC · re: Jonathan Nieder · lore

Re: [PATCH] t0090: mark add-interactive test with PERL prerequisite

Jonathan Nieder wrote:
> Jeff King wrote:
Show 9 quoted lines
>> Subject: Makefile: have perl scripts depend on NO_PERL setting
> [...]
>> ---
>>  Makefile | 3 +++
>>  1 file changed, 3 insertions(+)
>
> Gah.  Good catch.
>
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
... and here's a patch on top to give git-p4 the same treatment.
-- >8 --
Subject: Makefile: have python scripts depend on NO_PYTHON setting

Like the perl scripts, python scripts need a dependency to ensure they are rebuilt when switching between the "dummy" versions that run without Python and the real thing.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 Makefile | 3 +++
 1 file changed, 3 insertions(+)
Show changes to Makefile +3 −0
diff --git a/Makefile b/Makefile
index 8f980e0..7482a4d 100644
--- a/Makefile
+++ b/Makefile
@@ -1736,6 +1736,9 @@ $(SCRIPT_PERL_GEN) git-instaweb: % : unimplemented.sh
 	mv $@+ $@
 endif # NO_PERL
 
+# This makes sure we depend on the NO_PYTHON setting itself.
+$(SCRIPT_PYTHON_GEN): GIT-BUILD-OPTIONS
+
 ifndef NO_PYTHON
 $(SCRIPT_PYTHON_GEN): GIT-CFLAGS GIT-PREFIX GIT-PYTHON-VARS
 $(SCRIPT_PYTHON_GEN): % : %.py

-- 
2.1.0.rc2.206.gedb03e5
Jeff King· Nov 18, 2014, 18:49 UTC · re: Jonathan Nieder · lore

Re: [PATCH] t0090: mark add-interactive test with PERL prerequisite

On Tue, Nov 18, 2014 at 10:43:47AM -0800, Jonathan Nieder wrote:
Show 8 quoted lines
> ... and here's a patch on top to give git-p4 the same treatment.
> 
> -- >8 --
> Subject: Makefile: have python scripts depend on NO_PYTHON setting
> 
> Like the perl scripts, python scripts need a dependency to ensure they
> are rebuilt when switching between the "dummy" versions that run
> without Python and the real thing.

Thanks, I didn't think to look for similar cases. It seems python is the only other thing that gets the "unimplemented" treatment. If you do:

  make
  make NO_TCLTK=Yes

I think you'll still end up with a crufty gitk build, but the fix there is much more involved (it would have to create a "Sorry, gitk wasn't built" script). I don't think it's worth the effort.

-Peff
Pete Wyckoff· Nov 18, 2014, 23:10 UTC · re: Jonathan Nieder · lore

Re: [PATCH] t0090: mark add-interactive test with PERL prerequisite

jrnieder@gmail.com wrote on Tue, 18 Nov 2014 10:43 -0800:
Show 30 quoted lines
> ... and here's a patch on top to give git-p4 the same treatment.
> 
> -- >8 --
> Subject: Makefile: have python scripts depend on NO_PYTHON setting
> 
> Like the perl scripts, python scripts need a dependency to ensure they
> are rebuilt when switching between the "dummy" versions that run
> without Python and the real thing.
> 
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
>  Makefile | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/Makefile b/Makefile
> index 8f980e0..7482a4d 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1736,6 +1736,9 @@ $(SCRIPT_PERL_GEN) git-instaweb: % : unimplemented.sh
>  	mv $@+ $@
>  endif # NO_PERL
>  
> +# This makes sure we depend on the NO_PYTHON setting itself.
> +$(SCRIPT_PYTHON_GEN): GIT-BUILD-OPTIONS
> +
>  ifndef NO_PYTHON
>  $(SCRIPT_PYTHON_GEN): GIT-CFLAGS GIT-PREFIX GIT-PYTHON-VARS
>  $(SCRIPT_PYTHON_GEN): % : %.py
> -- 
> 2.1.0.rc2.206.gedb03e5

Looks obviously correct, thanks for remembering the other scripting languages. :)

Acked-by: Pete Wyckoff <pw@padd.com>
Jeff King· Nov 18, 2014, 18:44 UTC · re: Jonathan Nieder · lore

Re: [PATCH] t0090: mark add-interactive test with PERL prerequisite

On Tue, Nov 18, 2014 at 10:38:38AM -0800, Jonathan Nieder wrote:
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Thanks.
Show 8 quoted lines
> > +# This makes sure we depend on the NO_PERL setting itself.
> > +$(patsubst %.perl,%,$(SCRIPT_PERL)): GIT-BUILD-OPTIONS
> > +
> >  ifndef NO_PERL
> >  $(patsubst %.perl,%,$(SCRIPT_PERL)): perl/perl.mak
> 
> Why do these repeat the 'patsubst ...' expression instead of using
> SCRIPT_PERL_GEN, by the way?
Dunno. I just cargo-culted from the context lines.
Show 15 quoted lines
> -- >8 --
> Subject: Makefile: simplify by using SCRIPT_{PERL,SH}_GEN macros
> 
> SCRIPT_PERL_GEN is defined as $(patsubst %.perl,%,$(SCRIPT_PERL)) for
> use in targets like build-perl-script used by makefiles in
> subdirectories that override SCRIPT_PERL (see v1.8.2-rc0~17^2,
> "git-remote-mediawiki: use toplevel's Makefile", 2013-02-08).
> 
> The same expression is used in the rules that actually write the
> generated perl scripts, and since this rules were introduced before
> SCRIPT_PERL_GEN, they use the longhand instead of that macro.  Use the
> macro to make reading easier.
> 
> Likewise for SCRIPT_SH_GEN.  The Python rules already got the same
> simplification in v1.8.4-rc0~162^2~8 (2013-05-24).

This makes sense, and looking over the Makefile, I don't see how it could cause any bad side effects.

Minor nit:
  s/this rules/these rules/
in your commit message. Otherwise:
  Reviewed-by: Jeff King <peff@peff.net>
-Peff
Jonathan Nieder· Nov 18, 2014, 18:51 UTC · re: Jeff King · lore

Re: [PATCH] t0090: mark add-interactive test with PERL prerequisite

Jeff King wrote:
Show 8 quoted lines
> The add-interactive system is built in perl. If you build
> with NO_PERL, running "git commit --interactive" will exit
> with an error and the test will fail.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> Noticed by Michael while working around gitweb failures by setting
> NO_PERL. :)
Heh.
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

← back to recent threads