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

From: Lars Schneider <larsxschneider@gmail.com>
Date: Apr 25, 2016, 08:33 UTC
Message-ID: <EDD0757C-2BD3-476F-B703-147D35BB1FAC@gmail.com>
In-Reply-To: <xmqqinz9pl3l.fsf@gitster.mtv.corp.google.com>

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

Show 39 quoted lines

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

Show 6 quoted lines

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

Show 10 quoted lines

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

Show 8 quoted lines

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