The sanity check to check the last few merge commit signatures on the main branch was accidentally and silently disabled while moving from the cirrus-ci.com platform to the GHA platform.
So fix that by re-enabling it.
Also, contains a few other lint cleanup commits.
Moving the python code out of the yaml string makes it easier to lint,
format, and edit.
This can be reviewed with the git options:
--color-moved=dimmed-zebra --color-moved-ws=ignore-all-space
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33888.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | janb84, willcl-ark |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
while moving from cirrus-ci to GHA.
Just to clarify, is this for the forked repos that run on GHA because the main repo still runs on Cirrus runners right ? see : https://github.com/bitcoin/bitcoin/actions/runs/19437220743/job/55611173427?pr=33888#step:1:2
while moving from cirrus-ci to GHA.
Just to clarify, is this for the forked repos that run on GHA because the main repo still runs on Cirrus runners right ?
No, it is about the platform switch from cirrus-ci.com to the GHA platform, which goes through github.com. The GHA runner type should be unrelated.
36+ run(build_cmd)
37+
38+ extra_env = []
39+ if os.environ.get("GITHUB_EVENT_NAME") == "pull_request":
40+ extra_env = ["--env", "LINT_CI_IS_PR=1"]
41+ if os.environ.get("GITHUB_REPOSITORY") == "bitcoin/bitcoin" and os.environ.get("GITHUB_EVENT_NAME") != "pull_request":
NIT (Maybe) switch order of the 2 conditions in the if statement. It’s slightly more optimized and it communicates the intent better of setting one or the other extra_env.
0 if os.environ.get("GITHUB_EVENT_NAME") != "pull_request" and os.environ.get("GITHUB_REPOSITORY") == "bitcoin/bitcoin":
os.environ[key] lookup, to get KeyError on key-typos or missing keys in the future
ACK fa6138993f01b548d811634073e97443f824c82b
PR fixes /should fix silent skipping of the sanity check In addition to this check it moves python code to it’s own file from the CI.yml file. This change improves readability and discoverability (imo). Small non-blocking NIT on the order of conditions of the second if statement.
The PR also changes the Lint readme to correct the example docker build command. I’ve tested this change on MacOS and the command /build /run works as expected.
The CIRRUS_PR env var was cirrus-specific and using a provider-agnostic
name makes more sense.
Also, enable pipefail, while touching this file.
This refactor is needed for the next commit.
With the move from cirrus-ci to GHA, the CIRRUS_REPO_FULL_NAME env var
was always unset, never triggering the sanity check.
Fix this by introducing a new vendor-agnostic env var and setting it
properly.
This is required to pick the native arch, similar to how the
CI_IMAGE_PLATFORM is set to linux.
re ACK 55555db055b59dd529526915dfc59e5a13e43160
changes since last ACK:
os.environ[key] lookupthanks for incorporating my suggestion !
ACK 55555db055b59dd529526915dfc59e5a13e43160
Nice catch! The move looks good with dimmed-zebra and the succeeding updates also look correct.
LINT_CI_IS_PR is correctly set in the job.
As there was a break in this check, I checked the top ~ 500 commits (back to May) locally for good measure, which were all signed.
0src/core/bitcoin on pr-33888 [$!?] via △ v4.1.2 via 🐍 v3.13.9 via ❄️ impure (nix-shell-env)
1❯ git log HEAD~500 -1 --format='%H' > ./contrib/verify-commits/trusted-sha512-root-commit
2
3src/core/bitcoin on pr-33888 [$!?] via △ v4.1.2 via 🐍 v3.13.9 via ❄️ impure (nix-shell-env)
4❯ git log HEAD~500 -1 --format='%H' > ./contrib/verify-commits/trusted-git-root
5
6src/core/bitcoin on pr-33888 [$!?] via △ v4.1.2 via 🐍 v3.13.9 via ❄️ impure (nix-shell-env) took 12s
7❯ ./contrib/verify-commits/verify-commits.py $(git log --merges -1 --format='%H')
8Using verify-commits data from /home/will/src/core/bitcoin/contrib/verify-commits
9There is a valid path from "ead849c9f177a3a175a22b35fa864b4b37fb9934" to 87860143be792d219aac7f4a04e79d00016df627 where all commits are signed!
Side-note: ruff check is happy with .github/ci-lint-exec.py but ruff format suggests some changes. I know we don’t enforce this, but would be happy to re-ACK with a final commit ruff-formatting the file too :)
Side-note:
ruff checkis happy with.github/ci-lint-exec.pybutruff formatsuggests some changes. I know we don’t enforce this, but would be happy to re-ACK with a final commitruff-formatting the file too :)
I tried formatting with black and yapf, but they contradicted each other, so I haven’t tried ruff format and I’ll leave this as-is for now. :sweat_smile:
Looks like it worked, fwiw: https://github.com/bitcoin/bitcoin/actions/runs/19545916567/job/55966862251#step:6:151:
0+ '[' 1 = 1 ']'
1+ git log HEAD~10 -1 --format=%H
2+ git log HEAD~10 -1 --format=%H
3+ mapfile -t KEYS
4+ git config user.email ci@ci.ci
5+ git config user.name ci
6+ gpg --keyserver hkps://keys.openpgp.org --recv-keys E777299FC265DD04793070EB944D35F9AC3DB76A D1DBF2C4B96F2DEBF4C16654410108112E7EA81F 152812300785C96444D3334D17565732E08E5E41 6B002C6EA3F91B1B0DF0C9BC8F617F1200A6D25C 4D1B3D5ECBA1A7E05371EEBE46800E30FC748A66
7gpg: directory '/root/.gnupg' created
8gpg: keybox '/root/.gnupg/pubring.kbx' created
9gpg: /root/.gnupg/trustdb.gpg: trustdb created
10...
11gpg: Total number processed: 5
12gpg: imported: 5
13+ ./contrib/verify-commits/verify-commits.py
14Using verify-commits data from /bitcoin/contrib/verify-commits
15There is a valid path from "HEAD" to b126f981943de0ddcc50a8f7f79a447f1f45cf60 where all commits are signed!