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

bootstrap: rustc-args is only passed to compiletest, not unit tests #113178

Closed
jyn514 opened this issue Jun 30, 2023 · 2 comments · Fixed by #113948 or #128841
Closed

bootstrap: rustc-args is only passed to compiletest, not unit tests #113178

jyn514 opened this issue Jun 30, 2023 · 2 comments · Fixed by #113948 or #128841
Assignees
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc C-bug Category: This is a bug. E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)

Comments

@jyn514
Copy link
Member

jyn514 commented Jun 30, 2023

flags.extend(builder.config.cmd.rustc_args().iter().map(|s| s.to_string()));

cc https://rust-lang.zulipchat.com/#narrow/stream/219381-t-libs/topic/Running.20tests.20in.20debug.20mode.20with.20debug.20symbols.3F/near/361025470

We should either pass this flag consistently or rip it out entirely; having it work only sometimes makes it an attractive nuisance.

Meta

HEAD is 3307274

@jyn514 jyn514 added C-bug Category: This is a bug. A-testsuite Area: The testsuite used to check the correctness of rustc T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Jun 30, 2023
@jyn514
Copy link
Member Author

jyn514 commented Jun 30, 2023

we should at least set them in prepare_cargo_test, probably through RUSTFLAGS since cargo doesn't allow setting them directly?

cargo.arg("--").args(&builder.config.test_args()).args(libtest_args);

@jyn514 jyn514 added E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. labels Jun 30, 2023
@chenyukang chenyukang self-assigned this Jul 3, 2023
@chenyukang
Copy link
Member

seems we have handled rustc-args for compiletest at here?

rust/src/bootstrap/test.rs

Lines 1595 to 1607 in 5122e88

let mut hostflags = flags.clone();
hostflags.push(format!("-Lnative={}", builder.test_helpers_out(compiler.host).display()));
hostflags.extend(builder.lld_flags(compiler.host));
for flag in hostflags {
cmd.arg("--host-rustcflags").arg(flag);
}
let mut targetflags = flags;
targetflags.push(format!("-Lnative={}", builder.test_helpers_out(target).display()));
targetflags.extend(builder.lld_flags(target));
for flag in targetflags {
cmd.arg("--target-rustcflags").arg(flag);
}

@bors bors closed this as completed in 3c83eab Jul 22, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Aug 13, 2024
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.
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Aug 13, 2024
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc C-bug Category: This is a bug. E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
2 participants