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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Mar 25, 2019, 21:28 UTC
Message-ID: <nycvar.QRO.7.76.6.1903252221300.41@tvgsbejvaqbjf.bet>
In-Reply-To: <20190324215534.9495-5-szeder.dev@gmail.com>

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:

Show 6 quoted lines

> 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

Show 42 quoted lines

>
> [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
>
>