Re: [PATCH v4 05/12] ci: explicit install all required packages

From: Danh Doan <congdanhqx@gmail.com>
To: "SZEDER Gábor" <szeder.dev@gmail.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH v4 05/12] ci: explicit install all required packages
Date: Fri, 10 Apr 2020 23:07:41 +0700	[thread overview]
Message-ID: <20200410160741.GA12360@danh.dev> (raw)
In-Reply-To: <20200410155322.GN2224@szeder.dev>

On 2020-04-10 17:53:22+0200, SZEDER Gábor <szeder.dev@gmail.com> wrote:
> On Wed, Apr 08, 2020 at 11:05:36AM +0700, Đoàn Trần Công Danh wrote:
> > In a later patch, we will support GitHub Action.
> > 
> > Explicitly install all of our build dependencies.
> 
> ... on Linux.  This patch doesn't touch the parts installing
> dependencies in the osx jobs, but we do rely on some packages being
> installed by default in the osx images we use.  This is worth
> clarifying in the commit message, and in its subject line.

Fair enough.

> > Since GitHub Action VM hasn't install our build dependencies, yet.
> 
> s/install/installed/

> > +UBUNTU_COMMON_PKGS="make libssl-dev libcurl4-openssl-dev libexpat-dev
> > + perl-modules liberror-perl tcl tk gettext zlib1g-dev apache2
> > + libauthen-sasl-perl libemail-valid-perl libio-socket-ssl-perl
> > + libnet-smtp-ssl-perl"
> 
> I note that this list includes 'make' and 'apache2'.

I'll remove apache2.

> >  case "$jobname" in
> >  linux-clang|linux-gcc)
> >  	sudo apt-add-repository -y "ppa:ubuntu-toolchain-r/test"
> >  	sudo apt-get -q update
> > -	sudo apt-get -q -y install language-pack-is libsvn-perl apache2
> > +	sudo apt-get -q -y install language-pack-is libsvn-perl apache2 \
> 
> 'apache2' is listed here again.
> 
> > +		$UBUNTU_COMMON_PKGS
> >  	case "$jobname" in
> >  	linux-gcc)
> >  		sudo apt-get -q -y install gcc-8
> > @@ -63,11 +68,16 @@ StaticAnalysis)
> >  	;;
> >  Documentation)
> >  	sudo apt-get -q update
> > -	sudo apt-get -q -y install asciidoc xmlto docbook-xsl-ns
> > +	sudo apt-get -q -y install asciidoc xmlto docbook-xsl-ns \
> > +		libcurl4-openssl-dev
> 
> Does the Documentation job really need the 'libcurl4-openssl-dev'
> package?  FWIW, I just removed this package from my system, and 'make
> doc' still succeeded.

At the time of writing this series.
pu requires `curl-config` for Documentation jobs.

Skimming over the mail archive, Peff has sent a patch to fix it.
I haven't checked again, though.

> Furthermore, this doesn't install 'make', though in other jobs it is
> installed explicitly.  Note that the StaticAnalysis job requires
> 'make' as well.

I copied them from Azure Pipelines declaration.
I think it's better to list make explicitly in all jobs.

> Also note that we have a 'linux-gcc-4.8' job as well...

Will do in the re-roll.

> > +GETTEXT_POISON)
> > +	sudo apt-get -q update
> > +	sudo apt-get -q -y install $UBUNTU_COMMON_PKGS
> 
> The GETTEXT_POISON job currently doesn't install 'apache2', but with
> this change it will.  If this change is intentional, then please
> justify it in the commit message.  But I think that we shouldn't
> include 'apache2' in $UBUNTU_COMMON_PKGS.

No, this patch shouldn't change it.
I will remove apache2 from $UBUNTU_COMMON_PKGS

-- 
Danh