Re: [PATCH 4/3] t/Makefile: introduce TEST_SHELL_PATH
Hi Peff,
On Wed, 25 Oct 2017, Jeff King wrote:
> On Wed, Oct 25, 2017 at 11:35:44PM +0200, Johannes Schindelin wrote:
>
> > > > Or alternatively we could prefix the assignment by
> > > >
> > > > test -n "$TEST_SHELL_PATH" ||
> > > >
> > > > or use the pattern
> > > >
> > > > TEST_SHELL_PATH="${TEST_SHELL_PATH:-[...]}"
> > >
> > > I'm not quite sure what this is fixing. Is there a case where we
> > > wouldn't have TEST_SHELL_PATH set when running the tests? I think there
> > > are already other bits that assume that "make" has been run (including
> > > the existing reference to $SHELL_PATH, I think).
> >
> > The way I read your patch, setting the environment variable differnently
> > at test time than at build time would be ignored: GIT-BUILD-OPTIONS is
> > sourced and would override whatever you told the test suite to use.
> >
> > I guess it does not really matter all that much in practice.
>
> Right. I find that behavior mildly irritating at times, but it's
> consistent with other items like NO_PERL, etc. E.g., you cannot do:
>
> make NO_PERL=
> cd t
> NO_PERL=Nope ./t3701-*
>
> and disable perl. It's testing what got built.
The difference between NO_PERL and TEST_SHELL_PATH, of course, is that
NO_PERL affects the build and the test phase, while TEST_SHELL_PATH really
has no impact whatsoever on the build phase.
Ciao,
Dscho
Re: [PATCH 4/3] t/Makefile: introduce TEST_SHELL_PATH
On Wed, Oct 25, 2017 at 11:35:44PM +0200, Johannes Schindelin wrote:
> > > Or alternatively we could prefix the assignment by
> > >
> > > test -n "$TEST_SHELL_PATH" ||
> > >
> > > or use the pattern
> > >
> > > TEST_SHELL_PATH="${TEST_SHELL_PATH:-[...]}"
> >
> > I'm not quite sure what this is fixing. Is there a case where we
> > wouldn't have TEST_SHELL_PATH set when running the tests? I think there
> > are already other bits that assume that "make" has been run (including
> > the existing reference to $SHELL_PATH, I think).
>
> The way I read your patch, setting the environment variable differnently
> at test time than at build time would be ignored: GIT-BUILD-OPTIONS is
> sourced and would override whatever you told the test suite to use.
>
> I guess it does not really matter all that much in practice.
Right. I find that behavior mildly irritating at times, but it's
consistent with other items like NO_PERL, etc. E.g., you cannot do:
make NO_PERL=
cd t
NO_PERL=Nope ./t3701-*
and disable perl. It's testing what got built.
-Peff
Re: [PATCH 4/3] t/Makefile: introduce TEST_SHELL_PATH
Hi Peff,
On Mon, 23 Oct 2017, Jeff King wrote:
> On Mon, Oct 23, 2017 at 01:01:42PM +0200, Johannes Schindelin wrote:
>
> > On Fri, 20 Oct 2017, Jeff King wrote:
> >
> > > @@ -2350,6 +2357,7 @@ GIT-LDFLAGS: FORCE
> > > # and the first level quoting from the shell that runs "echo".
> > > GIT-BUILD-OPTIONS: FORCE
> > > @echo SHELL_PATH=\''$(subst ','\'',$(SHELL_PATH_SQ))'\' >$@+
> > > + @echo TEST_SHELL_PATH=\''$(subst ','\'',$(TEST_SHELL_PATH_SQ))'\' >$@+
> >
> > Do we really want to force the test shell path to be hardcoded at runtime?
> > It may be a better idea not to write this into GIT-BUILD-OPTIONS.
>
> My intent was to make it work "out of the box" in the same way as
> SHELL_PATH does now. So that:
>
> echo TEST_SHELL_PATH=whatever >>config.mak
> make test
> cd t && ./t1234-*
>
> both respect it. Without going through BUILD-OPTIONS, I don't think it
> makes it into the environment via config.mak (it _does_ if you specify
> it on the command-line of "make", though).
>
> For my purposes it would be fine to just use the environment, but I was
> trying to have it match the other variables for consistency.
Makes sense.
> > Or alternatively we could prefix the assignment by
> >
> > test -n "$TEST_SHELL_PATH" ||
> >
> > or use the pattern
> >
> > TEST_SHELL_PATH="${TEST_SHELL_PATH:-[...]}"
>
> I'm not quite sure what this is fixing. Is there a case where we
> wouldn't have TEST_SHELL_PATH set when running the tests? I think there
> are already other bits that assume that "make" has been run (including
> the existing reference to $SHELL_PATH, I think).
The way I read your patch, setting the environment variable differnently
at test time than at build time would be ignored: GIT-BUILD-OPTIONS is
sourced and would override whatever you told the test suite to use.
I guess it does not really matter all that much in practice.
Ciao,
Dscho
Re: [PATCH 4/3] t/Makefile: introduce TEST_SHELL_PATH
On Mon, Oct 23, 2017 at 01:01:42PM +0200, Johannes Schindelin wrote:
> On Fri, 20 Oct 2017, Jeff King wrote:
>
> > @@ -2350,6 +2357,7 @@ GIT-LDFLAGS: FORCE
> > # and the first level quoting from the shell that runs "echo".
> > GIT-BUILD-OPTIONS: FORCE
> > @echo SHELL_PATH=\''$(subst ','\'',$(SHELL_PATH_SQ))'\' >$@+
> > + @echo TEST_SHELL_PATH=\''$(subst ','\'',$(TEST_SHELL_PATH_SQ))'\' >$@+
>
> Do we really want to force the test shell path to be hardcoded at runtime?
> It may be a better idea not to write this into GIT-BUILD-OPTIONS.
My intent was to make it work "out of the box" in the same way as
SHELL_PATH does now. So that:
echo TEST_SHELL_PATH=whatever >>config.mak
make test
cd t && ./t1234-*
both respect it. Without going through BUILD-OPTIONS, I don't think it
makes it into the environment via config.mak (it _does_ if you specify
it on the command-line of "make", though).
For my purposes it would be fine to just use the environment, but I was
trying to have it match the other variables for consistency.
> Or alternatively we could prefix the assignment by
>
> test -n "$TEST_SHELL_PATH" ||
>
> or use the pattern
>
> TEST_SHELL_PATH="${TEST_SHELL_PATH:-[...]}"
I'm not quite sure what this is fixing. Is there a case where we
wouldn't have TEST_SHELL_PATH set when running the tests? I think there
are already other bits that assume that "make" has been run (including
the existing reference to $SHELL_PATH, I think).
-Peff
Re: [PATCH 4/3] t/Makefile: introduce TEST_SHELL_PATH
Hi Peff,
On Fri, 20 Oct 2017, Jeff King wrote:
> @@ -2350,6 +2357,7 @@ GIT-LDFLAGS: FORCE
> # and the first level quoting from the shell that runs "echo".
> GIT-BUILD-OPTIONS: FORCE
> @echo SHELL_PATH=\''$(subst ','\'',$(SHELL_PATH_SQ))'\' >$@+
> + @echo TEST_SHELL_PATH=\''$(subst ','\'',$(TEST_SHELL_PATH_SQ))'\' >$@+
Do we really want to force the test shell path to be hardcoded at runtime?
It may be a better idea not to write this into GIT-BUILD-OPTIONS.
Or alternatively we could prefix the assignment by
test -n "$TEST_SHELL_PATH" ||
or use the pattern
TEST_SHELL_PATH="${TEST_SHELL_PATH:-[...]}"
Ciao,
Dscho
Re: [PATCH 4/3] t/Makefile: introduce TEST_SHELL_PATH
On Sat, Oct 21, 2017 at 01:50:01AM +0200, SZEDER Gábor wrote: > > On Thu, Oct 19, 2017 at 05:01:40PM -0400, Jeff King wrote: > > > > > I sometimes run git's test suite as part of an automated testing > > > process. I was hoping to add "-x" support to get more details when a > > > test fails (since failures are sometimes hard to reproduce). > > Would it make sense (or be feasible) to enable "-x" on Travis CI? Yes, after this series I think it may be workable to do so. > > @@ -2350,6 +2357,7 @@ GIT-LDFLAGS: FORCE > > # and the first level quoting from the shell that runs "echo". > > GIT-BUILD-OPTIONS: FORCE > > @echo SHELL_PATH=\''$(subst ','\'',$(SHELL_PATH_SQ))'\' >$@+ > > + @echo TEST_SHELL_PATH=\''$(subst ','\'',$(TEST_SHELL_PATH_SQ))'\' >$@+ > > This redirection overwrites, loosing the just written SHELL_PATH. It > should append like the lines below. Doh, thank you. It's interesting that nothing broke with this error. But I think when run via Make that we end up with SHELL_PATH via the environment (set to the default /bin/sh, which was suitable for my system). I'll fix it. -Peff
Re: [PATCH 4/3] t/Makefile: introduce TEST_SHELL_PATH
> On Thu, Oct 19, 2017 at 05:01:40PM -0400, Jeff King wrote: > > > I sometimes run git's test suite as part of an automated testing > > process. I was hoping to add "-x" support to get more details when a > > test fails (since failures are sometimes hard to reproduce). Would it make sense (or be feasible) to enable "-x" on Travis CI? > diff --git a/Makefile b/Makefile > index cd75985991..9baa3c4b50 100644 > --- a/Makefile > +++ b/Makefile > @@ -425,6 +425,10 @@ all:: > # > # to say "export LESS=FRX (and LV=-c) if the environment variable > # LESS (and LV) is not set, respectively". > +# > +# Define TEST_SHELL_PATH if you want to use a shell besides SHELL_PATH for > +# running the test scripts (e.g., bash has better support for "set -x" > +# tracing). > > GIT-VERSION-FILE: FORCE > @$(SHELL_PATH) ./GIT-VERSION-GEN > @@ -727,6 +731,8 @@ endif > export PERL_PATH > export PYTHON_PATH > > +TEST_SHELL_PATH = $(SHELL_PATH) > + > LIB_FILE = libgit.a > XDIFF_LIB = xdiff/lib.a > VCSSVN_LIB = vcs-svn/lib.a > @@ -1721,6 +1727,7 @@ prefix_SQ = $(subst ','\'',$(prefix)) > gitwebdir_SQ = $(subst ','\'',$(gitwebdir)) > > SHELL_PATH_SQ = $(subst ','\'',$(SHELL_PATH)) > +TEST_SHELL_PATH_SQ = $(subst ','\'',$(TEST_SHELL_PATH)) > PERL_PATH_SQ = $(subst ','\'',$(PERL_PATH)) > PYTHON_PATH_SQ = $(subst ','\'',$(PYTHON_PATH)) > TCLTK_PATH_SQ = $(subst ','\'',$(TCLTK_PATH)) > @@ -2350,6 +2357,7 @@ GIT-LDFLAGS: FORCE > # and the first level quoting from the shell that runs "echo". > GIT-BUILD-OPTIONS: FORCE > @echo SHELL_PATH=\''$(subst ','\'',$(SHELL_PATH_SQ))'\' >$@+ > + @echo TEST_SHELL_PATH=\''$(subst ','\'',$(TEST_SHELL_PATH_SQ))'\' >$@+ This redirection overwrites, loosing the just written SHELL_PATH. It should append like the lines below. > @echo PERL_PATH=\''$(subst ','\'',$(PERL_PATH_SQ))'\' >>$@+ > @echo DIFF=\''$(subst ','\'',$(subst ','\'',$(DIFF)))'\' >>$@+ > @echo PYTHON_PATH=\''$(subst ','\'',$(PYTHON_PATH_SQ))'\' >>$@+ Gábor
