Re: [PATCH v4 03/21] ci/lib.sh: encapsulate Travis-specific things

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Junio,

On Wed, 23 Jan 2019, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
> 
> > diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
> > index fe65144152..bcdcc71592 100755
> > --- a/ci/install-dependencies.sh
> > +++ b/ci/install-dependencies.sh
> > @@ -37,7 +37,8 @@ osx-clang|osx-gcc)
> >  	brew update --quiet
> >  	# Uncomment this if you want to run perf tests:
> >  	# brew install gnu-time
> 
> Isn't this comment now stale?

No, not really. Actually, not at all.

> It would be, under this new arrangement of the code, most natural for
> Those who want to use gnu-time to arrange it to be somehow added to
> $BREW_INSTALL_PACKAGES, no?

The purpose of BREW_INSTALL_PACKAGES is to list the packages necessary to
build Git and run its test suite, and the only reason why this is no
longer a hard-coded list of packages is that it depends on the CI platform
(or more concretely, on the available macOS agents of said CI platform)
which packages need to be installed to do so.

The gnu-time package is not such a package, and it is unlikely to be
dependent on the particular CI you want to use.

So this spot really is the appropriate place to say that you want to
install gnu-time.

> > -	brew install git-lfs gettext
> > +	test -z "$BREW_INSTALL_PACKAGES" ||
> > +	brew install $BREW_INSTALL_PACKAGES
> 
> It is unclear how this relates to "encapsulate Travis-specific"; it
> is guessable that perhaps only under Travis we'd use
> BREW_INSTALL_PACKAGES, but it still is unexplained why under other
> CIs we don't do lfs or gettext (if that is the plan---that is not
> clear, either).

I added this paragraph to the commit message:

	Also, the configurations of the available agents are different. For
	example, Travis' and Azure Pipelines' macOS agents are set up
	differently, so that on Travis, we have to install the git-lfs and
	gettext Homebrew packages, and on Azure Pipelines we do not need to.

Does that sound good enough?

> >  	brew link --force gettext
> >  	brew install caskroom/cask/perforce
> >  	;;
> > diff --git a/ci/lib.sh b/ci/lib.sh
> > index c26bb6a274..4456dbbcb0 100755
> > --- a/ci/lib.sh
> > +++ b/ci/lib.sh
> > @@ -1,8 +1,26 @@
> >  # Library of functions shared by all CI scripts
> >  
> > -# When building a PR, TRAVIS_BRANCH refers to the *target* branch. Not what we
> > -# want here. We want the source branch instead.
> > -CI_BRANCH="${TRAVIS_PULL_REQUEST_BRANCH:-$TRAVIS_BRANCH}"
> > +if test true = "$TRAVIS"
> > +then
> > +	# When building a PR, TRAVIS_BRANCH refers to the *target* branch. Not
> > +	# what we want here. We want the source branch instead.
> > +	CI_BRANCH="${TRAVIS_PULL_REQUEST_BRANCH:-$TRAVIS_BRANCH}"
> > +	CI_COMMIT="$TRAVIS_COMMIT"
> > +	CI_JOB_ID="$TRAVIS_JOB_ID"
> > +	CI_JOB_NUMBER="$TRAVIS_JOB_NUMBER"
> > +	CI_OS_NAME="$TRAVIS_OS_NAME"
> > +	CI_REPO_SLUG="$TRAVIS_REPO_SLUG"
> > +
> > +	cache_dir="$HOME/travis-cache"
> > +
> > +	url_for_job_id () {
> > +		echo "https://travis-ci.org/$CI_REPO_SLUG/jobs/$1"
> > +	}
> > +
> > +	BREW_INSTALL_PACKAGES="git-lfs gettext"
> > +	export GIT_PROVE_OPTS="--timer --jobs 3 --state=failed,slow,save"
> > +	export GIT_TEST_OPTS="--verbose-log -x --immediate"
> > +fi
> 
> OK.  I actually was expecting this to be something like
> 
> 	case "$CI_TYPE" in
> 	Travis) ... the reindented travis specific stuff here ... ;;
> 	*) echo >&2 "unknown CI_TYPE: $CI_TYPE"; exit 1 ;;
> 	esac
> 
> to make it equally easy to add not just the second one but the third
> one.

That's a very valid point.

There is not actually a CI_TYPE available, so I still use an `if`
construct, but I added an `else` that will error out if we could not
identify the CI platform.

> > @@ -28,7 +46,7 @@ skip_branch_tip_with_tag () {
> >  # job if we encounter the same tree again and can provide a useful info
> >  # message.
> >  save_good_tree () {
> > -	echo "$(git rev-parse $TRAVIS_COMMIT^{tree}) $TRAVIS_COMMIT $TRAVIS_JOB_NUMBER $TRAVIS_JOB_ID" >>"$good_trees_file"
> > +	echo "$(git rev-parse $CI_COMMIT^{tree}) $CI_COMMIT $CI_JOB_NUMBER $CI_JOB_ID" >>"$good_trees_file"
> 
> Makes sense.  $CI_COMMIT is still coming from $TRAVIS_COMMIT but is
> that different from "git rev-parse $CI_BRANCH" I wonder (thinking
> aloud, not suggesting any changes).

Actually, I found out the hard way (a succeeding build that was a false
negative) that `TRAVIS_COMMIT` does, at least sometimes, refer to the
*merge* commit merging e.g. `pu` into `master` (which does not make sense
at all to me, because that's neither what you did nor what you asked
Travis to do for you).

So no, we need to use `TRAVIS_COMMIT` here lest we record a tree that was
not tested.

> > @@ -38,7 +56,7 @@ save_good_tree () {
> > ...
> > diff --git a/ci/print-test-failures.sh b/ci/print-test-failures.sh
> > index 7aef39a2fd..d2045b63a6 100755
> > --- a/ci/print-test-failures.sh
> > +++ b/ci/print-test-failures.sh
> > @@ -69,7 +69,7 @@ do
> >  	fi
> >  done
> >  
> > -if [ $combined_trash_size -gt 0 ]
> > +if [ -n "$TRAVIS_JOB_ID" -a $combined_trash_size -gt 0 ]
> 
> I do not quite understand this change.
> 
> If this wants to say that "we know how to show failures only when we
> know the job id", then testing for $CI_JOB_ID would be more
> appropriate, and if this wants to say that "we know how to show
> failures only when we are running under travis", then it would be
> more appropriate to switch on test "$TRAVIS" = true that is used at
> the beginning of ci/lib.sh (or if you would switch to "case $CI_TYPE"
> there, use the same construct).  I cannot quite make an argument to
> support the use of $TRAVIS_JOB_ID in either case.

I tried to be cheap and *not* introduce that CI_TYPE variable. But you are
correct, it is the right thing to do.

> > +test -n "$ALREADY_HAVE_ASCIIDOCTOR" ||
> >  gem install asciidoctor
> >  
> >  make check-builtins
> 
> OK, BREW_INSTALL_PACKAGES has an assignment in the travis specific
> section to trigger install of two separate packages, and there is no
> assignment to ALREADY_HAVE_ASCIIDOCTOR in the travis specific
> codepath, so we will end up installing asciidoctor here.  Makes
> sense, even though this one too is under-explained just like the
> other one is.

Hopefully the added paragraph (which says "for example" and then uses the
Homebrew packages as example) is enough to make the reader not puzzled
about this here change, too?

Ciao,
Dscho