travis-ci: install packages in 'ci/install-dependencies.sh'
diff mbox series

Message ID 20181101114714.14710-1-szeder.dev@gmail.com
State New
Headers show
Series
  • travis-ci: install packages in 'ci/install-dependencies.sh'
Related show

Commit Message

SZEDER Gábor Nov. 1, 2018, 11:47 a.m. UTC
Ever since we started using Travis CI, we specified the list of
packages to install in '.travis.yml' via the APT addon.  While running
our builds on Travis CI's container-based infrastructure we didn't
have another choice, because that environment didn't support 'sudo',
and thus we didn't have permission to install packages ourselves.  With
the switch to the VM-based infrastructure in the previous patch we do
get a working 'sudo', so we can install packages by running 'sudo
apt-get -y install ...' as well.

Let's make use of this and install necessary packages in
'ci/install-dependencies.sh', so all the dependencies (i.e. both
packages and "non-packages" (P4 and Git-LFS)) are handled in the same
file.  Install gcc-8 only in the 'linux-gcc' build job; so far it has
been unnecessarily installed in the 'linux-clang' build job as well.
Print the versions of P4 and Git-LFS conditionally, i.e. only when
they have been installed; with this change even the static analysis
and documentation build jobs start using 'ci/install-dependencies.sh'
to install packages, and neither of these two build jobs depend on and
thus install those.

This change will presumably be beneficial for the upcoming Azure
Pipelines integration [1]: preliminary versions of that patch series
run a couple of 'apt-get' commands to install the necessary packages
before running 'ci/install-dependencies.sh', but with this patch it
will be sufficient to run only 'ci/install-dependencies.sh'.

[1] https://public-inbox.org/git/1a22efe849d6da79f2c639c62a1483361a130238.1539598316.git.gitgitgadget@gmail.com/

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---

This patch should go on top of 'ss/travis-ci-force-vm-mode'.

I'm not sure about the last paragraph, because:

  - It talks about presumed benefits for a currently still
    work-in-progress patch series of an other contributor, and I'm not
    really sure that that's a good thing.  Perhaps I should have
    rather put it below the '---'.

  - I'm confused about the name of this Azure thing.  The cover letter
    mentions "Azure Pipelines", the file is called
    'azure-pipelines.yml', but the relevant patch I link to talks
    about "Azure DevOps" in the commit message.

Anyway, keep that last paragraph or drop it as you see fit.


 .travis.yml                | 21 ---------------------
 ci/install-dependencies.sh | 35 +++++++++++++++++++++++++++++------
 2 files changed, 29 insertions(+), 27 deletions(-)

Comments

Junio C Hamano Nov. 2, 2018, 2:25 a.m. UTC | #1
SZEDER Gábor <szeder.dev@gmail.com> writes:

> Ever since we started using Travis CI, we specified the list of
> packages to install in '.travis.yml' via the APT addon.  While running
> our builds on Travis CI's container-based infrastructure we didn't
> have another choice, because that environment didn't support 'sudo',
> and thus we didn't have permission to install packages ourselves.  With
> the switch to the VM-based infrastructure in the previous patch we do
> get a working 'sudo', so we can install packages by running 'sudo
> apt-get -y install ...' as well.

OK, so far we learned that this is now _doable_; but not enough to
decide if this is a good thing to do or not.  Let's read on to find
out.

> Let's make use of this and install necessary packages in
> 'ci/install-dependencies.sh', so all the dependencies (i.e. both
> packages and "non-packages" (P4 and Git-LFS)) are handled in the same
> file.  

So we used to have two waysto prepare the test environment; non
packaged software were done via install-dependencies.sh, but
packaged ones weren't.  Unifying them so that the script installs
both would be a good change to simplify the procedure.  

Is that how this sentence argues for this change?

> Install gcc-8 only in the 'linux-gcc' build job; so far it has
> been unnecessarily installed in the 'linux-clang' build job as well.

Is this "unneeded gcc-8 was installed" something we can fix only
because we now stopped doing the installation via apt addon?  Or we
could have fixed it while we were on apt addon but we didn't bother,
and this patch fixes it "while at it"---primarily because the shell
script is far more flexible to work with than travis.yml matrix and
this kind of customization is far easier to do?

> Print the versions of P4 and Git-LFS conditionally, i.e. only when
> they have been installed; with this change even the static analysis
> and documentation build jobs start using 'ci/install-dependencies.sh'
> to install packages, and neither of these two build jobs depend on and
> thus install those.
>
> This change will presumably be beneficial for the upcoming Azure
> Pipelines integration [1]: preliminary versions of that patch series
> run a couple of 'apt-get' commands to install the necessary packages
> before running 'ci/install-dependencies.sh', but with this patch it
> will be sufficient to run only 'ci/install-dependencies.sh'.

So the main point of this change is to have less knowledge to
prepare the target configuration in the .travis.yml file and keep
them all in ci/install-dependencies.sh, which hopefully is more
reusable than .travis.yml in a non Travis environment?

If that is the case, it makes sense to me.

> This patch should go on top of 'ss/travis-ci-force-vm-mode'.
>
> I'm not sure about the last paragraph, because:
>
>   - It talks about presumed benefits for a currently still
>     work-in-progress patch series of an other contributor, and I'm not
>     really sure that that's a good thing.  Perhaps I should have
>     rather put it below the '---'.
>
>   - I'm confused about the name of this Azure thing.  The cover letter
>     mentions "Azure Pipelines", the file is called
>     'azure-pipelines.yml', but the relevant patch I link to talks
>     about "Azure DevOps" in the commit message.
>
> Anyway, keep that last paragraph or drop it as you see fit.

I hope we'll hear from Dscho in one or two revolutions of the Earth
;-)

> diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
> index 75a9fd2475..06c3546e1e 100755
> --- a/ci/install-dependencies.sh
> +++ b/ci/install-dependencies.sh
> @@ -10,6 +10,15 @@ LFSWHENCE=https://github.com/github/git-lfs/releases/download/v$LINUX_GIT_LFS_VE
>  
>  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 git-svn apache2
> +	case "$jobname" in
> +	linux-gcc)
> +		sudo apt-get -q -y install gcc-8
> +		;;
> +	esac
> +
>  	mkdir --parents "$P4_PATH"
>  	pushd "$P4_PATH"
>  		wget --quiet "$P4WHENCE/bin.linux26x86_64/p4d"
> @@ -32,11 +41,25 @@ osx-clang|osx-gcc)
>  	brew link --force gettext
>  	brew install caskroom/cask/perforce
>  	;;
> +StaticAnalysis)
> +	sudo apt-get -q update
> +	sudo apt-get -q -y install coccinelle
> +	;;
> +Documentation)
> +	sudo apt-get -q update
> +	sudo apt-get -q -y install asciidoc xmlto
> +	;;
>  esac
>  
> -echo "$(tput setaf 6)Perforce Server Version$(tput sgr0)"
> -p4d -V | grep Rev.
> -echo "$(tput setaf 6)Perforce Client Version$(tput sgr0)"
> -p4 -V | grep Rev.
> -echo "$(tput setaf 6)Git-LFS Version$(tput sgr0)"
> -git-lfs version
> +if type p4d >/dev/null && type p4 >/dev/null
> +then
> +	echo "$(tput setaf 6)Perforce Server Version$(tput sgr0)"
> +	p4d -V | grep Rev.
> +	echo "$(tput setaf 6)Perforce Client Version$(tput sgr0)"
> +	p4 -V | grep Rev.
> +fi
> +if type git-lfs >/dev/null
> +then
> +	echo "$(tput setaf 6)Git-LFS Version$(tput sgr0)"
> +	git-lfs version
> +fi
SZEDER Gábor Nov. 8, 2018, 9:51 p.m. UTC | #2
On Fri, Nov 02, 2018 at 11:25:17AM +0900, Junio C Hamano wrote:
> SZEDER Gábor <szeder.dev@gmail.com> writes:
> 
> > Ever since we started using Travis CI, we specified the list of
> > packages to install in '.travis.yml' via the APT addon.  While running
> > our builds on Travis CI's container-based infrastructure we didn't
> > have another choice, because that environment didn't support 'sudo',
> > and thus we didn't have permission to install packages ourselves.  With
> > the switch to the VM-based infrastructure in the previous patch we do
> > get a working 'sudo', so we can install packages by running 'sudo
> > apt-get -y install ...' as well.
> 
> OK, so far we learned that this is now _doable_; but not enough to
> decide if this is a good thing to do or not.  Let's read on to find
> out.

Yeah, this paragraph is just a bit of background about how the current
situation came to be and what recent change made the switch possible.

> > Let's make use of this and install necessary packages in
> > 'ci/install-dependencies.sh', so all the dependencies (i.e. both
> > packages and "non-packages" (P4 and Git-LFS)) are handled in the same
> > file.  
> 
> So we used to have two waysto prepare the test environment; non
> packaged software were done via install-dependencies.sh, but
> packaged ones weren't.  Unifying them so that the script installs
> both would be a good change to simplify the procedure.  
> 
> Is that how this sentence argues for this change?

Yes.

> > Install gcc-8 only in the 'linux-gcc' build job; so far it has
> > been unnecessarily installed in the 'linux-clang' build job as well.
> 
> Is this "unneeded gcc-8 was installed" something we can fix only
> because we now stopped doing the installation via apt addon?

Now that you mention it: no.  It would have been possible to install
gcc-8 only in the 'linux-gcc' build job even via the apt addon, namely
by removing the two Linux build jobs from the implicit build matrix
and adding them as two independent build jobs in the 'matrix.include'
section of '.travis.yml'.  The drawback is that all the extra packages
used in both build jobs would have to be duplicated.

> Or we
> could have fixed it while we were on apt addon but we didn't bother,
> and this patch fixes it "while at it"---primarily because the shell
> script is far more flexible to work with than travis.yml matrix and
> this kind of customization is far easier to do?

Basically yes (though I think it's not about not bothering; I don't
know about others, but it just occured to me that it would have been
doable, however, even if it occured to me earlier, because of the
duplicated list of common packages I wouldn't have done it).

Doing it in good old shell is indeed easier and the common packages
are then only listed once.

> > Print the versions of P4 and Git-LFS conditionally, i.e. only when
> > they have been installed; with this change even the static analysis
> > and documentation build jobs start using 'ci/install-dependencies.sh'
> > to install packages, and neither of these two build jobs depend on and
> > thus install those.
> >
> > This change will presumably be beneficial for the upcoming Azure
> > Pipelines integration [1]: preliminary versions of that patch series
> > run a couple of 'apt-get' commands to install the necessary packages
> > before running 'ci/install-dependencies.sh', but with this patch it
> > will be sufficient to run only 'ci/install-dependencies.sh'.
> 
> So the main point of this change is to have less knowledge to
> prepare the target configuration in the .travis.yml file and keep
> them all in ci/install-dependencies.sh, which hopefully is more
> reusable than .travis.yml in a non Travis environment?

Oh, "more reusable" indeed, that's a more eloquent way to put it.

> If that is the case, it makes sense to me.
> 
> > This patch should go on top of 'ss/travis-ci-force-vm-mode'.
> >
> > I'm not sure about the last paragraph, because:
> >
> >   - It talks about presumed benefits for a currently still
> >     work-in-progress patch series of an other contributor, and I'm not
> >     really sure that that's a good thing.  Perhaps I should have
> >     rather put it below the '---'.
> >
> >   - I'm confused about the name of this Azure thing.  The cover letter
> >     mentions "Azure Pipelines", the file is called
> >     'azure-pipelines.yml', but the relevant patch I link to talks
> >     about "Azure DevOps" in the commit message.
> >
> > Anyway, keep that last paragraph or drop it as you see fit.
> 
> I hope we'll hear from Dscho in one or two revolutions of the Earth
> ;-)

... revolutions around what? :)
Junio C Hamano Nov. 9, 2018, 2:59 a.m. UTC | #3
SZEDER Gábor <szeder.dev@gmail.com> writes:

>> > I'm not sure about the last paragraph, because:
>> >
>> >   - It talks about presumed benefits for a currently still
>> >     work-in-progress patch series of an other contributor, and I'm not
>> >     really sure that that's a good thing.  Perhaps I should have
>> >     rather put it below the '---'.
>> >
>> >   - I'm confused about the name of this Azure thing.  The cover letter
>> >     mentions "Azure Pipelines", the file is called
>> >     'azure-pipelines.yml', but the relevant patch I link to talks
>> >     about "Azure DevOps" in the commit message.
>> >
>> > Anyway, keep that last paragraph or drop it as you see fit.
>> 
>> I hope we'll hear from Dscho in one or two revolutions of the Earth
>> ;-)
>
> ... revolutions around what? :)

Originally I meant its own axis, but perhaps the moon.
Johannes Schindelin Nov. 9, 2018, 12:08 p.m. UTC | #4
Hi,

On Fri, 9 Nov 2018, Junio C Hamano wrote:

> SZEDER Gábor <szeder.dev@gmail.com> writes:
> 
> >> > I'm not sure about the last paragraph, because:
> >> >
> >> >   - It talks about presumed benefits for a currently still
> >> >     work-in-progress patch series of an other contributor, and I'm not
> >> >     really sure that that's a good thing.  Perhaps I should have
> >> >     rather put it below the '---'.
> >> >
> >> >   - I'm confused about the name of this Azure thing.  The cover letter
> >> >     mentions "Azure Pipelines", the file is called
> >> >     'azure-pipelines.yml', but the relevant patch I link to talks
> >> >     about "Azure DevOps" in the commit message.
> >> >
> >> > Anyway, keep that last paragraph or drop it as you see fit.
> >> 
> >> I hope we'll hear from Dscho in one or two revolutions of the Earth
> >> ;-)
> >
> > ... revolutions around what? :)
> 
> Originally I meant its own axis, but perhaps the moon.

I see, you had fun talking about a revolution [*1*]...

I am much in favor of this patch, as it indeed will simplify the
integration into Azure Pipelines.

As to the DevOps vs Pipelines thing: DevOps is the umbrella, it consists
of much more than just the automated builds (Pipelines), it also has
Boards, Packages, etc, but I don't think we will ever use that in the Git
project, as our workflow is based out of a mailing list.

Ciao,
Dscho


Footnote *1*: https://www.youtube.com/watch?v=Xv8FBjo1Y8I

Patch
diff mbox series

diff --git a/.travis.yml b/.travis.yml
index 8d2499739e..a5a82d6832 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -12,16 +12,6 @@  compiler:
   - clang
   - gcc
 
-addons:
-  apt:
-    sources:
-    - ubuntu-toolchain-r-test
-    packages:
-    - language-pack-is
-    - git-svn
-    - apache2
-    - gcc-8
-
 matrix:
   include:
     - env: jobname=GETTEXT_POISON
@@ -50,22 +40,11 @@  matrix:
     - env: jobname=StaticAnalysis
       os: linux
       compiler:
-      addons:
-        apt:
-          packages:
-          - coccinelle
-      before_install:
       script: ci/run-static-analysis.sh
       after_failure:
     - env: jobname=Documentation
       os: linux
       compiler:
-      addons:
-        apt:
-          packages:
-          - asciidoc
-          - xmlto
-      before_install:
       script: ci/test-documentation.sh
       after_failure:
 
diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
index 75a9fd2475..06c3546e1e 100755
--- a/ci/install-dependencies.sh
+++ b/ci/install-dependencies.sh
@@ -10,6 +10,15 @@  LFSWHENCE=https://github.com/github/git-lfs/releases/download/v$LINUX_GIT_LFS_VE
 
 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 git-svn apache2
+	case "$jobname" in
+	linux-gcc)
+		sudo apt-get -q -y install gcc-8
+		;;
+	esac
+
 	mkdir --parents "$P4_PATH"
 	pushd "$P4_PATH"
 		wget --quiet "$P4WHENCE/bin.linux26x86_64/p4d"
@@ -32,11 +41,25 @@  osx-clang|osx-gcc)
 	brew link --force gettext
 	brew install caskroom/cask/perforce
 	;;
+StaticAnalysis)
+	sudo apt-get -q update
+	sudo apt-get -q -y install coccinelle
+	;;
+Documentation)
+	sudo apt-get -q update
+	sudo apt-get -q -y install asciidoc xmlto
+	;;
 esac
 
-echo "$(tput setaf 6)Perforce Server Version$(tput sgr0)"
-p4d -V | grep Rev.
-echo "$(tput setaf 6)Perforce Client Version$(tput sgr0)"
-p4 -V | grep Rev.
-echo "$(tput setaf 6)Git-LFS Version$(tput sgr0)"
-git-lfs version
+if type p4d >/dev/null && type p4 >/dev/null
+then
+	echo "$(tput setaf 6)Perforce Server Version$(tput sgr0)"
+	p4d -V | grep Rev.
+	echo "$(tput setaf 6)Perforce Client Version$(tput sgr0)"
+	p4 -V | grep Rev.
+fi
+if type git-lfs >/dev/null
+then
+	echo "$(tput setaf 6)Git-LFS Version$(tput sgr0)"
+	git-lfs version
+fi