Re: [PATCH v1] travis-ci: build and test Git on Windows

[Junio C Hamano <gitster@pobox.com>]

Lars Schneider <larsxschneider@gmail.com> writes:

> Therefore, we did the following:
> * Johannes Schindelin set up a Visual Studio Team Services build
>   sponsored by Microsoft and made it accessible via an Azure Function
>   that speaks a super-simple API. We made TravisCI use this API to
>   trigger a build, wait until its completion, and print the build and
>   test results.
> * A Windows build and test run takes up to 3h and TravisCI has a timeout
>   after 50min for Open Source projects. Since the TravisCI job does not
>   use heavy CPU/memory/etc. resources, the friendly TravisCI folks
>   extended the job timeout for git/git to 3h.

The benefit is that Windows CI does not have to subscribe to the
GitHub repository to get notified (instead uses Travis as a relay
for update notification) and the result can be seen at the same
place as results on other platforms Travis natively support are
shown?  

Very nice.

> Things, that would need to be done:
> * Someone with write access to https://travis-ci.org/git/git would need
>   to add the secret token as "GFW_CI_TOKEN" variable in the TravisCI
>   repository setting [1]. Afterwards the build should just work.

We need to make sure this does not leak to the execution log of
Travis.

For example, in https://travis-ci.org/git/git/jobs/213616973, which
is a log of the documentation build for #1335.6 ran for the 'master'
branch, you can see "ci/test-documentation.sh" string appearing in
the log twice.  This comes from "script:" part, which is the same
mechanism this patch uses to invoke the new script with sekrit on
the command line.

I am expecting that no expansion of "$GFW_CI_TOKEN" will be shown in
the output, but I've seen an incident where an unsuspecting 'set -x'
or '$cmd -v' revealed something that shouldn't have been made
public.  I want to make sure we are sure that the command line this
patch adds does not get echoed with expansion to the log.

Is GFW_CI_TOKEN known to be safe without double-quote around it, by
the way?

> Things, that might need to be done:
> * The Windows box can only process a single build at a time. A second
>   Windows build would need to wait until the first finishes.

Perhaps instead of accumulating pending requests, perhaps we can
arrange so that Travis skips a build/test request that is not even
started yet for the same branch?  For branches that are never
rewound, a breakage in an earlier pushout would either show in a
later pushout of the same branch (if breakage is not fixed yet), or
doesn't (if the later pushout was to fix that breakage), and in
either case, it is not useful to test the earlier pushout when a
newer one is already available for testing.  For branches that are
constantly rewound, again, it is not useful to test the earlier
pushout when a newer one is already available for testing.

> diff --git a/ci/run-windows-build.sh b/ci/run-windows-build.sh
> new file mode 100755
> index 0000000000..324a9ea4e6
> --- /dev/null
> +++ b/ci/run-windows-build.sh
> @@ -0,0 +1,55 @@
> +#!/usr/bin/env bash

I know this is not a usual scripted Porcelain that must be usable by
all users, but I do not see anything that requires bash-ism in the
script.  Can we just do "#!/bin/sh", avoid bash-isms, and follow the
usual coding guidelines in general?

> +[ $# -eq 3 ] || (echo "Unexpected number of parameters" && exit 1)

i.e.e.g "test $# = 3" || ...

> +# Check if the $BUILD_ID contains a number
> +case $BUILD_ID in
> +	''|*[!0-9]*) echo $BUILD_ID && exit 1
> +esac

Too deep an indent of a case arm, i.e. align them with case/esac, like

        case $BUILD_ID in
        ''|*[!0-9]*) echo $BUILD_ID && exit 1
        esac

Thanks.