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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 22, 2017, 19:29 UTC
Message-ID: <xmqqwpbhjej6.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170322065612.18797-1-larsxschneider@gmail.com>

Lars Schneider <larsxschneider@gmail.com> writes:

Show 10 quoted lines

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

Show 7 quoted lines

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