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: Unify test argument handling #110576

Merged
merged 6 commits into from
Apr 29, 2023
Merged

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented Apr 20, 2023

Fixes #104198. Does not help with #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.

@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 Apr 20, 2023
@jyn514 jyn514 marked this pull request as draft April 20, 2023 04:16
@jyn514 jyn514 marked this pull request as ready for review April 21, 2023 02:38
@jyn514
Copy link
Member Author

jyn514 commented Apr 21, 2023

r? bootstrap

@jyn514
Copy link
Member Author

jyn514 commented Apr 21, 2023

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 😅

Copy link
Member

@onur-ozkan onur-ozkan left a comment

Choose a reason for hiding this comment

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

great effort!

LGTM

@onur-ozkan
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Apr 22, 2023

📌 Commit b3ee81a has been approved by ozkanonur

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 Apr 22, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Apr 22, 2023
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`.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Apr 22, 2023
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`.
compiler-errors added a commit to compiler-errors/rust that referenced this pull request Apr 22, 2023
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`.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Apr 23, 2023
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`.
@matthiaskrgr
Copy link
Member

@bors rollup=iffy might cause hang in auto - x86_64-msvc-cargo

@jyn514
Copy link
Member Author

jyn514 commented Apr 24, 2023

test concurrent::killing_cargo_releases_the_lock has been running for a long time
test death::ctrl_c_kills_everyone has been running for a long time

@matthiaskrgr I think these might just be spurious? I'm not sure how they could be related.

@bors
Copy link
Contributor

bors commented Apr 24, 2023

⌛ Testing commit b3ee81a with merge c00fa943ad370da9d5b5fb5596e5abeab3101162...

@matthiaskrgr
Copy link
Member

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 😅🤷

@bors
Copy link
Contributor

bors commented Apr 24, 2023

💥 Test timed out

@jyn514
Copy link
Member Author

jyn514 commented Apr 27, 2023

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?

@onur-ozkan
Copy link
Member

I will also debug it tonight/early tomorrow. Let's see if I can find the reason

@onur-ozkan
Copy link
Member

I have repreduced the same errors(mostly about network errors and command not found errors) on windows machine with the default tools configuration created with x setup.

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

@jyn514
Copy link
Member Author

jyn514 commented Apr 27, 2023

that's so strange I couldn't reproduce locally. I'll try again this weekend. Thanks for looking into it :)

@jyn514
Copy link
Member Author

jyn514 commented Apr 29, 2023

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.

@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels Apr 29, 2023
@onur-ozkan
Copy link
Member

onur-ozkan commented Apr 29, 2023

what version of windows are you on? are you using gnu or MSVC?

Windows 11 / MSVC

I just run x setup and picked tools profile and then x test cargo using Powershell.

I'm on Windows 10 Pro 22H2 using MSVC.

That's weird, how can Windows 10 work and 11 can't???

To achieve logs, I had to ignore ctrl_c_kills_everyone, killing_cargo_releases_the_lock, close_output, close_output_during_drain tests because they were hanging the process.

--

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.

@jyn514
Copy link
Member Author

jyn514 commented Apr 29, 2023

Oh! I have a global rustc install and CI doesn't. If you run rustc --version in your home directory, does that output anything? Or are you only using the rustc managed by bootstrap?

@onur-ozkan
Copy link
Member

Oh! I have a global rustc install and CI doesn't. If you run rustc --version in your home directory, does that output anything?

It's fresh Windows 11 just has couple dependencies to build rustc. Just like CI, I also don't have rustc installed globally.

Are you only using the rustc managed by bootstrap?

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
@jyn514
Copy link
Member Author

jyn514 commented Apr 29, 2023

@bors r=ozkanonur rollup=iffy

@bors
Copy link
Contributor

bors commented Apr 29, 2023

📌 Commit 78a7093 has been approved by ozkanonur

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 29, 2023
@bors
Copy link
Contributor

bors commented Apr 29, 2023

⌛ Testing commit 78a7093 with merge 87b1f89...

@bors
Copy link
Contributor

bors commented Apr 29, 2023

☀️ Test successful - checks-actions
Approved by: ozkanonur
Pushing 87b1f89 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 29, 2023
@bors bors merged commit 87b1f89 into rust-lang:master Apr 29, 2023
@rustbot rustbot added this to the 1.71.0 milestone Apr 29, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (87b1f89): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
0.3% [0.3%, 0.3%] 2
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results

This 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.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-1.4% [-2.4%, -0.4%] 11
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -1.4% [-2.4%, -0.4%] 11

Cycles

This benchmark run did not return any relevant results for this metric.

@jyn514 jyn514 deleted the unify-test-args branch April 30, 2023 01:31
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 merged-by-bors This PR was explicitly merged by bors. 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) T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unify test code in bootstrap
7 participants