-
Notifications
You must be signed in to change notification settings - Fork 37.7k
ci: Avoid CI failures from temp env file reuse #29441
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This also avoids the sudo requirement for self-hosted CI runners.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK fa91bf2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK fa91bf2
Looks good to me
@@ -55,7 +55,7 @@ base_template: &BASE_TEMPLATE | |||
<< : *FILTER_TEMPLATE | |||
merge_base_script: | |||
# Unconditionally install git (used in fingerprint_script). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment got outdated, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
git
is still installed unconditionally, otherwise, it wouldn't be present, no?
Might replace "install" with "require" if I re-touch, or in a follow-up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK fa91bf2.
rfm? :) |
Post merge ACK fa91bf2. I ran multiple CI jobs at the same time and observed the nicer naming in the /tmp directory. |
Github-Pull: bitcoin#29441 Rebased-From: c65fde4
This also avoids the sudo requirement for self-hosted CI runners. Github-Pull: bitcoin#29441 Rebased-From: fa91bf2
Github-Pull: bitcoin#29441 Rebased-From: c65fde4
This also avoids the sudo requirement for self-hosted CI runners. Github-Pull: bitcoin#29441 Rebased-From: fa91bf2
576828e ci: test-each-commit merge base optional (Sjors Provoost) e9bfbb5 ci: forks can opt-out of CI branch push (Cirrus only) (Sjors Provoost) Pull request description: Maintainer note: `SKIP_BRANCH_PUSH=true` must be set in Cirrus for `bitcoin-core/gui` before merging this. See `https://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: 1. 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_PUSH` configuration option that allows skipping CI runs not directly associated with a PR. The fix is a generalization of [#20328](#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)](#29274 (comment)). Initially this PR tried to prevent that with b9fdd0d, but this had some potentially negative side effects, see [#29274 (comment)](#29274 (comment)), so that commit was dropped for now. 2. 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: 1. spin up at least two [self hosted runners](https://github.com/cirruslabs/cirrus-cli/blob/master/PERSISTENT-WORKERS.md). Either use a seperate VM for each, or give them their own user. 3. Install Podman and other CI dependencies (see .cirrus.yml) 4. Give Cirrus access to your fork at https://cirrus-ci.com/settings/github/YOU 5. Get a token from Cirrus and use it to start your worker(s) 6. Optionally set SKIP_BRANCH_PUSH=true ~and NO_ARM=true~ env 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. ACKs for top commit: maflcko: ACK 576828e ryanofsky: Code review ACK 576828e. Tree-SHA512: fb6be2f228aa62f45a65ce5c613c979b6f387df396f9601ce4622b27aa317a66f198e7d7a194592b0bb397b32a2f50f8be47065834d74af4ea09407c5c8d306d
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