patchtravis-ci: fix running P4 and Git LFS tests in Linux build jobs

Linux build jobs on Travis CI skip the P4 and Git LFS tests since commit 657343a60 (travis-ci: move Travis CI code into dedicated scripts, 2017-09-10), claiming there are no P4 or Git LFS installed.

The reason is that P4 and Git LFS binaries are not installed to a directory in the default $PATH, but their directories are prepended to $PATH. This worked just fine before said commit, because $PATH was set in a scriptlet embedded in our '.travis.yml', thus its new value was visible during the rest of the build job. However, after these embedded scriptlets were moved into dedicated scripts executed in separate shell processes, any variable set in one of those scripts is only visible in that single script but not in any of the others. In this case, 'ci/install-dependencies.sh' downloads P4 and Git LFS and modifies $PATH, but to no effect, because 'ci/run-tests.sh' only sees Travis CI's default $PATH.

Move adjusting $PATH to 'ci/lib-travisci.sh', which is sourced in all other 'ci/' scripts, so all those scripts will see the updated $PATH value.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 ci/install-dependencies.sh | 10 ++++------
 ci/lib-travisci.sh         |  8 ++++++++
 2 files changed, 12 insertions(+), 6 deletions(-)

(Was: [PATCH] travis-ci: fix running P4 and Git LFS tests in Linux build jobs)

On Wed, Nov 1, 2017 at 12:55 PM, SZEDER Gábor <szeder.dev@gmail.com> wrote:

Astute readers ;) might have been wondering what's the deal with this environment variable in the patch context, since this won't have any effect outside of this script, either. Alas, it's not as easy as moving setting GIT_TEST_HTTPD to 'ci/lib-travisci.sh', like the quoted patch did with paths to P4 and Git LFS, because then it would be set for the 32 bit Linux build job which runs everything as root, thus can't run https tests.

This patch series deals with this by adding means that 'ci/*' scripts can use to identify which build job they are taking part in (patch 3), and then setting environment variables in 'ci/lib-travisci.sh', where we have now more freedom to set a specific variable only for specific build jobs (patches 3 and 4).

This patch series conflicts with the last patch in Thomas' split index fix series.

  https://public-inbox.org/git/20171210212202.28231-4-t.gummerer@gmail.com/

The conflict is not overly difficult, but to resolve it we should first come up with a reasonable job name for that build job, e.g. something like "misc-knobs" or whatever, because the combination "GETTEXT_POISON-SPLIT_INDEX" is just too long to exist and won't scale if we enable further knobs in the future.

SZEDER Gábor (4):
  travis-ci: use 'set -x' in 'ci/*' scripts for extra tracing output
  travis-ci: introduce a $jobname variable for 'ci/*' scripts
  travis-ci: move setting environment variables to 'ci/lib-travisci.sh'
  travis-ci: set GIT_TEST_HTTPD in 'ci/lib-travisci.sh'

 .travis.yml                | 26 +++++---------------------
 ci/install-dependencies.sh |  8 +++-----
 ci/lib-travisci.sh         | 34 +++++++++++++++++++++++++++++++---
 3 files changed, 39 insertions(+), 29 deletions(-)

While the build logic was embedded in our '.travis.yml', Travis CI used to produce a nice trace log including all commands executed in those embedded scriptlets. Since 657343a60 (travis-ci: move Travis CI code into dedicated scripts, 2017-09-10), however, we only see the name of the dedicated scripts, but not what those scripts are actually doing, resulting in a less useful trace log. A patch later in this series will move setting environment variables from '.travis.yml' to the 'ci/*' scripts, so not even those will be included in the trace log.

Use 'set -x' in 'ci/lib-travisci.sh', which is sourced in most other 'ci/*' scripts, so we get trace log about the commands executed in all of those scripts.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 ci/lib-travisci.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

I kind of did that intentionally to avoid clutter in the logs. However, I also agree with your reasoning. Therefore, the change looks good to me!

Thanks, Lars

On Tue, Dec 12, 2017 at 7:00 PM, Lars Schneider <larsxschneider@gmail.com> wrote:

Great, 'cause I'm starting to have second thoughts about this change :)

It sure helped a lot while I worked on this patch series and a couple of other Travis CI related patches (will submit them later)... OTOH it definitely creates clutter in the trace log. The worst offender might be 'ci/print-test-failures.sh', which iterates over all 't/test-results/*.exit' files to find which tests failed and to show their output, and 'set -x' makes every iteration visible. And we have about 800 tests, which means 800 iterations. Yuck.

Perhaps we should use other means to show what's going on instead, e.g. use more 'echo's and '--verbose' options, or just avoid using '--quiet'. And if some brave souls really want to tweak '.travis.yml' or the 'ci/*' scripts, then they can set 'set -x' for themselves during development... as the patch below shows it's easy enough, just a single character :)

Gábor

Hm... in that case. Would it be an option to "set -x" only in the header of "install-dependencies.sh"?

In "lib-travisci.sh" we could keep your "set -x" and execute "set +x" at the end of the file. Wouldn't that give us the interesting traces without much clutter (e.g. what is $PATH etc)?

- Lars

On Thu, Dec 14, 2017 at 12:10 AM, Lars Schneider <larsxschneider@gmail.com> wrote:

Hmm, that's an idea worth considering.

Scripts like 'run-build.sh', 'run-tests.sh' and 'run-static-analysis.sh' do basically nothing more than run make with different targets, so on one hand 'set -x' doesn't cause any clutter in the trace log, on the other hand there is no benefit from it either. 'run-linux32-docker.sh' runs docker (the command) twice, so it's basically in the same boat.

I think both 'lib-travisci.sh' and 'install-dependencies.sh' benefit from 'set -x'. So does 'test-documentation.sh': it executes about 15 commands, among them a bunch of 'test -s <file>' which fail quietly. With 'set -x' we would see the last executed command and know that that's the one that failed.

As mentioned above, 'print-test-failures.sh' definitely suffers from 'set -x'.

There is a lot going on in 'run-windows-build.sh', so the output of 'set -x' might be useful or might be considered too much clutter, I don't know. I put Dscho on Cc, I think it's mainly his call.

So it seems that there are more scripts that would benefit from tracing executed command using 'set -x' than scripts that would suffer because of it, and it doesn't matter for the rest. This means we could issue a 'set -x' in 'lib-travisci.sh' and disable it only in 'print-test-failures.sh'.

There is one thing that triggers my OCD: whenever we echo something, it ends up being duplicated in the trace log, e.g.:

  + echo foo bar baz
  foo bar baz

We could workaround it by writing 'echo "<msg>" >/dev/null', but it feels hackish and I'm not sure it's worth it.

Gábor

Hi,

Certainly it might be useful.

However, please make sure that the secret token is not leaked that way. Like, *really* sure. Due to the failure of Git to use a portable and performant test suite, it does take about 90 minutes to build and test a revision, therefore it would be very easy to DOS my build system, and I really, really need it not to be DOSed because I use it in my day job, too.

Ciao, Dscho

On Fri, Dec 15, 2017 at 1:10 PM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Ugh, I was completely unaware of this issue, and the first version of this patch is already in 'pu'... **runs to check the trace logs**

Great, it seems we are in luck, as the secret token was specified as an encrypted environment variable in git/git repository settings on Travis CI. It's redacted in the trace log and I only see lines like these:

  Setting environment variables from repository settings
  $ export GFW_CI_TOKEN=[secure]

  +test -z [secure]

  +++curl -H 'Authentication: Bearer [secure]' --silent --retry 5
--write-out '%{HTTP_CODE}' --output /dev/fd/63
'https://git-for-windows-ci.azurewebsites.net/api/TestNow?action=trigger&branch=pu&commit=1229713f78cd2883798e95f33c19c81b523413fd&skipTests=false'

  https://travis-ci.org/git/git/jobs/316791071

Phew.

However, I don't feel competent enough with Travis CI's encrypted environment variables to say that this qualifies as "*really* sure" "that the secret token is not leaked". Anyway, note, that that '$ export GFW_CI_TOKEN=[secure]' line is already present in all 'git/git' trace logs independently of this 'set -x' change in question.

Gábor

Hi Gábor,

On Fri, 15 Dec 2017, SZEDER Gábor wrote:

That's good enough.

Thanks for making sure, Dscho

A couple of 'ci/*' scripts are shared between different build jobs: 'ci/lib-travisci.sh', being a common library, is sourced from almost every script, while 'ci/install-dependencies.sh', 'ci/run-build.sh' and 'ci/run-tests.sh' are shared between the "regular" GCC and Clang Linux and OSX build jobs, and the latter two scripts are used in the GETTEXT_POISON Linux build job as well.

Our builds could benefit from these shared scripts being able to easily tell which build job they are taking part in. Now, it's already quite easy to tell apart Linux vs OSX and GCC vs Clang build jobs, but it gets trickier with all the additional Linux-based build jobs included explicitly in the build matrix.

Unfortunately, Travis CI doesn't provide much help in this regard. The closest we've got is the $TRAVIS_JOB_NUMBER variable, the value of which is two dot-separated integers, where the second integer indicates a particular build job. While it would be possible to use that second number to identify the build job in our shared scripts, it doesn't seem like a good idea to rely on that:

  - Though the build job numbering sequence seems to be stable so far,
    Travis CI's documentation doesn't explicitly states that it is
    indeed stable and will remain so in the future.  And even if it
    were stable,

  - if we were to remove or insert a build job in the middle, then the
    job numbers of all subsequent build jobs would change accordingly.

So roll our own means of simple build job identification and introduce the $jobname environment variable in our builds, setting it in the environments of the explicitly included jobs in '.travis.yml', while constructing one in 'ci/lib-travisci.sh' as the combination of the OS and compiler name for the GCC and Clang Linux and OSX build jobs. Use $jobname instead of $TRAVIS_OS_NAME in scripts taking different actions based on the OS and build job (when installing P4 and Git LFS dependencies and including them in $PATH). The following two patches will also rely on $jobname.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 .travis.yml                | 10 +++++-----
 ci/install-dependencies.sh |  6 +++---
 ci/lib-travisci.sh         |  9 +++++++--
 3 files changed, 15 insertions(+), 10 deletions(-)

Our '.travis.yml's 'env.global' section sets a bunch of environment variables for all build jobs, though none of them actually affects all build jobs. It's convenient for us, and in most cases it works just fine, because irrelevant environment variables are simply ignored.

However, $GIT_SKIP_TESTS is an exception: it tells the test harness to skip the two test scripts that are prone to occasional failures on OSX, but as it's set for all build jobs those tests are not run in any of the build jobs that are capable to run them reliably, either.

Therefore $GIT_SKIP_TESTS should only be set in the OSX build jobs, but those build jobs are included in the build matrix implicitly (i.e. by combining the matrix keys 'os' and 'compiler'), and there is no way to set an environment variable only for a subset of those implicit build jobs. (Unless we were to add new scriptlets to '.travis.yml', which is exactly the opposite direction that we took with commit 657343a60 (travis-ci: move Travis CI code into dedicated scripts, 2017-09-10)).

So move setting $GIT_SKIP_TESTS to 'ci/lib-travisci.sh', where it can trivially be set only for the OSX build jobs.

Furthermore, move setting all other environment variables from '.travis.yml' to 'ci/lib-travisci.sh', too, because a couple of environment variables are already set there, and this way all environment variables will be set in the same place. All the logic controlling our builds is already in the 'ci/*' scripts anyway, so there is really no good reason to keep the environment variables separately.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 .travis.yml        | 18 +-----------------
 ci/lib-travisci.sh | 21 +++++++++++++++++++++
 2 files changed, 22 insertions(+), 17 deletions(-)

Commit 657343a60 (travis-ci: move Travis CI code into dedicated scripts, 2017-09-10) converted '.travis.yml's default 'before_install' scriptlet to the 'ci/install-dependencies.sh' script, and while doing so moved setting GIT_TEST_HTTPD=YesPlease for the 64-bit GCC and Clang Linux build jobs to that script. This is wrong for two reasons:

 - The purpose of that script is, as its name suggests, to install
   dependencies, not to set any environment variables influencing
   which tests should be run (though, arguably, this was already an
   issue with the original 'before_install' scriptlet).

 - Setting the variable has no effect anymore, because that script is
   run in a separate shell process, and the variable won't be visible
   in any of the other scripts, notably in 'ci/run-tests.sh'
   responsible for, well, running the tests.

Luckily, this didn't have a negative effect on our Travis CI build jobs, because GIT_TEST_HTTPD is a tri-state variable defaulting to "auto" and a functioning web server was installed in those Linux build jobs, so the httpd tests were run anyway.

Apparently the httpd tests run just fine without GIT_TEST_HTTPD being set, therefore we could simply remove this environment variable. However, if a bug were to creep in to change the Travis CI build environment to run the tests as root or to not install Apache, then the httpd tests would be skipped and the build job would still succeed. We would only notice if someone actually were to look through the build job's trace log; but who would look at the trace log of a successful build job?!

Since httpd tests are important, we do want to run them and we want to be loudly reminded if they can't be run. Therefore, move setting GIT_TEST_HTTPD=YesPlease for the 64-bit GCC and Clang Linux build jobs to 'ci/lib-travisci.sh' to ensure that the build job fails when the httpd tests can't be run. (We could set it in 'ci/run-tests.sh' just as well, but it's better to keep all environment variables in one place in 'ci/lib-travisci.sh'.)

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 ci/install-dependencies.sh | 2 --
 ci/lib-travisci.sh         | 2 ++
 2 files changed, 2 insertions(+), 2 deletions(-)

This is a reroll of 'sg/travis-fixes'.

Changes since the previous round:

 - Patch 1 got updated following the discussion:

   - I went with enabling tracing executed commands everywhere,
     including the Windows build job, except where we know it causes way
     too much clutter, which is currently only
     'ci/print-test-failures.sh'.

   - Also enable this tracing in 'ci/run-linux32-build.sh', which
     doesn't source 'ci/lib-travisci.sh' as it's run inside a Docker
     container.

   - The commit message got updated accordingly, including a note about
     the Windows build job's secret token.
     I would like to get an Acked-by: from Dscho on this patch before it
     gets merged.

 - Patches 5-8 are new.  They are various fixes/cleanups unrelated to
   the Travis CI scriptification, but I don't think it's worth to have
   them in separate patch series.

SZEDER Gábor (8):
  travis-ci: use 'set -x' in select 'ci/*' scripts for extra tracing
  travis-ci: introduce a $jobname variable for 'ci/*' scripts
  travis-ci: move setting environment variables to 'ci/lib-travisci.sh'
  travis-ci: set GIT_TEST_HTTPD in 'ci/lib-travisci.sh'
  travis-ci: don't install default addon packages for the 32 bit Linux
    build
  travis-ci: don't install 'language-pack-is' package
  travis-ci: save prove state for the 32 bit Linux build
  travis-ci: only print test failures if there are test results
    available

 .travis.yml                | 28 ++++++----------------------
 ci/install-dependencies.sh |  8 +++-----
 ci/lib-travisci.sh         | 38 ++++++++++++++++++++++++++++++++++----
 ci/print-test-failures.sh  |  9 +++++++++
 ci/run-linux32-build.sh    |  3 +++
 ci/run-linux32-docker.sh   |  1 +
 6 files changed, 56 insertions(+), 31 deletions(-)

While the build logic was embedded in our '.travis.yml', Travis CI used to produce a nice trace log including all commands executed in those embedded scriptlets. Since 657343a60 (travis-ci: move Travis CI code into dedicated scripts, 2017-09-10), however, we only see the name of the dedicated scripts, but not what those scripts are actually doing, resulting in a less useful trace log about e.g. installing dependencies. A patch later in this series will move setting environment variables from '.travis.yml' to 'ci/lib-travisci.sh', so not even those will be included in the trace log. Unrelated to 657343a60, 'ci/test-documentation.sh' runs a bunch of 'test -s <file>' checks which would fail quietly if something were wrong, leaving no clue about which one of those checks triggered the failure.

Use 'set -x' in 'ci/lib-travisci.sh' to get more detailed trace log about the commands executed in the 'ci/*' scripts. Use it in 'ci/run-linux32-build.sh' as well, which is run in a Docker container and therefore doesn't source 'ci/lib-travisci.sh'. The secret token used for the Windows builds is specified as an encrypted environment variable in git/git repository settings on Travis CI and it's redacted in the trace logs even with 'set -x'. However, disable this tracing in 'ci/print-test-failures.sh', as it produces far too much noise in the output of that script.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 ci/lib-travisci.sh        | 6 ++++--
 ci/print-test-failures.sh | 3 +++
 ci/run-linux32-build.sh   | 2 ++
 3 files changed, 9 insertions(+), 2 deletions(-)

A couple of 'ci/*' scripts are shared between different build jobs: 'ci/lib-travisci.sh', being a common library, is sourced from almost every script, while 'ci/install-dependencies.sh', 'ci/run-build.sh' and 'ci/run-tests.sh' are shared between the "regular" GCC and Clang Linux and OSX build jobs, and the latter two scripts are used in the GETTEXT_POISON Linux build job as well.

Our builds could benefit from these shared scripts being able to easily tell which build job they are taking part in. Now, it's already quite easy to tell apart Linux vs OSX and GCC vs Clang build jobs, but it gets trickier with all the additional Linux-based build jobs included explicitly in the build matrix.

Unfortunately, Travis CI doesn't provide much help in this regard. The closest we've got is the $TRAVIS_JOB_NUMBER variable, the value of which is two dot-separated integers, where the second integer indicates a particular build job. While it would be possible to use that second number to identify the build job in our shared scripts, it doesn't seem like a good idea to rely on that:

  - Though the build job numbering sequence seems to be stable so far,
    Travis CI's documentation doesn't explicitly states that it is
    indeed stable and will remain so in the future.  And even if it
    were stable,

  - if we were to remove or insert a build job in the middle, then the
    job numbers of all subsequent build jobs would change accordingly.

So roll our own means of simple build job identification and introduce the $jobname environment variable in our builds, setting it in the environments of the explicitly included jobs in '.travis.yml', while constructing one in 'ci/lib-travisci.sh' as the combination of the OS and compiler name for the GCC and Clang Linux and OSX build jobs. Use $jobname instead of $TRAVIS_OS_NAME in scripts taking different actions based on the OS and build job (when installing P4 and Git LFS dependencies and including them in $PATH). The following two patches will also rely on $jobname.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 .travis.yml                | 10 +++++-----
 ci/install-dependencies.sh |  6 +++---
 ci/lib-travisci.sh         |  9 +++++++--
 3 files changed, 15 insertions(+), 10 deletions(-)

Our '.travis.yml's 'env.global' section sets a bunch of environment variables for all build jobs, though none of them actually affects all build jobs. It's convenient for us, and in most cases it works just fine, because irrelevant environment variables are simply ignored.

However, $GIT_SKIP_TESTS is an exception: it tells the test harness to skip the two test scripts that are prone to occasional failures on OSX, but as it's set for all build jobs those tests are not run in any of the build jobs that are capable to run them reliably, either.

Therefore $GIT_SKIP_TESTS should only be set in the OSX build jobs, but those build jobs are included in the build matrix implicitly (i.e. by combining the matrix keys 'os' and 'compiler'), and there is no way to set an environment variable only for a subset of those implicit build jobs. (Unless we were to add new scriptlets to '.travis.yml', which is exactly the opposite direction that we took with commit 657343a60 (travis-ci: move Travis CI code into dedicated scripts, 2017-09-10)).

So move setting $GIT_SKIP_TESTS to 'ci/lib-travisci.sh', where it can trivially be set only for the OSX build jobs.

Furthermore, move setting all other environment variables from '.travis.yml' to 'ci/lib-travisci.sh', too, because a couple of environment variables are already set there, and this way all environment variables will be set in the same place. All the logic controlling our builds is already in the 'ci/*' scripts anyway, so there is really no good reason to keep the environment variables separately.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 .travis.yml        | 18 +-----------------
 ci/lib-travisci.sh | 21 +++++++++++++++++++++
 2 files changed, 22 insertions(+), 17 deletions(-)

Commit 657343a60 (travis-ci: move Travis CI code into dedicated scripts, 2017-09-10) converted '.travis.yml's default 'before_install' scriptlet to the 'ci/install-dependencies.sh' script, and while doing so moved setting GIT_TEST_HTTPD=YesPlease for the 64-bit GCC and Clang Linux build jobs to that script. This is wrong for two reasons:

 - The purpose of that script is, as its name suggests, to install
   dependencies, not to set any environment variables influencing
   which tests should be run (though, arguably, this was already an
   issue with the original 'before_install' scriptlet).

 - Setting the variable has no effect anymore, because that script is
   run in a separate shell process, and the variable won't be visible
   in any of the other scripts, notably in 'ci/run-tests.sh'
   responsible for, well, running the tests.

Luckily, this didn't have a negative effect on our Travis CI build jobs, because GIT_TEST_HTTPD is a tri-state variable defaulting to "auto" and a functioning web server was installed in those Linux build jobs, so the httpd tests were run anyway.

Apparently the httpd tests run just fine without GIT_TEST_HTTPD being set, therefore we could simply remove this environment variable. However, if a bug were to creep in to change the Travis CI build environment to run the tests as root or to not install Apache, then the httpd tests would be skipped and the build job would still succeed. We would only notice if someone actually were to look through the build job's trace log; but who would look at the trace log of a successful build job?!

Since httpd tests are important, we do want to run them and we want to be loudly reminded if they can't be run. Therefore, move setting GIT_TEST_HTTPD=YesPlease for the 64-bit GCC and Clang Linux build jobs to 'ci/lib-travisci.sh' to ensure that the build job fails when the httpd tests can't be run. (We could set it in 'ci/run-tests.sh' just as well, but it's better to keep all environment variables in one place in 'ci/lib-travisci.sh'.)

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 ci/install-dependencies.sh | 2 --
 ci/lib-travisci.sh         | 2 ++
 2 files changed, 2 insertions(+), 2 deletions(-)

The 32 bit Linux build job compiles Git and runs the test suite in a Docker container, while the additional packages (apache2, git-svn, language-pack-is) are installed on the host, therefore don't have any effect and are unnecessary.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 .travis.yml | 1 +
 1 file changed, 1 insertion(+)

Ever since we have started to use Travis CI in 522354d70 (Add Travis CI support, 2015-11-27), our 64 bit Linux build jobs install the 'languate-pack-is' package. That commit doesn't discuss why it was deemed necessary back then, but Travis CI can build and test Git without that package just fine, even that commit introducing Travis CI support.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 .travis.yml | 1 -
 1 file changed, 1 deletion(-)

If I remember correctly then we had to install the Icelandic language pack for the i18n tests (e.g. t0204):

https://github.com/git/git/blob/master/t/lib-gettext.sh https://packages.ubuntu.com/trusty/language-pack-is

However, I checked your Travis-CI and I cannot spot any errors: https://travis-ci.org/szeder/git/jobs/317494789

I am a bit puzzled. Maybe the Icelandic language pack was not part of the Ubuntu image that was used when we introduced Travis CI?

The Ubuntu default image was 12.04 back then: https://travis-ci.org/git/git/jobs/100241871

Nowadays it is 14.04.

@Avar: Do you know what kind of errors we should expect if the language pack is not installed?

Thanks, Lars

On Mon, Dec 18, 2017 at 10:33 PM, Lars Schneider <larsxschneider@gmail.com> wrote:

Ah, now I start to understand. I was only looking for errors, too, but there are none, because the tests requiring the language pack are protected by the appropriate prerequisites. On my box, without and with the language pack:

  $ ./t0204-gettext-reencode-sanity.sh
  # lib-gettext: No is_IS UTF-8 locale available
  # lib-gettext: No is_IS ISO-8859-1 locale available
  ok 1 # skip gettext: Emitting UTF-8 from our UTF-8 *.mo files /
Icelandic (missing GETTEXT_LOCALE)
  ok 2 # skip gettext: Emitting UTF-8 from our UTF-8 *.mo files /
Runes (missing GETTEXT_LOCALE)
  ok 3 # skip gettext: Emitting ISO-8859-1 from our UTF-8 *.mo files /
Icelandic (missing GETTEXT_ISO_LOCALE)
  ok 4 # skip gettext: impossible ISO-8859-1 output (missing GETTEXT_ISO_LOCALE)
  ok 5 # skip gettext: Fetching a UTF-8 msgid -> UTF-8 (missing GETTEXT_LOCALE)
  ok 6 # skip gettext: Fetching a UTF-8 msgid -> ISO-8859-1 (missing
GETTEXT_ISO_LOCALE)
  ok 7 # skip gettext.c: git init UTF-8 -> UTF-8 (missing GETTEXT_LOCALE)
  ok 8 # skip gettext.c: git init UTF-8 -> ISO-8859-1 (missing
GETTEXT_ISO_LOCALE)
  # passed all 8 test(s)

  $ sudo apt-get install language-pack-is
  [...]
  $ ./t0204-gettext-reencode-sanity.sh
  # lib-gettext: Found 'is_IS.utf8' as an is_IS UTF-8 locale
  # lib-gettext: No is_IS ISO-8859-1 locale available
  ok 1 - gettext: Emitting UTF-8 from our UTF-8 *.mo files / Icelandic
  ok 2 - gettext: Emitting UTF-8 from our UTF-8 *.mo files / Runes
  ok 3 # skip gettext: Emitting ISO-8859-1 from our UTF-8 *.mo files /
Icelandic (missing GETTEXT_ISO_LOCALE)
  ok 4 # skip gettext: impossible ISO-8859-1 output (missing GETTEXT_ISO_LOCALE)
  ok 5 - gettext: Fetching a UTF-8 msgid -> UTF-8
  ok 6 # skip gettext: Fetching a UTF-8 msgid -> ISO-8859-1 (missing
GETTEXT_ISO_LOCALE)
  ok 7 - gettext.c: git init UTF-8 -> UTF-8
  ok 8 # skip gettext.c: git init UTF-8 -> ISO-8859-1 (missing
GETTEXT_ISO_LOCALE)
  # passed all 8 test(s)
  1..8

I'd expect something like this in the Travis CI build jobs as well, but prove hides the detailed output.

It seems we would loose coverage with this patch, so it should be dropped.

Ahh. That explains it!

Yeah. I think we should add a comment to the travis.yml to avoid future confusion. I'll do it unless you beat me to it with a re-roll.

- Lars

Lars Schneider <larsxschneider@gmail.com> writes:

Rather, it should be commented close to where is locale is used in tests, and possibly in t/README, I would think. Those who want to use t/*, not necessarily using Travis, would benefit from the hint, "installing icelandic locale may give you better test coverage", or something like that.

On Mon, Dec 18, 2017 at 11:04 PM, SZEDER Gábor <szeder.dev@gmail.com> wrote:

Not so fast!

Notice how there are still a few tests skipped above, because of missing GETTEXT_ISO_LOCALE.

  # grep is_IS /etc/locale.gen
  # is_IS ISO-8859-1
  # is_IS.UTF-8 UTF-8
  # sed -i -e 's/^# is_IS/is_IS/' /etc/locale.gen
  # locale-gen
  Generating locales (this might take a while)...
  [...]
  is_IS.ISO-8859-1... done
  is_IS.UTF-8... done
  Generation complete.

Both UTF-8 and ISO Icelandic locales are generated, good.

  $ $ ./t0204-gettext-reencode-sanity.sh
  # lib-gettext: Found 'is_IS.utf8' as an is_IS UTF-8 locale
  # lib-gettext: Found 'is_IS.iso88591' as an is_IS ISO-8859-1 locale
  ok 1 - gettext: Emitting UTF-8 from our UTF-8 *.mo files / Icelandic
  ok 2 - gettext: Emitting UTF-8 from our UTF-8 *.mo files / Runes
  ok 3 - gettext: Emitting ISO-8859-1 from our UTF-8 *.mo files / Icelandic
  ok 4 - gettext: impossible ISO-8859-1 output
  ok 5 - gettext: Fetching a UTF-8 msgid -> UTF-8
  ok 6 - gettext: Fetching a UTF-8 msgid -> ISO-8859-1
  ok 7 - gettext.c: git init UTF-8 -> UTF-8
  ok 8 - gettext.c: git init UTF-8 -> ISO-8859-1
  # passed all 8 test(s)

And now all those tests are run, great! But look what happens without the language pack:

  # dpkg -P language-pack-is{,-base}
  [...]
  # grep is_IS /etc/locale.gen
  is_IS ISO-8859-1
  is_IS.UTF-8 UTF-8
  buzz ~# locale-gen
  Generating locales (this might take a while)...
  [...]
  is_IS.ISO-8859-1... done
  is_IS.UTF-8... done
  Generation complete.

Still both Icelandic locales are generated.

  $ ./t0204-gettext-reencode-sanity.sh
  # lib-gettext: Found 'is_IS.utf8' as an is_IS UTF-8 locale
  # lib-gettext: Found 'is_IS.iso88591' as an is_IS ISO-8859-1 locale
  ok 1 - gettext: Emitting UTF-8 from our UTF-8 *.mo files / Icelandic
  ok 2 - gettext: Emitting UTF-8 from our UTF-8 *.mo files / Runes
  ok 3 - gettext: Emitting ISO-8859-1 from our UTF-8 *.mo files / Icelandic
  ok 4 - gettext: impossible ISO-8859-1 output
  ok 5 - gettext: Fetching a UTF-8 msgid -> UTF-8
  ok 6 - gettext: Fetching a UTF-8 msgid -> ISO-8859-1
  ok 7 - gettext.c: git init UTF-8 -> UTF-8
  ok 8 - gettext.c: git init UTF-8 -> ISO-8859-1
  # passed all 8 test(s)

And still all those tests are run!

I did run all test scripts sourcing 'lib-gettext.sh' with both locales generated and didn't see any errors, independently from whether the language pack was installed or not. I still have a few skipped tests (because of no GETTEXT_POISON and something PCRE-related), but those, too, are independent from the language pack.

So it seems that we don't need 'language-pack-is' after all; what we do need are the ISO and UTF-8 Icelandic locales. Not sure we can modify /etc/locale.gen without sudo in the Travis CI build job, though, and I've run out of time to try.

Gábor

This change follows suit of 6272ed319 (travis-ci: run previously failed tests first, then slowest to fastest, 2016-01-26), which did this for the Linux and OSX build jobs. Travis CI build jobs run the tests parallel, which is sligtly faster when tests are run in slowest to fastest order, shortening the overall runtime of this build job by about a minute / 10%.

Note, that the 32 bit Linux build job runs the tests suite in a Docker container and we have to share the Travis CI cache directory with the container as a second volume. Otherwise we couldn't use a symlink pointing to the prove state file in the cache directory, because that's outside of the directory hierarchy accessible from within the container.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 ci/run-linux32-build.sh  | 1 +
 ci/run-linux32-docker.sh | 1 +
 2 files changed, 2 insertions(+)

When a build job running the test suite fails, our 'ci/print-test-failures.sh' script scans all 't/test-results/*.exit' files to find failed tests and prints their verbose output. However, if a build job were to fail before it ever gets to run the test suite, then there will be no files to match the above pattern and the shell will take the pattern literally, resulting in errors like this in the trace log:

  cat: t/test-results/*.exit: No such file or directory
  ------------------------------------------------------------------------
  t/test-results/*.out...
  ------------------------------------------------------------------------
  cat: t/test-results/*.out: No such file or directory

Check upfront and proceed only if there are any such files present.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 ci/print-test-failures.sh | 6 ++++++
 1 file changed, 6 insertions(+)

On Sat, Dec 16, 2017 at 7:58 AM, SZEDER Gábor <szeder.dev@gmail.com> wrote:

Isn't the above going to cause 'test' to error out?

    $ mkdir -p t/test-results
    $ >t/test-results/a.exit
    $ >t/test-results/b.exit
    $ test t/test-results/*.exit = 't/test-results/*.exit'
    -bash: test: too many arguments

I'd think you'd want to capture the result of the expansion of t/test-result/*.exit as a string and compare that instead.

Indeed it does, though Travis CI's /bin/sh gives a different error message. Thanks for pointin it out, I didn't notice the error msg, because the script as a whole still did the right thing and then I didn't look close enough.

I'm not sure how the result of the expansion could be captured in the shell, because the shell performs the expansion only just before it executes a command. And if we do have to execute a command anyway, then we can simply rely on the command exiting with an error code upon not finding any files, and there's no need to capture the expansion or for the comparison for that matter.

So I propose this updated patch, using 'ls' instead of 'test':

  -- >8 --

Subject: [PATCH v2.1 8/8] travis-ci: only print test failures if there are test results available

When a build job running the test suite fails, our 'ci/print-test-failures.sh' script scans all 't/test-results/*.exit' files to find failed tests and prints their verbose output. However, if a build job were to fail before it ever gets to run the test suite, then there will be no files to match the above pattern and the shell will take the pattern literally, resulting in errors like this in the trace log:

  cat: t/test-results/*.exit: No such file or directory
  ------------------------------------------------------------------------
  t/test-results/*.out...
  ------------------------------------------------------------------------
  cat: t/test-results/*.out: No such file or directory

Check upfront and proceed only if there are any such files present.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 ci/print-test-failures.sh | 6 ++++++
 1 file changed, 6 insertions(+)

On Sat, Dec 16, 2017 at 5:48 PM, SZEDER Gábor <szeder.dev@gmail.com> wrote:

To capture the expansion as a string, I was thinking along the lines of:

    x=$(echo t/test-results/*.exit)

but that's a minor point. Checking the explicit exit code of a command, as you suggest, is probably cleaner.

Hi Gábor,

On Sat, 16 Dec 2017, SZEDER Gábor wrote:

ACK, Dscho

I ACK too soon in my other email ;-)

Can you disable the trace log for the loop in "run-windows-build.sh": https://github.com/git/git/blob/52015aaf9d19c97b52c47c7046058e6d029ff856/ci/run-windows-build.sh#L75-L88

Otherwise the Travis output look like this: https://travis-ci.org/git/git/jobs/316791071

- Lars

This cleanup series looks very good. ACK. Thank you, Gabor!

The is the only potential nit I see: https://public-inbox.org/git/8F53EF33-6FDA-484C-91A4-49CF24C0B417@gmail.com/

- Lars

These are the rest of the fixes that were in the second half of the v2 patch series but not already in 'sg/travis-fixes', which was already merged to next, perhaps a bit prematurely.

Patch 1/4 fixes some rough edges related to tracing executed commands according to the discussion. Patches 2/4, 3/4 are the same as patches 5/8 and 7/8 in v2, while patch 4/4 includes the small update I sent out as v2.1 of patch 8/8.

I dropped v2's patch 6/8 (travis-ci: don't install 'language-pack-is' package) for now: these locale-related things require more investigation.

v2: https://public-inbox.org/git/20171216125418.10743-1-szeder.dev@gmail.com/ v1: https://public-inbox.org/git/20171211233446.10596-1-szeder.dev@gmail.com/

SZEDER Gábor (4):
  travis-ci: fine tune the use of 'set -x' in 'ci/*' scripts
  travis-ci: don't install default addon packages for the 32 bit Linux
    build
  travis-ci: save prove state for the 32 bit Linux build
  travis-ci: only print test failures if there are test results
    available

 .travis.yml               | 1 +
 ci/lib-travisci.sh        | 4 +++-
 ci/print-test-failures.sh | 9 +++++++++
 ci/run-linux32-build.sh   | 3 +++
 ci/run-linux32-docker.sh  | 1 +
 ci/run-windows-build.sh   | 5 +++++
 6 files changed, 22 insertions(+), 1 deletion(-)

The change in commit 4f2636667 (travis-ci: use 'set -x' in 'ci/*' scripts for extra tracing output, 2017-12-12) left a couple of rough edges:

  - 'ci/run-linux32-build.sh' is executed in a Docker container and
    therefore doesn't source 'ci/lib-travisci.sh', which would enable
    tracing executed commands.  Enable 'set -x' in this script, too.

  - 'ci/print-test-failures.sh' iterates over all the files containing
    the exit codes of all the execued test scripts.  Since there are
    over 800 such files, the loop produces way too much noise with
    tracing executed commands enabled, so disable 'set -x' for this
    script.

  - 'ci/run-windows-build.sh' busily waits in a loop for the result of
    the Windows build, producing too much noise with tracing executed
    commands enabled as well.  Disable 'set -x' for the duration of
    that loop.

igned-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 ci/lib-travisci.sh        | 4 +++-
 ci/print-test-failures.sh | 3 +++
 ci/run-linux32-build.sh   | 2 ++
 ci/run-windows-build.sh   | 5 +++++
 4 files changed, 13 insertions(+), 1 deletion(-)

s/execued/executed/

Maybe:

  # something went wrong. Enable tracing to ease debugging on TravisCI.

I would move the "primarily setting environment ..." either to the top of the file or to the respective section below.

But this is a minor nit. The rest of the patch looks very good.

Thanks,
Lars
 

The 32 bit Linux build job compiles Git and runs the test suite in a Docker container, while the additional packages (apache2, git-svn, language-pack-is) are installed on the host, therefore don't have any effect and are unnecessary.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 .travis.yml | 1 +
 1 file changed, 1 insertion(+)

Nice optimization - ACK!

Thanks, Lars

This change follows suit of 6272ed319 (travis-ci: run previously failed tests first, then slowest to fastest, 2016-01-26), which did this for the Linux and OSX build jobs. Travis CI build jobs run the tests parallel, which is sligtly faster when tests are run in slowest to fastest order, shortening the overall runtime of this build job by about a minute / 10%.

Note, that the 32 bit Linux build job runs the tests suite in a Docker container and we have to share the Travis CI cache directory with the container as a second volume. Otherwise we couldn't use a symlink pointing to the prove state file in the cache directory, because that's outside of the directory hierarchy accessible from within the container.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 ci/run-linux32-build.sh  | 1 +
 ci/run-linux32-docker.sh | 1 +
 2 files changed, 2 insertions(+)

I assume "${HOME}/travis-cache:/usr/src/git/t/.prove" would not work because that would be a mapping in another mapping?

Thanks, Lars

On Wed, Dec 27, 2017 at 7:46 PM, Lars Schneider <larsxschneider@gmail.com> wrote:

't/.prove' is a file, but '.../travis-cache' is a directory. It must be, because Travis CI caches whole directories.

Gábor

Of course. Your solution is the right one.

Thanks, Lars

When a build job running the test suite fails, our 'ci/print-test-failures.sh' script scans all 't/test-results/*.exit' files to find failed tests and prints their verbose output. However, if a build job were to fail before it ever gets to run the test suite, then there will be no files to match the above pattern and the shell will take the pattern literally, resulting in errors like this in the trace log:

  cat: t/test-results/*.exit: No such file or directory
  ------------------------------------------------------------------------
  t/test-results/*.out...
  ------------------------------------------------------------------------
  cat: t/test-results/*.out: No such file or directory

Check upfront and proceed only if there are any such files present.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 ci/print-test-failures.sh | 6 ++++++
 1 file changed, 6 insertions(+)

Look good to me!

Minor nit: I am not 100% sure about the grammar but I am no native speaker and can't really tell.

Thanks, Lars