ci: Provide `COMMIT_RANGE` variable for non-PRs #20728

hebasto · 2020-12-20T11:12:37Z

This PR provides the COMMIT_RANGE environment variable, which is used in lint-git-commit-check.py and lint-whitespace.py, for non-PR cases, including our release branches and personal repos.

Fixes https://github.com/bitcoin/bitcoin/runs/8664441400

Has been split out from #20697 as requested by MarcoFalke.

hebasto · 2020-12-21T11:27:21Z

Rebased on top of #20697.

DrahtBot · 2021-01-13T11:46:25Z

🕵️ @practicalswift has been requested to review this pull request as specified in the REVIEWERS file.

decryp2kanon · 2021-01-19T18:35:18Z

Concept ACK

fanquake · 2021-02-28T05:18:54Z

@MarcoFalke can you take a look here. It seems this should either be straight forward to merge, or can just be closed. I think complicating our CI further, just so people can run the linter in the CI of their forked repos is very low priority, and they can always just run it locally.

fanquake · 2021-03-26T07:07:59Z

Closing for now.

hebasto · 2022-05-28T09:18:21Z

Reopened due to the recent message on IRC:

02:52 <DavidBakin> so I am now running CI tests on a branch on my fork. lint fails in lint-git-commit-check.py with "Not a valid object name master" - is this something I did in my fork when creating my branch? or what? there isn't any other useful information printed here, e.g., commit hash. I'll try to run it locally to see if I can debug it but if you happen to know ...

Also rebased.

cc @MarcoFalke @david-bakin

fanquake · 2022-10-03T11:36:19Z

This was reopened ~6 months ago, but has received no additional comments / interest. I still think that adding more code to our CI scripts, to make it easier for someone to run the lint job via cirrus in their own fork is ~0.

maflcko · 2022-10-03T11:44:58Z

Doesn't this also affect non-master branches in our repo?

https://github.com/bitcoin/bitcoin/runs/8664441400

jonatack · 2022-10-03T11:46:10Z

Thanks for bringing this PR up in my notifications. I used to run our CI on Travis before we migrated to Cirrus and it was useful to check that CI would pass before opening a PR. If this enables doing the same with no downside apart from these two lines I would be Concept ACK.

fanquake · 2022-10-03T11:46:27Z

Doesn't this also affect non-master branches in our repo?

Not sure. If it does, that should be in the PR description, and the primary reason for making this change.

maflcko · 2022-10-03T11:49:25Z

ci/lint/06_script.sh

@@ -10,6 +10,8 @@ GIT_HEAD=$(git rev-parse HEAD)
 if [ -n "$CIRRUS_PR" ]; then
  COMMIT_RANGE="${CIRRUS_BASE_SHA}..$GIT_HEAD"
  test/lint/commit-script-check.sh "$COMMIT_RANGE"
+else
+  COMMIT_RANGE="$(git fetch "$CIRRUS_REPO_CLONE_URL" "${CIRRUS_LAST_GREEN_CHANGE:-$CIRRUS_DEFAULT_BRANCH}" && git merge-base FETCH_HEAD HEAD)..$GIT_HEAD"


wouldn't it be better to skip the check if this is not a pull request? I presume this already happens when pushing to master?

Could you clarify please what code change you are suggesting?

I don't know the code changes needed, but it seems the check currently only runs on pull requests against the base commit.

On master it is presumably skipped because the base commit is equal to the HEAD commit?

So maybe all places where COMMIT_RANGE is used can be guarded by a check on CIRRUS_PR? Or instead skip if the range is empty?

it seems the check currently only runs on pull requests against the base commit.

ci/lint/06_script.sh runs always except cases defined in the FILTER_TEMPLATE.

On master it is presumably skipped because the base commit is equal to the HEAD commit?

According to Cirrus CI docs, the CIRRUS_BASE_SHA variable is defined for PRs only. Therefore, it is natural to have distinct code paths to evaluate COMMIT_RANGE for PRs and non-PRs.

So maybe all places where COMMIT_RANGE is used can be guarded by a check on CIRRUS_PR?

Given COMMIT_RANGE is used in two python scripts, it's okay to evaluate it before test/lint/lint-all.py. That is the case for the current ci/lint/06_script.sh file.

Or instead skip if the range is empty?

I cannot figure out such cases.

Yes, I understand. My point is that there is no point in running COMMIT_RANGE tests on branches, only on PRs

The CIRRUS_DEFAULT_BRANCH is being used in the git fetch command, which should succeed regardless of the current branch in the git tree.

CI fails in the 24.x branch because the COMMIT_RANGE variable is not set at all, and the failed python scripts use the hardcoded "master" name of a branch. For example:

bitcoin/test/lint/lint-whitespace.py

Lines 92 to 101 in ba48fcf

if not os.getenv("COMMIT_RANGE"):

if args.prev_commits:

commit_range = "HEAD~" + args.prev_commits + "...HEAD"

else:

# This assumes that the target branch of the pull request will be master.

merge_base = check_output(["git", "merge-base", "HEAD", "master"], universal_newlines=True, encoding="utf8").rstrip("\n")

commit_range = merge_base + "..HEAD"

else:

commit_range = os.getenv("COMMIT_RANGE")

I believe, such an approach won't leave un-linted commits in the commit history.

yes, maybe.

However, I still don't understand why this is needed.

If you run the linters on a branch, it is too late, because force pushing is not allowed.

For example, it is not possible to rewrite the commit subject line if test/lint/lint-git-commit-check.py fails on a commit in a non-pull-request branch. (Same for whitespace from test/lint/lint-whitespace.py, or the eval from test/lint/commit-script-check.sh). There are no other uses of COMMIT_RANGE other than those 3.

If we didn't sanity check verify-commits.py on branches, I would have already completely disabled the linters for branches.

If you run the linters on a branch, it is too late, because force pushing is not allowed.

For example, it is not possible to rewrite the commit subject line if test/lint/lint-git-commit-check.py fails on a commit in a non-pull-request branch. (Same for whitespace from test/lint/lint-whitespace.py, or the eval from test/lint/commit-script-check.sh). There are no other uses of COMMIT_RANGE other than those 3.

There is still a use case when running CI in personal repos though.

I would have already completely disabled the linters for branches.

That sounds reasonable. But again, this change allows to use the linter CI task in personal repos.

Can you explain that use case? It seems fragile to support that, given that CIRRUS_LAST_GREEN_CHANGE might "accidentally" be green for "broken" commits (for example if the CI was skipped, etc...). Or CIRRUS_DEFAULT_BRANCH might be too far ahead, so that commits are "skipped".

So to summarize, I fail to see the use case, and even if there was a use case, it would be impossible to support it.

hebasto · 2022-10-05T12:00:14Z

@MarcoFalke

Doesn't this also affect non-master branches in our repo?

https://github.com/bitcoin/bitcoin/runs/8664441400

It does.

@fanquake

Not sure. If it does, that should be in the PR description, and the primary reason for making this change.

The PR description has been reworked.

hebasto · 2022-10-20T16:41:10Z

@MarcoFalke @fanquake

Mind having another look into this PR?

DrahtBot · 2022-11-22T13:26:52Z

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	decryp2kanon, jonatack

maflcko · 2022-11-23T12:45:01Z

ci/lint/06_script.sh

@@ -10,6 +10,8 @@ GIT_HEAD=$(git rev-parse HEAD)
 if [ -n "$CIRRUS_PR" ]; then
  COMMIT_RANGE="${CIRRUS_BASE_SHA}..$GIT_HEAD"
  test/lint/commit-script-check.sh "$COMMIT_RANGE"
+else
+  COMMIT_RANGE="$(git fetch "$CIRRUS_REPO_CLONE_URL" "${CIRRUS_LAST_GREEN_CHANGE:-$CIRRUS_DEFAULT_BRANCH}" && git merge-base FETCH_HEAD HEAD)..$GIT_HEAD"


My suggestion would be to simply set COMMIT_RANGE="EMPTY" and then use that to skip the affected linters

hebasto force-pushed the 201220-cirrus branch 4 times, most recently from 759b54b to b1c1c4c Compare December 20, 2020 11:50

hebasto mentioned this pull request Dec 20, 2020

ci: Fix COMMIT_RANGE variable value for cloned repos #20717

Closed

hebasto changed the title ~~[DO NOT MERGE] ci: Debugging Cirrus CI~~ ci: Fix COMMIT_RANGE variable value for cloned repos Dec 20, 2020

DrahtBot added the Tests label Dec 20, 2020

hebasto mentioned this pull request Dec 20, 2020

ci: Fix COMMIT_RANGE variable value for PRs #20697

Closed

hebasto force-pushed the 201220-cirrus branch from b1c1c4c to 59b948c Compare December 21, 2020 07:23

hebasto marked this pull request as draft December 21, 2020 07:35

hebasto force-pushed the 201220-cirrus branch 4 times, most recently from a6fa882 to 4f1ca2a Compare December 21, 2020 08:52

hebasto marked this pull request as ready for review December 21, 2020 09:28

hebasto force-pushed the 201220-cirrus branch 2 times, most recently from 206f455 to c8488e0 Compare December 21, 2020 10:14

hebasto marked this pull request as draft December 21, 2020 10:16

hebasto force-pushed the 201220-cirrus branch 4 times, most recently from c8488e0 to 0512c2d Compare December 21, 2020 11:25

hebasto marked this pull request as ready for review December 21, 2020 11:27

fanquake requested a review from maflcko February 28, 2021 05:17

fanquake closed this Mar 26, 2021

ci: Fix COMMIT_RANGE variable value for cloned repos

d4fb891

hebasto reopened this May 28, 2022

hebasto force-pushed the 201220-cirrus branch from 0512c2d to d4fb891 Compare May 28, 2022 09:17

hebasto changed the title ~~ci: Fix COMMIT_RANGE variable value for cloned repos~~ ci: Fix COMMIT_RANGE variable value for cloned repos May 28, 2022

maflcko reviewed Oct 3, 2022

View reviewed changes

hebasto changed the title ~~ci: Fix COMMIT_RANGE variable value for cloned repos~~ ci: Provide COMMIT_RANGE variable for non-PRs Oct 5, 2022

hebasto closed this Nov 23, 2022

maflcko reviewed Nov 23, 2022

View reviewed changes

hebasto mentioned this pull request Nov 26, 2022

CI: Bump CI Lint base image from bionic to jammy #26581

Closed

hebasto deleted the 201220-cirrus branch December 3, 2022 18:13

bitcoin locked and limited conversation to collaborators Dec 3, 2023

	if not os.getenv("COMMIT_RANGE"):
	if args.prev_commits:
	commit_range = "HEAD~" + args.prev_commits + "...HEAD"
	else:
	# This assumes that the target branch of the pull request will be master.
	merge_base = check_output(["git", "merge-base", "HEAD", "master"], universal_newlines=True, encoding="utf8").rstrip("\n")
	commit_range = merge_base + "..HEAD"
	else:
	commit_range = os.getenv("COMMIT_RANGE")

ci: Provide COMMIT_RANGE variable for non-PRs #20728

ci: Provide COMMIT_RANGE variable for non-PRs #20728

Uh oh!

Conversation

hebasto commented Dec 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto commented Dec 21, 2020

Uh oh!

DrahtBot commented Jan 13, 2021

Uh oh!

decryp2kanon commented Jan 19, 2021

Uh oh!

fanquake commented Feb 28, 2021

Uh oh!

fanquake commented Mar 26, 2021

Uh oh!

hebasto commented May 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fanquake commented Oct 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Oct 3, 2022

Uh oh!

jonatack commented Oct 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fanquake commented Oct 3, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto Oct 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto commented Oct 5, 2022

Uh oh!

hebasto commented Oct 20, 2022

Uh oh!

DrahtBot commented Nov 22, 2022

Reviews

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ci: Provide `COMMIT_RANGE` variable for non-PRs #20728

ci: Provide `COMMIT_RANGE` variable for non-PRs #20728

hebasto commented Dec 20, 2020 •

edited

Loading

hebasto commented May 28, 2022 •

edited

Loading

fanquake commented Oct 3, 2022 •

edited

Loading

jonatack commented Oct 3, 2022 •

edited

Loading

hebasto Oct 14, 2022 •

edited

Loading