Re: [PATCH v2 4/4] t/Makefile: introduce TEST_SHELL_PATH

[Jeff King]

On Fri, Dec 15, 2017 at 08:58:22AM -0800, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > I think (4) and (5) are the only things that actually change the
> > behavior in a meaningful way. But they're a bit more hacky and
> > repetitive than I'd like. Especially given that I'm not really sure
> > we're solving an interesting problem. I'm happy enough with the patch as
> > shown, and I do not recall anybody complaining about the current
> > behavior of these options.
> 
> OK.  Thanks for thinking it through.

I took one final look at this, wondering if it ought to follow the
"write to BUILD-OPTIONS only if set" pattern that some other variables
do. But I think that just ends up more confusing, because of the way we
use the variable from both the Makefile and test-lib.sh. So it makes
this work:

  make
  make -C t TEST_SHELL_PATH=whatever

but not quite this:

  make TEST_SHELL_PATH=one
  make -C t TEST_SHELL_PATH=two

because in the second case, we use "two" to invoke the test script, but
a "--tee" re-exec would use "one". Which is pretty subtle.

I really wish there was a way for a shell script to find out which
interpreter it was currently using, but I couldn't come up with a
portable way to do so (on some systems, /proc/$$/exe works, but that's
obviously not something we should count on).

So anyway. I think I'm OK with the series as-is.

-Peff

Re: [PATCH v2 4/4] t/Makefile: introduce TEST_SHELL_PATH

[Junio C Hamano]

Jeff King  writes:

> I think (4) and (5) are the only things that actually change the
> behavior in a meaningful way. But they're a bit more hacky and
> repetitive than I'd like. Especially given that I'm not really sure
> we're solving an interesting problem. I'm happy enough with the patch as
> shown, and I do not recall anybody complaining about the current
> behavior of these options.

OK.  Thanks for thinking it through.

>> There is a long outstanding NEEDSWORK comment in help.c that wonders
>> if we want to embed contents from GIT-BUILD-OPTIONS in the resulting
>> binary, and the distinction Dscho brought up between "build" and
>> "test" phases would start to matter even more once we go in that
>> direction.
>
> I guess you're implying having a GIT-BUILD-OPTIONS and a
> GIT-TEST-OPTIONS here.

I admit that my thinking did not go that far to introduce the
latter, as "git version --how-did-we-build-this-exact-git" only
needs the former.  But you're right that some information given
at the top-level must be stored somewhere t/test-lib.sh reads in
order to allow us run tests from outside Makefile (your point 1.)

Re: [PATCH v2 4/4] t/Makefile: introduce TEST_SHELL_PATH

[Jeff King]

On Mon, Dec 11, 2017 at 12:37:42PM -0800, Junio C Hamano wrote:

> > Interestingly, many of those do something like this in the Makefile:
> >
> >   ifdef GIT_TEST_CMP
> > @echo GIT_TEST_OPTS=... >>$@+
> >   endif
> >
> > which seems utterly confusing to me. Because it means that if you build
> > with those options set, then they will override anything in the
> > environment. But if you don't, then you _may_ override them in the
> > environment. In other words:
> >
> >   make
> >   cd t
> >   GIT_TEST_CMP=foo ./t-*
> >
> > will respect that variable. But:
> >
> >   make GIT_TEST_CMP=foo
> >   cd t
> >   GIT_TEST_CMP=bar ./t-*
> >
> > will not. Which seems weird.  But I guess we could follow that pattern
> > with TEST_SHELL_PATH.
> 
> Or perhaps we can start setting a better example with the new
> variable, and migrate those weird existing ones over to the new way
> of not forbidding run-time overriding?

This turns out to be quite tricky, because GIT-BUILD-OPTIONS must be
parsed as both a Makefile and shell-script.

So we can do:

  1. Omit them from GIT-BUILD-OPTIONS, which means they don't work for
 cases where the tests aren't started from the Makefile (which would
 have put them in the environment). I think this is a non-starter.

  2. Add a Makefile-level ifdef when generating GIT-BUILD-OPTIONS, so
 that unused options can be overridden by the environment That's the
 "weird" thing above that we do now for some variables.

  3. Add something like:

   test -z "$FOO" && FOO=...

 when building GIT-BUILD-OPTIONS (instead of just FOO=...). But that
 doesn't work because it must parse as Makefile.

  4. In test-lib.sh, save and restore each such variable so that the
 existing environment takes precedence. Like:

   FOO_save=$FOO
   BAR_save=$BAR
   ...etc for each...

   . GIT-BUILD-OPTIONS

   test -n "$FOO_save" && FOO=$FOO_save
   test -n "$BAR_save" && BAR=$BAR_save
   ...etc...

  We have to do the save/restore dance rather than just reordering
  the assignments, because the existing environment is being passed
  into us (so we can't just "assign" it to overwrite what's in the
  build options file).

  This could be made slightly less tedious with a loop and an eval.
  It could possibly be made very succinct but very magical with
  something like:

saved=$(export)
. GIT-BUILD-OPTIONS
eval "$saved"

  5. Give up on the dual nature of GIT-BUILD-OPTIONS, and generate two
 such files (with identical values but different syntax).

I think (4) and (5) are the only things that actually change the
behavior in a meaningful way. But they're a bit more hacky and
repetitive than I'd like. Especially given that I'm not really sure
we're solving an interesting problem. I'm happy enough with the patch as
shown, and I do not recall anybody complaining about the current
behavior of these options.

> There is a long outstanding NEEDSWORK comment in help.c that wonders
> if we want to embed contents from GIT-BUILD-OPTIONS in the resulting
> binary, and the distinction Dscho brought up between "build" and
> "test" phases would start to matter even more once we go in that
> direction.

I guess you're implying having a GIT-BUILD-OPTIONS and a
GIT-TEST-OPTIONS here. But that doesn't save us from the Makefile/shell
duality, unfortunately. Some of the test options need to be read by
t/Makefile, and some need to be read by test-lib.sh (and I suspect there
are some needed in both places).

-Peff

Re: [PATCH v2 4/4] t/Makefile: introduce TEST_SHELL_PATH

[Junio C Hamano]

Jeff King  writes:

> I'm not sure that's true. Look at what already goes into
> GIT-BUILD-OPTIONS: TEST_OUTPUT_DIRECTORY, GIT_TEST_CMP, GIT_PERF_*, etc.
>
> Interestingly, many of those do something like this in the Makefile:
>
>   ifdef GIT_TEST_CMP
>   @echo GIT_TEST_OPTS=... >>$@+
>   endif
>
> which seems utterly confusing to me. Because it means that if you build
> with those options set, then they will override anything in the
> environment. But if you don't, then you _may_ override them in the
> environment. In other words:
>
>   make
>   cd t
>   GIT_TEST_CMP=foo ./t-*
>
> will respect that variable. But:
>
>   make GIT_TEST_CMP=foo
>   cd t
>   GIT_TEST_CMP=bar ./t-*
>
> will not. Which seems weird.  But I guess we could follow that pattern
> with TEST_SHELL_PATH.

Or perhaps we can start setting a better example with the new
variable, and migrate those weird existing ones over to the new way
of not forbidding run-time overriding?

There is a long outstanding NEEDSWORK comment in help.c that wonders
if we want to embed contents from GIT-BUILD-OPTIONS in the resulting
binary, and the distinction Dscho brought up between "build" and
"test" phases would start to matter even more once we go in that
direction.

Re: [PATCH v2 4/4] t/Makefile: introduce TEST_SHELL_PATH

[Jeff King]

On Sat, Dec 09, 2017 at 02:44:44PM +0100, Johannes Schindelin wrote:

> > > ... and we could simply see whether the environment variable
> > > TEST_SHELL_PATH (which we would set in t/Makefile from the passed-in
> > > SHELL_PATH) is set, and override it again.
> > > 
> > > I still think we can do without recording test-phase details in the
> > > build-phase (which may, or may not, know what the test-phase wants to do).
> > > 
> > > In other words, I believe that we can make the invocation you mentioned
> > > above work, by touching only t/Makefile (to pass SHELL_PATH as
> > > TEST_SHELL_PATH) and t/test-lib.sh (to override the SHELL_PATH from
> > > GIT-BUILD-OPTIONS with TEST_SHELL_PATH, if set).
> > 
> > We could do that, but it makes TEST_SHELL_PATH inconsistent with all of
> > the other config.mak variables.
> 
> It is already inconsistent with the other variables because its scope is
> the "test" phase, not the "build" phase.

I'm not sure that's true. Look at what already goes into
GIT-BUILD-OPTIONS: TEST_OUTPUT_DIRECTORY, GIT_TEST_CMP, GIT_PERF_*, etc.

Interestingly, many of those do something like this in the Makefile:

  ifdef GIT_TEST_CMP
@echo GIT_TEST_OPTS=... >>$@+
  endif

which seems utterly confusing to me. Because it means that if you build
with those options set, then they will override anything in the
environment. But if you don't, then you _may_ override them in the
environment. In other words:

  make
  cd t
  GIT_TEST_CMP=foo ./t-*

will respect that variable. But:

  make GIT_TEST_CMP=foo
  cd t
  GIT_TEST_CMP=bar ./t-*

will not. Which seems weird.  But I guess we could follow that pattern
with TEST_SHELL_PATH.

-Peff

Re: [PATCH v2 4/4] t/Makefile: introduce TEST_SHELL_PATH

[Johannes Schindelin]

Hi Peff,

On Fri, 8 Dec 2017, Jeff King wrote:

> On Fri, Dec 08, 2017 at 04:08:19PM +0100, Johannes Schindelin wrote:
> 
> > > Yes, but test-lib.sh sources GIT-BUILD-OPTIONS, which we
> > > built during the first "make". And that overrides the
> > > environment, giving us the original SHELL_PATH again.
> > 
> > ... and we could simply see whether the environment variable
> > TEST_SHELL_PATH (which we would set in t/Makefile from the passed-in
> > SHELL_PATH) is set, and override it again.
> > 
> > I still think we can do without recording test-phase details in the
> > build-phase (which may, or may not, know what the test-phase wants to do).
> > 
> > In other words, I believe that we can make the invocation you mentioned
> > above work, by touching only t/Makefile (to pass SHELL_PATH as
> > TEST_SHELL_PATH) and t/test-lib.sh (to override the SHELL_PATH from
> > GIT-BUILD-OPTIONS with TEST_SHELL_PATH, if set).
> 
> We could do that, but it makes TEST_SHELL_PATH inconsistent with all of
> the other config.mak variables.

It is already inconsistent with the other variables because its scope is
the "test" phase, not the "build" phase.

Ciao,
Dscho

Re: [PATCH v2 4/4] t/Makefile: introduce TEST_SHELL_PATH

[Jeff King]

On Fri, Dec 08, 2017 at 04:08:19PM +0100, Johannes Schindelin wrote:

> > Yes, but test-lib.sh sources GIT-BUILD-OPTIONS, which we
> > built during the first "make". And that overrides the
> > environment, giving us the original SHELL_PATH again.
> 
> ... and we could simply see whether the environment variable
> TEST_SHELL_PATH (which we would set in t/Makefile from the passed-in
> SHELL_PATH) is set, and override it again.
> 
> I still think we can do without recording test-phase details in the
> build-phase (which may, or may not, know what the test-phase wants to do).
> 
> In other words, I believe that we can make the invocation you mentioned
> above work, by touching only t/Makefile (to pass SHELL_PATH as
> TEST_SHELL_PATH) and t/test-lib.sh (to override the SHELL_PATH from
> GIT-BUILD-OPTIONS with TEST_SHELL_PATH, if set).

We could do that, but it makes TEST_SHELL_PATH inconsistent with all of
the other config.mak variables.

-Peff

Re: [PATCH v2 4/4] t/Makefile: introduce TEST_SHELL_PATH

[Johannes Schindelin]

Hi Peff,

the other three patches look good to me.

On Fri, 8 Dec 2017, Jeff King wrote:

> You may want to run the test suite with a different shell
> than you use to build Git. For instance, you may build with
> SHELL_PATH=/bin/sh (because it's faster, or it's what you
> expect to exist on systems where the build will be used) but
> want to run the test suite with bash (e.g., since that
> allows using "-x" reliably across the whole test suite).
> There's currently no good way to do this.
> 
> You might think that doing two separate make invocations,
> like:
> 
>   make &&
>   make -C t SHELL_PATH=/bin/bash
> 
> would work. And it _almost_ does. The second make will see
> our bash SHELL_PATH, and we'll use that to run the
> individual test scripts (or tell prove to use it to do so).
> So far so good.
> 
> But this breaks down when "--tee" or "--verbose-log" is
> used. Those options cause the test script to actually
> re-exec itself using $SHELL_PATH. But wait, wouldn't our
> second make invocation have set SHELL_PATH correctly in the
> environment?
> 
> Yes, but test-lib.sh sources GIT-BUILD-OPTIONS, which we
> built during the first "make". And that overrides the
> environment, giving us the original SHELL_PATH again.

... and we could simply see whether the environment variable
TEST_SHELL_PATH (which we would set in t/Makefile from the passed-in
SHELL_PATH) is set, and override it again.

I still think we can do without recording test-phase details in the
build-phase (which may, or may not, know what the test-phase wants to do).

In other words, I believe that we can make the invocation you mentioned
above work, by touching only t/Makefile (to pass SHELL_PATH as
TEST_SHELL_PATH) and t/test-lib.sh (to override the SHELL_PATH from
GIT-BUILD-OPTIONS with TEST_SHELL_PATH, if set).

Ciao,
Dscho