ci: Fix COMMIT_RANGE variable value for PRs #20697

pull hebasto wants to merge 4 commits into bitcoin:master from hebasto:201218-range changing 6 files +6 −36

hebasto commented at 12:19 PM on December 18, 2020: member
This PR:
- is a #20658 and #20682 followup
- set the COMMIT_RANGE variable correctly for PRs
- cleans up Travis-specific code
- prints COMMIT_RANGE value to the log for convenience as it was in Travis CI
hebasto marked this as a draft on Dec 18, 2020
in ci/lint/06_script.sh:13 in 7f8643bcb9 outdated
```
  12 | -  # https://cirrus-ci.org/guide/writing-tasks/#environment-variables
  13 | -  COMMIT_RANGE="$CIRRUS_BRANCH..HEAD"
  14 | +  COMMIT_RANGE="$CIRRUS_BASE_BRANCH..HEAD"
  15 |    test/lint/commit-script-check.sh $COMMIT_RANGE
  16 | +else
  17 | +  COMMIT_RANGE="$(git fetch $CIRRUS_REPO_CLONE_URL $CIRRUS_LAST_GREEN_CHANGE && git merge-base $CIRRUS_LAST_GREEN_CHANGE HEAD)..HEAD"
```
MarcoFalke commented at 12:29 PM on December 18, 2020:

we don't run the linters on any branch because it is too late to fixup any issues that have been merged

hebasto commented at 12:35 PM on December 18, 2020:

Correct. But this is for forked repos and personal Cirrus accounts.

test/lint/lint-git-commit-check.sh and test/lint/lint-whitespace.sh work in all scenarios now.

MarcoFalke commented at 1:20 PM on December 18, 2020:

Maybe submit this as a separate pull? ;)

hebasto commented at 12:20 PM on December 20, 2020:

Thanks! Done.
MarcoFalke commented at 12:31 PM on December 18, 2020: member

this still "fails" because master doesn't exist when the repo was cloned with go. I think creating the master branch after git fetch will do.
hebasto force-pushed on Dec 18, 2020
in ci/lint/05_before_script.sh:9 in 9d17a1498a outdated
```
   5 | @@ -6,4 +6,4 @@
   6 |  
   7 |  export LC_ALL=C
   8 |  
   9 | -git fetch
  10 | +git fetch $CIRRUS_REPO_CLONE_URL $CIRRUS_BASE_BRANCH
```
MarcoFalke commented at 12:47 PM on December 18, 2020:

The command to create a branch would be git branch master origin/master
hebasto force-pushed on Dec 18, 2020
in ci/lint/05_before_script.sh:10 in 144250e3f0 outdated
```
   5 | @@ -6,4 +6,5 @@
   6 |  
   7 |  export LC_ALL=C
   8 |  
   9 | -git fetch
  10 | +git fetch origin
  11 | +git branch $CIRRUS_BASE_BRANCH origin/$CIRRUS_BASE_BRANCH
```
MarcoFalke commented at 1:24 PM on December 18, 2020:

needs to be guarded by if [ -n "$CIRRUS_PR" ]; then

hebasto commented at 12:20 PM on December 20, 2020:

Thanks! Updated.
DrahtBot added the label Tests on Dec 18, 2020
DrahtBot commented at 3:25 AM on December 19, 2020: member



The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Conflicts

No conflicts as of last run.
hebasto force-pushed on Dec 20, 2020
hebasto commented at 12:19 PM on December 20, 2020: member
Updated 144250e3f055d47d18379b01056e9d3eb11e9231 -> 83c07d1e3f8809183cce4ca88176d700249e9f55 (pr20697.03 -> pr20697.04):
- addressed @MarcoFalke's comment:
Maybe submit this as a separate pull? ;)

See: #20728.
- addressed @MarcoFalke's comment:
needs to be guarded by if [ -n "$CIRRUS_PR" ]; then
- added printing of the COMMIT_RANGE variable value to the log as it was in Travis CI
hebasto marked this as ready for review on Dec 20, 2020
hebasto renamed this:
~~ci: Fix COMMIT_RANGE variable value~~
ci: Fix COMMIT_RANGE variable value for PRs
on Dec 20, 2020
in ci/lint/06_script.sh:11 in 83c07d1e3f outdated
```
  10 |  if [ -n "$CIRRUS_PR" ]; then
  11 | -  # CIRRUS_PR will be present in a Cirrus environment. For builds triggered
  12 | -  # by a pull request this is the name of the branch targeted by the pull request.
  13 | -  # https://cirrus-ci.org/guide/writing-tasks/#environment-variables
  14 | -  COMMIT_RANGE="$CIRRUS_BRANCH..HEAD"
  15 | +  git fetch $CIRRUS_REPO_CLONE_URL $CIRRUS_BASE_SHA
```
MarcoFalke commented at 7:06 AM on December 21, 2020:

Is this needed?

hebasto commented at 7:15 AM on December 21, 2020:

Actually, it replaces the git fetch command from the removed ci/lint/05_before_script.sh

Oh, I see. The CIRRUS_BASE_SHA must already be in the commit history for pulls, right? I'll push another change without this command.

hebasto commented at 9:25 AM on December 21, 2020:

Thanks! Updated.
MarcoFalke approved
MarcoFalke commented at 7:08 AM on December 21, 2020: member

ACK
hebasto force-pushed on Dec 21, 2020
hebasto commented at 9:24 AM on December 21, 2020: member
Updated 83c07d1e3f8809183cce4ca88176d700249e9f55 -> b4cd93d6f3e822365b107ccd5f0426fef31b7156 (pr20697.04 -> pr20697.05, diff):
- addressed @MarcoFalke's comment
- fixed COMMIT_RANGE value when a changed PR is force pushed into the base branch that has already grown, i.e., the pull is effectively based on the non-top commit of the base branch
- added git log command to the lint task log
in ci/lint/06_script.sh:11 in b4cd93d6f3 outdated
```
  10 |  if [ -n "$CIRRUS_PR" ]; then
  11 | -  # CIRRUS_PR will be present in a Cirrus environment. For builds triggered
  12 | -  # by a pull request this is the name of the branch targeted by the pull request.
  13 | -  # https://cirrus-ci.org/guide/writing-tasks/#environment-variables
  14 | -  COMMIT_RANGE="$CIRRUS_BRANCH..HEAD"
  15 | +  COMMIT_RANGE="$(git merge-base $CIRRUS_BASE_SHA $CIRRUS_CHANGE_IN_REPO)..$GIT_HEAD"
```
MarcoFalke commented at 9:55 AM on December 21, 2020:

Why is this changed again? Now it will include master commits, because master has already been merged in for pull requests (see .cirrus.yml)

COMMIT_RANGE="${CIRRUS_BASE_SHA}..$GIT_HEAD" was correct.

hebasto commented at 10:00 AM on December 21, 2020:

... master has already been merged in for pull requests (see .cirrus.yml)

Right.

For described case "when a changed PR is force pushed into the base branch that has already grown, i.e., the pull is effectively based on the non-top commit of the base branch" I think COMMIT_RANGE="${CIRRUS_BASE_SHA}..$GIT_HEAD" is wrong, as $CIRRUS_BASE_SHA is not the commit on which the pull is based on.

MarcoFalke commented at 10:17 AM on December 21, 2020:

$CIRRUS_BASE_SHA is not the commit on which the pull is based on.

It is the commit that this pull is de-facto based on, assuming that git merge A is approximately doing the same like git rebase A.

As I mentioned previously the current version is broken and will include all commits that have been pushed to master since. See:

https://cirrus-ci.com/task/5396234322051072?command=lint#L931

hebasto commented at 10:24 AM on December 21, 2020:

In .cirrus.yml the base branch is merged into the pull one. That's why I think that listed commits are correct COMMIT_RANGE to lint.

OTOH, we could merge the pull branch into the base one. In that case $CIRRUS_BASE_SHA will work as documented.

MarcoFalke commented at 10:29 AM on December 21, 2020:

That's why I think that listed commits are correct COMMIT_RANGE to lint.

It is not the way it worked on travis

hebasto commented at 10:31 AM on December 21, 2020:

Agree.

Is it acceptable to s/git merge A/git rebase A/ ?

hebasto commented at 10:56 AM on December 21, 2020:

I've tested COMMIT_RANGE="${CIRRUS_BASE_SHA}..$GIT_HEAD". It works for all cases. git is smart enough to know what to do :)

But for a human the value of COMMIT_RANGE is a bit cryptic (regarding to pull commits) in cases of a pull based on the non-top commit of the base branch. So going to skip output of it.

MarcoFalke commented at 10:58 AM on December 21, 2020:

Is it acceptable to s/git merge A/git rebase A/ ?

In theory yes, in practise it doesn't work with subtree bumps.
hebasto force-pushed on Dec 21, 2020
MarcoFalke commented at 11:07 AM on December 21, 2020: member

ACK 3c2478c38522c176e81befd4d991a259b09be063
ci: Fix COMMIT_RANGE variable value for PRs 1b16ac36cf
ci: Drop Travis-specific way to set COMMIT_RANGE variable 46c96d615b
ci: Drop Travis-specific workaround for shellcheck 31cb012368
ci: Print COMMIT_RANGE to the log as it was in Travis CI f89d374069
hebasto force-pushed on Dec 21, 2020
MarcoFalke referenced this in commit f061da2887 on Dec 21, 2020
hebasto commented at 11:10 AM on December 21, 2020: member
Updated b4cd93d6f3e822365b107ccd5f0426fef31b7156 -> f89d374069e2c4b7897d4b95f69876fc42604550 (pr20697.05 -> pr20697.07, diff):
- addressed @MarcoFalke's comment:
Why is this changed again? Now it will include master commits, because master has already been merged in for pull requests (see .cirrus.yml)

COMMIT_RANGE="${CIRRUS_BASE_SHA}..$GIT_HEAD" was correct.
MarcoFalke closed this on Dec 21, 2020
MarcoFalke commented at 11:11 AM on December 21, 2020: member

This was merged two seconds before you force pushed :sweat_smile:
hebasto commented at 11:12 AM on December 21, 2020: member

This was merged two seconds before you force pushed sweat_smile

slow typing :)
sidhujag referenced this in commit 885721f23e on Dec 21, 2020
fanquake locked this on Feb 22, 2021

Contributors

Labels

Linked (view graph)

#20658 ci: Move linter task to cirrus #20682 ci: Install missing lint packages #20716 ci: Fix COMMIT_RANGE variable value for cloned repos #20717 ci: Fix COMMIT_RANGE variable value for cloned repos #20728 ci: Provide `COMMIT_RANGE` variable for non-PRs

ci: Fix COMMIT_RANGE variable value for PRs #20697

Conflicts