patchci: fix GitHub workflow when on a tagged revision

From: Johannes Schindelin <johannes.schindelin@gmx.de>

When `master` is tagged, and then both `master` and the tag are pushed, Travis CI will happily build both. That is a waste of energy, which is why we skip the build for `master` in that case.

However, our GitHub workflow does not trigger on tags, therefore, this logic results in a missing build for that revision.

Even worse: the run would _fail_ because we would skip the Windows build, there are no artifacts to publish, and therefore no artifacts to download in the Windows test jobs.

Let's just change the GitHub workflow to skip the logic to skip revisions that are tagged.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
    Do not skip tagged revisions in the GitHub workflow runs
    
    I noticed a failure in the run for Git for Windows v2.26.2 which was
    caused by this.
    
    This patch is based on dd/ci-swap-azure-pipelines-with-github-actions.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-619%2Fdscho%2Fgithub-workflows-and-tags-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-619/dscho/github-workflows-and-tags-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/619

 ci/lib.sh | 2 ++
 1 file changed, 2 insertions(+)

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes:

Thanks for noticing. The arrangement we had, which essentially said "we know we will always build tagged version, so a push of a branch that happens to have a tagged version at the tip can be ignored", served us wonderfully. Now the maintainers of projects (not just this one) are forbidden from tagging the tip of master, queue something else on top and push the result out, as it won't build or test the tagged version, which is a bit sad.

Quite straight-forward.  Thanks, will queue.

Hi Junio,

On Fri, 24 Apr 2020, Junio C Hamano wrote:

Nobody forbids this... ;-)

And I was wrong, our current GitHub workflow _is_ triggered by tags. See e.g. https://github.com/git-for-windows/git/actions/runs/83103626 which was triggered by v2.26.2.windows.1.

However, you also see that it failed due to the reason I described in the commit message.

I guess that we should either go with this patch (which would trigger full runs also on tags), or we could alternatively trigger only on branch pushes (and not tag pushes)?

Ciao, Dscho

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

Hmph, even if we were to go with this patch (which should be safer), we'd definitely need an updated log message, then.

Thanks for double-checking.

Whenever a GitGitGadget Pull Request is sent to the Git mailing list, a tag is pushed to gitgitgadget/git to commemorate that iteration.

The push event caused for that triggers the CI/PR workflow, and reveals a pretty old bug where the windows-build steps are skipped for tagged revisions, but the windows-test steps are not (and will therefore fail).

That means, of course, that every GitGitGadget PR is marked with a failed test once it is submitted.

This patch series is designed to address this issue, and is based on am/ci-wsfix (the initial round was based on dd/ci-swap-azure-pipelines-with-github-actions but would now cause merge conflicts).

Changes since v1:

 * Rather than returning early from skip_branch_tip_with_tag(), we now skip
   the function call altogether when run in a GitHub workflow.
 * The intention of the tag skipping was replicated by introducing another
   check in ci-config: is there a successful workflow run for the same
   commit (or at least for the same tree)? If yes, skip, referring to that
   successful run.

Johannes Schindelin (2):
  ci: skip GitHub workflow runs for already-tested commits/trees
  ci: do not skip tagged revisions in GitHub workflows

 .github/workflows/main.yml | 39 +++++++++++++++++++++++++++++++++++++-
 ci/lib.sh                  |  2 ++
 2 files changed, 40 insertions(+), 1 deletion(-)

base-commit: 055747cd75c0904cc8122e5c12bd45e9f4743c30
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-619%2Fdscho%2Fgithub-workflows-and-tags-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-619/dscho/github-workflows-and-tags-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/619

Range-diff vs v1:

 -:  ---------- > 1:  914868d558 ci: skip GitHub workflow runs for already-tested commits/trees
 1:  d9823f82ee ! 2:  931a2b8482 ci: fix GitHub workflow when on a tagged revision
     @@ Metadata
      Author: Johannes Schindelin <Johannes.Schindelin@gmx.de>
      
       ## Commit message ##
     -    ci: fix GitHub workflow when on a tagged revision
     +    ci: do not skip tagged revisions in GitHub workflows
      
          When `master` is tagged, and then both `master` and the tag are pushed,
          Travis CI will happily build both. That is a waste of energy, which is
          why we skip the build for `master` in that case.
      
     -    However, our GitHub workflow does not trigger on tags, therefore, this
     -    logic results in a missing build for that revision.
     +    Our GitHub workflow is also triggered by tags. However, the run would
     +    fail because the `windows-test` jobs are _not_ skipped on tags, but the
     +    `windows-build` job _is skipped (and therefore fails to upload the
     +    build artifacts needed by the test jobs).
      
     -    Even worse: the run would _fail_ because we would skip the Windows
     -    build, there are no artifacts to publish, and therefore no artifacts to
     -    download in the Windows test jobs.
     +    In addition, we just added logic to our GitHub workflow that will skip
     +    runs altogether if there is already a successful run for the same commit
     +    or at least for the same tree.
      
     -    Let's just change the GitHub workflow to skip the logic to skip
     -    revisions that are tagged.
     +    Let's just change the GitHub workflow to no longer specifically skip
     +    tagged revisions.
      
          Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
      
       ## ci/lib.sh ##
     -@@
     - # Library of functions shared by all CI scripts
     - 
     - skip_branch_tip_with_tag () {
     -+	test -z "$DONT_SKIP_TAGS" || return 0
     - 	# Sometimes, a branch is pushed at the same time the tag that points
     - 	# at the same commit as the tip of the branch is pushed, and building
     - 	# both at the same time is a waste.
      @@ ci/lib.sh: then
     + 	CI_REPO_SLUG="$GITHUB_REPOSITORY"
     + 	CI_JOB_ID="$GITHUB_RUN_ID"
       	CC="${CC:-gcc}"
     ++	DONT_SKIP_TAGS=t
       
       	cache_dir="$HOME/none"
     -+	DONT_SKIP_TAGS=t
       
     - 	export GIT_PROVE_OPTS="--timer --jobs 10"
     - 	export GIT_TEST_OPTS="--verbose-log -x"
     +@@ ci/lib.sh: good_trees_file="$cache_dir/good-trees"
     + 
     + mkdir -p "$cache_dir"
     + 
     ++test -n "${DONT_SKIP_TAGS-}" ||
     + skip_branch_tip_with_tag
     + skip_good_tree
     + 

From: Johannes Schindelin <johannes.schindelin@gmx.de>

When pushing a commit that has already passed a CI or PR build successfully, it makes sense to save some energy and time and skip the new build.

Let's teach our GitHub workflow to do that.

For good measure, we also compare the tree ID, which is what we actually test (the commit ID might have changed due to a reworded commit message, which should not affect the outcome of the run).

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 .github/workflows/main.yml | 39 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 38 insertions(+), 1 deletion(-)

On Thu, Oct 08, 2020 at 03:29:34PM +0000, Johannes Schindelin via GitGitGadget wrote:

We have been doing this on Travis CI for a few years now. Does that approach not work on GitHub actions? Please explain in the commit message why a different approach is taken here.

Hi Gábor,

On Fri, 9 Oct 2020, SZEDER Gábor wrote:

You're not being terribly clear about what exactly "We have been doing".

Are you referring to the `skip_good_tree()` function that stores information in a file in the `good_trees_file`?

If so, no, we cannot do that anywhere else than on Travis because that relies on a directory that is somehow shared between runs. And that is a feature that only Travis offers as far as I know (and it does not come without issues, e.g. when two concurrent runs try to write to the same file at the same time).

Since this strategy relies on a Travis-only feature that does not work on the three other CI services we use (Cirrus CI, Azure DevOps, GitHub Actions), I see little point mentioning it in this commit message...

However, I might be very well wrong on that assessment.

Ciao, Dscho

On Fri, Oct 09, 2020 at 01:13:03PM +0200, Johannes Schindelin wrote:

Yes, in the commit message you pretty accurately described the exact purpose of that function.

Every branchname-job combination has its own cache, and while the job is running it writes to a local copy of its own cache, so concurrent runs don't seem to be an issue.

This commit duplicates already existing functionality, so, yes, the commit message should definitely have explained why that already existing approach was not suitable for GitHub Actions.

Hi Gábor,

On Sat, 10 Oct 2020, SZEDER Gábor wrote:

No, this is not duplicating functionality.

The `skip_good_tree()` function requires a persistent directory into which it writes a record of the trees it considers good, based on past runs. It later recalls which trees are considers good and skips the current run if there is a record for this tree.

The fact that it requires a persistent directory binds it to Travis CI. As far as I can tell, no other CI service offers that feature (and from where I sit, for good reason, because it is asking for all kinds of fun in concurrent scenarios).

What my patch does might duplicate the intention, but absolutely not the functionality. For one, there is no extra record required. It uses the API to query the existing logs.

Also, the patch specifically adjusts the GitHub workflow itself. Therefore, unlike the `skip_good_tree()` function, it does not pretend to be generic (which `skip_good_tree()` really is not, as pointed out above).

Ciao, Dscho

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

I think skip_good_tree aspired to be a generic one, by having the "if we are not in travis nor GitHub actions, return early" at its very beginning. The person who adds support to GitHub workflow could have done one of two things.

One is to recognise the aspiration, and restructure existing skip_good_tree from

    skip_good_tree () {
	return if not travis and if not github actions

	bunch of code that happens to work only on travis
    }

to

    skip_good_tree () {
	if travis
	then
	    bunch of code that happens to work only on travis
	else if github actions
	    bunch of code that happens to work only with github
	fi
    }

without touching the caller. That lets skip_good_tree to be generic.

Another is to completely ignore that aspiration, maybe doing all that inside the workflow script (which by definition works only with github).

I think the latter (i.e. what the patch choose to do, which is not to bother with the ci/*.sh scripts at all) would be cleaner in this particular case, but then it would have made the intention more clear if the conditional at the beginning of skip_good_tree() were adjusted, perhaps?

Hi Junio,

On Mon, 12 Oct 2020, Junio C Hamano wrote:

Full disclosure: I am not even sure what to do with Travis support at this stage (but then, I am not the one to decide over that).

In any case, it appears that no matter how generic the `skip_good_tree()` function meant to be, by virtue of using that `cache: directories:` feature (https://docs.travis-ci.com/user/caching) it is limited to Travis CI nevertheless.

So if we were to adjust the conditional at the beginning of `skip_good_tree()`, it would probably make sense to make it clear that this not only does not work with GitHub Actions, but in fact it does not work with anything but Travis.

Ciao, Dscho

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

Are you talking about the migration to travis-ci.com and having to give them potentially more access to github.com/git/git/ repository?

It does worry me, too.

Hi Junio,

On Thu, 15 Oct 2020, Junio C Hamano wrote:

I meant that, together with worries about the future of Travis CI given that they've been acquired by Idera.

Ciao, Dscho

From: Johannes Schindelin <johannes.schindelin@gmx.de>

When `master` is tagged, and then both `master` and the tag are pushed, Travis CI will happily build both. That is a waste of energy, which is why we skip the build for `master` in that case.

Our GitHub workflow is also triggered by tags. However, the run would fail because the `windows-test` jobs are _not_ skipped on tags, but the `windows-build` job _is skipped (and therefore fails to upload the build artifacts needed by the test jobs).

In addition, we just added logic to our GitHub workflow that will skip runs altogether if there is already a successful run for the same commit or at least for the same tree.

Let's just change the GitHub workflow to no longer specifically skip tagged revisions.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 ci/lib.sh | 2 ++
 1 file changed, 2 insertions(+)

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes:

Nice.

Our tests and builds do not do anything differently depending on the refname we are on or the exact commit object name for that matter (other than using it in "git version" output), so "has this tree tested?" is good optimization strategy for this particular project.