t0090: mark add-interactive test with PERL prerequisite

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.

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

index 158cf4f..067f4c6 100755
@@ -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

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.

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

index 52034c8..c4c3c49 100755
@@ -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 &&

index 1fd5142..a4b3db2 100755
@@ -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 <[email protected]>

2.1.2.596.g7379948

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.

---
 t/lib-cvs.sh | 6 ++++++
 1 file changed, 6 insertions(+)

index 9b2bcfb..b75df11 100644
@@ -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

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

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.

---
 Makefile | 3 +++
 1 file changed, 3 insertions(+)

index 827006b..0fa02ff 100644
@@ -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

[...]

Gah. Good catch.

[...]

---
 Makefile | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

index 0fa02ff..8f980e0 100644
@@ -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

... 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.

---
 Makefile | 3 +++
 1 file changed, 3 insertions(+)

index 8f980e0..7482a4d 100644
@@ -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

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

[email protected] wrote on Tue, 18 Nov 2014 10:43 -0800:

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

Thanks.

Dunno. I just cargo-culted from the context lines.

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:

-Peff

Heh.