Fix rustc-args passing issue in bootstrap #113948

chenyukang · 2023-07-22T07:40:44Z

Fixes #113178, r? @jyn514

lqd · 2023-07-22T08:19:28Z

https://rust-lang.zulipchat.com/#narrow/stream/122651-general/topic/vacation/near/376793983

r? bootstrap

clubby789 · 2023-07-22T15:26:23Z

@bors r+ rollup

bors · 2023-07-22T15:26:25Z

📌 Commit c0156f1 has been approved by clubby789

It is now in the queue for this repository.

…iaskrgr Rollup of 6 pull requests Successful merges: - rust-lang#112508 (Tweak spans for self arg, fix borrow suggestion for signature mismatch) - rust-lang#113901 (Get rid of subst-relate incompleteness in new solver) - rust-lang#113948 (Fix rustc-args passing issue in bootstrap) - rust-lang#113950 (Remove Scope::Elision from bound-vars resolution.) - rust-lang#113957 (Add regression test for issue rust-lang#113941 - naive layout isn't refined) - rust-lang#113959 (Migrate GUI colors test to original CSS color format) r? `@ghost` `@rustbot` modify labels: rollup

RalfJung · 2024-01-04T11:05:51Z

src/bootstrap/builder.rs

+            self.config.cmd.rustc_args().iter().map(|s| s.to_string()).collect::<Vec<_>>();
+        if !rustc_args.is_empty() {
+            cargo.env("RUSTFLAGS", &rustc_args.join(" "));
+        }


I don't understand this patch, it doesn't make sense to me. This is setting the RUSTFLAGS env var, but when this Cargo instance is later turned into a Command, the RUSTFLAGS env var is anyway overwritten again:

rust/src/bootstrap/src/core/builder.rs

Lines 2414 to 2418 in c350d3c

fn from(mut cargo: Cargo) -> Command {

let rustflags = &cargo.rustflags.0;

if !rustflags.is_empty() {

cargo.command.env("RUSTFLAGS", rustflags);

}

Shouldn't this be added to rustflags instead?

Also the --help text says this should only be passed when running tests; I don't see that reflected in the code.

Yeah, this isn't correct and fixed in #123489.

bootstrap: don't use rustflags for `--rustc-args` r? `@onur-ozkan` This is going to require a bit of context. rust-lang#47558 has added `--rustc-args` to `./x test` to allow passing flags when building `compiletest` tests. It was made specifically because using `RUSTFLAGS` would rebuild the compiler/stdlib, which would in turn require the flag you want to build tests with to successfully bootstrap. rust-lang#113178 made the request that it also works for other tests and doctests. This is not trivial to support across the board for `library`/`compiler` unit-tests/doctests and across stages. This issue was closed in rust-lang#113948 by using `RUSTFLAGS`, seemingly incorrectly since rust-lang#123489 fixed that part to make it work. Unfortunately rust-lang#123489/rust-lang#113948 have regressed the goals of `--rustc-args`: - now we can't use rustc args that don't bootstrap, to run the UI tests: we can't test incomplete features. The new trait solver doesn't bootstrap, in-progress borrowck/polonius changes don't bootstrap, some other features are similarly incomplete, etc. - using the flag now rebuilds everything from scratch: stage0 stdlib, stage1 compiler, stage1 stdlib. You don't need to re-do all this to compile UI tests, you only need the latter to run stdlib tests with a new flag, etc. This happens for contributors, but also on CI today. (Not to mention that in doing that it will rebuild things with flags that are not meant to be used, e.g. stdlib cfgs that don't exist in the compiler; or you could also imagine that this silently enables flags that were not meant to be enabled in this way). Since then, rust-lang@bd71c71 has started using it to test a stdlib feature, relying on the fact that it now rebuilds everything. So rust-lang#125011 also regressed CI times more than necessary because it rebuilds everything instead of just stage 1 stdlib. It's not easy for me to know how to properly fix rust-lang#113178 in bootstrap, but rust-lang#113948/rust-lang#123489 are not it since they regress the initial intent. I'd think bootstrap would have to know from the list of test targets that are passed that the `library` or `compiler` paths that are passed could require rebuilding these crates with different rustflags, probably also depending on stages. Therefore I would not be able to fix it, and will just try in this PR to unregress the situation to unblock the initial use-case. It seems miri now also uses `./x miri --rustc-args` in this incorrect meaning to rebuild the `library` paths they support to run with the new args. I've not made any bootstrap changes related to `./x miri` in this PR, so `--rustc-args` wouldn't work there anymore. I'd assume this would need to use rustflags again but I don't know how to make that work properly in bootstrap, hence opening as draft, so you can tell me how to do that. I assume we don't want to break their use-case again now that it exists, even though there are ways to use `./x test` to do exactly that. `RUSTFLAGS_NOT_BOOTSTRAP=flag ./x test library/std` is a way to run unit tests with a new flag without rebuilding everything, while with rust-lang#123489 there is no way anymore to run tests with a flag that doesn't bootstrap. --- edit: after review, this PR: - renames `./x test --rustc-args` to `./x test --compiletest-rustc-args` as it only applies there, and cannot use rustflags for this purpose. - fixes the regression that using these args rebuilt everything from scratch - speeds up some CI jobs via the above point - removes `./x miri --rustc-args` as only library tests are supported, needs to rebuild libstd, and `./x miri --compiletest-rustc-args` wouldn't work since compiletests are not supported.

Rollup merge of rust-lang#128841 - lqd:rustc-args, r=onur-ozkan bootstrap: don't use rustflags for `--rustc-args` r? `@onur-ozkan` This is going to require a bit of context. rust-lang#47558 has added `--rustc-args` to `./x test` to allow passing flags when building `compiletest` tests. It was made specifically because using `RUSTFLAGS` would rebuild the compiler/stdlib, which would in turn require the flag you want to build tests with to successfully bootstrap. rust-lang#113178 made the request that it also works for other tests and doctests. This is not trivial to support across the board for `library`/`compiler` unit-tests/doctests and across stages. This issue was closed in rust-lang#113948 by using `RUSTFLAGS`, seemingly incorrectly since rust-lang#123489 fixed that part to make it work. Unfortunately rust-lang#123489/rust-lang#113948 have regressed the goals of `--rustc-args`: - now we can't use rustc args that don't bootstrap, to run the UI tests: we can't test incomplete features. The new trait solver doesn't bootstrap, in-progress borrowck/polonius changes don't bootstrap, some other features are similarly incomplete, etc. - using the flag now rebuilds everything from scratch: stage0 stdlib, stage1 compiler, stage1 stdlib. You don't need to re-do all this to compile UI tests, you only need the latter to run stdlib tests with a new flag, etc. This happens for contributors, but also on CI today. (Not to mention that in doing that it will rebuild things with flags that are not meant to be used, e.g. stdlib cfgs that don't exist in the compiler; or you could also imagine that this silently enables flags that were not meant to be enabled in this way). Since then, rust-lang@bd71c71 has started using it to test a stdlib feature, relying on the fact that it now rebuilds everything. So rust-lang#125011 also regressed CI times more than necessary because it rebuilds everything instead of just stage 1 stdlib. It's not easy for me to know how to properly fix rust-lang#113178 in bootstrap, but rust-lang#113948/rust-lang#123489 are not it since they regress the initial intent. I'd think bootstrap would have to know from the list of test targets that are passed that the `library` or `compiler` paths that are passed could require rebuilding these crates with different rustflags, probably also depending on stages. Therefore I would not be able to fix it, and will just try in this PR to unregress the situation to unblock the initial use-case. It seems miri now also uses `./x miri --rustc-args` in this incorrect meaning to rebuild the `library` paths they support to run with the new args. I've not made any bootstrap changes related to `./x miri` in this PR, so `--rustc-args` wouldn't work there anymore. I'd assume this would need to use rustflags again but I don't know how to make that work properly in bootstrap, hence opening as draft, so you can tell me how to do that. I assume we don't want to break their use-case again now that it exists, even though there are ways to use `./x test` to do exactly that. `RUSTFLAGS_NOT_BOOTSTRAP=flag ./x test library/std` is a way to run unit tests with a new flag without rebuilding everything, while with rust-lang#123489 there is no way anymore to run tests with a flag that doesn't bootstrap. --- edit: after review, this PR: - renames `./x test --rustc-args` to `./x test --compiletest-rustc-args` as it only applies there, and cannot use rustflags for this purpose. - fixes the regression that using these args rebuilt everything from scratch - speeds up some CI jobs via the above point - removes `./x miri --rustc-args` as only library tests are supported, needs to rebuild libstd, and `./x miri --compiletest-rustc-args` wouldn't work since compiletests are not supported.

Fix rustc-args passing issue in bootstrap

c0156f1

rustbot assigned jyn514 Jul 22, 2023

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

rustbot assigned clubby789 and unassigned jyn514 Jul 22, 2023

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

matthiaskrgr mentioned this pull request Jul 22, 2023

Rollup of 6 pull requests #113968

Merged

bors merged commit 3c83eab into rust-lang:master Jul 22, 2023
11 checks passed

rustbot added this to the 1.73.0 milestone Jul 22, 2023

RalfJung reviewed Jan 4, 2024

View reviewed changes

RalfJung mentioned this pull request Mar 30, 2024

bootstrap: Builder::cargo treats rustc_args in ways that don't make sense #123228

Closed

lqd mentioned this pull request Aug 8, 2024

bootstrap: don't use rustflags for --rustc-args #128841

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix rustc-args passing issue in bootstrap #113948

Fix rustc-args passing issue in bootstrap #113948

chenyukang commented Jul 22, 2023 •

edited

Loading

lqd commented Jul 22, 2023

clubby789 commented Jul 22, 2023

bors commented Jul 22, 2023

RalfJung Jan 4, 2024

RalfJung Jan 4, 2024

RalfJung Jan 4, 2024

onur-ozkan Apr 5, 2024

	fn from(mut cargo: Cargo) -> Command {
	let rustflags = &cargo.rustflags.0;
	if !rustflags.is_empty() {
	cargo.command.env("RUSTFLAGS", rustflags);
	}

Fix rustc-args passing issue in bootstrap #113948

Fix rustc-args passing issue in bootstrap #113948

Conversation

chenyukang commented Jul 22, 2023 • edited Loading

lqd commented Jul 22, 2023

clubby789 commented Jul 22, 2023

bors commented Jul 22, 2023

RalfJung Jan 4, 2024

Choose a reason for hiding this comment

RalfJung Jan 4, 2024

Choose a reason for hiding this comment

RalfJung Jan 4, 2024

Choose a reason for hiding this comment

onur-ozkan Apr 5, 2024

Choose a reason for hiding this comment

chenyukang commented Jul 22, 2023 •

edited

Loading