[v2,4/6] ci: install Asciidoctor in 'ci/install-dependencies.sh'
diff mbox series

Message ID 20190324215534.9495-5-szeder.dev@gmail.com
State New
Headers show
Series
  • Asciidoctor-related formatting and CI fixes
Related show

Commit Message

SZEDER Gábor March 24, 2019, 9:55 p.m. UTC
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.

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

Comments

Johannes Schindelin March 25, 2019, 9:28 p.m. UTC | #1
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
>
>
SZEDER Gábor March 25, 2019, 9:46 p.m. UTC | #2
On Mon, Mar 25, 2019 at 10:28:21PM +0100, Johannes Schindelin wrote:
> 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.

It is only installed in the 'Documentation' build job.

> 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
> >
> >
Johannes Schindelin March 26, 2019, 1:41 p.m. UTC | #3
Hi Gábor,

On Mon, 25 Mar 2019, SZEDER Gábor wrote:

> On Mon, Mar 25, 2019 at 10:28:21PM +0100, Johannes Schindelin wrote:
>
> > 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.
>
> It is only installed in the 'Documentation' build job.

Oy, you're right!

> > 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.
> >
> > [...]
> >
> > > 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)
>
>      ^^^^^^^^^^^^^^

To be honest, I think it was kind of easy to miss that. Maybe the commit
message could talk about this, so that future Dscho will have a helpful
hint when looking at the commit history and stumbling over this move?

If you could add that hint, I would be most grateful.

In any case, based on what you taught me I retract my objection to this
patch.

Thanks,
Dscho

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

Patch
diff mbox series

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