chore: detect the channel a PR wants to merge into #12181

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 :)

…r=ehuss [beta-1.71.0] 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? This is like #12181 but targeted on 1.71.0. r? ehuss

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...

…ort, r=ehuss" This reverts commit f7b95e3, reversing changes made to 202eb1d.

… r=epage Revert "chore: detect the channel a PR wants to merge into" Reverts #12181 GitHub Action expression `success()` will look into the chain of dependent jobs. On master CI passed but on a backport it still failed <#12200 (comment)> This isn't worth pursuing at this moment either. To fix it, it's unavoidable to write something like below for bors job ```yaml if: "always() && !(contains(needs.*.result, 'cancelled') || contains(needs.*.result, 'skipped') || contains(needs.*.result, 'failure') ) && github.event_name == 'push' && github.ref == 'refs/heads/auto-cargo' " ``` I don't think that's good in long-term. r? ehuss

Update cargo 17 commits in 64fb38c97ac4d3a327fc9032c862dd28c8833b17..f7b95e31642e09c2b6eabb18ed75007dda6677a0 2023-05-23 18:53:23 +0000 to 2023-05-30 19:25:02 +0000 - chore: detect the channel a PR wants to merge into (rust-lang/cargo#12181) - refactor: de-depulicate `make_dep_prefix` implementation (rust-lang/cargo#12203) - Re-enable code_generation test on Windows (rust-lang/cargo#12199) - docs: add doc comments for git source and friends (rust-lang/cargo#12192) - test: set retry sleep to 1ms for all tests (rust-lang/cargo#12194) - fix(add): Reduce the chance we re-format the user's `[features]` table (rust-lang/cargo#12191) - test(add): Remove expensive test (rust-lang/cargo#12188) - Add a description of `Cargo.lock` conflicts in the Cargo FAQ (rust-lang/cargo#12185) - refactor(tests): Reduce cargo-add setup load (rust-lang/cargo#12189) - Warn when an edition 2021 crate is in a virtual workspace with default resolver (rust-lang/cargo#10910) - refactor(tests): Reduce cargo-remove setup load (rust-lang/cargo#12184) - chore: Lexicographically order `-Z` flags (rust-lang/cargo#12182) - chore(ci): remove temporary fix for rustup 1.24.1 (rust-lang/cargo#12180) - fix: AIX searches dynamic libraries in `LIBPATH`. (rust-lang/cargo#11968) - deps: remove unused features from windows-sys (rust-lang/cargo#12176) - Automatically inherit workspace lints when running cargo new/init (rust-lang/cargo#12174) - Test that the new `debuginfo` options match between cargo and rustc (rust-lang/cargo#12022) r? `@ghost`

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

weihanglo force-pushed the which-channel-backport branch from e38136a to 5f91c78 Compare May 30, 2023 10:45

chore: detect the channel a PR wants to merge into

40be718

weihanglo force-pushed the which-channel-backport branch from 5f91c78 to 40be718 Compare May 30, 2023 10:46

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 30, 2023

bors merged commit f7b95e3 into rust-lang:master May 30, 2023

weihanglo deleted the which-channel-backport branch May 30, 2023 20:27

weihanglo added a commit to weihanglo/cargo that referenced this pull request May 30, 2023

Revert "Auto merge of rust-lang#12181 - weihanglo:which-channel-backp…

ced6262

…ort, r=ehuss" This reverts commit f7b95e3, reversing changes made to 202eb1d.

weihanglo mentioned this pull request May 30, 2023

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

Merged

weihanglo mentioned this pull request May 30, 2023

Update cargo rust-lang/rust#112120

Merged

ehuss added this to the 1.72.0 milestone Jun 2, 2023

weihanglo mentioned this pull request Aug 7, 2023

Bump cargo library to 0.72.2 for CVE patches #12462

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: detect the channel a PR wants to merge into #12181

chore: detect the channel a PR wants to merge into #12181

weihanglo commented May 25, 2023 •

edited

Loading

rustbot commented May 25, 2023

rustbot commented May 25, 2023

weihanglo May 25, 2023

ehuss May 25, 2023

weihanglo May 25, 2023

ehuss May 25, 2023

weihanglo May 25, 2023

weihanglo May 25, 2023

ehuss May 25, 2023

weihanglo May 25, 2023

weihanglo May 25, 2023

ehuss May 27, 2023

weihanglo May 27, 2023

ehuss May 27, 2023

weihanglo May 27, 2023

weihanglo commented May 27, 2023

ehuss commented May 29, 2023

bors commented May 29, 2023

bors commented May 29, 2023

weihanglo commented May 29, 2023

bors commented May 29, 2023

ehuss commented May 29, 2023

weihanglo commented May 29, 2023

weihanglo commented May 29, 2023

weihanglo commented May 30, 2023

ehuss commented May 30, 2023

bors commented May 30, 2023

bors commented May 30, 2023

bors commented May 30, 2023

chore: detect the channel a PR wants to merge into #12181

chore: detect the channel a PR wants to merge into #12181

Conversation

weihanglo commented May 25, 2023 • edited Loading

What does this PR try to resolve?

How should we test and review this PR?

Additional information

rustbot commented May 25, 2023

rustbot commented May 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weihanglo commented May 27, 2023

ehuss commented May 29, 2023

bors commented May 29, 2023

bors commented May 29, 2023

weihanglo commented May 29, 2023

bors commented May 29, 2023

ehuss commented May 29, 2023

weihanglo commented May 29, 2023

weihanglo commented May 29, 2023

weihanglo commented May 30, 2023

ehuss commented May 30, 2023

bors commented May 30, 2023

bors commented May 30, 2023

bors commented May 30, 2023

weihanglo commented May 25, 2023 •

edited

Loading