Re: [PATCH] NO_PERL support

From: Jeff King <peff@peff.net>
Date: Apr 3, 2009, 13:20 UTC
Message-ID: <20090403132029.GC21153@coredump.intra.peff.net>
In-Reply-To: <20090403T065545Z@curie.orbis-terrarum.net>

[cc'ing Johannes because of some possibly related work in the test scripts; see below]

On Fri, Apr 03, 2009 at 12:03:50AM -0700, Robin H. Johnson wrote:

> The attached patch adds NO_PERL to the build system, in the same fashion
> as NO_TCLTK.

I think this is a good change. One of my less-abled test platforms has a broken perl, and I end up having to manually skip a lot of tests. Being able to set NO_PERL and have the tests themselves realize they must be skipped will be much cleaner.

Show 9 quoted lines

> Effects:
> - No perl-based scripts or code are installed:
> 	$SCRIPT_PERL
> 	git-instaweb
> 	gitweb
> 	git-cvsserver
> 	git-svn
> - git-add does NOT have interactive support.
> - None of the tests for the disabled stuff gets run.

Hmm. With this patch, a user with a NO_PERL build will just get:

  $ git svn
  git: 'svn' is not a git-command. See 'git --help'.
  $ git add -i
  error: unknown switch `i'
  usage: ...

I wonder if we should be a little nicer and say "we know about this command or option, but we did not build support for it". OTOH, what you have mirrors what NO_TCLTK does, so perhaps it is not a big deal.

Show 9 quoted lines

> @@ -1067,6 +1079,11 @@ endif
>  ifeq ($(TCLTK_PATH),)
>  NO_TCLTK=NoThanks
>  endif
> +ifeq ($(PERL_PATH),)
> +NO_PERL=NoThanks
> +export NO_PERL
> +export NO_PERL_MAKEMAKER
> +endif

Do we have to actually export here? NO_TCLTK doesn't. If it's for the test scripts, then should it be part of GIT-BUILD-OPTIONS?

Show 6 quoted lines

> +if test -n "$NO_PERL"
> +then
> +	test_expect_success 'skipping git-svn tests, NO_PERL defined' :
> +	test_done
> +	exit
> +fi

This probably got copied from an older example, but I think the recommended way to skip tests these days is to use 'say' instead of test_expect_success (since we have statistics on passing/failing tests now).

Also, it may make sense to integrate this with Johannes Sixt's test_have_prereq work (which is still in next), but I haven't looked too closely at that.

> -test_expect_success \
> +[ -z "$NO_PERL" ] && test_expect_success \

I think it is nicer to actually say "skipping test" as the other spots do instead of just not running it.

-Peff