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
  1. 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
  2. hebasto marked this as a draft on Dec 18, 2020
  3. 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.
  4. 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.
  5. hebasto force-pushed on Dec 18, 2020
  6. 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
  7. hebasto force-pushed on Dec 18, 2020
  8. 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.
  9. DrahtBot added the label Tests on Dec 18, 2020
  10. 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.

  11. hebasto force-pushed on Dec 20, 2020
  12. hebasto commented at 12:19 pm on December 20, 2020: member

    Updated 144250e3f055d47d18379b01056e9d3eb11e9231 -> 83c07d1e3f8809183cce4ca88176d700249e9f55 (pr20697.03 -> pr20697.04):

    Maybe submit this as a separate pull? ;)

    See: #20728.

    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
  13. hebasto marked this as ready for review on Dec 20, 2020
  14. hebasto renamed this:
    ci: Fix COMMIT_RANGE variable value
    ci: Fix COMMIT_RANGE variable value for PRs
    on Dec 20, 2020
  15. 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.
  16. MarcoFalke approved
  17. MarcoFalke commented at 7:08 am on December 21, 2020: member
    ACK
  18. hebasto force-pushed on Dec 21, 2020
  19. 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

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

  21. hebasto force-pushed on Dec 21, 2020
  22. MarcoFalke commented at 11:07 am on December 21, 2020: member
    ACK 3c2478c38522c176e81befd4d991a259b09be063
  23. ci: Fix COMMIT_RANGE variable value for PRs 1b16ac36cf
  24. ci: Drop Travis-specific way to set COMMIT_RANGE variable 46c96d615b
  25. ci: Drop Travis-specific workaround for shellcheck 31cb012368
  26. ci: Print COMMIT_RANGE to the log as it was in Travis CI f89d374069
  27. hebasto force-pushed on Dec 21, 2020
  28. MarcoFalke referenced this in commit f061da2887 on Dec 21, 2020
  29. hebasto commented at 11:10 am on December 21, 2020: member

    Updated b4cd93d6f3e822365b107ccd5f0426fef31b7156 -> f89d374069e2c4b7897d4b95f69876fc42604550 (pr20697.05 -> pr20697.07, diff):

    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.

  30. MarcoFalke closed this on Dec 21, 2020
  31. MarcoFalke commented at 11:11 am on December 21, 2020: member
    This was merged two seconds before you force pushed :sweat_smile:
  32. hebasto commented at 11:12 am on December 21, 2020: member

    This was merged two seconds before you force pushed sweat_smile

    slow typing :)

  33. sidhujag referenced this in commit 885721f23e on Dec 21, 2020
  34. fanquake locked this on Feb 22, 2021


hebasto MarcoFalke DrahtBot

Labels
Tests

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-22 12:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me