threads / patch / 47518

patch, 2 partsTravis CI: check unignored build artifacts

Subject: [PATCH 0/2] Travis CI: check unignored build artifacts

## tl;dr

7 messages between Dec 31, 2017 and Jan 3, 2018. Diffs are folded; open one to read it.

replies: 6people: 2as markdown or json

SZEDER Gábor· Dec 31, 2017, 16:02 UTC · lore

Every once in a while our explicit .gitignore files get out of sync when our build process learns to create new artifacts, but the .gitignore files are not updated accordingly. It was recently that we got a report about unignored test helper executables, see 44103f419 (t/helper: ignore everything but sources, 2017-12-12).

This short patch series teaches our Travis CI build scripts to detect unignored build artifacts at the end of builds, in the hope to catch these issues earlier.

These patches should go on top 'sg/travis-skip-identical-test'. The two patch series are conceptually independent, but would have a couple of conflicts when applied separately and then merged together, and I don't think it's worth carrying them in separate branches.

SZEDER Gábor (2):
  travis-ci: don't store P4 and Git LFS in the working tree
  travis-ci: check that all build artifacts are .gitignore-d
 ci/lib-travisci.sh       | 14 ++++++++++++--
 ci/run-linux32-docker.sh |  2 ++
 ci/run-tests.sh          |  2 ++
 ci/test-documentation.sh |  6 ++++++
 4 files changed, 22 insertions(+), 2 deletions(-)
-- 
2.16.0.rc0.67.g3a46dbca7
SZEDER Gábor· Dec 31, 2017, 16:02 UTC · re: SZEDER Gábor · lore

[PATCH 1/2] travis-ci: don't store P4 and Git LFS in the working tree

The Clang and GCC 64 bit Linux build jobs download and store the P4 and Git LFS executables under the current directory, which is the working tree that we are about to build and test. This means that Git commands like 'status' or 'ls-files' would list these files as untracked. The next commit is about to make sure that there are no untracked files present after the build, and the downloaded executables in the working tree are interfering with those upcoming checks.

Therefore, let's download P4 and Git LFS in the home directory, outside of the working tree.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 ci/lib-travisci.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Show changes to ci/lib-travisci.sh +2 −2
diff --git a/ci/lib-travisci.sh b/ci/lib-travisci.sh
index bade71617..1543b7959 100755
--- a/ci/lib-travisci.sh
+++ b/ci/lib-travisci.sh
@@ -99,8 +99,8 @@ linux-clang|linux-gcc)
 	export LINUX_P4_VERSION="16.2"
 	export LINUX_GIT_LFS_VERSION="1.5.2"
 
-	P4_PATH="$(pwd)/custom/p4"
-	GIT_LFS_PATH="$(pwd)/custom/git-lfs"
+	P4_PATH="$HOME/custom/p4"
+	GIT_LFS_PATH="$HOME/custom/git-lfs"
 	export PATH="$GIT_LFS_PATH:$P4_PATH:$PATH"
 	;;
 osx-clang|osx-gcc)
-- 
2.16.0.rc0.67.g3a46dbca7
Lars Schneider· Jan 2, 2018, 19:38 UTC · re: SZEDER Gábor · lore

Re: [PATCH 1/2] travis-ci: don't store P4 and Git LFS in the working tree

Show 13 quoted lines
> On 31 Dec 2017, at 17:02, SZEDER Gábor <szeder.dev@gmail.com> wrote:
> 
> The Clang and GCC 64 bit Linux build jobs download and store the P4
> and Git LFS executables under the current directory, which is the
> working tree that we are about to build and test.  This means that Git
> commands like 'status' or 'ls-files' would list these files as
> untracked.  The next commit is about to make sure that there are no
> untracked files present after the build, and the downloaded
> executables in the working tree are interfering with those upcoming
> checks.
> 
> Therefore, let's download P4 and Git LFS in the home directory,
> outside of the working tree.

I was concerned for a moment that the executables would not be available to the 32-bit build anymore... but we don't use them in that build anyways.

Looks good to me!
- Lars
Show 24 quoted lines
> 
> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
> ---
> ci/lib-travisci.sh | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/ci/lib-travisci.sh b/ci/lib-travisci.sh
> index bade71617..1543b7959 100755
> --- a/ci/lib-travisci.sh
> +++ b/ci/lib-travisci.sh
> @@ -99,8 +99,8 @@ linux-clang|linux-gcc)
> 	export LINUX_P4_VERSION="16.2"
> 	export LINUX_GIT_LFS_VERSION="1.5.2"
> 
> -	P4_PATH="$(pwd)/custom/p4"
> -	GIT_LFS_PATH="$(pwd)/custom/git-lfs"
> +	P4_PATH="$HOME/custom/p4"
> +	GIT_LFS_PATH="$HOME/custom/git-lfs"
> 	export PATH="$GIT_LFS_PATH:$P4_PATH:$PATH"
> 	;;
> osx-clang|osx-gcc)
> -- 
> 2.16.0.rc0.67.g3a46dbca7
> 
SZEDER Gábor· Dec 31, 2017, 16:02 UTC · re: SZEDER Gábor · lore

[PATCH 2/2] travis-ci: check that all build artifacts are .gitignore-d

Every once in a while our explicit .gitignore files get out of sync when our build process learns to create new artifacts, like test helper executables, but the .gitignore files are not updated accordingly.

Use Travis CI to help catch such issues earlier: check that there are no untracked files at the end of any build jobs building Git (i.e. the 64 bit Clang and GCC Linux and OSX build jobs, plus the GETTEXT_POISON and 32 bit Linux build jobs) or its documentation, and fail the build job if there are any present.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 ci/lib-travisci.sh       | 10 ++++++++++
 ci/run-linux32-docker.sh |  2 ++
 ci/run-tests.sh          |  2 ++
 ci/test-documentation.sh |  6 ++++++
 4 files changed, 20 insertions(+)
Show changes to 4 files +20 −0

ci/lib-travisci.sh, ci/run-linux32-docker.sh, ci/run-tests.sh, ci/test-documentation.sh

diff --git a/ci/lib-travisci.sh b/ci/lib-travisci.sh
index 1543b7959..07f27c727 100755
--- a/ci/lib-travisci.sh
+++ b/ci/lib-travisci.sh
@@ -67,6 +67,16 @@ skip_good_tree () {
 	exit 0
 }
 
+check_unignored_build_artifacts ()
+{
+	! git ls-files --other --exclude-standard --error-unmatch \
+		-- ':/*' 2>/dev/null ||
+	{
+		echo "$(tput setaf 1)error: found unignored build artifacts$(tput sgr0)"
+		false
+	}
+}
+
 # Set 'exit on error' for all CI scripts to let the caller know that
 # something went wrong.
 # Set tracing executed commands, primarily setting environment variables
diff --git a/ci/run-linux32-docker.sh b/ci/run-linux32-docker.sh
index 870a41246..4f191c5bb 100755
--- a/ci/run-linux32-docker.sh
+++ b/ci/run-linux32-docker.sh
@@ -23,4 +23,6 @@ docker run \
 	daald/ubuntu32:xenial \
 	/usr/src/git/ci/run-linux32-build.sh $(id -u $USER)
 
+check_unignored_build_artifacts
+
 save_good_tree
diff --git a/ci/run-tests.sh b/ci/run-tests.sh
index eb5ba4058..22355f009 100755
--- a/ci/run-tests.sh
+++ b/ci/run-tests.sh
@@ -8,4 +8,6 @@
 ln -s $HOME/travis-cache/.prove t/.prove
 make --quiet test
 
+check_unignored_build_artifacts
+
 save_good_tree
diff --git a/ci/test-documentation.sh b/ci/test-documentation.sh
index 3d62e6c95..a20de9ca1 100755
--- a/ci/test-documentation.sh
+++ b/ci/test-documentation.sh
@@ -18,6 +18,9 @@ test -s Documentation/git.xml
 test -s Documentation/git.1
 grep '<meta name="generator" content="AsciiDoc ' Documentation/git.html
 
+rm -f stdout.log stderr.log
+check_unignored_build_artifacts
+
 # Build docs with AsciiDoctor
 make clean
 make --jobs=2 USE_ASCIIDOCTOR=1 doc > >(tee stdout.log) 2> >(tee stderr.log >&2)
@@ -26,4 +29,7 @@ sed '/^GIT_VERSION = / d' stderr.log
 test -s Documentation/git.html
 grep '<meta name="generator" content="Asciidoctor ' Documentation/git.html
 
+rm -f stdout.log stderr.log
+check_unignored_build_artifacts
+
 save_good_tree
-- 
2.16.0.rc0.67.g3a46dbca7
Lars Schneider· Jan 2, 2018, 19:40 UTC · re: SZEDER Gábor · lore

Re: [PATCH 2/2] travis-ci: check that all build artifacts are .gitignore-d

Show 33 quoted lines
> On 31 Dec 2017, at 17:02, SZEDER Gábor <szeder.dev@gmail.com> wrote:
> 
> Every once in a while our explicit .gitignore files get out of sync
> when our build process learns to create new artifacts, like test
> helper executables, but the .gitignore files are not updated
> accordingly.
> 
> Use Travis CI to help catch such issues earlier: check that there are
> no untracked files at the end of any build jobs building Git (i.e. the
> 64 bit Clang and GCC Linux and OSX build jobs, plus the GETTEXT_POISON
> and 32 bit Linux build jobs) or its documentation, and fail the build
> job if there are any present.
> 
> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
> ---
> ci/lib-travisci.sh       | 10 ++++++++++
> ci/run-linux32-docker.sh |  2 ++
> ci/run-tests.sh          |  2 ++
> ci/test-documentation.sh |  6 ++++++
> 4 files changed, 20 insertions(+)
> 
> diff --git a/ci/lib-travisci.sh b/ci/lib-travisci.sh
> index 1543b7959..07f27c727 100755
> --- a/ci/lib-travisci.sh
> +++ b/ci/lib-travisci.sh
> @@ -67,6 +67,16 @@ skip_good_tree () {
> 	exit 0
> }
> 
> +check_unignored_build_artifacts ()
> +{
> +	! git ls-files --other --exclude-standard --error-unmatch \
> +		-- ':/*' 2>/dev/null ||
What does "-- ':/*'" do? Plus, why do you redirect stderr?
--
Both patches look good to me!

Thanks, Lars

SZEDER Gábor· Jan 2, 2018, 23:12 UTC · re: Lars Schneider · lore

Re: [PATCH 2/2] travis-ci: check that all build artifacts are .gitignore-d

On Tue, Jan 2, 2018 at 8:40 PM, Lars Schneider <larsxschneider@gmail.com> wrote:
Show 36 quoted lines
>
>> On 31 Dec 2017, at 17:02, SZEDER Gábor <szeder.dev@gmail.com> wrote:
>>
>> Every once in a while our explicit .gitignore files get out of sync
>> when our build process learns to create new artifacts, like test
>> helper executables, but the .gitignore files are not updated
>> accordingly.
>>
>> Use Travis CI to help catch such issues earlier: check that there are
>> no untracked files at the end of any build jobs building Git (i.e. the
>> 64 bit Clang and GCC Linux and OSX build jobs, plus the GETTEXT_POISON
>> and 32 bit Linux build jobs) or its documentation, and fail the build
>> job if there are any present.
>>
>> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
>> ---
>> ci/lib-travisci.sh       | 10 ++++++++++
>> ci/run-linux32-docker.sh |  2 ++
>> ci/run-tests.sh          |  2 ++
>> ci/test-documentation.sh |  6 ++++++
>> 4 files changed, 20 insertions(+)
>>
>> diff --git a/ci/lib-travisci.sh b/ci/lib-travisci.sh
>> index 1543b7959..07f27c727 100755
>> --- a/ci/lib-travisci.sh
>> +++ b/ci/lib-travisci.sh
>> @@ -67,6 +67,16 @@ skip_good_tree () {
>>       exit 0
>> }
>>
>> +check_unignored_build_artifacts ()
>> +{
>> +     ! git ls-files --other --exclude-standard --error-unmatch \
>> +             -- ':/*' 2>/dev/null ||
>
> What does "-- ':/*'" do?

It makes the whole thing work :) ':/*' is a pattern matching all paths, and '--others --exclude-standard' limit the command to list files that are both untracked and unignored. '--error-unmatch' causes the command to error out if any of the files given on the command line doesn't match anything in the working tree, in this case if there are no untracked-unignored files. Without a path given on the cmdline '--error-unmatch' has basically no effect[1].

In a clean worktree:
  $ git ls-files --other --exclude-standard --error-unmatch ; echo $?
  0
  $ git ls-files --other --exclude-standard --error-unmatch ':/*' ; echo $?
  error: pathspec ':/*' did not match any file(s) known to git.
  Did you forget to 'git add'?
  1

The disambiguating double-dash is not really necessary, because the :/* pattern can't be confused with any --options, but doesn't hurt, either.

> Plus, why do you redirect stderr?

To prevent the above error message from appearing in the trace log of a successful build.

[1] - Which makes me think whether we should consider '--error-unmatch'
      without any paths given on the command line as an error.
      On a related note: that error message doesn't seem to make much
      sense with '--other'...
Lars Schneider· Jan 3, 2018, 09:45 UTC · re: SZEDER Gábor · lore

Re: [PATCH 2/2] travis-ci: check that all build artifacts are .gitignore-d

Show 47 quoted lines
> On 03 Jan 2018, at 00:12, SZEDER Gábor <szeder.dev@gmail.com> wrote:
> 
> On Tue, Jan 2, 2018 at 8:40 PM, Lars Schneider <larsxschneider@gmail.com> wrote:
>> 
>>> On 31 Dec 2017, at 17:02, SZEDER Gábor <szeder.dev@gmail.com> wrote:
>>> 
>>> Every once in a while our explicit .gitignore files get out of sync
>>> when our build process learns to create new artifacts, like test
>>> helper executables, but the .gitignore files are not updated
>>> accordingly.
>>> 
>>> Use Travis CI to help catch such issues earlier: check that there are
>>> no untracked files at the end of any build jobs building Git (i.e. the
>>> 64 bit Clang and GCC Linux and OSX build jobs, plus the GETTEXT_POISON
>>> and 32 bit Linux build jobs) or its documentation, and fail the build
>>> job if there are any present.
>>> 
>>> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
>>> ---
>>> ci/lib-travisci.sh       | 10 ++++++++++
>>> ci/run-linux32-docker.sh |  2 ++
>>> ci/run-tests.sh          |  2 ++
>>> ci/test-documentation.sh |  6 ++++++
>>> 4 files changed, 20 insertions(+)
>>> 
>>> diff --git a/ci/lib-travisci.sh b/ci/lib-travisci.sh
>>> index 1543b7959..07f27c727 100755
>>> --- a/ci/lib-travisci.sh
>>> +++ b/ci/lib-travisci.sh
>>> @@ -67,6 +67,16 @@ skip_good_tree () {
>>>      exit 0
>>> }
>>> 
>>> +check_unignored_build_artifacts ()
>>> +{
>>> +     ! git ls-files --other --exclude-standard --error-unmatch \
>>> +             -- ':/*' 2>/dev/null ||
>> 
>> What does "-- ':/*'" do?
> 
> It makes the whole thing work :)
> ':/*' is a pattern matching all paths, and '--others --exclude-standard'
> limit the command to list files that are both untracked and unignored.
> '--error-unmatch' causes the command to error out if any of the files
> given on the command line doesn't match anything in the working tree,
> in this case if there are no untracked-unignored files.  Without a path
> given on the cmdline '--error-unmatch' has basically no effect[1].

That was the missing piece. Thank you! I've used the exact command before but I've never looked at the exit code. I just assumed that it would behave like grep if there is no match (as you described in [1])

Show 16 quoted lines
> In a clean worktree:
> 
>  $ git ls-files --other --exclude-standard --error-unmatch ; echo $?
>  0
>  $ git ls-files --other --exclude-standard --error-unmatch ':/*' ; echo $?
>  error: pathspec ':/*' did not match any file(s) known to git.
>  Did you forget to 'git add'?
>  1
> 
> The disambiguating double-dash is not really necessary, because the :/*
> pattern can't be confused with any --options, but doesn't hurt, either.
> 
>> Plus, why do you redirect stderr?
> 
> To prevent the above error message from appearing in the trace log of a
> successful build.
Makes sense. As I never specified the path, I did never see the error :-)
> [1] - Which makes me think whether we should consider '--error-unmatch'
>      without any paths given on the command line as an error.
Agreed!
- Lars

← back to recent threads