-
Notifications
You must be signed in to change notification settings - Fork 13k
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: Unify test argument handling #110576
Conversation
r? bootstrap |
I tried to fix #80124 but ran into #100599 (comment) so this is probably enough experimenting for tonight and I'm going to put it off for another day 😅 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great effort!
LGTM
@bors r+ rollup |
bootstrap: Unify test argument handling Fixes rust-lang#104198. Does *not* help with rust-lang#80124 because I couldn't figure out a reasonable way to omit `--lib` only for `panic_abort` and not other `std` dependencies. - Remove unnecessary `test_kind` field and `TestKind` struct. These are just subsets of the existing `builder.kind` / `Kind` struct. - Add a new `run_cargo_test` function which handles passing arguments to cargo based on `builder.config` - Switch all Steps in `mod test` to `run_cargo_test` where possible - Combine several steps into one `CrateBootstrap` step. These tests all do the same thing, just with different crate names. - Fix `x test --no-doc`. This is much simpler after the refactors mentioned earlier, but I'm happy to split it into a separate PR if desired. Before, this would panic a lot because steps forgot to pass `--lib`.
bootstrap: Unify test argument handling Fixes rust-lang#104198. Does *not* help with rust-lang#80124 because I couldn't figure out a reasonable way to omit `--lib` only for `panic_abort` and not other `std` dependencies. - Remove unnecessary `test_kind` field and `TestKind` struct. These are just subsets of the existing `builder.kind` / `Kind` struct. - Add a new `run_cargo_test` function which handles passing arguments to cargo based on `builder.config` - Switch all Steps in `mod test` to `run_cargo_test` where possible - Combine several steps into one `CrateBootstrap` step. These tests all do the same thing, just with different crate names. - Fix `x test --no-doc`. This is much simpler after the refactors mentioned earlier, but I'm happy to split it into a separate PR if desired. Before, this would panic a lot because steps forgot to pass `--lib`.
bootstrap: Unify test argument handling Fixes rust-lang#104198. Does *not* help with rust-lang#80124 because I couldn't figure out a reasonable way to omit `--lib` only for `panic_abort` and not other `std` dependencies. - Remove unnecessary `test_kind` field and `TestKind` struct. These are just subsets of the existing `builder.kind` / `Kind` struct. - Add a new `run_cargo_test` function which handles passing arguments to cargo based on `builder.config` - Switch all Steps in `mod test` to `run_cargo_test` where possible - Combine several steps into one `CrateBootstrap` step. These tests all do the same thing, just with different crate names. - Fix `x test --no-doc`. This is much simpler after the refactors mentioned earlier, but I'm happy to split it into a separate PR if desired. Before, this would panic a lot because steps forgot to pass `--lib`.
bootstrap: Unify test argument handling Fixes rust-lang#104198. Does *not* help with rust-lang#80124 because I couldn't figure out a reasonable way to omit `--lib` only for `panic_abort` and not other `std` dependencies. - Remove unnecessary `test_kind` field and `TestKind` struct. These are just subsets of the existing `builder.kind` / `Kind` struct. - Add a new `run_cargo_test` function which handles passing arguments to cargo based on `builder.config` - Switch all Steps in `mod test` to `run_cargo_test` where possible - Combine several steps into one `CrateBootstrap` step. These tests all do the same thing, just with different crate names. - Fix `x test --no-doc`. This is much simpler after the refactors mentioned earlier, but I'm happy to split it into a separate PR if desired. Before, this would panic a lot because steps forgot to pass `--lib`.
@bors rollup=iffy might cause hang in |
@matthiaskrgr I think these might just be spurious? I'm not sure how they could be related. |
⌛ Testing commit b3ee81a with merge c00fa943ad370da9d5b5fb5596e5abeab3101162... |
Hm, I've seen hangs on two rollups both on the same runner, and both included this pr which seemed a bit suspicious to me 😅🤷 |
💥 Test timed out |
Ok I take it back, the failure I saw locally was unrelated :/ rust-lang/cargo#12042 I am not sure how to investigate this further. I guess I can revert the change for cargo only? |
I will also debug it tonight/early tomorrow. Let's see if I can find the reason |
I have repreduced the same errors(mostly about network errors and command not found errors) on windows machine with the default Commits that are causing the errors:
-- I only had limited time, couldn't work on the fix. But would work on the fix at the weekend if you don't want to work on the fixes(probably multiple fixes needed for both commits) @jyn514 |
that's so strange I couldn't reproduce locally. I'll try again this weekend. Thanks for looking into it :) |
I still can't reproduce this :( @ozkanonur what version of windows are you on? are you using gnu or MSVC? what was the output from the failing tests? I'm on Windows 10 Pro 22H2 using MSVC. |
Windows 11 / MSVC I just run
That's weird, how can Windows 10 work and 11 can't??? To achieve logs, I had to ignore
-- Some of the OS err messages are in Turkish language, but I guess that shouldn't be problem because they are mostly duplicated messages and can be easily translated to English on google. |
Oh! I have a global rustc install and CI doesn't. If you run |
It's fresh Windows 11 just has couple dependencies to build rustc. Just like CI, I also don't have rustc installed globally.
Yes |
- Use `cargo metadata` to determine whether a crate has a library package or not - Collect metadata for all workspaces, not just the root workspace and cargo - Don't pass `--lib` for crates without a library - Use `run_cargo_test` for rust-installer - Don't build documentation in `lint-docs` if `--no-doc` is passed
@bors r=ozkanonur rollup=iffy |
☀️ Test successful - checks-actions |
Finished benchmarking commit (87b1f89): comparison URL. Overall result: ❌ regressions - no action needed@rustbot label: -perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesThis benchmark run did not return any relevant results for this metric. |
Fixes #104198. Does not help with #80124 because I couldn't figure out a reasonable way to omit
--lib
only forpanic_abort
and not otherstd
dependencies.test_kind
field andTestKind
struct. These are just subsets of the existingbuilder.kind
/Kind
struct.run_cargo_test
function which handles passing arguments to cargo based onbuilder.config
mod test
torun_cargo_test
where possibleCrateBootstrap
step. These tests all do the same thing, just with different crate names.x test --no-doc
. This is much simpler after the refactors mentioned earlier, but I'm happy to split it into a separate PR if desired. Before, this would panic a lot because steps forgot to pass--lib
.