On Fri, Oct 19, 2018 at 11:06:25AM +0900, Junio C Hamano wrote: > SZEDER Gábor <szeder.dev@xxxxxxxxx> writes: > > > On Mon, Oct 15, 2018 at 03:12:00AM -0700, Johannes Schindelin via GitGitGadget wrote: > >> diff --git a/ci/lib.sh b/ci/lib.sh > >> index 06970f7213..8532555b4e 100755 > >> --- a/ci/lib.sh > >> +++ b/ci/lib.sh > >> @@ -1,5 +1,26 @@ > >> # Library of functions shared by all CI scripts > >> > >> +if test true = "$TRAVIS" > >> +then > >> +... > >> + export GIT_PROVE_OPTS="--timer --jobs 3 --state=failed,slow,save" > >> + export GIT_TEST_OPTS="--verbose-log -x --immediate" > >> +fi > > > > Please set all these variables ... > > Do you mean "VAR=VAL; export VAR" is kosher, "export VAR=VAL" is > not? > > >> @@ -81,7 +102,6 @@ check_unignored_build_artifacts () > >> # and installing dependencies. > >> set -ex > > > > ... after we turn on 'set -x', so the variables' values will be > > visible in the logs. > > Ah, no, you didn't. Although I think both are valid points, I think > ci/lib.sh is expected to be used only inside a more predictable > environment (e.g. we know the shell used is not a random POSIX shell > but one that is happy with "export VAR=VAL"), so it should be OK. Yes. Travis CI runs an Ubuntu LTS, where /bin/sh is dash, which understands 'export VAR=val' just fine. I don't know what Linux distro runs on Azure Pipelines, but the build definition in patch 6 explicitly asks for Bash, so that should be fine as well. > Showing the values of these variables in the log may still be good > idea. > > > (Or move this 'set -ex' to the beginning of the script? Then we > > could perhaps avoid similar issues in the future.) > > Sure (provided that it is an issue to begin with---if we are > interested in the value of TRAVIS_BRANCH, for example, being able to > see it only because "CI_BRANCH=$TRAVIS_BRANCH" assignment is made > feels a bit sideways---we'd be better off explicitly logging > anything we are interested in in the longer term, no?). Well, all the $TRAVIS_* and $CI_* variables are not that interessant, because they are only used in the build scripts, and then we can see their substituted values in the build logs. The same applies to the variables $cache_dir and $BREW_INSTALL_PACKAGES as well. $GIT_PROVE_OPTS and $GIT_TEST_OPTS, however, are only used in 't/Makefile' but not in the build scripts, thus their values don't show up in the build logs. I run some Travis CI builds with custom $GIT_TEST_OPTS, which, of course, conflicted with this patch, and I messed up the conflict resolution. I think I would have noticed sooner what went wrong, if the value of $GIT_TEST_OPTS were visible in the build logs. I've found the output with 'set -x' sufficient so far, and don't think that an explicit logging facility is worth it.
