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

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

[SZEDER Gábor]

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

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

[SZEDER Gábor]

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

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

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

[Lars Schneider]

> 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

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

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

[SZEDER Gábor]

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

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

[Lars Schneider]

> 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

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

[SZEDER Gábor]

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

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

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

[Lars Schneider]

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


> 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