Re: [PATCH v2 4/6] ci: install Asciidoctor in 'ci/install-dependencies.sh'

From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
To: "SZEDER Gábor" <szeder.dev@gmail.com>
Cc: git@vger.kernel.org, "Jeff King" <peff@peff.net>,
	"Todd Zullinger" <tmz@pobox.com>,
	"Martin Ågren" <martin.agren@gmail.com>,
	"brian m. carlson" <sandals@crustytoothpaste.net>,
	"Junio C Hamano" <gitster@pobox.com>
Subject: Re: [PATCH v2 4/6] ci: install Asciidoctor in 'ci/install-dependencies.sh'
Date: Mon, 25 Mar 2019 22:28:21 +0100 (STD)	[thread overview]
Message-ID: <nycvar.QRO.7.76.6.1903252221300.41@tvgsbejvaqbjf.bet> (raw)
In-Reply-To: <20190324215534.9495-5-szeder.dev@gmail.com>

[-- Attachment #1: Type: text/plain, Size: 1947 bytes --]

Hi,

I like the rest of the patch series, but this patch, I am not so sure
about...

On Sun, 24 Mar 2019, SZEDER Gábor wrote:

> When our '.travis.yml' was split into several 'ci/*' scripts [1], the
> installation of the 'asciidoctor' gem somehow ended up in
> 'ci/test-documentation.sh'.
>
> Install it in 'ci/install-dependencies.sh', where we install
> everything else.

The big difference you introduce is that asciidoctor is now installed
with every job, not only with the Documentation job that actually uses it.

Even if it affects me very little (because I don't pay much attention to
Travis, it's been too flakey for me, and it does not test our Windows
side, and it is too slow), I'd rather install asciidoctor really only when
needed.

So I'd like to recommend to drop this patch from the series.

Thanks,
Dscho

>
> [1] 657343a602 (travis-ci: move Travis CI code into dedicated scripts,
>     2017-09-10)
>
> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
> ---
>  ci/install-dependencies.sh | 3 +++
>  ci/test-documentation.sh   | 3 ---
>  2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
> index d64667fcbf..76ec308965 100755
> --- a/ci/install-dependencies.sh
> +++ b/ci/install-dependencies.sh
> @@ -54,6 +54,9 @@ StaticAnalysis)
>  Documentation)
>  	sudo apt-get -q update
>  	sudo apt-get -q -y install asciidoc xmlto
> +
> +	test -n "$ALREADY_HAVE_ASCIIDOCTOR" ||
> +	gem install asciidoctor
>  	;;
>  esac
>
> diff --git a/ci/test-documentation.sh b/ci/test-documentation.sh
> index be3b7d376a..8f91f48c81 100755
> --- a/ci/test-documentation.sh
> +++ b/ci/test-documentation.sh
> @@ -5,9 +5,6 @@
>
>  . ${0%/*}/lib.sh
>
> -test -n "$ALREADY_HAVE_ASCIIDOCTOR" ||
> -gem install asciidoctor
> -
>  make check-builtins
>  make check-docs
>
> --
> 2.21.0.539.g07239c3a71.dirty
>
>