Skip to content

Conversation

@Sjors
Copy link
Owner

@Sjors Sjors commented Jan 18, 2024

This tests CI for a pull request on a fork against a branch that's not master.

Demo for bitcoin#29274

@Sjors Sjors force-pushed the 2024/01/ci-test branch 19 times, most recently from 15240d1 to e92bdbf Compare January 18, 2024 14:36
@Sjors Sjors force-pushed the 2024/01/ci-test branch 9 times, most recently from 4979aae to 5d88b3d Compare January 18, 2024 17:23
@Sjors Sjors force-pushed the 2023/01/ci-fork branch 3 times, most recently from 8d228bb to 3c998b0 Compare June 18, 2024 09:19
@Sjors Sjors force-pushed the 2024/01/ci-test branch from 6d7fbd0 to 6a7fd20 Compare June 18, 2024 09:43
@Sjors
Copy link
Owner Author

Sjors commented Jun 18, 2024

bitcoin#30193 landed.

Currently trying if qemu-aarch64 is quick enough, see bitcoin#29274 (comment)


Update: giving up on qemu for now.

@Sjors Sjors force-pushed the 2023/01/ci-fork branch from 3c998b0 to e1c25f9 Compare June 18, 2024 10:35
@Sjors Sjors force-pushed the 2024/01/ci-test branch from 935f272 to cda4419 Compare June 18, 2024 10:43
@Sjors Sjors force-pushed the 2023/01/ci-fork branch from e1c25f9 to ff7f6c1 Compare June 18, 2024 11:02
@Sjors Sjors force-pushed the 2024/01/ci-test branch 2 times, most recently from ea8303c to d8229d5 Compare June 20, 2024 08:42
@Sjors Sjors force-pushed the 2023/01/ci-fork branch from ff7f6c1 to 015710f Compare June 20, 2024 12:15
@Sjors Sjors force-pushed the 2024/01/ci-test branch 3 times, most recently from e66f4e9 to 2981904 Compare June 20, 2024 16:42
@Sjors Sjors force-pushed the 2023/01/ci-fork branch from 015710f to 5004bd9 Compare June 20, 2024 16:58
@Sjors Sjors force-pushed the 2024/01/ci-test branch from ae74354 to 029cdb1 Compare June 20, 2024 16:59
@Sjors Sjors force-pushed the 2023/01/ci-fork branch 2 times, most recently from b80a573 to d55b91a Compare June 25, 2024 12:43
@Sjors Sjors force-pushed the 2024/01/ci-test branch from 029cdb1 to b21ae8d Compare June 25, 2024 12:46
@Sjors Sjors force-pushed the 2023/01/ci-fork branch from d55b91a to da1aaed Compare June 25, 2024 14:33
@Sjors Sjors force-pushed the 2024/01/ci-test branch from b21ae8d to 4df6504 Compare June 25, 2024 14:41
Sjors and others added 2 commits June 25, 2024 17:10
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>
@Sjors Sjors force-pushed the 2023/01/ci-fork branch from da1aaed to 0882bab Compare June 25, 2024 15:11
@Sjors Sjors force-pushed the 2024/01/ci-test branch from 4df6504 to 8d52c09 Compare June 25, 2024 15:14
@Sjors Sjors force-pushed the 2023/01/ci-fork branch from 0882bab to 576828e Compare June 25, 2024 18:10
ryanofsky added a commit to bitcoin/bitcoin 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
@Sjors Sjors closed this Jul 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants