Skip to content

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented Feb 16, 2024

  • 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

Sjors and others added 2 commits January 18, 2024 18:18
This also avoids the sudo requirement for self-hosted CI runners.
@DrahtBot
Copy link
Contributor

DrahtBot commented Feb 16, 2024

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK Sjors, BrandonOdiwuor, hebasto

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

@DrahtBot DrahtBot added the Tests label Feb 16, 2024
@maflcko maflcko requested a review from Sjors February 16, 2024 15:18
Copy link
Member

@Sjors Sjors left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK fa91bf2

Copy link
Contributor

@BrandonOdiwuor BrandonOdiwuor left a 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

@maflcko maflcko added this to the 27.0 milestone Feb 17, 2024
@@ -55,7 +55,7 @@ base_template: &BASE_TEMPLATE
<< : *FILTER_TEMPLATE
merge_base_script:
# Unconditionally install git (used in fingerprint_script).
Copy link
Member

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?

Copy link
Member Author

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?

Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK fa91bf2.

@maflcko
Copy link
Member Author

maflcko commented Feb 20, 2024

rfm? :)

@fanquake fanquake merged commit bdddf36 into bitcoin:master Feb 20, 2024
@maflcko maflcko deleted the 2402-ci-bugfix- branch February 20, 2024 10:58
@m3dwards
Copy link
Contributor

Post merge ACK fa91bf2.

I ran multiple CI jobs at the same time and observed the nicer naming in the /tmp directory.

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Mar 5, 2024
Github-Pull: bitcoin#29441
Rebased-From: c65fde4
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Mar 5, 2024
This also avoids the sudo requirement for self-hosted CI runners.

Github-Pull: bitcoin#29441
Rebased-From: fa91bf2
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Mar 13, 2024
Github-Pull: bitcoin#29441
Rebased-From: c65fde4
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Mar 13, 2024
This also avoids the sudo requirement for self-hosted CI runners.

Github-Pull: bitcoin#29441
Rebased-From: fa91bf2
ryanofsky added a commit that referenced this pull request Jul 10, 2024
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
@bitcoin bitcoin locked and limited conversation to collaborators Feb 20, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants