[PATCH v1] travis-ci: build documentation

[PATCH v1] travis-ci: build documentation

[larsxschneider]

From: Lars Schneider <larsxschneider@gmail.com>

Run "make doc" to check if all documentation can be build without errors.
Since the documentation is the same on every platform/compiler, the check
is only performed as part of the Linux/GCC build job to maintain a fast
CI process.

Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
---

Patch as promised in http://article.gmane.org/gmane.comp.version-control.git/291726

Cheers,
Lars

 .travis.yml | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/.travis.yml b/.travis.yml
index c3bf9c6..6ca7fb2 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -12,6 +12,8 @@ addons:
   apt:
     packages:
     - language-pack-is
+    - asciidoc
+    - xmlto

 env:
   global:
@@ -70,7 +72,16 @@ before_install:

 before_script: make --jobs=2

-script: make --quiet test
+script:
+  - >
+      make --quiet test &&
+      if [[ "$TRAVIS_OS_NAME" = linux ]] && [[ "$CC" = gcc ]];
+          then
+          echo ""
+          echo "------------------------------------------------------------------------" &&
+          echo "$(tput setaf 2)Building documentation...$(tput sgr0)" &&
+          make --quiet doc
+      fi;

 after_failure:
   - >
--
2.5.1

Re: [PATCH v1] travis-ci: build documentation

[Matthieu Moy]

larsxschneider@gmail.com writes:

> +      if [[ "$TRAVIS_OS_NAME" = linux ]] && [[ "$CC" = gcc ]];

[[ is a bashism, and doesn't bring anything here compared to the POSIX
[ ... ], or "test" which is prefered in Git's source code.

The ; or the newline is not needed either.

I'd write

if test "$TRAVIS_OS_NAME" = linux && test "$CC" = gcc; then

> +          then
> +          echo ""
> +          echo "------------------------------------------------------------------------" &&

I usualy avoid "echo <something-starting-with-dash>" as I'm not sure how
portable it is across variants of "echo". Maybe this one is portable
enough, I don't know. Perhaps printf, or cat << EOF ...?

> +          echo "$(tput setaf 2)Building documentation...$(tput sgr0)" &&
> +          make --quiet doc
> +      fi;

Nit: useless ;

I think it makes sense to do some lightweight checks after "make doc",
rather than just check the return code. For example, check that a few
generated files exist and are non-empty, like

test -s Documentation/git.html &&
test -s Documentation/git.1

Thanks,

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH v1] travis-ci: build documentation

[stefan.naewe]

Am 22.04.2016 um 10:34 schrieb larsxschneider@gmail.com:
> From: Lars Schneider <larsxschneider@gmail.com>
> 
> Run "make doc" to check if all documentation can be build without errors.

s/build/built/


> Since the documentation is the same on every platform/compiler, the check
> is only performed as part of the Linux/GCC build job to maintain a fast
> CI process.
> 
> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
> ---
> 
> Patch as promised in http://article.gmane.org/gmane.comp.version-control.git/291726
> 
> Cheers,
> Lars
> 
>  .travis.yml | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/.travis.yml b/.travis.yml
> index c3bf9c6..6ca7fb2 100644
> --- a/.travis.yml
> +++ b/.travis.yml
> @@ -12,6 +12,8 @@ addons:
>    apt:
>      packages:
>      - language-pack-is
> +    - asciidoc
> +    - xmlto
> 
>  env:
>    global:
> @@ -70,7 +72,16 @@ before_install:
> 
>  before_script: make --jobs=2
> 
> -script: make --quiet test
> +script:
> +  - >
> +      make --quiet test &&
> +      if [[ "$TRAVIS_OS_NAME" = linux ]] && [[ "$CC" = gcc ]];
> +          then
> +          echo ""
> +          echo "------------------------------------------------------------------------" &&
> +          echo "$(tput setaf 2)Building documentation...$(tput sgr0)" &&
> +          make --quiet doc
> +      fi;
> 
>  after_failure:
>    - >

Stefan
-- 
----------------------------------------------------------------
/dev/random says: The cat that ate the ball of yarn....had mittens!
python -c "print '73746566616e2e6e616577654061746c61732d656c656b74726f6e696b2e636f6d'.decode('hex')" 
GPG Key fingerprint = 2DF5 E01B 09C3 7501 BCA9  9666 829B 49C5 9221 27AF

Re: [PATCH v1] travis-ci: build documentation

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

> larsxschneider@gmail.com writes:
>
>> +      if [[ "$TRAVIS_OS_NAME" = linux ]] && [[ "$CC" = gcc ]];
>
> [[ is a bashism, and doesn't bring anything here compared to the POSIX
> [ ... ], or "test" which is prefered in Git's source code.
>
> The ; or the newline is not needed either.

Honestly, I didn't know that we were even trying to be pure POSIX,
avoid bashism or GNUism, or in general to follow our shell scripting
style in the scriptlet in the .travis.yml file.

While I feel fairly strongly about keeping the generic part generic,
I am actually OK with things that are known to be used in a specific
environment to be specific to that environment.

Having said all that, if we are not benefiting from using features
beyond POSIX, then by all means we should strive to be writing our
stuff in a portable way, as we do not have firm control over from
where and to where people cut and paste code snippets.

And I do think bashism [[ ... ]] is *NOT* buying anything in this
particular case, so I do agree with you that

	if test "$TRAVIS_OS_NAME" = linux && test "$CC" = gcc
        then
        	...

or even

	case "$TRAVIS_OS_NAME,$CC" in
        linux,gcc)
        	...

is what I would have written instead if I were writing this
conditional.

If we were to shoot for "be POSIX unless we can clearly benefit from
being bash/gnu/linux specific in bash/gnu/linux specific parts", the
existing scriptlets in .travis.yml file has a few things that may
need to be cleaned up already.  There are "mkdir --parents" (POSIX
spells it "-p"), "pushd/popd" and invocation of "tar" is very GNU
specific in the part that appears in the case arm for "linux".

There also are existing instances of "useless ;" that would want to
be cleaned up regardless of portability issues.

>> +          then
>> +          echo ""
>> +          echo "------------------------------------------------------------------------" &&
>
> I usualy avoid "echo <something-starting-with-dash>" as I'm not sure how
> portable it is across variants of "echo". Maybe this one is portable
> enough, I don't know. Perhaps printf, or cat << EOF ...?

Do you even need a long divider there?

> I think it makes sense to do some lightweight checks after "make doc",
> rather than just check the return code. For example, check that a few
> generated files exist and are non-empty, like
>
> test -s Documentation/git.html &&
> test -s Documentation/git.1

Yup, or the formatter does not give new/unknown warnings.

Thanks.

Re: [PATCH v1] travis-ci: build documentation

[Jeff King]

On Fri, Apr 22, 2016 at 10:34:02AM +0200, larsxschneider@gmail.com wrote:

> From: Lars Schneider <larsxschneider@gmail.com>
> 
> Run "make doc" to check if all documentation can be build without errors.
> Since the documentation is the same on every platform/compiler, the check
> is only performed as part of the Linux/GCC build job to maintain a fast
> CI process.

This does slow down the normal test results for linux/gcc, though. I
don't know very much about Travis, but is it possible to break out the
documentation build into its own test, with a separate build status from
the other runs?

-Peff

Re: [PATCH v1] travis-ci: build documentation

[Lars Schneider]

On 22 Apr 2016, at 11:07, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote:

> larsxschneider@gmail.com writes:
> 
>> +      if [[ "$TRAVIS_OS_NAME" = linux ]] && [[ "$CC" = gcc ]];
> 
> [[ is a bashism, and doesn't bring anything here compared to the POSIX
> [ ... ], or "test" which is prefered in Git's source code.
OK! Thanks for the hint.


> The ; or the newline is not needed either.
Unfortunately it seems to be required. Travis CI generates a shell script
out of the yml file and I think they don't respect newlines or something...


> 
> I'd write
> 
> if test "$TRAVIS_OS_NAME" = linux && test "$CC" = gcc; then
> 
>> +          then
>> +          echo ""
>> +          echo "------------------------------------------------------------------------" &&
> 
> I usualy avoid "echo <something-starting-with-dash>" as I'm not sure how
> portable it is across variants of "echo". Maybe this one is portable
> enough, I don't know. Perhaps printf, or cat << EOF ...?
I am curious. Do you have an example on what platform echo "-something" 
could go wrong? I wasn't aware of such an issue.


> 
>> +          echo "$(tput setaf 2)Building documentation...$(tput sgr0)" &&
>> +          make --quiet doc
>> +      fi;
> 
> Nit: useless ;
Again, seems to be required by the Travis yml converter.


> I think it makes sense to do some lightweight checks after "make doc",
> rather than just check the return code. For example, check that a few
> generated files exist and are non-empty, like
> 
> test -s Documentation/git.html &&
> test -s Documentation/git.1
Great idea. Added to v2! The && is not necessary as Travis runs this script with "set -e".

Thanks,
Lars

Re: [PATCH v1] travis-ci: build documentation

[Lars Schneider]

On 22 Apr 2016, at 12:45, stefan.naewe@atlas-elektronik.com wrote:

> Am 22.04.2016 um 10:34 schrieb larsxschneider@gmail.com:
>> From: Lars Schneider <larsxschneider@gmail.com>
>> 
>> Run "make doc" to check if all documentation can be build without errors.
> 
> s/build/built/

Fixed. Thank you!

- Lars

Re: [PATCH v1] travis-ci: build documentation

[Lars Schneider]

On 22 Apr 2016, at 20:14, Junio C Hamano <gitster@pobox.com> wrote:

> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
> 
>> larsxschneider@gmail.com writes:
>> 
>>> +      if [[ "$TRAVIS_OS_NAME" = linux ]] && [[ "$CC" = gcc ]];
>> 
>> [[ is a bashism, and doesn't bring anything here compared to the POSIX
>> [ ... ], or "test" which is prefered in Git's source code.
>> 
>> The ; or the newline is not needed either.
> 
> Honestly, I didn't know that we were even trying to be pure POSIX,
> avoid bashism or GNUism, or in general to follow our shell scripting
> style in the scriptlet in the .travis.yml file.
> 
> While I feel fairly strongly about keeping the generic part generic,
> I am actually OK with things that are known to be used in a specific
> environment to be specific to that environment.
> 
> Having said all that, if we are not benefiting from using features
> beyond POSIX, then by all means we should strive to be writing our
> stuff in a portable way, as we do not have firm control over from
> where and to where people cut and paste code snippets.
> 
> And I do think bashism [[ ... ]] is *NOT* buying anything in this
> particular case, so I do agree with you that
> 
> 	if test "$TRAVIS_OS_NAME" = linux && test "$CC" = gcc
>        then
>        	...
> 
> or even
> 
> 	case "$TRAVIS_OS_NAME,$CC" in
>        linux,gcc)
>        	...
> 
> is what I would have written instead if I were writing this
> conditional.
Oh, thanks! I didn't know that "case" can process two variables :-)


> If we were to shoot for "be POSIX unless we can clearly benefit from
> being bash/gnu/linux specific in bash/gnu/linux specific parts", the
> existing scriptlets in .travis.yml file has a few things that may
> need to be cleaned up already.  There are "mkdir --parents" (POSIX
> spells it "-p"), "pushd/popd" and invocation of "tar" is very GNU
> specific in the part that appears in the case arm for "linux".
All code in the .travis.yml is quite Travis CI specific and therefore
I think portability is not really an issue. However, I agree that
the .travis.yml should follow the Git coding guidelines for 
consistency.


> There also are existing instances of "useless ;" that would want to
> be cleaned up regardless of portability issues.
Unfortunately it seems to be required. Travis CI generates a shell script
out of the yml file and I think they don't respect newlines or something...


> 
>>> +          then
>>> +          echo ""
>>> +          echo "------------------------------------------------------------------------" &&
>> 
>> I usualy avoid "echo <something-starting-with-dash>" as I'm not sure how
>> portable it is across variants of "echo". Maybe this one is portable
>> enough, I don't know. Perhaps printf, or cat << EOF ...?
> 
> Do you even need a long divider there?
I thought it is nice as it generates a visual distinctive separation
in the Travis CI output. However, Peff suggested a dedicated Documentation 
build job which makes this separation obsolete.


>> I think it makes sense to do some lightweight checks after "make doc",
>> rather than just check the return code. For example, check that a few
>> generated files exist and are non-empty, like
>> 
>> test -s Documentation/git.html &&
>> test -s Documentation/git.1
> 
> Yup, or the formatter does not give new/unknown warnings.
What do you mean by "formatter does not give new/unknown warnings"?
Can you give me a hint what I could test here in addition?


Thanks,
Lars

Re: [PATCH v1] travis-ci: build documentation

[Lars Schneider]

On 23 Apr 2016, at 05:39, Jeff King <peff@peff.net> wrote:

> On Fri, Apr 22, 2016 at 10:34:02AM +0200, larsxschneider@gmail.com wrote:
> 
>> From: Lars Schneider <larsxschneider@gmail.com>
>> 
>> Run "make doc" to check if all documentation can be build without errors.
>> Since the documentation is the same on every platform/compiler, the check
>> is only performed as part of the Linux/GCC build job to maintain a fast
>> CI process.
> 
> This does slow down the normal test results for linux/gcc, though. I
> don't know very much about Travis, but is it possible to break out the
> documentation build into its own test, with a separate build status from
> the other runs?

Great idea. This is how it looks.

All jobs of a run:
https://travis-ci.org/larsxschneider/git/builds/125506781

The documentation job:
https://travis-ci.org/larsxschneider/git/jobs/125506786

Thanks,
Lars

Re: [PATCH v1] travis-ci: build documentation

[Matthieu Moy]

Junio C Hamano <gitster@pobox.com> writes:

> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
>
>> larsxschneider@gmail.com writes:
>>
>>> +      if [[ "$TRAVIS_OS_NAME" = linux ]] && [[ "$CC" = gcc ]];
>>
>> [[ is a bashism, and doesn't bring anything here compared to the POSIX
>> [ ... ], or "test" which is prefered in Git's source code.
>>
>> The ; or the newline is not needed either.
>
> Honestly, I didn't know that we were even trying to be pure POSIX,
> avoid bashism or GNUism, or in general to follow our shell scripting
> style in the scriptlet in the .travis.yml file.

I'm not implying that we should absolutely avoid non-POSIX constructs,
just that the conjuction "non-POSIX + not useful" made me prefer to
remain POSIX.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH v1] travis-ci: build documentation

[Matthieu Moy]

Lars Schneider <larsxschneider@gmail.com> writes:

> On 22 Apr 2016, at 11:07, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote:
>
>> The ; or the newline is not needed either.
> Unfortunately it seems to be required. Travis CI generates a shell script
> out of the yml file and I think they don't respect newlines or something...

OK.

>>> +          then
>>> +          echo ""
>>> +          echo "------------------------------------------------------------------------" &&
>> 
>> I usualy avoid "echo <something-starting-with-dash>" as I'm not sure how
>> portable it is across variants of "echo". Maybe this one is portable
>> enough, I don't know. Perhaps printf, or cat << EOF ...?
> I am curious. Do you have an example on what platform echo "-something" 
> could go wrong? I wasn't aware of such an issue.

echo can, but doesn't have to implement non-POSIX options. For example,

  $ dash -c 'echo -e foo'
  -e foo
  $ bash -c 'echo -e foo'
  foo

This is a good reason to avoid

  echo "$string"

if $string is a user-supplied string.

I don't know if any shell have issue with --------, but I wouldn't be
surprised if some implementation complained with "invalid option
---------".

But it is also likely that I'm the one being paranoid and your code is
fine ;-).

>> test -s Documentation/git.html &&
>> test -s Documentation/git.1
> Great idea. Added to v2! The && is not necessary as Travis runs this script with "set -e".

Maybe I'm doing too much Python, but "explicit is better than
implicit" ;-). I'd keep the && in case someone ever moves the code to a
script that doesn't have -e, but I'm really nit-picking here ;-).

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH v1] travis-ci: build documentation

[Junio C Hamano]

Lars Schneider <larsxschneider@gmail.com> writes:

>> There also are existing instances of "useless ;" that would want to
>> be cleaned up regardless of portability issues.
> Unfortunately it seems to be required. Travis CI generates a shell script
> out of the yml file and I think they don't respect newlines or something...

If they squash all the lines into a single long line before
executing, these semicolons do indeed become necessary (we have to
write a logical single line shell script in our Makefiles with ';',
and I'd imagine Travis's scriptlets are done similarly).

Thanks.

Re: [PATCH v1] travis-ci: build documentation

[Jeff King]

On Mon, Apr 25, 2016 at 10:35:24AM +0200, Lars Schneider wrote:

> > This does slow down the normal test results for linux/gcc, though. I
> > don't know very much about Travis, but is it possible to break out the
> > documentation build into its own test, with a separate build status from
> > the other runs?
> 
> Great idea. This is how it looks.
> 
> All jobs of a run:
> https://travis-ci.org/larsxschneider/git/builds/125506781
> 
> The documentation job:
> https://travis-ci.org/larsxschneider/git/jobs/125506786

Very cool. Thanks!

-Peff

Re: [PATCH v1] travis-ci: build documentation

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Lars Schneider <larsxschneider@gmail.com> writes:
>
>>> There also are existing instances of "useless ;" that would want to
>>> be cleaned up regardless of portability issues.
>> Unfortunately it seems to be required. Travis CI generates a shell script
>> out of the yml file and I think they don't respect newlines or something...
>
> If they squash all the lines into a single long line before
> executing, these semicolons do indeed become necessary (we have to
> write a logical single line shell script in our Makefiles with ';',
> and I'd imagine Travis's scriptlets are done similarly).
>
> Thanks.

... but the above does not quite explain it.  The newlines are
mostly honoured as logical end-of-line in existing .travis.yml e.g.
we do not see a semicolon before "pushd".

            case "${TRAVIS_OS_NAME:-linux}" in
            linux)
              mkdir --parents custom/p4
              pushd custom/p4
            ...
            esac;
            echo "$(tput setaf 6)Perforce Server Version$(tput sgr0)";
            ...

everything outside the big "case/esac" seems to have ';' in the
current incarnation of the script.

Puzzled...

Re: [PATCH v1] travis-ci: build documentation

[Matthieu Moy]

Junio C Hamano <gitster@pobox.com> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Lars Schneider <larsxschneider@gmail.com> writes:
>>
>>>> There also are existing instances of "useless ;" that would want to
>>>> be cleaned up regardless of portability issues.
>>> Unfortunately it seems to be required. Travis CI generates a shell script
>>> out of the yml file and I think they don't respect newlines or something...
>>
>> If they squash all the lines into a single long line before
>> executing, these semicolons do indeed become necessary (we have to
>> write a logical single line shell script in our Makefiles with ';',
>> and I'd imagine Travis's scriptlets are done similarly).

Actually, it's not Travis, but just the Yaml syntax:

  http://docs.ansible.com/ansible/YAMLSyntax.html
  
  Values can span multiple lines using | or >. Spanning multiple lines
  using a | will include the newlines. Using a > will ignore newlines;
  it’s used to make what would otherwise be a very long line easier to
  read and edit

So, a simpler solution would be to apply something like this:

--- a/.travis.yml
+++ b/.travis.yml
@@ -33,7 +33,7 @@ env:
     - GIT_SKIP_TESTS="t9810 t9816"
 
 before_install:
-  - >
+  - |
     case "${TRAVIS_OS_NAME:-linux}" in
     linux)
       mkdir --parents custom/p4
@@ -81,7 +81,7 @@ before_script: make --jobs=2
 script: make --quiet test
 
 after_failure:
-  - >
+  - |
     : '<-- Click here to see detailed test output!                                          
            ';                                                                              
     for TEST_EXIT in t/test-results/*.exit;
     do

(untested)

Actually, this may also be a motivation to move anything non-trivial out
of .travic.yml and to start using separate scripts (to avoid writting
shell within a Yaml syntax).

> ... but the above does not quite explain it.  The newlines are
> mostly honoured as logical end-of-line in existing .travis.yml e.g.
> we do not see a semicolon before "pushd".
>
>             case "${TRAVIS_OS_NAME:-linux}" in
>             linux)
>               mkdir --parents custom/p4
>               pushd custom/p4

I'm tempted to think that these lines create directories pushd/ and
custom/p4/, but that doesn't quite explain it either. There's more magic
somewhere ...

> Puzzled...

Likewise.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH v1] travis-ci: build documentation

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

> Actually, this may also be a motivation to move anything non-trivial out
> of .travic.yml and to start using separate scripts (to avoid writting
> shell within a Yaml syntax).

Excellent suggestion.  I do not mind if we added a new directory at
the top-level of the tree to hold helper scripts for CI (perhaps
call it ci/ not travis/ so that people may later add helper scripts
for other CI systems of their liking if they wanted to).