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

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

[Jeff King]

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

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

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

[Jeff King]

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

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

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

[Jonathan Nieder]

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

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

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

[Jeff King]

On Tue, Nov 18, 2014 at 10:56:22AM -0800, Jonathan Nieder wrote:

> 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

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

[Jeff King]

On Tue, Nov 18, 2014 at 12:22:31PM -0500, Jeff King wrote:

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

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

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

[Jonathan Nieder]

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>

[...]
> --- 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(-)

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

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

[Jonathan Nieder]

Jonathan Nieder wrote:
> 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>

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

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

[Jeff King]

On Tue, Nov 18, 2014 at 10:43:47AM -0800, Jonathan Nieder wrote:

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

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

[Pete Wyckoff]

jrnieder@gmail.com wrote on Tue, 18 Nov 2014 10:43 -0800:
> ... 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>

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

[Jeff King]

On Tue, Nov 18, 2014 at 10:38:38AM -0800, Jonathan Nieder wrote:

> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks.

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

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

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

[Jonathan Nieder]

Jeff King wrote:

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