Skip to content

opt-dist: propagate channel info to bootstrap#134528

Merged
bors merged 2 commits intorust-lang:masterfrom
jieyouxu:fix-rustc-bootstrap-test
Dec 23, 2024
Merged

opt-dist: propagate channel info to bootstrap#134528
bors merged 2 commits intorust-lang:masterfrom
jieyouxu:fix-rustc-bootstrap-test

Conversation

@jieyouxu
Copy link
Member

@jieyouxu jieyouxu commented Dec 19, 2024 •
edited
Loading

Fixes #133503.

Previously, tests/ui/bootstrap/rustc_bootstap.rs [sic] failed during beta bump in opt-dist tests. This is because:

  • opt-dist tried to run ./x test against beta-channel dist rustc through bootstrap.
  • The dist build produced during the beta bump produces a rustc which correctly thinks that it is a beta compiler based on src/ci/channel info.
  • opt-dist tries to run ./x test on the beta rustc from the dist build, but without specifying channel through a synthetic config.toml, so bootstrap tells compiletest that we're on the nightly channel (by default).
  • Now there's a channel mismatch: compiletest believes the rustc under test is a nightly rustc, but the rustc under test actually considers itself a beta rustc. This means that //@ only-nightly will be satisfied yet the test will fail as the beta rustc is not a nightly rustc.

This PR:

  • Fixes the test failure during beta bump (i.e. tests/ui/bootstrap/rustc_bootstap.rs fails in post-PGO/opt-dist tests #133503) by having opt-dist faithfully report the channel of the dist rustc being tested (i.e. "beta" in a beta bump PR). This will properly make the test be ignored during beta bump as the rustc under test is not a nightly rustc.
  • Fixes the test name rustc_bootstap.rs -> rustc_bootstrap.rs. No more stapping.
  • Slightly adjusts the doc comment in the test to make it more clear.

I ran a try-job against the beta branch (explicitly running the opt-dist tests by modifying the job definition) with these changes in #134131, and it appears that the try-job was successful. The two commits in this PR are cherry-picked from #134131, with the test commit slightly modified (to also adjust the test comments).

r? @Kobzol (or compiler or bootstrap or infra I guess?)

jieyouxu and others added 2 commits December 20, 2024 02:32
@jieyouxu
opt-dist: propagate channel info to bootstrap
42bf2af
@jieyouxu
Adjust rustc_bootstap.rs ui test
d6ceae0 
- Fixed test name, it should've been `rustc_bootstrap.rs`, oops.
- Slightly reworded test comment to make it more clear.
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 19, 2024
@rustbot
Copy link
Collaborator

rustbot commented Dec 19, 2024

Some changes occurred in src/tools/opt-dist

cc @Kobzol

jieyouxu
jieyouxu commented Dec 19, 2024
View reviewed changes
src/tools/opt-dist/src/tests.rs
Comment on lines +126 to +128
/// Roughly convert a version string (`nightly`, `beta`, or `1.XY.Z`) to channel string (`nightly`,
/// `beta` or `stable`).
fn version_to_channel(version_str: &str) -> &'static str {
Copy link
Member Author

@jieyouxu jieyouxu Dec 19, 2024

Choose a reason for hiding this comment

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

The dist version extraction above this is slightly hacky, and so is this, but oh well.

@jieyouxu jieyouxu added the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label Dec 20, 2024
Kobzol
Kobzol approved these changes Dec 23, 2024
View reviewed changes
Copy link
Member

@Kobzol Kobzol left a comment

Choose a reason for hiding this comment

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

Thank you! Sorry for the delay in review.

@bors r+

src/tools/opt-dist/src/tests.rs
let host_triple = env.host_tuple();
let version = find_dist_version(&dist_dir)?;

let channel = version_to_channel(&version);
Copy link
Member

@Kobzol Kobzol Dec 23, 2024

Choose a reason for hiding this comment

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

I guess that we could also read the src/ci/channel file? But this seems good enough.

Copy link
Member Author

@jieyouxu jieyouxu Dec 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

AFAIK we could (that's the initial approach considered), but the logic above doesn't read that file either, and instead derives it from the archive name of the dist artifact, which is why I wrote this hack instead...

FTR, this is what I meant by #134528 (comment), should've clarified.

@jieyouxu
Copy link
Member Author

jieyouxu commented Dec 23, 2024

@Kobzol you fell into the classic trap of r+ in the top-level review comment (bors doesn't pick that up) 😆

@jieyouxu
Copy link
Member Author

jieyouxu commented Dec 23, 2024

@bors r=@Kobzol rollup

@bors
Copy link
Collaborator

bors commented Dec 23, 2024

📌 Commit d6ceae0 has been approved by Kobzol

It is now in the queue for this repository.

@bors 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 Dec 23, 2024
@matthiaskrgr matthiaskrgr mentioned this pull request Dec 23, 2024
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 23, 2024
@bors
Auto merge of rust-lang#134687 - matthiaskrgr:rollup-m32tkax, r=matth…
54b4d06 
…iaskrgr

Rollup of 5 pull requests

Successful merges:

 - rust-lang#134363 (Use `#[derive(Default)]` instead of manual `impl` when possible)
 - rust-lang#134517 (Add tests for coverage attribute on trait functions)
 - rust-lang#134528 (opt-dist: propagate channel info to bootstrap)
 - rust-lang#134669 (Document the `--dev` flag for `src/ci/docker/run.sh`)
 - rust-lang#134680 (Clean up a few rmake tests  )

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 23, 2024
@bors
Auto merge of rust-lang#134687 - matthiaskrgr:rollup-m32tkax, r=matth…
addbd00 
…iaskrgr

Rollup of 5 pull requests

Successful merges:

 - rust-lang#134363 (Use `#[derive(Default)]` instead of manual `impl` when possible)
 - rust-lang#134517 (Add tests for coverage attribute on trait functions)
 - rust-lang#134528 (opt-dist: propagate channel info to bootstrap)
 - rust-lang#134669 (Document the `--dev` flag for `src/ci/docker/run.sh`)
 - rust-lang#134680 (Clean up a few rmake tests  )

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit cfb7a56 into rust-lang:master Dec 23, 2024
6 checks passed
@rustbot rustbot added this to the 1.85.0 milestone Dec 23, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Dec 23, 2024
@rust-timer
Unrolled build for rust-lang#134528
381d37c 
Rollup merge of rust-lang#134528 - jieyouxu:fix-rustc-bootstrap-test, r=Kobzol

opt-dist: propagate channel info to bootstrap

Fixes rust-lang#133503.

Previously, `tests/ui/bootstrap/rustc_bootstap.rs` [sic] failed during [beta bump](rust-lang#133447 (comment)) in opt-dist tests. This is because:

- `opt-dist` tried to run `./x test` against beta-channel dist `rustc` through `bootstrap`.
- The dist build produced during the beta bump produces a `rustc` which correctly thinks that it is a beta compiler based on `src/ci/channel` info.
- `opt-dist` tries to run `./x test` on the beta `rustc` from the dist build, but without specifying channel through a synthetic `config.toml`, so `bootstrap` tells `compiletest` that we're on the `nightly` channel (by default).
- Now there's a channel mismatch: `compiletest` believes the `rustc` under test is a *nightly* rustc, but the `rustc` under test actually considers itself a *beta* rustc. This means that `//@ only-nightly` will be satisfied yet the test will fail as the *beta* rustc is not a *nightly* rustc.

This PR:

- Fixes the test failure during beta bump (i.e. rust-lang#133503) by having `opt-dist` faithfully report the channel of the dist `rustc` being tested (i.e. "beta" in a beta bump PR). This will properly make the test be ignored during beta bump as the `rustc` under test is not a *nightly* rustc.
- Fixes the test name `rustc_bootstap.rs` -> `rustc_bootstrap.rs`. No more stapping.
- Slightly adjusts the doc comment in the test to make it more clear.

I ran a try-job against the beta branch (explicitly running the opt-dist tests by modifying the job definition) with these changes in rust-lang#134131, and it appears that the try-job was [successful](rust-lang#134131 (comment)). The two commits in this PR are cherry-picked from rust-lang#134131, with the test commit slightly modified (to also adjust the test comments).

r? `@Kobzol` (or compiler or bootstrap or infra I guess?)
@jieyouxu jieyouxu deleted the fix-rustc-bootstrap-test branch December 24, 2024 04:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Kobzol Kobzol Kobzol approved these changes

Assignees

@Kobzol Kobzol

Labels

S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Milestone

1.85.0

Development

Successfully merging this pull request may close these issues.

tests/ui/bootstrap/rustc_bootstap.rs fails in post-PGO/opt-dist tests

4 participants

@jieyouxu @rustbot @bors @Kobzol