Fix issues with CI on forks #29274
Fix issues with CI on forks #29274
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsNo conflicts as of last run. |
.cirrus.yml
Outdated
| @@ -161,11 +172,13 @@ task: | |||
|
|
|||
| task: | |||
| name: 'ASan + LSan + UBSan + integer, no depends, USDT' | |||
| # Skip if no Noble worker exists (for a fork) | |||
There was a problem hiding this comment.
Is this needed? Won't this be skipped already by default if the worker is offline?
There was a problem hiding this comment.
It stays pending, at least in the Cirrus interface.
There was a problem hiding this comment.
It stays pending, at least in the Cirrus interface.
But that seems preferable to me, as it communicates clearly that the task didn't run.
There was a problem hiding this comment.
Trying that UX in Sjors#36 ...
There was a problem hiding this comment.
On the first push of a branch, due to the UI glitched described below (#29274 (comment)) they show as skipped:
That seems fine. On the second push they are put in "queued sate" (forever?).
I agree that hiding the task altogether is not ideal (as ecf01f4 does). Perhaps marking it as skipped is better.
There was a problem hiding this comment.
Well for one thing it's way faster (using just an idle desktop computer).
There was a problem hiding this comment.
It's also not this black and white. Sure, Microsoft and Cirrus can screw us over by faking the CI result. But by not relying on them for hosting for the runners, it's both easier to catch them doing shenanigans and it's easier to move elsewhere.
There was a problem hiding this comment.
easier to catch them
Why? Someone will have to run a fully local backup either way, whose cost is independent of what is running on the other side.
easier to move elsewhere
Why? Moving elsewhere also requires moving the repo off of the GitHub mirror, or writing a GH integration and a CI framework from scratch, both of whose costs are largely independent of where the CI CPUs happen to sit.
There was a problem hiding this comment.
If I run a self-hosted CI I can see what it's doing. If it fails on my machine and is marked as successful on Github, I can see that. If I collude with Github that's a different story of course.
There was a problem hiding this comment.
The CI cleans up after itself, latest when the next run starts, so I don't think it is reliably possible to inspect a failure of a remote-triggered CI run locally, even assuming no "attack" from GH.
More importantly, the CI machine is literally doing RCE of downloaded binary executables and random code, so reliably detecting a test failure (assuming a GH attack) seems questionable at best.
.github/workflows/ci.yml
Outdated
| # Do not run branch pushes on forks, because they are typically redundant with | ||
| # pull requests to the main repo. | ||
| - 'bitcoin/**' | ||
| - 'bitcoin-core/**' |
There was a problem hiding this comment.
No objection, but I presume this will cause issues on forks (Inquisition, Knots, ...)
There was a problem hiding this comment.
Good point, this probably needs an ENV flag.
There was a problem hiding this comment.
Knots pull requests seem to be typically made against the 25.x-knots branch. Those would run normally. But with the above code CI would not run when e.g. a future 26.x.knots is created (or rebased).
Inquisition seems to follow the some pattern.
There was a problem hiding this comment.
Aaargh, afaik Github CI doesn't support custom ENV variables for forks. So this is solved, but on Github forks are unconditionally skipped.
There was a problem hiding this comment.
Well, what problem is this trying to solve, given that it introduces new problems?
There was a problem hiding this comment.
In commit "ci: skip Github CI on branch pushes for forks" (b9fdd0d)
re: #29274 (comment)
Well, what problem is this trying to solve, given that it introduces new problems?
It seems like this commit is trying to be more efficient by not running CI twice for branch pushes to pull requests. I think it might also stop the "[ryanofsky/bitcoin] Run failed" emails I currently see when I push branches to my repository.
But I think it would better to drop this commit from this PR and move it to separate PR. This PR title and description only mention Cirrus CI, so it's confusing for it to also affect Github actions. I think this change would also be easier to understand if it was implemented separately and explained on its own terms instead of in relation to a cirrus change.
There was a problem hiding this comment.
It seems like this commit is trying to be more efficient by not running CI twice for branch pushes to pull requests. I think it might also stop the "[ryanofsky/bitcoin] Run failed" emails I currently see when I push branches to my repository.
I'd say it is best to disable the CI in your normal fork, if you care about efficiency and not getting emails. Better, you could create a separate fork/repo with CI enabled, and only push to it if you really want to trigger the CI.
40bfae2 to
baa7109
Compare
ci/lint/06_script.sh
Outdated
| @@ -11,7 +11,7 @@ set -ex | |||
| if [ -n "$LOCAL_BRANCH" ]; then | |||
| # To faithfully recreate CI linting locally, specify all commits on the current | |||
| # branch. | |||
| COMMIT_RANGE="$(git merge-base HEAD master)..HEAD" | |||
| COMMIT_RANGE="$(git merge-base HEAD origin/master)..HEAD" | |||
There was a problem hiding this comment.
Preserving @maflcko's comment on the previous PR:
unrelated: Generally I'd find it better if we get rid of the commit range completely. For example, when running the whitespace linter, it seems easier to just lint all code at
HEAD, instead of trying to guess which code was modified and only lint that.
There was a problem hiding this comment.
In any case, I don't understand the change here. This is only run when LOCAL_BRANCH is set, which does not happen in CI, so I doubt that your "fix" fixes anything.
There was a problem hiding this comment.
I remember 0ac57f8 fixed an error complaining that there's no master branch. But I can retest.
There was a problem hiding this comment.
I remember 0ac57f8 fixed an error complaining that there's no master branch. But I can retest.
Sure, but I wonder if it is worth it. Given that COMMIT_RANGE is only really required for the scripted-diff check, I think it is fine to force devs to locally specify the range.
There are endless ways in which this can fail. I don't have a master branch locally either. origin/master isn't guaranteed to exist either. And finally, all of them (if they exist) may point to different commits, leading to even more bugs
There was a problem hiding this comment.
IIUC only the cirrus linter CI calls this script. That's not self-hosted even on forks. So it's possible this commit was only useful in #29259, where I added that linter to a Github Action. I'll drop it if CI passes on Sjors#30
Yeah, obviously it will be passing. As I said, this code in this line is never executed in CI. For pulls, the branch below will be run.
There was a problem hiding this comment.
I dropped 0ac57f8.
Someone can drop COMMIT_RANGE altogether in a separate PR. Maybe move the incantation to a README, because it's certainly useful to know how to run the linter on each commit.
.github/workflows/ci.yml
Outdated
| @@ -29,7 +29,10 @@ jobs: | |||
| test-each-commit: | |||
| name: 'test each commit' | |||
| runs-on: ubuntu-22.04 | |||
| if: github.event_name == 'pull_request' && github.event.pull_request.commits != 1 | |||
| # Only run on pull requests, skip if there's only commit. | |||
| # Do not run on forks. | |||
There was a problem hiding this comment.
It would be nicer to fix the actual problem, but I don't understand what this job is doing.
There was a problem hiding this comment.
@hebasto @ryanofsky any thoughts on how to make the test-each-commit task work for a scenario like in Sjors#30 where the base branch is fork/some-branch and the PR branch is fork/some-pr?
There was a problem hiding this comment.
Example of earlier failure: https://github.com/Sjors/bitcoin/actions/runs/7556559882/job/20573828392?pr=28#step:4:42
shell: /usr/bin/bash -e {0}
env:
DANGER_RUN_CI_ON_HOST: 1
CI_FAILFAST_TEST_LEAVE_DANGLING: 1
MAKEJOBS: -j10
MAX_COUNT: 6
FETCH_DEPTH: 8
Warning: you are leaving 1 commit behind, not connected to
any of your branches:
3b5c5d4 ci: add self-hosted runners for GitHub
If you want to keep it by creating a new branch, this may be a good time
to do so with:
git branch <new-branch-name> 3b5c5d4
HEAD is now at cbe740b ci: vary /tmp/env
fatal: bad revision '^^@'
f79ca32 to
e44e9d2
Compare
|
lgtm ACK c65fde4 Didn't review the other commits. |
|
As for the other commits, as mentioned previously, I am not sure if this is really needed. But no objection, of course. (#29259 (comment)) |
|
@maflcko that comment refers to running CI for checking my own work. But what this pull request enables is running CI for pull requests, which may be from myself but also from others. I'm somewhat puzzled why Knots (cc @luke-jr) and Inquisition (cc @ajtowns) are not running into the same issues. But I guess there's at least two differences:
|
Inquisition disabled all the automatic tests except the trivial linter when cirrus imposed limits earlier in the year, and tests are only run manually by reviewers. Haven't worked out what it should be doing, so don't have suggestions to offer, but interested to see what the resolution here is. |
Maybe because I switch the self-hosted CI to not-self-hosted? |
ci/test/02_run_container.sh
Outdated
| @@ -30,6 +30,10 @@ if [ -z "$DANGER_RUN_CI_ON_HOST" ]; then | |||
| docker volume create "${CONTAINER_NAME}_previous_releases" || true | |||
|
|
|||
| if [ -n "${RESTART_CI_DOCKER_BEFORE_RUN}" ] ; then | |||
| # Kill harder, prevents "The container name "/ci_..." is already in use by container" | |||
| docker stop "$(docker ps -a -q)" || true | |||
There was a problem hiding this comment.
What is "kill harder"? Containers are already being --all --force removed below:
-a, --all Remove all containers
-f, --force Force removal of a running or unusable containerso they shouldn't need to be "stopped" beforehand?
Also, shouldn't this be podman?
There was a problem hiding this comment.
An alternative (with podman 4.8+) would be to use run --replace, see containers/podman@f21c1d2
There was a problem hiding this comment.
It was intentionally vague, because I might just be doing something else wrong. I'll read @maflcko's suggestion.
There was a problem hiding this comment.
Looks like https://packages.ubuntu.com/noble/podman and https://packages.debian.org/trixie/podman includes the fixed --replace, so might try that at one point.
There was a problem hiding this comment.
Oh, this is only available in Noble? In that case I might just improve the code comment (including sleep) and add a note that can go away with Noble.
There was a problem hiding this comment.
Why do we add a dependency on podman at all?
A dependency on a container engine is needed, because this repo is the wrong place to implement a container engine from scratch.
Is the plan to move to podman entirely?
For running the CI locally, any container engine should work. Inside the RESTART_CI_DOCKER_BEFORE_RUN, only podman is supported.
If you want to switch to docker, the podman commands would have to replaced by docker commands inside the RESTART_CI_DOCKER_BEFORE_RUN block. Otherwise, both podman and docker are required to run on docker, which seems confusing.
There was a problem hiding this comment.
Oh, I see where my confusion comes from. I thought podman was just a wrapper for Docker. But it's actually an alternative which can run the same containers. Why not switch over entirely to Podman? It's quite confusing that we're using both.
Since Podman doesn't need root either, I might just nuke Docker and give it a try.
There was a problem hiding this comment.
I nuked Docker in favor of Podman. So far it seems to work. I dropped 7cdb75d.
There was a problem hiding this comment.
Why not switch over entirely to Podman? It's quite confusing that we're using both.
I don't see the benefit of requiring that docker is uninstalled. If someone wants to use their container engine of their choice locally, just let them do it?
There was a problem hiding this comment.
Not specifically related to the issue at hand: the new podman desktop app is pretty smart, it can show you containers running via docker and podman (and kubernetes clusters) all in the same UI: https://podman-desktop.io/
e44e9d2 to
1c05b6e
Compare
|
I switched my own machine from Docker to Podman. This removes the need for 7cdb75d so that's dropped. Added a commit that explains the process a bit more clearly. |
fa91bf2 ci: Skip git install if it is already installed (MarcoFalke) c65fde4 ci: vary /tmp/env (Sjors Provoost) Pull request description: * Currently, running separate CI tasks at the same time may intermittently fail, because they race to read/write `/tmp/env`. Fix this by adding `$CONTAINER_NAME` to the file name. * Also, add `$USER`, while touching the line, to allow different users to run the same CI task at the same time. * Also, skip the git install if there is no need. Ref: #29274 ACKs for top commit: Sjors: ACK fa91bf2 BrandonOdiwuor: ACK fa91bf2 hebasto: ACK fa91bf2. Tree-SHA512: 9a8479255a2afb6618f9d0796488d9430ba95266b90ce39536a9817c1974ca4049beeaab5355a38b25171f76fc386dbec06b1919aaa079f08a5a0c0a146232c8
1c05b6e to
2ff8c34
Compare
|
Rebased after #29441 landed which contains two commits from this PR. |
|
I dropped b9fdd0d as suggested by @ryanofsky so the PR is focussed on Cirrus. This does cause Github tasks to appear twice, but as long as they pass (or get skipped) it's fine for now: I updated 026d864 to drop the |
|
Dropped the So now the first commit is a refactor to replace the hardcoded |
Can you rename this to something that actually explains what it's doing; |
|
Renamed |
|
re: #29274 (comment)
I just now went to that URL and set this. There were not any other environment variables set. I wonder if there is a good place to document this configuration, but probably it's not too important as the main effect is just to avoiding wastefully running CI twice on pushes to master (at least according to the original PR which added the skip statement: #20328) |
There was a problem hiding this comment.
Code review ACK 0882bab.
I think the current PR title / description are a little hard to understand. Maybe consider editing it to something like the following:
-
Fix issues with CI on forks
I find myself making pull requests against my fork (mostly on top of #28983, or asking others to do so. Currently only the Github actions are run on forks, because we use self-hosted runners for the Cirrus tasks.
While setting up my own self-hosted runners for my fork, I ran into a number of issues. Some of those were addressed by #29441, but remaining issues are:
-
When PRs are opened in the fork, cirrus CI jobs are run twice because PRs and branches reside in the same repository, rather than a main repository and a fork repository, as is the case with bitcoin/bitcoin PRs. Fix this by adding a
SKIP_BRANCH_PUSHconfiguration option that allows skipping CI runs not directly associated with a PR. The fix is a generalization of #20328, which fixed a similar problem for the bitcoin-core/gui mirror repository, and it allows removing a hardcoded reference that repository.Github actions jobs will still run twice despite this change, see #29274 (comment). Initially this PR tried to prevent that with b9fdd0d, but this had some potentially negative side effects, see #29274 (comment), so that commit was dropped for now.
-
When PRs are opened in the fork, the "test-each-commit" github action can fail due to not being able to find a recent merge commit. This problem doesn't happen in the bitcoin/bitcoin repository because branches in this repository used as the base for pull requests always point at merge commits.
-
This PR replaces #29259 using the self hosted workers via Cirrus instead of Github.
You can see this PR in action on this pull request to my fork: Sjors#30
To test it yourself:
- spin up at least two self hosted runners. Either use a seperate VM for each, or give them their own user.
- Install Podman and other CI dependencies (see
.cirrus.yml) - Give Cirrus access to your fork at
https://cirrus-ci.com/settings/github/YOU - Get a token from Cirrus and use it to start your worker(s)
- Optionally set
NO_BRANCH=trueandenv variables (seeNO_ARM=true.cirrus.yml) - make a pull request to your own fork, with this PR as the base branch
Security wise: when dealing with code from strangers on the internet, review it first before running the CI. There's a Cirrus check-box that requires approval for people without write access to trigger CI.
.cirrus.yml
Outdated
| << : *GLOBAL_TASK_TEMPLATE | ||
| skip: $NO_ARM == "true" |
There was a problem hiding this comment.
re: #29274 (comment)
I don't have a strong opinion, but I do think the NO_ARM commit 859e1c5 is a nice change because I don't think it fixes a purely cosmetic issue. I think it removes confusing behavior of opening a PR and seeing a job hang forever. The actual code change is very simple, just adding skip: $NO_ARM == "true" and I think the documentation change makes the whole "A self-hosted machine(s) can be used via Cirrus CI..." section clearer and more concrete. But I agree with dropping it as long as any objections remain.
The ci "test-each-commit" job fetches the PR branch being tested with a depth of (# of commits in PR + 2), and then tries to run tests on commits after the most recent merge commit. When a PR is opened against a bitcoin core branch, a merge commit is always guaranteed to exist within the fetch depth, because bitcoin core branches always point at merge commits. However, in fork repositories, pull requests can be opened based on other branches that don't contain recent merge commits, and this will currently cause git rev-list to fail with fatal: bad revision '^^@'. Work around this problem by not requiring a recent merge commit, and just testing on all fetched commits if a merge commit can't be found. Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
|
@ryanofsky thanks for the PR description rewrite! |
There was a problem hiding this comment.
Code review ACK 576828e.
I also tweaked formatting in the PR description to fix a few problems in the markdown copy / paste.
The suggested text "Fix issues with CI on forks" in the description was actually meant to be a replacement title for the PR, since the second commit in the PR fixes an issue with a github actions job, not a cirrus job. So could remove this and maybe update the title.
There was a problem hiding this comment.
lgtm ACK
My preference would be to have the .cirrus.yml as single source of ground truth. However, requiring forks to update this file is too cumbersome, so this alternative seems fine.
(The "config resolution strategy" is another setting outside the yaml that needs to be modified to "merge for prs", which could be documented in a follow-up)
But this looks good in any case.
This is wrong in the description. The name was changed. |
|
@maflcko fixed in description |
|
@maflcko is your "lgtm ACK" more of a partial ACK, or do you think this is good to merge? I think practically speaking this looks ok to merge, but I wouldn't want to merge it myself if I'm the only full reviewer. I guess there are also number of possible followups to this PR if it is merged:
|
|
ACK 576828e |
|
Picked @ryanofsky's first suggestion into #30487. Re. the second suggestion: so far ARM jobs have not been a source of headaches on my own fork (only TSAN and MSAN are atm). |
5555395 lint: Use git --no-pager to print any output in one go (MarcoFalke) fa57294 lint: Fix lint-whitespace issues (MarcoFalke) Pull request description: The lint check has many issues: * It uses `COMMIT_RANGE`, which is brittle code, apparently making it harder to run the CI locally, or self-hosted. See bitcoin#29274 (comment) * The result depends on `COMMIT_RANGE`, or the number of commits passed to the script, which can cause false negatives or false positives. * It is based on the diff output, parsing it, and printing it again, which is brittle as well. * The output does not include line number, making it harder to act on a lint error. Fix all issues by removing the script and replacing it with a simple call to `git grep -I --line-number ...`. ACKs for top commit: TheCharlatan: Re-ACK 5555395 Tree-SHA512: 71ea8b6382af064beb72fb17f21a0ae9e9238c97e7fa43c2ec353fd1dd73a7bbd696ba0f0a9f65d1eff7c86cbf6cc104a992cb5450a3d50f122955e835270065
fa1146d lint: Fix COMMIT_RANGE issues (MarcoFalke) Pull request description: `COMMIT_RANGE` has problems on forks or local branches: * When `LOCAL_BRANCH` is set, it assumes the presence of a `master` branch, and that the `master` branch is up-to-date. Both of which may be false. (See also discussion in bitcoin#29274 (comment)) * When `COMMIT_RANGE` isn't set in `lint-git-commit-check.py`, and `--prev-commits` isn't set either, it has the same (broken) assumptions. Fix all issues by simply assuming a merge commit exists. This allows to drop `LOCAL_BRANCH`. It also allows to drop `SKIP_EMPTY_NOT_A_PR`, because scripts will already skip an empty range. Finally, it allows to drop `--prev-commits n`, because one can simply say `COMMIT_RANGE='HEAD~n..HEAD'` to achieve the same. ACKs for top commit: Sjors: tACK fa1146d Tree-SHA512: f1477a38267fd4fdb8d396211a5d6bed5f418798c7edaba43487957aaf726cd45244ccf15187b3dd896d398fa1df3fe0a37323e49cf44d60a2018786ed41e5ba
5555395 lint: Use git --no-pager to print any output in one go (MarcoFalke) fa57294 lint: Fix lint-whitespace issues (MarcoFalke) Pull request description: The lint check has many issues: * It uses `COMMIT_RANGE`, which is brittle code, apparently making it harder to run the CI locally, or self-hosted. See bitcoin#29274 (comment) * The result depends on `COMMIT_RANGE`, or the number of commits passed to the script, which can cause false negatives or false positives. * It is based on the diff output, parsing it, and printing it again, which is brittle as well. * The output does not include line number, making it harder to act on a lint error. Fix all issues by removing the script and replacing it with a simple call to `git grep -I --line-number ...`. ACKs for top commit: TheCharlatan: Re-ACK 5555395 Tree-SHA512: 71ea8b6382af064beb72fb17f21a0ae9e9238c97e7fa43c2ec353fd1dd73a7bbd696ba0f0a9f65d1eff7c86cbf6cc104a992cb5450a3d50f122955e835270065
Maintainer note:
SKIP_BRANCH_PUSH=truemust be set in Cirrus forbitcoin-core/guibefore merging this. Seehttps://cirrus-ci.com/github/bitcoin-core/gui-> Settings.I find myself making pull requests against my fork (mostly on top of #28983, or asking others to do so. Currently only the Github actions are run on forks, because we use self-hosted runners for the Cirrus tasks.
While setting up my own self-hosted runners for my fork, I ran into a number of issues. Some of those were addressed by #29441, but remaining issues are:
When PRs are opened in the fork, cirrus CI jobs are run twice because PRs and branches reside in the same repository, rather than a main repository and a fork repository, as is the case with bitcoin/bitcoin PRs. Fix this by adding a
SKIP_BRANCH_PUSHconfiguration option that allows skipping CI runs not directly associated with a PR. The fix is a generalization of #20328, which fixed a similar problem for the bitcoin-core/gui mirror repository, and it allows removing a hardcoded reference to that repository.Github actions jobs will still run twice despite this change, see #29274 (comment). Initially this PR tried to prevent that with b9fdd0d, but this had some potentially negative side effects, see #29274 (comment), so that commit was dropped for now.
When PRs are opened in the fork, the "test-each-commit" github action can fail due to not being able to find a recent merge commit. This problem doesn't happen in the bitcoin/bitcoin repository because branches in this repository used as the base for pull requests always point at merge commits.
This PR replaces #29259 using the self hosted workers via Cirrus instead of Github.
You can see this PR in action on this pull request to my fork: Sjors#30
To test it yourself:
and NO_ARM=trueenv variables (see .cirrus.yml)make a pull request to your own fork, with this PR as the base branch
Security wise: when dealing with code from strangers on the internet, review it first before running the CI. There's a Cirrus check-box that requires approval for people without write access to trigger CI.