Skip to content
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

handle ci-rustc incompatible options during config parse #127322

Merged
merged 2 commits into from
Jul 14, 2024

Conversation

onur-ozkan
Copy link
Member

@onur-ozkan onur-ozkan commented Jul 4, 2024

This PR ensures that config.toml does not use CI rustc incompatible options when CI rustc is enabled (just like ci-llvm checks). Some options can change compiler's behavior in certain scenarios. If we don't check these incompatible options, CI runners using CI rustc might ignore options we have explicitly set. This could be dangerous as we might think a rustc test passed with option T but in fact it wasn't tested with option T.

Later in #122709, I will disable CI rustc if any of those options were used (similar to this approach). If CI runners fail because of these checks, it means the logic in run.sh isn't covering the incompatible options correctly (since any incompatible option should turn off CI rustc).

The list may not be complete, but should be a good first step as it's better than nothing!

Blocker for #122709

@rustbot
Copy link
Collaborator

rustbot commented Jul 4, 2024

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Jul 4, 2024
@rustbot
Copy link
Collaborator

rustbot commented Jul 4, 2024

This PR modifies config.example.toml.

If appropriate, please update CONFIG_CHANGE_HISTORY in src/bootstrap/src/utils/change_tracker.rs.

This PR modifies src/bootstrap/src/core/config.

If appropriate, please update CONFIG_CHANGE_HISTORY in src/bootstrap/src/utils/change_tracker.rs.

Leading spaces in config options cause `configure` script to fail.

Signed-off-by: onur-ozkan <work@onurozkan.dev>
@onur-ozkan onur-ozkan force-pushed the ci-rustc-incompatible-options branch 3 times, most recently from e8412f7 to d3dce36 Compare July 4, 2024 14:31
err_incompatible_ci_rustc_option!(optimize_toml, "optimize");
err_incompatible_ci_rustc_option!(debug_toml, "debug");
err_incompatible_ci_rustc_option!(codegen_units, "codegen-units");
err_incompatible_ci_rustc_option!(codegen_units_std, "codegen-units-std");
Copy link
Member

Choose a reason for hiding this comment

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

Are these really all incompatible? I'd have assumed that you still get to use all of these for a full build (i.e., basically stage 2 build). Those options related to std definitely feel like they still are fine -- they would certainly apply to the standard library built by the downloaded rustc, right?

Copy link
Member Author

@onur-ozkan onur-ozkan Jul 5, 2024

Choose a reason for hiding this comment

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

Building std from source is not currently allowed with download-rustc:

// When using `download-rustc`, we already have artifacts for the host available. Don't
// recompile them.
if builder.download_rustc() && target == builder.build.build
// NOTE: the beta compiler may generate different artifacts than the downloaded compiler, so
// its artifacts can't be reused.
&& compiler.stage != 0
// This check is specific to testing std itself; see `test::Std` for more details.
&& !self.force_recompile
{
let sysroot = builder.ensure(Sysroot { compiler, force_recompile: false });
cp_rustc_component_to_ci_sysroot(
builder,
&sysroot,
builder.config.ci_rust_std_contents(),
);
return;
}

But it will eventually be possible with redesign stage 0 std (I will make it possible before landing use precompiled rustc for non-dist builders), so I will remove the std-related options from the list.

@onur-ozkan onur-ozkan force-pushed the ci-rustc-incompatible-options branch from d3dce36 to 99336cc Compare July 5, 2024 07:24
@onur-ozkan
Copy link
Member Author

I got some ideas to improve the current approach. I will work on them later today.

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 6, 2024
@onur-ozkan onur-ozkan force-pushed the ci-rustc-incompatible-options branch 3 times, most recently from 7527d8d to d9a9491 Compare July 6, 2024 12:04
This change ensures that `config.toml` does not use CI rustc incompatible
options when CI rustc is enabled. This is necessary because some options
can change compiler's behavior in certain scenarios.

The list may not be complete, but should be a good first step as it's better than nothing!

Signed-off-by: onur-ozkan <work@onurozkan.dev>
@onur-ozkan onur-ozkan force-pushed the ci-rustc-incompatible-options branch from d9a9491 to 335bdd4 Compare July 6, 2024 13:08
@onur-ozkan
Copy link
Member Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 6, 2024
@Mark-Simulacrum
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Jul 13, 2024

📌 Commit 335bdd4 has been approved by Mark-Simulacrum

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 Jul 13, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 14, 2024
…iaskrgr

Rollup of 5 pull requests

Successful merges:

 - rust-lang#127083 (Add release notes for 1.80)
 - rust-lang#127322 (handle ci-rustc incompatible options during config parse)
 - rust-lang#127697 (use std for file mtime and atime modifications)
 - rust-lang#127704 (Fix minor typos in std::process doc on Win argv)
 - rust-lang#127710 (clarify the meaning of the version number for accepted/removed features)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 60e10e6 into rust-lang:master Jul 14, 2024
6 checks passed
@rustbot rustbot added this to the 1.81.0 milestone Jul 14, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jul 14, 2024
Rollup merge of rust-lang#127322 - onur-ozkan:ci-rustc-incompatible-options, r=Mark-Simulacrum

handle ci-rustc incompatible options during config parse

This PR ensures that `config.toml` does not use CI rustc incompatible options when CI rustc is enabled (just like [ci-llvm checks](https://github.com/rust-lang/rust/blob/e2cf31a6148725bde4ea48acf1e4fe72675257a2/src/bootstrap/src/core/config/config.rs#L1809-L1836)). Some options can change compiler's behavior in certain scenarios. If we don't check these incompatible options, CI runners using CI rustc might ignore options we have explicitly set. This could be dangerous as we might think a rustc test passed with option T but in fact it wasn't tested with option T.

Later in rust-lang#122709, I will disable CI rustc if any of those options were used (similar to [this approach](https://github.com/rust-lang/rust/blob/dd2c24aafddbd9cc170f32f5b447c7d3005c7412/src/ci/run.sh#L165-L169)). If CI runners fail because of these checks, it means the logic in run.sh isn't covering the incompatible options correctly (since any incompatible option should turn off CI rustc).

The list may not be complete, but should be a good first step as it's better than nothing!

Blocker for rust-lang#122709
@onur-ozkan onur-ozkan deleted the ci-rustc-incompatible-options branch July 14, 2024 18:11
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 4, 2024
…k-Simulacrum

force compiling std from source if modified

This allows the standard library to be compiled even with `download-rustc` enabled. Which means it's no longer a requirement to compile `rustc` in order to compile `std`.

Ref. rust-lang#127322 (comment).
Blocker for rust-lang#122709.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 4, 2024
…k-Simulacrum

force compiling std from source if modified

This allows the standard library to be compiled even with `download-rustc` enabled. Which means it's no longer a requirement to compile `rustc` in order to compile `std`.

Ref. rust-lang#127322 (comment).
Blocker for rust-lang#122709.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 4, 2024
…k-Simulacrum

force compiling std from source if modified

This allows the standard library to be compiled even with `download-rustc` enabled. Which means it's no longer a requirement to compile `rustc` in order to compile `std`.

Ref. rust-lang#127322 (comment).
Blocker for rust-lang#122709.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 4, 2024
…k-Simulacrum

force compiling std from source if modified

This allows the standard library to be compiled even with `download-rustc` enabled. Which means it's no longer a requirement to compile `rustc` in order to compile `std`.

Ref. rust-lang#127322 (comment).
Blocker for rust-lang#122709.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 5, 2024
…k-Simulacrum

force compiling std from source if modified

This allows the standard library to be compiled even with `download-rustc` enabled. Which means it's no longer a requirement to compile `rustc` in order to compile `std`.

Ref. rust-lang#127322 (comment).
Blocker for rust-lang#122709.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 5, 2024
…k-Simulacrum

force compiling std from source if modified

This allows the standard library to be compiled even with `download-rustc` enabled. Which means it's no longer a requirement to compile `rustc` in order to compile `std`.

Ref. rust-lang#127322 (comment).
Blocker for rust-lang#122709.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Aug 5, 2024
Rollup merge of rust-lang#127974 - onur-ozkan:force-std-builds, r=Mark-Simulacrum

force compiling std from source if modified

This allows the standard library to be compiled even with `download-rustc` enabled. Which means it's no longer a requirement to compile `rustc` in order to compile `std`.

Ref. rust-lang#127322 (comment).
Blocker for rust-lang#122709.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants