Skip to content

download-rustc fails to detect when profiler settings differ from local settings #133675

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
RalfJung opened this issue Nov 30, 2024 · 3 comments · Fixed by #133705
Closed

download-rustc fails to detect when profiler settings differ from local settings #133675

RalfJung opened this issue Nov 30, 2024 · 3 comments · Fixed by #133705
Labels
A-download-rustc Area: The `rust.download-rustc` build option. C-bug Category: This is a bug. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)

Comments

@RalfJung
Copy link
Member

See #133499 (comment) for context: the x86_64-gnu-aux runner builds rustc without profiling support. The CI build that is downloaded via download-rustc does have profiling support. This means if a test relies on profiling support, it will incorrectly pass on x86_64-gnu-aux if download-rustc is used, only to fail later on an unrelated PR when download-rustc cannot be used.

bootstrap tries to ensure that all the settings are the same between the local configuration and the prebuilt rustc, but that does not seem to fully work.

Cc @onur-ozkan

@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Nov 30, 2024
@RalfJung
Copy link
Member Author

RalfJung commented Nov 30, 2024

@onur-ozkan wrote

That PR doesn't change any contig at all? I guess we are on different points, I am talking about config.toml here.

The PR doesn't change the config indeed. x86_64-gnu-aux locally changes its config in CI (if I understood correctly). The download-rustc logic should detect that and never use the prebuilt rustc on that runner, since it was built with a different configuration than what is being locally set up.

@onur-ozkan
Copy link
Member

Probably, this happens because we are not checking "--set" flags yet.

@jieyouxu jieyouxu added T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) C-bug Category: This is a bug. A-download-rustc Area: The `rust.download-rustc` build option. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Dec 1, 2024
@onur-ozkan
Copy link
Member

"profiler" option is not part of the "rust" section in config.toml, which is why it was missed in the config compatibility check logic (it was designed to check "rust" section only). #133705 makes it to be handled properly.

github-merge-queue bot pushed a commit to rust-lang/cargo that referenced this issue Dec 3, 2024
### What does this PR try to resolve?

Only run PGO test on nightly,
so it won't run on rust-lang/rust's CI
because `CARGO_TEST_DISABLE_NIGHTLY` is set on that.

### How should we test and review this PR?

This is a temporary fix to unblock Cargo submoule update (assume that
rust-lang/rust#133675 won't be fixed this week).

In the future We should seek a solution that

* Disable test on rust-lang/rust's CI
* Maximize test coverage on different channels and platforms on Cargo's
CI.
@bors bors closed this as completed in 7d67af9 Dec 3, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Dec 3, 2024
Rollup merge of rust-lang#133705 - onur-ozkan:profiler-check, r=jieyouxu

add "profiler" and "optimized-compiler-builtins" option coverage for ci-rustc

Adds "profiler" and "optimized-compiler-builtins" option coverage in CI-rustc config compatibility check.

Resolves rust-lang#133675
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-download-rustc Area: The `rust.download-rustc` build option. C-bug Category: This is a bug. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants