Re: [PATCH 2/2] ci: restore running httpd tests

From: SZEDER Gábor <szeder.dev@gmail.com>
Date: Sep 7, 2019, 10:16 UTC
Message-ID: <20190907101637.GE32087@szeder.dev>
In-Reply-To: <20190906191300.GA10769@sigill.intra.peff.net>

On Fri, Sep 06, 2019 at 03:13:01PM -0400, Jeff King wrote:

Show 12 quoted lines

> On Fri, Sep 06, 2019 at 02:13:26PM +0200, SZEDER Gábor wrote:
> 
> > Once upon a time GIT_TEST_HTTPD was a tristate variable and we
> > exported 'GIT_TEST_HTTPD=YesPlease' in our CI scripts to make sure
> > that we run the httpd tests in the Linux Clang and GCC build jobs, or
> > error out if they can't be run for any reason [1].
> 
> Yikes, good catch.
> 
> I wonder if it would be possible for the test suite to catch this. I
> think env--helper would have written a message to stderr, but because we
> use --exit-code, we can't tell the difference between that and "false".

No, '--exit-code' only suppresses the printing of 'true' and 'false', but it doesn't have any effect on the command's exit code. And if the value of the environment variable is not a bool, then it does print an error message to standard error, and, more importantly, exits with 128 (as opposed to 1 indicating false). So we could tell the difference, but as it happens the command is invoked as 'if ! git env-helper ...', which then interprets that 128 the same as on ordinary false.

  $ VAR=false git env--helper --type=bool --default=false --exit-code VAR ; echo $?
  1
  $ VAR=true git env--helper --type=bool --default=false --exit-code VAR ; echo $?
  0
  $ VAR=YesPlease git env--helper --type=bool --default=false --exit-code VAR ; echo $?
  fatal: bad numeric config value 'YesPlease' for 'VAR': invalid unit
  128

> I think we'd have go back to something more like:
> 
>   test_tristate () {

The env var is supposed to be a bool, so there is no third state anymore.

> 	bool=$(git env--helper --type=bool --default=true "$1") ||

Some callsites use '--default=false', so we need a second parameter to specify the default.

Show 10 quoted lines

> 		eval "error \"$1 is not a bool: \$$1\""
> 	test "$bool" = "true"
>   }
>   ...
>   if test_tristate GIT_TEST_HTTPD
>   then
> 	... use httpd ...
>   fi
> 
> Not sure if it's worth it.

Well... On one hand, there are no other similar issues in our CI scripts (there is still a GIT_TEST_CLONE_2GB=YesPlease, but it's only ever checked with 'test -z' in 't5608-clone-2gb.sh', so it works as expected, even though it's inconsistent, and 'GIT_TEST_CLONE_2GB=NoThanks' would do the wrong thing).

OTOH, some unsuspecting devs might still have a 'GIT_TEST_foo=YesPlease' in their 'config.mak' from the old days...

Furthermore, as for the "good catch", I just got lucky, and not only once but several times in a row: that Perforce filehost outage the other day errored one of my builds, so I came up with a fix [1], for once took the extra effort and checked it on Azure Pipelines, and since that was my first ever build over there, out of curiosity I scrolled through the whole build output, by chance those "skipped: Network testing disabled (unset GIT_TEST_HTTPD to enable)" lines caught my eye, shrugged and thought that Dscho should better fix this, but then an hour later got suspicious, so looked up a recent Travis CI build, and... here we are.

I have to wonder how much longer it could have been remained unnoticed, if the Perforce filehost hadn't failed or if I hadn't made the gitgitgadget PR for my fix, or...

Anyway, this is just a long-winded way to say that I think we should validate those bools properly and error loudly on an invalid value even if it doesn't seem to worth it.

[1] https://public-inbox.org/git/20190906102711.6401-1-szeder.dev@gmail.com/T/