[PATCH] ci: fix GitHub workflow when on a tagged revision

[PATCH] ci: fix GitHub workflow when on a tagged revision

[Johannes Schindelin via GitGitGadget]

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(+)

diff --git a/ci/lib.sh b/ci/lib.sh
index dac36886e37..f151e2f0ecb 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -1,6 +1,7 @@
 # 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.
@@ -151,6 +152,7 @@ then
 	CC="${CC:-gcc}"
 
 	cache_dir="$HOME/none"
+	DONT_SKIP_TAGS=t
 
 	export GIT_PROVE_OPTS="--timer --jobs 10"
 	export GIT_TEST_OPTS="--verbose-log -x"

base-commit: f72f328bc57e1b0db380ef76e0c1e94a9ed0ac7c
-- 
gitgitgadget

Re: [PATCH] ci: fix GitHub workflow when on a tagged revision

[Junio C Hamano]

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

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

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.

>  ci/lib.sh | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/ci/lib.sh b/ci/lib.sh
> index dac36886e37..f151e2f0ecb 100755
> --- a/ci/lib.sh
> +++ b/ci/lib.sh
> @@ -1,6 +1,7 @@
>  # 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.
> @@ -151,6 +152,7 @@ then
>  	CC="${CC:-gcc}"
>  
>  	cache_dir="$HOME/none"
> +	DONT_SKIP_TAGS=t

Quite straight-forward.  Thanks, will queue.

>  	export GIT_PROVE_OPTS="--timer --jobs 10"
>  	export GIT_TEST_OPTS="--verbose-log -x"
>
> base-commit: f72f328bc57e1b0db380ef76e0c1e94a9ed0ac7c

Re: [PATCH] ci: fix GitHub workflow when on a tagged revision

[Johannes Schindelin]

Hi Junio,

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

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> > However, our GitHub workflow does not trigger on tags, therefore, this
> > logic results in a missing build for that revision.
>
> 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.

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

>
> >  ci/lib.sh | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/ci/lib.sh b/ci/lib.sh
> > index dac36886e37..f151e2f0ecb 100755
> > --- a/ci/lib.sh
> > +++ b/ci/lib.sh
> > @@ -1,6 +1,7 @@
> >  # 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.
> > @@ -151,6 +152,7 @@ then
> >  	CC="${CC:-gcc}"
> >
> >  	cache_dir="$HOME/none"
> > +	DONT_SKIP_TAGS=t
>
> Quite straight-forward.  Thanks, will queue.
>
> >  	export GIT_PROVE_OPTS="--timer --jobs 10"
> >  	export GIT_TEST_OPTS="--verbose-log -x"
> >
> > base-commit: f72f328bc57e1b0db380ef76e0c1e94a9ed0ac7c
>
>

Re: [PATCH] ci: fix GitHub workflow when on a tagged revision

[Junio C Hamano]

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

> Hi Junio,
>
> On Fri, 24 Apr 2020, Junio C Hamano wrote:
>
>> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
>> writes:
>>
>> > However, our GitHub workflow does not trigger on tags, therefore, this
>> > logic results in a missing build for that revision.
>>
>> 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.
>
> 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)?

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.

[PATCH v2 0/2] Do not skip tagged revisions in the GitHub workflow runs

[Johannes Schindelin via GitGitGadget]

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
     + 

-- 
gitgitgadget

[PATCH v2 1/2] ci: skip GitHub workflow runs for already-tested commits/trees

[Johannes Schindelin via GitGitGadget]

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

diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
index 5bd321e5e1..7391b46d61 100644
--- a/.github/workflows/main.yml
+++ b/.github/workflows/main.yml
@@ -9,7 +9,7 @@ jobs:
   ci-config:
     runs-on: ubuntu-latest
     outputs:
-      enabled: ${{ steps.check-ref.outputs.enabled }}
+      enabled: ${{ steps.check-ref.outputs.enabled }}${{ steps.skip-if-redundant.outputs.enabled }}
     steps:
       - name: try to clone ci-config branch
         continue-on-error: true
@@ -35,6 +35,43 @@ jobs:
             enabled=no
           fi
           echo "::set-output name=enabled::$enabled"
+      - name: skip if the commit or tree was already tested
+        id: skip-if-redundant
+        uses: actions/github-script@v3
+        if: steps.check-ref.outputs.enabled == 'yes'
+        with:
+          github-token: ${{secrets.GITHUB_TOKEN}}
+          script: |
+            // Figure out workflow ID, commit and tree
+            const { data: run } = await github.actions.getWorkflowRun({
+              owner: context.repo.owner,
+              repo: context.repo.repo,
+              run_id: context.runId,
+            });
+            const workflow_id = run.workflow_id;
+            const head_sha = run.head_sha;
+            const tree_id = run.head_commit.tree_id;
+
+            // See whether there is a successful run for that commit or tree
+            const { data: runs } = await github.actions.listWorkflowRuns({
+              owner: context.repo.owner,
+              repo: context.repo.repo,
+              per_page: 500,
+              status: 'success',
+              workflow_id,
+            });
+            for (const run of runs.workflow_runs) {
+              if (head_sha === run.head_sha) {
+                core.warning(`Successful run for the commit ${head_sha}: ${run.html_url}`);
+                core.setOutput('enabled', ' but skip');
+                break;
+              }
+              if (tree_id === run.head_commit.tree_id) {
+                core.warning(`Successful run for the tree ${tree_id}: ${run.html_url}`);
+                core.setOutput('enabled', ' but skip');
+                break;
+              }
+            }
 
   windows-build:
     needs: ci-config
-- 
gitgitgadget

Re: [PATCH v2 1/2] ci: skip GitHub workflow runs for already-tested commits/trees

[SZEDER Gábor]

On Thu, Oct 08, 2020 at 03:29:34PM +0000, Johannes Schindelin via GitGitGadget wrote:
> 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).

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.

> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  .github/workflows/main.yml | 39 +++++++++++++++++++++++++++++++++++++-
>  1 file changed, 38 insertions(+), 1 deletion(-)
> 
> diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
> index 5bd321e5e1..7391b46d61 100644
> --- a/.github/workflows/main.yml
> +++ b/.github/workflows/main.yml
> @@ -9,7 +9,7 @@ jobs:
>    ci-config:
>      runs-on: ubuntu-latest
>      outputs:
> -      enabled: ${{ steps.check-ref.outputs.enabled }}
> +      enabled: ${{ steps.check-ref.outputs.enabled }}${{ steps.skip-if-redundant.outputs.enabled }}
>      steps:
>        - name: try to clone ci-config branch
>          continue-on-error: true
> @@ -35,6 +35,43 @@ jobs:
>              enabled=no
>            fi
>            echo "::set-output name=enabled::$enabled"
> +      - name: skip if the commit or tree was already tested
> +        id: skip-if-redundant
> +        uses: actions/github-script@v3
> +        if: steps.check-ref.outputs.enabled == 'yes'
> +        with:
> +          github-token: ${{secrets.GITHUB_TOKEN}}
> +          script: |
> +            // Figure out workflow ID, commit and tree
> +            const { data: run } = await github.actions.getWorkflowRun({
> +              owner: context.repo.owner,
> +              repo: context.repo.repo,
> +              run_id: context.runId,
> +            });
> +            const workflow_id = run.workflow_id;
> +            const head_sha = run.head_sha;
> +            const tree_id = run.head_commit.tree_id;
> +
> +            // See whether there is a successful run for that commit or tree
> +            const { data: runs } = await github.actions.listWorkflowRuns({
> +              owner: context.repo.owner,
> +              repo: context.repo.repo,
> +              per_page: 500,
> +              status: 'success',
> +              workflow_id,
> +            });
> +            for (const run of runs.workflow_runs) {
> +              if (head_sha === run.head_sha) {
> +                core.warning(`Successful run for the commit ${head_sha}: ${run.html_url}`);
> +                core.setOutput('enabled', ' but skip');
> +                break;
> +              }
> +              if (tree_id === run.head_commit.tree_id) {
> +                core.warning(`Successful run for the tree ${tree_id}: ${run.html_url}`);
> +                core.setOutput('enabled', ' but skip');
> +                break;
> +              }
> +            }
>  
>    windows-build:
>      needs: ci-config
> -- 
> gitgitgadget
> 

Re: [PATCH v2 1/2] ci: skip GitHub workflow runs for already-tested commits/trees

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 4415 bytes --]

Hi Gábor,

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

> On Thu, Oct 08, 2020 at 03:29:34PM +0000, Johannes Schindelin via GitGitGadget wrote:
> > 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).
>
> 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.

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

>
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >  .github/workflows/main.yml | 39 +++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 38 insertions(+), 1 deletion(-)
> >
> > diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
> > index 5bd321e5e1..7391b46d61 100644
> > --- a/.github/workflows/main.yml
> > +++ b/.github/workflows/main.yml
> > @@ -9,7 +9,7 @@ jobs:
> >    ci-config:
> >      runs-on: ubuntu-latest
> >      outputs:
> > -      enabled: ${{ steps.check-ref.outputs.enabled }}
> > +      enabled: ${{ steps.check-ref.outputs.enabled }}${{ steps.skip-if-redundant.outputs.enabled }}
> >      steps:
> >        - name: try to clone ci-config branch
> >          continue-on-error: true
> > @@ -35,6 +35,43 @@ jobs:
> >              enabled=no
> >            fi
> >            echo "::set-output name=enabled::$enabled"
> > +      - name: skip if the commit or tree was already tested
> > +        id: skip-if-redundant
> > +        uses: actions/github-script@v3
> > +        if: steps.check-ref.outputs.enabled == 'yes'
> > +        with:
> > +          github-token: ${{secrets.GITHUB_TOKEN}}
> > +          script: |
> > +            // Figure out workflow ID, commit and tree
> > +            const { data: run } = await github.actions.getWorkflowRun({
> > +              owner: context.repo.owner,
> > +              repo: context.repo.repo,
> > +              run_id: context.runId,
> > +            });
> > +            const workflow_id = run.workflow_id;
> > +            const head_sha = run.head_sha;
> > +            const tree_id = run.head_commit.tree_id;
> > +
> > +            // See whether there is a successful run for that commit or tree
> > +            const { data: runs } = await github.actions.listWorkflowRuns({
> > +              owner: context.repo.owner,
> > +              repo: context.repo.repo,
> > +              per_page: 500,
> > +              status: 'success',
> > +              workflow_id,
> > +            });
> > +            for (const run of runs.workflow_runs) {
> > +              if (head_sha === run.head_sha) {
> > +                core.warning(`Successful run for the commit ${head_sha}: ${run.html_url}`);
> > +                core.setOutput('enabled', ' but skip');
> > +                break;
> > +              }
> > +              if (tree_id === run.head_commit.tree_id) {
> > +                core.warning(`Successful run for the tree ${tree_id}: ${run.html_url}`);
> > +                core.setOutput('enabled', ' but skip');
> > +                break;
> > +              }
> > +            }
> >
> >    windows-build:
> >      needs: ci-config
> > --
> > gitgitgadget
> >
>

Re: [PATCH v2 1/2] ci: skip GitHub workflow runs for already-tested commits/trees

[SZEDER Gábor]

On Fri, Oct 09, 2020 at 01:13:03PM +0200, Johannes Schindelin wrote:
> Hi Gábor,
> 
> On Fri, 9 Oct 2020, SZEDER Gábor wrote:
> 
> > On Thu, Oct 08, 2020 at 03:29:34PM +0000, Johannes Schindelin via GitGitGadget wrote:
> > > 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).
> >
> > 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.
> 
> 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`?

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

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

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.

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

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.

Re: [PATCH v2 1/2] ci: skip GitHub workflow runs for already-tested commits/trees

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 1532 bytes --]

Hi Gábor,

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

> On Fri, Oct 09, 2020 at 01:13:03PM +0200, Johannes Schindelin wrote:
>
> > 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...
>
> 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.

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

Re: [PATCH v2 1/2] ci: skip GitHub workflow runs for already-tested commits/trees

[Junio C Hamano]

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

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

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?

Re: [PATCH v2 1/2] ci: skip GitHub workflow runs for already-tested commits/trees

[Johannes Schindelin]

Hi Junio,

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

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > 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).
>
> 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. [...] it would have made the intention more clear if the
> conditional at the beginning of skip_good_tree() were adjusted, perhaps?

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

Re: [PATCH v2 1/2] ci: skip GitHub workflow runs for already-tested commits/trees

[Junio C Hamano]

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

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

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.

Re: [PATCH v2 1/2] ci: skip GitHub workflow runs for already-tested commits/trees

[Johannes Schindelin]

Hi Junio,

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

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > 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).
>
> 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.

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

Ciao,
Dscho

[PATCH v2 2/2] ci: do not skip tagged revisions in GitHub workflows

[Johannes Schindelin via GitGitGadget]

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(+)

diff --git a/ci/lib.sh b/ci/lib.sh
index 3eefec500d..79f81563cb 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -149,6 +149,7 @@ then
 	CI_REPO_SLUG="$GITHUB_REPOSITORY"
 	CI_JOB_ID="$GITHUB_RUN_ID"
 	CC="${CC:-gcc}"
+	DONT_SKIP_TAGS=t
 
 	cache_dir="$HOME/none"
 
@@ -167,6 +168,7 @@ good_trees_file="$cache_dir/good-trees"
 
 mkdir -p "$cache_dir"
 
+test -n "${DONT_SKIP_TAGS-}" ||
 skip_branch_tip_with_tag
 skip_good_tree
 
-- 
gitgitgadget

Re: [PATCH v2 0/2] Do not skip tagged revisions in the GitHub workflow runs

[Junio C Hamano]

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

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

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.