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

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

[Lars Schneider]

Most Git developers work on Linux and they have no way to know if their
changes would break the Git for Windows build. Let's fix that by adding
a job to TravisCI that builds and tests Git on Windows. Unfortunately,
TravisCI does not support Windows.

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.

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.

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. This
  waiting time and the build time after the wait could exceed the 3h
  threshold. If this is a problem, then it is likely to happen every day
  as usually multiple branches are pushed at the same time (pu/next/
  master/maint). I cannot test this as my TravisCI account has the 50min
  timeout. One solution could be to limit the number of concurrent
  TravisCI jobs [2].

[1] https://docs.travis-ci.com/user/environment-variables#Defining-Variables-in-Repository-Settings
[2] https://docs.travis-ci.com/user/customizing-the-build#Limiting-Concurrent-Builds

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

Notes:
    Base Ref: master
    Web-Diff: https://github.com/larsxschneider/git/commit/322094c0a2
    Checkout: git fetch https://github.com/larsxschneider/git travisci/win-v1 && git checkout 322094c0a2

 .travis.yml             | 11 ++++++++++
 ci/run-windows-build.sh | 55 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 66 insertions(+)
 create mode 100755 ci/run-windows-build.sh

diff --git a/.travis.yml b/.travis.yml
index 591cc57b80..a7e98ae519 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -39,6 +39,17 @@ env:
 
 matrix:
   include:
+    - env: Windows
+      os: linux
+      compiler:
+      addons:
+      before_install:
+      before_script:
+      script:
+        - >
+          test "$TRAVIS_REPO_SLUG" != "git/git" ||
+          ci/run-windows-build.sh $GFW_CI_TOKEN $TRAVIS_BRANCH $(git rev-parse HEAD)
+      after_failure:
     - env: Linux32
       os: linux
       services:
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
+#
+# Script to trigger the a Git for Windows build and test run.
+# Pass a token, the branch (only branches on https://github.com/git/git)
+# are supported), and a commit hash.
+#
+
+[ $# -eq 3 ] || (echo "Unexpected number of parameters" && exit 1)
+
+TOKEN=$1
+BRANCH=$2
+COMMIT=$3
+
+gfwci () {
+	curl \
+		-H "Authentication: Bearer $TOKEN" \
+		--silent --retry 5 \
+		"https://git-for-windows-ci.azurewebsites.net/api/TestNow?$1" |
+	sed "$(printf '1s/^\xef\xbb\xbf//')"  # Remove the Byte Order Mark
+}
+
+# Trigger build job
+BUILD_ID=$(gfwci "action=trigger&branch=$BRANCH&commit=$COMMIT&skipTests=false")
+
+# Check if the $BUILD_ID contains a number
+case $BUILD_ID in
+	''|*[!0-9]*) echo $BUILD_ID && exit 1
+esac
+
+echo "Visual Studio Team Services Build #${BUILD_ID}"
+
+# Wait until build job finished
+STATUS=
+RESULT=
+while true
+do
+	LAST_STATUS=$STATUS
+	STATUS=$(gfwci "action=status&buildId=$BUILD_ID")
+	[ "$STATUS" == "$LAST_STATUS" ] || printf "\nStatus: $STATUS "
+	printf "."
+
+	case $STATUS in
+		inProgress|postponed|notStarted) sleep 10                      ;; # continue
+		         "completed: succeeded") RESULT="success";        break;; # success
+		                              *) echo "Unknown: $STATUS"; break;; # failure
+	esac
+done
+
+# Print log
+echo ""
+echo ""
+gfwci "action=log&buildId=$BUILD_ID" | cut -c 30-
+
+# Set exit code for TravisCI
+[ "$RESULT" == "success" ]

base-commit: afd6726309f57f532b4b989a75c1392359c611cc
-- 
2.11.1

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

[Johannes Schindelin]

Hi Lars,

On Wed, 22 Mar 2017, Lars Schneider wrote:

> 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. This
>   waiting time and the build time after the wait could exceed the 3h
>   threshold. If this is a problem, then it is likely to happen every day
>   as usually multiple branches are pushed at the same time (pu/next/
>   master/maint). I cannot test this as my TravisCI account has the 50min
>   timeout. One solution could be to limit the number of concurrent
>   TravisCI jobs [2].

There is another possibility, too, of course: I may be able to find more
resources and add other VMs which could be used to build and run the
tests.

Of course, if those tests would not spawn billions of POSIX processes,
things would be faster, too, and we could afford to wait for a single
build agent.

> 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
> +#
> +# Script to trigger the a Git for Windows build and test run.
> +# Pass a token, the branch (only branches on https://github.com/git/git)
> +# are supported), and a commit hash.
> +#
> +
> +[ $# -eq 3 ] || (echo "Unexpected number of parameters" && exit 1)
> +
> +TOKEN=$1
> +BRANCH=$2
> +COMMIT=$3
> +
> +gfwci () {
> +	curl \
> +		-H "Authentication: Bearer $TOKEN" \
> +		--silent --retry 5 \
> +		"https://git-for-windows-ci.azurewebsites.net/api/TestNow?$1" |
> +	sed "$(printf '1s/^\xef\xbb\xbf//')"  # Remove the Byte Order Mark
> +}
> +
> +# Trigger build job
> +BUILD_ID=$(gfwci "action=trigger&branch=$BRANCH&commit=$COMMIT&skipTests=false")
> +
> +# Check if the $BUILD_ID contains a number
> +case $BUILD_ID in
> +	''|*[!0-9]*) echo $BUILD_ID && exit 1

Error messages are delivered that way, and they do not start with digits,
true. But maybe there is an exit status to indicate to Travis that we
cannot decide whether the build failed or succeeded in that case? The most
common cause for an error here is that the VM I use for testing is down
(which happens every once in a while to save on resources, and I have to
manually restart it)...

> +echo "Visual Studio Team Services Build #${BUILD_ID}"

Nice plug, thanks! ;-)

> +# Wait until build job finished
> +STATUS=
> +RESULT=
> +while true
> +do
> +	LAST_STATUS=$STATUS
> +	STATUS=$(gfwci "action=status&buildId=$BUILD_ID")
> +	[ "$STATUS" == "$LAST_STATUS" ] || printf "\nStatus: $STATUS "
> +	printf "."
> +
> +	case $STATUS in
> +		inProgress|postponed|notStarted) sleep 10                      ;; # continue
> +		         "completed: succeeded") RESULT="success";        break;; # success
> +		                              *) echo "Unknown: $STATUS"; break;; # failure

Well, there are more values for the status, and we know them, but we do
not handle them. Maybe "Unhandled status:"?

> +	esac
> +done
> +
> +# Print log
> +echo ""
> +echo ""
> +gfwci "action=log&buildId=$BUILD_ID" | cut -c 30-
> +
> +# Set exit code for TravisCI
> +[ "$RESULT" == "success" ]

Very nice. Thank you so much for keeping me working on this!

Ciao,
Dscho

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

[Lars Schneider]

> On 22 Mar 2017, at 16:49, Johannes Schindelin <johannes.schindelin@gmx.de> wrote:
> 
> Hi Lars,
> 
> On Wed, 22 Mar 2017, Lars Schneider wrote:
...
>> +
>> +gfwci () {
>> +	curl \
>> +		-H "Authentication: Bearer $TOKEN" \
>> +		--silent --retry 5 \
>> +		"https://git-for-windows-ci.azurewebsites.net/api/TestNow?$1" |
>> +	sed "$(printf '1s/^\xef\xbb\xbf//')"  # Remove the Byte Order Mark
>> +}
>> +
>> +# Trigger build job
>> +BUILD_ID=$(gfwci "action=trigger&branch=$BRANCH&commit=$COMMIT&skipTests=false")
>> +
>> +# Check if the $BUILD_ID contains a number
>> +case $BUILD_ID in
>> +	''|*[!0-9]*) echo $BUILD_ID && exit 1
> 
> Error messages are delivered that way, and they do not start with digits,
> true. But maybe there is an exit status to indicate to Travis that we
> cannot decide whether the build failed or succeeded in that case? The most
> common cause for an error here is that the VM I use for testing is down
> (which happens every once in a while to save on resources, and I have to
> manually restart it)...

Curl can return the HTTP code... I'll try to find a way to check for this.

> 
>> +echo "Visual Studio Team Services Build #${BUILD_ID}"
> 
> Nice plug, thanks! ;-)
> 
>> +# Wait until build job finished
>> +STATUS=
>> +RESULT=
>> +while true
>> +do
>> +	LAST_STATUS=$STATUS
>> +	STATUS=$(gfwci "action=status&buildId=$BUILD_ID")
>> +	[ "$STATUS" == "$LAST_STATUS" ] || printf "\nStatus: $STATUS "
>> +	printf "."
>> +
>> +	case $STATUS in
>> +		inProgress|postponed|notStarted) sleep 10                      ;; # continue
>> +		         "completed: succeeded") RESULT="success";        break;; # success
>> +		                              *) echo "Unknown: $STATUS"; break;; # failure
> 
> Well, there are more values for the status, and we know them, but we do
> not handle them. Maybe "Unhandled status:"?

Sure! I'll fix that in v2 :-)

- Lars

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

[Junio C Hamano]

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.

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

[Johannes Schindelin]

Hi Junio,

On Wed, 22 Mar 2017, Junio C Hamano wrote:

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

Almost... Windows CI *cannot* subscribe to the GitHub repository, as only
owners can install web hooks and give permission to update build status.

But yeah, you understand correctly: this innocuous change (along with a
ton of work I already finished on my side) allows us to let Travis trigger
Windows build & test and to attach the log in the same place as the
Linux/OSX results are already accessible.

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

Right, typically there is a way in CI setups that marks certain strings as
secret and whenever they appear in the log, they will be blotted out.

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

Yes, it is safe without double-quotes. I generated it using:

	dd if=/dev/urandom bs=20 count=1 2> /dev/null | base64

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

Yes, I think we have to use some kind of "skip" status if the build failed
to run or finish in time. But I thought that the "timeout" status would
fulfill that desire...

Ciao,
Dscho

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

[Jeff King]

On Thu, Mar 23, 2017 at 05:22:51PM +0100, Johannes Schindelin wrote:

> > 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?  
> 
> Almost... Windows CI *cannot* subscribe to the GitHub repository, as only
> owners can install web hooks and give permission to update build status.
> 
> But yeah, you understand correctly: this innocuous change (along with a
> ton of work I already finished on my side) allows us to let Travis trigger
> Windows build & test and to attach the log in the same place as the
> Linux/OSX results are already accessible.

I don't mind adding a webhook if it helps. If I understand correctly
that would just handle the notification site. But then if the Windows
build status were public, we could have Travis simply fetch it to keep
the build reports all together, without having to worry about a secret
token.

I don't mind proceeding with the secret token, though. You're the owner
of the service the token accesses, so if you're comfortable with it, I
am.

> > > 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.
> [...]
> 
> Right, typically there is a way in CI setups that marks certain strings as
> secret and whenever they appear in the log, they will be blotted out.

I think both Junio and I have access to the Travis config. Travis does
have a "this is secret" flag for setup config. But I think we'd need to
verify that running the Travis build does not leak the variable in any
other way.

For instance, if it's in the environment, can I push up a branch that
does "set | grep GFW_CI_TOKEN", open a PR, and see it? I don't know the
answer.

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> For instance, if it's in the environment, can I push up a branch that
> does "set | grep GFW_CI_TOKEN", open a PR, and see it? I don't know the
> answer.

I think the documentation said

    Variables defined in repository settings are the same for all
    builds, and when you restart an old build, it uses the latest
    values. These variables are not automatically available to
    forks.

so we should be safe as long as we do not build against PRs.

On the other hand, perhaps a contributor may want to build and test
his own PR that may affect Windows platform, and such a contributor
may be helped if the main repository sets things up to build against
PRs.  

I personally think it is a separate issue and we shouldn't set it up
to build against PRs.  If Windows CI wants to help these
contributors, it can give out the token to them, without relying on
the travis setup for the main repository.


https://docs.travis-ci.com/user/environment-variables#Defining-Variables-in-Repository-Settings

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

[Jeff King]

On Thu, Mar 23, 2017 at 12:12:15PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > For instance, if it's in the environment, can I push up a branch that
> > does "set | grep GFW_CI_TOKEN", open a PR, and see it? I don't know the
> > answer.
> 
> I think the documentation said
> 
>     Variables defined in repository settings are the same for all
>     builds, and when you restart an old build, it uses the latest
>     values. These variables are not automatically available to
>     forks.
> 
> so we should be safe as long as we do not build against PRs.

I think we do build against PRs now. E.g.:

  https://travis-ci.org/git/git/builds/213896051

But it looks like we can turn that off.

> On the other hand, perhaps a contributor may want to build and test
> his own PR that may affect Windows platform, and such a contributor
> may be helped if the main repository sets things up to build against
> PRs.
> 
> I personally think it is a separate issue and we shouldn't set it up
> to build against PRs.  If Windows CI wants to help these
> contributors, it can give out the token to them, without relying on
> the travis setup for the main repository.

Hrm, it does mean that people have no way to test on Windows until the
branch hits pu. Which is not ideal.

-Peff

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

[Lars Schneider]

> On 23 Mar 2017, at 20:17, Jeff King <peff@peff.net> wrote:
> 
> On Thu, Mar 23, 2017 at 12:12:15PM -0700, Junio C Hamano wrote:
> 
>> Jeff King <peff@peff.net> writes:
>> 
>>> For instance, if it's in the environment, can I push up a branch that
>>> does "set | grep GFW_CI_TOKEN", open a PR, and see it? I don't know the
>>> answer.
>> 
>> I think the documentation said
>> 
>>    Variables defined in repository settings are the same for all
>>    builds, and when you restart an old build, it uses the latest
>>    values. These variables are not automatically available to
>>    forks.
>> 
>> so we should be safe as long as we do not build against PRs.
> 
> I think we do build against PRs now. E.g.:
> 
>  https://travis-ci.org/git/git/builds/213896051
> 
> But it looks like we can turn that off.

When we add a secret variable, then TravisCI will not build Pull Requests
for git/git anymore:

"[...] we do not provide these values to untrusted builds, 
triggered by pull requests from another repository."

See: https://docs.travis-ci.com/user/environment-variables/#Defining-Variables-in-Repository-Settings

However, I don't think that is a big deal because git/git doesn't
support Pull Requests anyways. Plus, if a contributor is interested
in TravisCI results, then the contributor can setup TravisCI for
their own fork easily.


>> On the other hand, perhaps a contributor may want to build and test
>> his own PR that may affect Windows platform, and such a contributor
>> may be helped if the main repository sets things up to build against
>> PRs.
>> 
>> I personally think it is a separate issue and we shouldn't set it up
>> to build against PRs.  If Windows CI wants to help these
>> contributors, it can give out the token to them, without relying on
>> the travis setup for the main repository.
> 
> Hrm, it does mean that people have no way to test on Windows until the
> branch hits pu. Which is not ideal.

I agree it's not ideal. But I think it is an improvement to check
pu/next/master/maint continuously :-)


Cheers,
Lars

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

[Junio C Hamano]

Lars Schneider <larsxschneider@gmail.com> writes:

> "[...] we do not provide these values to untrusted builds, 
> triggered by pull requests from another repository."
>
> See: https://docs.travis-ci.com/user/environment-variables/#Defining-Variables-in-Repository-Settings

OK, it is a releaf to see an indication that somebody over there is
thinking.  Thanks.

So we do _not_ have to turn it off; as soon as we define sekrit
variables, they will turn it off for us ;-).

>> Hrm, it does mean that people have no way to test on Windows until the
>> branch hits pu. Which is not ideal.
>
> I agree it's not ideal. But I think it is an improvement to check
> pu/next/master/maint continuously :-)

I am not sure what you mean.  We are building each and every branch
updates already, and I do not see any improvement over what we are
doing now.  Care to elaborate?

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

[Lars Schneider]

> On 23 Mar 2017, at 20:30, Junio C Hamano <gitster@pobox.com> wrote:
> 
> Lars Schneider <larsxschneider@gmail.com> writes:
> 
>> "[...] we do not provide these values to untrusted builds, 
>> triggered by pull requests from another repository."
>> 
>> See: https://docs.travis-ci.com/user/environment-variables/#Defining-Variables-in-Repository-Settings
> 
> OK, it is a releaf to see an indication that somebody over there is
> thinking.  Thanks.
> 
> So we do _not_ have to turn it off; as soon as we define sekrit
> variables, they will turn it off for us ;-).
> 
>>> Hrm, it does mean that people have no way to test on Windows until the
>>> branch hits pu. Which is not ideal.
>> 
>> I agree it's not ideal. But I think it is an improvement to check
>> pu/next/master/maint continuously :-)
> 
> I am not sure what you mean.  We are building each and every branch
> updates already, and I do not see any improvement over what we are
> doing now.  Care to elaborate?

We are building each and every branch on TravisCI right now - but 
only on Linux and OSX. With this change we also build it on
Windows. That should help to spot Windows related issues more
quickly I think.

- Lars

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

[Junio C Hamano]

Lars Schneider <larsxschneider@gmail.com> writes:

>>> I agree it's not ideal. But I think it is an improvement to check
>>> pu/next/master/maint continuously :-)
>> 
>> I am not sure what you mean.  We are building each and every branch
>> updates already, and I do not see any improvement over what we are
>> doing now.  Care to elaborate?
>
> We are building each and every branch on TravisCI right now - but 
> only on Linux and OSX. With this change we also build it on
> Windows. That should help to spot Windows related issues more
> quickly I think.

We are losing the build for PR for folks who later submit patches as
the price for that.  "building each and every branch" stay constant
and the trade-off is between building four integration branches on
Windows and allowing contributors to easily check their work early.

If your "But I think it is an improvement" was followed by "to check
the four branches on Windows continuously", though, I wouldn't have
had to ask the question, as I tend to agree it is a better trade
off.

Thanks.

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

[Jeff King]

On Thu, Mar 23, 2017 at 08:26:15PM +0100, Lars Schneider wrote:

> > I think we do build against PRs now. E.g.:
> > 
> >  https://travis-ci.org/git/git/builds/213896051
> > 
> > But it looks like we can turn that off.
> 
> When we add a secret variable, then TravisCI will not build Pull Requests
> for git/git anymore:
> 
> "[...] we do not provide these values to untrusted builds, 
> triggered by pull requests from another repository."
> 
> See: https://docs.travis-ci.com/user/environment-variables/#Defining-Variables-in-Repository-Settings

Ah, OK, that makes sense. So we would only have to worry about our _own_
code accidentally disclosing it. But that should be easy to look for by
grepping the log (did somebody do that?).

> However, I don't think that is a big deal because git/git doesn't
> support Pull Requests anyways. Plus, if a contributor is interested
> in TravisCI results, then the contributor can setup TravisCI for
> their own fork easily.

Yeah, agreed. It's not like we are blocking the merge button with a
failing status.

> > Hrm, it does mean that people have no way to test on Windows until the
> > branch hits pu. Which is not ideal.
> 
> I agree it's not ideal. But I think it is an improvement to check
> pu/next/master/maint continuously :-)

Oh, I agree this is a step forward from the status quo. I just wondered
if we could do even better.

As a side note, I've started having travis run on all of the topic
branches in peff/git, with the idea to get early feedback on OS X
problems (and now hopefully Windows ones). My two issues so far are:

  - I have a lot of work-in-progress branches. I put "-wip" at the end
    of the branch name for my own scripts. It looks like I can put "[ci
    skip]" in the commit subject to convince Travis to skip them, but
    that's a little annoying. Is there any way to skip based on just the
    branch name? I couldn't find one.

  - The OS X builds seem to regularly time out. That at least marks a
    "!" in the build status screen instead of an "X", but it's a lot of
    noise (and it misses the point for me, which is testing on OS X; I
    already build regularly on Linux).

-Peff

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

[Lars Schneider]

> On 23 Mar 2017, at 20:38, Jeff King <peff@peff.net> wrote:
> 
> On Thu, Mar 23, 2017 at 08:26:15PM +0100, Lars Schneider wrote:
> 
>>> I think we do build against PRs now. E.g.:
>>> 
>>> https://travis-ci.org/git/git/builds/213896051
>>> 
>>> But it looks like we can turn that off.
>> 
>> When we add a secret variable, then TravisCI will not build Pull Requests
>> for git/git anymore:
>> 
>> "[...] we do not provide these values to untrusted builds, 
>> triggered by pull requests from another repository."
>> 
>> See: https://docs.travis-ci.com/user/environment-variables/#Defining-Variables-in-Repository-Settings
> 
> Ah, OK, that makes sense. So we would only have to worry about our _own_
> code accidentally disclosing it. But that should be easy to look for by
> grepping the log (did somebody do that?).

This is how a successful Windows build would look like:
https://travis-ci.org/larsxschneider/git/jobs/214391822

Dscho's token is not in the log.


>> However, I don't think that is a big deal because git/git doesn't
>> support Pull Requests anyways. Plus, if a contributor is interested
>> in TravisCI results, then the contributor can setup TravisCI for
>> their own fork easily.
> 
> Yeah, agreed. It's not like we are blocking the merge button with a
> failing status.
> 
>>> Hrm, it does mean that people have no way to test on Windows until the
>>> branch hits pu. Which is not ideal.
>> 
>> I agree it's not ideal. But I think it is an improvement to check
>> pu/next/master/maint continuously :-)
> 
> Oh, I agree this is a step forward from the status quo. I just wondered
> if we could do even better.
> 
> As a side note, I've started having travis run on all of the topic
> branches in peff/git, with the idea to get early feedback on OS X
> problems (and now hopefully Windows ones). My two issues so far are:
> 
>  - I have a lot of work-in-progress branches. I put "-wip" at the end
>    of the branch name for my own scripts. It looks like I can put "[ci
>    skip]" in the commit subject to convince Travis to skip them, but
>    that's a little annoying. Is there any way to skip based on just the
>    branch name? I couldn't find one.

We can blacklist these branches with a regex in the travis.yml:
https://docs.travis-ci.com/user/customizing-the-build#Building-Specific-Branches


>  - The OS X builds seem to regularly time out. That at least marks a
>    "!" in the build status screen instead of an "X", but it's a lot of
>    noise (and it misses the point for me, which is testing on OS X; I
>    already build regularly on Linux).

Well, I guess the reason is that OSX is a bit harder to virtualize 
compared to Linux. In general on git/git the OSX builds are OK.

I build a little stats page to visualize the results over time:
https://larsxschneider.github.io/git-ci-stats/

We have 2 OSX jobs and 2 Linux jobs per build. I am usually only
digging into issues if both jobs are red.

Maybe TravisCI throttles your usage somehow as you push a lot of commits?

Keep in mind, all the TravisCI compute hours are free... considering
that I think it is quite OK :-)

- Lars

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

[Jeff King]

On Thu, Mar 23, 2017 at 09:00:33PM +0100, Lars Schneider wrote:

> > Ah, OK, that makes sense. So we would only have to worry about our _own_
> > code accidentally disclosing it. But that should be easy to look for by
> > grepping the log (did somebody do that?).
> 
> This is how a successful Windows build would look like:
> https://travis-ci.org/larsxschneider/git/jobs/214391822
> 
> Dscho's token is not in the log.

Great, thank you for double-checking.

> >  - I have a lot of work-in-progress branches. I put "-wip" at the end
> >    of the branch name for my own scripts. It looks like I can put "[ci
> >    skip]" in the commit subject to convince Travis to skip them, but
> >    that's a little annoying. Is there any way to skip based on just the
> >    branch name? I couldn't find one.
> 
> We can blacklist these branches with a regex in the travis.yml:
> https://docs.travis-ci.com/user/customizing-the-build#Building-Specific-Branches

I had a feeling it might be something like that. So we would all need to
agree on the convention for WIP branch names. If other people like the
idea, I'm happy to make a patch, but I don't want to impose my own weird
conventions on everyone else.

> Maybe TravisCI throttles your usage somehow as you push a lot of commits?

Could be. Looking at:

  https://travis-ci.org/peff/git/branches

It seems to timeout on over half the branches (in fact, there are only a
few that passed all of the tests). My pattern is particularly spiky from
Travis's perspective, because once a day I rebase everything on top of
master and push them the whole thing in a bunch. So they 75 branches,
all at once. That seems like it would be ripe for throttling (though I
would much rather they just queue the builds and do them one at a time).

> Keep in mind, all the TravisCI compute hours are free... considering
> that I think it is quite OK :-)

I don't blame Travis at all. But if the tool does not produce reliable
results, then I will start to ignore it. And somebody on this thread
(not you) has been complaining repeatedly about developers ignoring CI
results.

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> We can blacklist these branches with a regex in the travis.yml:
>> https://docs.travis-ci.com/user/customizing-the-build#Building-Specific-Branches
>
> I had a feeling it might be something like that. So we would all need to
> agree on the convention for WIP branch names. If other people like the
> idea, I'm happy to make a patch, but I don't want to impose my own weird
> conventions on everyone else.

I can go with any convention, but I'd be more pleased if you made
sure that "do not build this with CI" and "this is WIP" are kept as
two separate concepts, as I can see having some WIP that I do want
to get tested.

Perhaps a substring "/noci-" anywhere in the branch name, or
something silly like that?

> I don't blame Travis at all. But if the tool does not produce reliable
> results, then I will start to ignore it.

Yes, that is a real issue.  I was wondering if we can have a knob
that can be controled with the repository setting to limit which
"build jobs" are run, so that you can use that web UI to set an
"environment variable" that lists only .3 and .4 (which correspond
to these two OSX builds) and .travis.yml takes notice of the
variable setting, or something.

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

[Jeff King]

On Thu, Mar 23, 2017 at 01:30:41PM -0700, Junio C Hamano wrote:

> >> We can blacklist these branches with a regex in the travis.yml:
> >> https://docs.travis-ci.com/user/customizing-the-build#Building-Specific-Branches
> >
> > I had a feeling it might be something like that. So we would all need to
> > agree on the convention for WIP branch names. If other people like the
> > idea, I'm happy to make a patch, but I don't want to impose my own weird
> > conventions on everyone else.
> 
> I can go with any convention, but I'd be more pleased if you made
> sure that "do not build this with CI" and "this is WIP" are kept as
> two separate concepts, as I can see having some WIP that I do want
> to get tested.
> 
> Perhaps a substring "/noci-" anywhere in the branch name, or
> something silly like that?

Hrm, most of the point for me was _not_ having to define the two
concepts separately. Let me try it for a while with "[ci skip]" in the
tip commit subject, and see how painful I find that. My goal is
eventually to turn on notifications, so I'd quickly be reminded if I
forgot such a marker.

-Peff

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

[Lars Schneider]

> On 23 Mar 2017, at 21:20, Jeff King <peff@peff.net> wrote:
> 
> On Thu, Mar 23, 2017 at 09:00:33PM +0100, Lars Schneider wrote:
> 
>>> Ah, OK, that makes sense. So we would only have to worry about our _own_
>>> code accidentally disclosing it. But that should be easy to look for by
>>> grepping the log (did somebody do that?).
>> 
>> This is how a successful Windows build would look like:
>> https://travis-ci.org/larsxschneider/git/jobs/214391822
>> 
>> Dscho's token is not in the log.
> 
> Great, thank you for double-checking.

I can see that one could think that this variables leaks, though. 
I think in v2 I won't pass the token as parameter. The token 
variable is an environment variable anyways and I can just use
it in the script.


>>> - I have a lot of work-in-progress branches. I put "-wip" at the end
>>>   of the branch name for my own scripts. It looks like I can put "[ci
>>>   skip]" in the commit subject to convince Travis to skip them, but
>>>   that's a little annoying. Is there any way to skip based on just the
>>>   branch name? I couldn't find one.
>> 
>> We can blacklist these branches with a regex in the travis.yml:
>> https://docs.travis-ci.com/user/customizing-the-build#Building-Specific-Branches
> 
> I had a feeling it might be something like that. So we would all need to
> agree on the convention for WIP branch names. If other people like the
> idea, I'm happy to make a patch, but I don't want to impose my own weird
> conventions on everyone else.

This makes sense to me. If someone complains with a good argument then
we can still revert it.


>> Maybe TravisCI throttles your usage somehow as you push a lot of commits?
> 
> Could be. Looking at:
> 
>  https://travis-ci.org/peff/git/branches
> 
> It seems to timeout on over half the branches (in fact, there are only a
> few that passed all of the tests). My pattern is particularly spiky from
> Travis's perspective, because once a day I rebase everything on top of
> master and push them the whole thing in a bunch. So they 75 branches,
> all at once. That seems like it would be ripe for throttling (though I
> would much rather they just queue the builds and do them one at a time).

Could you try to set this to 7 or less in your TravisCI?
https://docs.travis-ci.com/user/customizing-the-build#Limiting-Concurrent-Builds

I am curious if this improves the situation.

- Lars

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

[Jeff King]

On Thu, Mar 23, 2017 at 09:39:14PM +0100, Lars Schneider wrote:

> > Could be. Looking at:
> > 
> >  https://travis-ci.org/peff/git/branches
> > 
> > It seems to timeout on over half the branches (in fact, there are only a
> > few that passed all of the tests). My pattern is particularly spiky from
> > Travis's perspective, because once a day I rebase everything on top of
> > master and push them the whole thing in a bunch. So they 75 branches,
> > all at once. That seems like it would be ripe for throttling (though I
> > would much rather they just queue the builds and do them one at a time).
> 
> Could you try to set this to 7 or less in your TravisCI?
> https://docs.travis-ci.com/user/customizing-the-build#Limiting-Concurrent-Builds
> 
> I am curious if this improves the situation.

Sure, that's easy enough to try. I'll let it go for a few days with that
and see if it improves.

-Peff

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

[Johannes Schindelin]

Hi Peff,

On Thu, 23 Mar 2017, Jeff King wrote:

> My pattern is particularly spiky from Travis's perspective, because once
> a day I rebase everything on top of master and push them the whole thing
> in a bunch. So they 75 branches, all at once. That seems like it would
> be ripe for throttling (though I would much rather they just queue the
> builds and do them one at a time).

Travis is optimized for Continuous Integration, i.e. one or maybe two
integration branches that merge PRs every once in a while.

What we would need is Continuous Testing, really, because we do not do
Continuous Integration at all.

On the point of the sekrit variable: all I tried to do is to avoid
overwhelming the (currently) single VM with plenty of jobs. If the build &
test would not take so darned long on Windows due to going in and out of
the POSIX emulation layer a gazillion times (caused by intense shell
scripting), the tests could be faster. But that would require a massive
amount of work, and I simply have too much on my plate to take that task
on in addition.

If you have an idea how to prevent the overloading of the VM by any means
that does not involve that token, please tell me.

Ciao,
Dscho

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

[Samuel Lijin]

On Thu, Mar 23, 2017 at 2:38 PM, Jeff King <peff@peff.net> wrote:
> On Thu, Mar 23, 2017 at 08:26:15PM +0100, Lars Schneider wrote:
>
>> > I think we do build against PRs now. E.g.:
>> >
>> >  https://travis-ci.org/git/git/builds/213896051
>> >
>> > But it looks like we can turn that off.
>>
>> When we add a secret variable, then TravisCI will not build Pull Requests
>> for git/git anymore:
>>
>> "[...] we do not provide these values to untrusted builds,
>> triggered by pull requests from another repository."
>>
>> See: https://docs.travis-ci.com/user/environment-variables/#Defining-Variables-in-Repository-Settings
>
> Ah, OK, that makes sense. So we would only have to worry about our _own_
> code accidentally disclosing it. But that should be easy to look for by
> grepping the log (did somebody do that?).
>
>> However, I don't think that is a big deal because git/git doesn't
>> support Pull Requests anyways. Plus, if a contributor is interested
>> in TravisCI results, then the contributor can setup TravisCI for
>> their own fork easily.
>
> Yeah, agreed. It's not like we are blocking the merge button with a
> failing status.
>
>> > Hrm, it does mean that people have no way to test on Windows until the
>> > branch hits pu. Which is not ideal.
>>
>> I agree it's not ideal. But I think it is an improvement to check
>> pu/next/master/maint continuously :-)
>
> Oh, I agree this is a step forward from the status quo. I just wondered
> if we could do even better.
>
> As a side note, I've started having travis run on all of the topic
> branches in peff/git, with the idea to get early feedback on OS X
> problems (and now hopefully Windows ones). My two issues so far are:
>
>   - I have a lot of work-in-progress branches. I put "-wip" at the end
>     of the branch name for my own scripts. It looks like I can put "[ci
>     skip]" in the commit subject to convince Travis to skip them, but
>     that's a little annoying. Is there any way to skip based on just the
>     branch name? I couldn't find one.

I think you can "safelist" (whitelist) branches to build with regexes:
https://docs.travis-ci.com/user/customizing-the-build#Building-Specific-Branches

>   - The OS X builds seem to regularly time out. That at least marks a
>     "!" in the build status screen instead of an "X", but it's a lot of
>     noise (and it misses the point for me, which is testing on OS X; I
>     already build regularly on Linux).

I suspect this happens because they don't have a lot of macOS runners
- I've seen those jobs wait for hours (even on private jobs) before a
runner gets freed up.

> -Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I think both Junio and I have access to the Travis config. Travis does
> have a "this is secret" flag for setup config. But I think we'd need to
> verify that running the Travis build does not leak the variable in any
> other way.

I am not sure if I want to "Authorize application" at the GitHub
site to give Travis that broad set of permissions, though.  That is
why I do not "log in with GitHub" to Travis.

I just logged into Travis and it seems to me that git/git is set to
build branch updates (which sounds sensible) and also is set to
build pull request updates.  That somehow sounds like a dangerous
mix with the "secret environment variables" thing, at least to me.

> For instance, if it's in the environment, can I push up a branch that
> does "set | grep GFW_CI_TOKEN", open a PR, and see it? I don't know the
> answer.

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

[Lars Schneider]

> On 22 Mar 2017, at 20:29, Junio C Hamano <gitster@pobox.com> wrote:
> 
> 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?  
Correct!

> Very nice.

Thanks :-)


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

True. I think I took care of this and it should not leak!


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

Well, look at this. TravisCi *just* released a solution to
exactly that problem! Good timing!
https://blog.travis-ci.com/2017-03-22-introducing-auto-cancellation


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

Sure!


>> +[ $# -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

Will do in v2!


Thanks,
Lars

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

[Junio C Hamano]

Lars Schneider <larsxschneider@gmail.com> writes:

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

As coordinating the timing of when this happens with the timing when
merging this topic to 'pu' happens is an unnecessary cognitive
burden, can you make a small change to this part:

> +      script:
> +        - >
> +          test "$TRAVIS_REPO_SLUG" != "git/git" ||

Add
	test -z "$GFW_CI_TOKEN" ||

here, so that the sript is not run while "Someone with write access"
hasn't got around to doing it. 

> +          ci/run-windows-build.sh $GFW_CI_TOKEN $TRAVIS_BRANCH $(git rev-parse HEAD)

It would still be a good idea to dq all these $VARIABLE and
$(COMMAND OUTPUT) references, as the script expects three
parameters.

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

Thanks.