chore: detect the channel a PR wants to merge into by weihanglo · Pull Request #12181 · rust-lang/cargo

weihanglo · 2023-05-25T14:52:21Z

What does this PR try to resolve?

This detects which channel a pull request wants to merge into.

It is hard because bors runs in a different repo.
Bors knows nothing about branches or tag in this repo.
It only see one base commit and a merge commit AFAIK.

Basically the assumption and idea are

bors create a merge commit, so HEAD~ should find the base branch.
Cargo maintains rust-1.y.0 branch for each Rust minor version releases.
Therefore, we can use the symbolic name of origin/rust-1.x.0
to determine if it aims for stable, beta, or nightly channel.
After we know which channel it is targeted at,
we can skip jobs that are likely to be broken.

How should we test and review this PR?

I setup a rust-1.70.0 branch on ec8a8a0 on my repo.
The script does find beta branch correctly.
https://github.com/weihanglo/cargo/actions/runs/5081469788/jobs/9129891293#step:3:36

Additional information

Sorry for bringing more shell scripts 🤯.

If this looks good, I'll file a PR for master branch.

rustbot · 2023-05-25T14:52:25Z

r? @ehuss

(rustbot has picked a reviewer for you, use r? to override)

rustbot · 2023-05-25T14:52:26Z

⚠️ Warning ⚠️

Pull requests are usually filed against the master branch for this repo, but this one is against rust-1.70.0. Please double check that you specified the right target!

weihanglo · 2023-05-25T14:56:17Z

.github/workflows/main.yml

    name: Tests ${{ matrix.name }}
    steps:
    - uses: actions/checkout@v3
+      if: matrix.if


Unfortunately job.if cannot access fields in matrix. We cannot simply write if: matrix.if for job and instead we need to do it in each step. That's terrible. Totally understand if someone objects to this PR due to that.

See actions/runner#1985

Ugh, that is really unfortunate. Is it possible to use something like https://stackoverflow.com/a/65434401 to have a job that runs first to generate the matrix? I'm not sure if that's any better, since it is also a pretty complex solution.

Fixed with 4ff82fd. Doesn't look too bad though.

ehuss · 2023-05-25T17:57:16Z

ci/which-channel.sh

+    beta="$(rustc +beta -V)"
+    stable="$(rustc +stable -V)"
+
+    if [[ "$beta" == "rustc ${version}"* ]]
+    then
+        CHANNEL="beta"
+    fi
+
+    if [[ "$stable" = "rustc ${version}"* ]]
+    then
+        CHANNEL="stable"
+    fi


I'm concerned that this looks to assume that beta gets published when it branches. However, it isn't uncommon for it to take a few weeks, and I know in the past we have needed to merge changes to cargo soon after the beta branch.

Perhaps that isn't a big deal, since the divergence of nightly and beta isn't that large, but this seems a little fragile. Would it be possible to say if the base ref matches the rust-1.*.0 pattern to assume it is for beta, and only set it to stable if rustc +stable -V matches?

Another option, instead of downloading rustc at all is to list all the rust-* branches, sort them, and see if the current branch is the last one, and assume it is beta? Otherwise, assume it is stable?

Either way looks more robust to me. I'll try the latter since it reduces one toolchain download.

Fixed with 6a973cf.

ehuss · 2023-05-25T18:02:49Z

ci/which-channel.sh

+ref=$(git name-rev --name-only --refs="origin/rust-1.*" $base_sha)
+
+# Remove the patch version `.0` and keep only `1.y`,
+# so that rustup can install the correct patched toolchain
+if [[ "$ref" =~ ^origin/rust-(.*).0$ ]]


I'm a bit uncertain about how this works, perhaps you can clarify.

Locally running git name-rev, my output looks like origin/rust-1.69.0~1 where there is a ~ character. But the regex here doesn't allot for that. I'm a bit confused by that.

Also, does this work if I open a PR to a beta branch, but my commit is a few commits behind the tip of the branch?

The assumption here is that a base_sha is always HEAD ofrust-1.*.0 branch. It either gets the commit from github.event.pull_request.base.sha, or from bors merge-commit~1. If this assumption is correct, I think git name-rev will always get a ref without ~ or ^.

Fixed with 6a973cf.

ehuss · 2023-05-27T19:16:34Z

ci/which-channel.sh

+
+# Get the latest `rust-1.*.0` branch from remote origin.
+# Assumption: The latest branch is always beta branch.
+beta=$(git branch --remotes --list 'origin/rust-1.*.0' | sort | tail -n1 | tr -d "[:space:]")


This looks like it might have some issues with three-digit version like 1.100. Perhaps it could be something like sed -E 's/ *upstream\/rust-1.([0-9]+).0/\1/g' | sort -n?

I went using sort --version-sort. Tested on ubuntu-latest (22.04). It can correctly sort the following versions from

origin/rust-1.100.0 origin/rust-1.50.0 origin/rust-1.222.0 origin/rust-1.48.0 origin/rust-1.70.0 origin/rust-1.60.0 origin/rust-1.69.0 origin/rust-1.9.0

to

origin/rust-1.9.0 origin/rust-1.48.0 origin/rust-1.50.0 origin/rust-1.60.0 origin/rust-1.69.0 origin/rust-1.70.0 origin/rust-1.100.0 origin/rust-1.222.0

ci/which-channel.sh

ehuss · 2023-05-27T19:27:03Z

.github/workflows/main.yml

+        MATRIX=$(
+          jq --arg C "$CHANNEL" 'map (. | 
+            if ($C == "beta") then select(.rust | startswith("nightly") | not)
+            elif ($C == "stable") then select(.rust | startswith("stable"))
+            else . end)' ci/matrix.json
+        )


Can this have a comment explaining roughly what it is doing? It's not too hard to follow, but just a high-level description could help.

Added. Let me know if it is clear enough.

weihanglo · 2023-05-27T22:47:20Z

Thanks for the review! I rebased the pull request. Change diff can be seen from here.

ehuss · 2023-05-29T17:12:54Z

Thanks! I'm not sure if it matters much that we're merging this to rust-1.70.0, but I think it should be fine. Do you also want to backport this to rust-1.71.0?

I kinda wish GitHub Actions would have a little more diagnostics or descriptions on the runs page to explain why a job was skipped, but hopefully it will be clear to people in the future.

@bors r+

bors · 2023-05-29T17:12:56Z

📌 Commit 11704cb has been approved by ehuss

It is now in the queue for this repository.

bors · 2023-05-29T17:13:03Z

⌛ Testing commit 11704cb with merge e032df1...

chore: detect the channel a PR wants to merge into ### What does this PR try to resolve? This detects which channel a pull request wants to merge into. It is hard because bors runs in a different repo. Bors knows nothing about branches or tag in this repo. It only see one base commit and a merge commit AFAIK. Basically the assumption and idea are 1. bors create a merge commit, so `HEAD~` should find the base branch. 2. Cargo maintains `rust-1.y.0` branch for each Rust minor version releases. Therefore, we can use the symbolic name of `origin/rust-1.x.0` to determine if it aims for stable, beta, or nightly channel. 3. After we know which channel it is targeted at, we can skip jobs that are likely to be broken. ### How should we test and review this PR? I setup a rust-1.70.0 branch on ec8a8a0 on my repo. The script does find beta branch correctly. https://github.com/weihanglo/cargo/actions/runs/5081469788/jobs/9129891293#step:3:36

weihanglo · 2023-05-29T17:42:04Z

Sure for 1.71.0.

Now it sounds like we need two more PRs — one for 1.71 beta and the other for master 😂

bors · 2023-05-29T17:55:01Z

💔 Test failed - checks-actions

ehuss · 2023-05-29T18:09:46Z

Oops, looks like skipped jobs cause a failure in the if condition.
See: https://github.com/orgs/community/discussions/45058 and actions/runner#491.
Are there any workarounds listed there that might work?

weihanglo · 2023-05-29T18:10:46Z

Ok. build-std job was skipped so bors failed. Maybe we need a weak depedency syntax for needs, like

needs: ["build-std?", "test"]

Just kidding.

weihanglo · 2023-05-29T19:14:43Z

Something like this? I can send PRs to 1.71.0 and master and then close this if this patch seems good.

diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
index 2f75f206b..5f955af89 100644
--- a/.github/workflows/main.yml
+++ b/.github/workflows/main.yml
@@ -227,11 +228,16 @@ jobs:
           "https://raw.githubusercontent.com/rust-lang/rust/$BRANCH/src/tools/linkchecker/linkcheck.sh"
         sh linkcheck.sh --all cargo
 
+  nightly-only-jobs:
+    needs: [build_std]
+    if: "(jobs.channel.outputs.CHANNEL != 'master' && contains(needs.*.result, 'skipped')) || success()"
+    runs-on: ubuntu-latest
+
   success:
     permissions:
       contents: none
     name: bors build finished
-    needs: [docs, rustfmt, test, resolver, build_std, test_gitoxide]
+    needs: [docs, rustfmt, test, resolver, nightly-only-jobs, test_gitoxide]
     runs-on: ubuntu-latest
     if: "success() && github.event_name == 'push' && github.ref == 'refs/heads/auto-cargo'"
     steps:
@@ -240,7 +246,7 @@ jobs:
     permissions:
       contents: none
     name: bors build finished
-    needs: [docs, rustfmt, test, resolver, build_std]
+    needs: [docs, rustfmt, test, resolver, nightly-only-jobs]
     runs-on: ubuntu-latest
     if: "!success() && github.event_name == 'push' && github.ref == 'refs/heads/auto-cargo'"
     steps:

weihanglo · 2023-05-30T10:48:58Z

Okay I've updated this to target on the current master. We can have a new round of review :)

ehuss · 2023-05-30T18:43:31Z

@bors r+

bors · 2023-05-30T18:43:33Z

📌 Commit 40be718 has been approved by ehuss

It is now in the queue for this repository.

bors · 2023-05-30T19:25:05Z

⌛ Testing commit 40be718 with merge f7b95e3...

bors · 2023-05-30T20:20:33Z

☀️ Test successful - checks-actions
Approved by: ehuss
Pushing f7b95e3 to master...

rustbot assigned ehuss May 25, 2023

rustbot added A-infrastructure Area: infrastructure around the cargo repo, ci, releases, etc. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 25, 2023

weihanglo marked this pull request as draft May 25, 2023 14:52

weihanglo commented May 25, 2023

View reviewed changes

weihanglo force-pushed the which-channel-backport branch from 32bae81 to a085f16 Compare May 25, 2023 15:00

weihanglo marked this pull request as ready for review May 25, 2023 16:04

ehuss reviewed May 25, 2023

View reviewed changes

weihanglo force-pushed the which-channel-backport branch 2 times, most recently from 9313c6f to 43ae784 Compare May 25, 2023 22:00

ehuss reviewed May 27, 2023

View reviewed changes

weihanglo mentioned this pull request May 27, 2023

[beta-1.70] backport #12165 #12167

Closed

weihanglo force-pushed the which-channel-backport branch from 43ae784 to 11704cb Compare May 27, 2023 22:42

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 29, 2023

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 29, 2023

weihanglo mentioned this pull request May 29, 2023

[beta-1.71.0] detect the channel a PR wants to merge into #12200

Closed

rustbot added Command-new Command-package Command-tree labels May 30, 2023

weihanglo changed the base branch from rust-1.70.0 to master May 30, 2023 10:43

chore: detect the channel a PR wants to merge into

40be718

This was referenced May 30, 2023

Revert "chore: detect the channel a PR wants to merge into" #12204

Merged

Update cargo rust-lang/rust#112120

Merged

weihanglo mentioned this pull request Aug 7, 2023

Bump cargo library to 0.72.2 for CVE patches #12462

Merged

Conversation

weihanglo commented May 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR try to resolve?

How should we test and review this PR?

Additional information

Uh oh!

rustbot commented May 25, 2023

Uh oh!

rustbot commented May 25, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weihanglo commented May 27, 2023

Uh oh!

ehuss commented May 29, 2023

Uh oh!

bors commented May 29, 2023

Uh oh!

bors commented May 29, 2023

Uh oh!

weihanglo commented May 29, 2023

Uh oh!

bors commented May 29, 2023

Uh oh!

ehuss commented May 29, 2023

Uh oh!

weihanglo commented May 29, 2023

Uh oh!

weihanglo commented May 29, 2023

Uh oh!

weihanglo commented May 30, 2023

Uh oh!

ehuss commented May 30, 2023

Uh oh!

bors commented May 30, 2023

Uh oh!

bors commented May 30, 2023

Uh oh!

bors commented May 30, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

weihanglo commented May 25, 2023 •

edited

Loading