"Johannes Schindelin via GitGitGadget" <gitgitgadget@xxxxxxxxx>
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?
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?
> - 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).
> 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.
> @@ -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).
> @@ -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.
> +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.
