Implement needs-subprocess directive, and cleanup a bunch of tests to use needs-{subprocess,threads} by jieyouxu · Pull Request #135926 · rust-lang/rust (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation34 Commits26 Checks6 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Summary
Closes #128295.
- Implements
//@ needs-subprocessdirective in compiletest as requested in compiletest: add a needs-subprocess directive #128295. However, compiletest is a host tool, so we can't just try to spawn process because that spawns the process on host, not the target, under cross-compilation scenarios.- The short-term solution is to add Yet Another list of allow-list targets.
- The long-term solution is to first check if a
$targetsupports std, then try to run a binary to do run-time capability detection on the target. But that is tricky because you have to build-and-run a binary for the target. - This PR picks the short-term solution, because the long-term solution is highly non-trivial, and it's already an improvement over individual
ignore-*s all over the place. - Opened an issue about the long-term solution in compiletest: investigate if it's possible to do run-time capability detection against *target* #135928.
- Documents
//@ needs-subprocessin rustc-dev-guide. - Replace
ignore-{wasm,wasm32,emscripten,sgx}withneeds-{subprocess,threads}where suitable in tests. - Some drive-by test changes as I was trying to figure out if I could use
needs-{subprocess,threads}and found some bits needlessly distracting.
Count of tests that use ignore-{wasm,wasm32,emscripten,sgx} before and after this PR:
| State | ignore-sgx | ignore-wasm | ignore-emscripten |
|---|---|---|---|
| Before this PR | 96 | 88 | 207 |
| After this PR | 36 | 38 | 61 |
Commands used to find out locally
--- before
[17:40] Joe:rust (fresh) | rg --no-ignore -l "ignore-sgx" tests | wc -l
96
[17:40] Joe:rust (fresh) | rg --no-ignore -l "ignore-wasm" tests | wc -l
88
[17:40] Joe:rust (fresh) | rg --no-ignore -l "ignore-emscripten" tests | wc -l
207
--- after
[17:39] Joe:rust (needs-subprocess-thread) | rg --no-ignore -l "ignore-sgx" tests | wc -l
36
[17:39] Joe:rust (needs-subprocess-thread) | rg --no-ignore -l "ignore-wasm" tests | wc -l
38
[17:39] Joe:rust (needs-subprocess-thread) | rg --no-ignore -l "ignore-emscripten" tests | wc -l
61
Review advice
- Best reviewed commit-by-commit.
- Non-trivial test changes (not mechanically simple replacements) are split into individual commits to help with review. Their individual commit messages give some basic description of the changes.
- I could split some test changes out into another PR, but I found that I needed to change some tests to
needs-threads, some toneeds-subprocess, and some needed to use both, so they might conflict and become very annoying.
r? @ghost (need to run try jobs)
try-job: x86_64-msvc-1
try-job: i686-msvc-1
try-job: i686-mingw
try-job: x86_64-mingw-1
try-job: x86_64-apple-1
try-job: aarch64-apple
try-job: aarch64-gnu
try-job: test-various
try-job: armhf-gnu
This comment was marked as resolved.
r? @oli-obk
rustbot has assigned @oli-obk.
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
Area: The compiletest test runner
Area: rustc-dev-guide
Area: The testsuite used to check the correctness of rustc
Status: Awaiting review from the assignee but also interested parties.
Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Relevant to the compiler team, which will review and decide on the PR/issue.
labels
This comment was marked as outdated.
bors added a commit to rust-lang-ci/rust that referenced this pull request
…
Implement needs-subprocess directive, and cleanup a bunch of tests to use `needs-{subprocess,threads}
Summary
- Implements
//@ needs-subprocessdirective in compiletest. - Documents
//@ needs-subprocessin rustc-dev-guide. - Replace
ignore-{wasm,wasm32,emscripten,sgx}withneeds-{subprocess,threads}where suitable in tests. - Some drive-by test changes as I was trying to figure out if I could use
needs-{subprocess,threads}and found some bits needlessly distracting.
Count of tests that use ignore-{wasm,wasm32,emscripten,sgx} before and after this PR:
| State | ignore-sgx |
ignore-wasm |
ignore-emscripten |
|---|---|---|---|
| Before this PR | 96 | 88 | 207 |
| After this PR | 36 | 38 | 61 |
Commands used to find out locally
--- before
[17:40] Joe:rust (fresh) | rg --no-ignore -l "ignore-sgx" tests | wc -l
96
[17:40] Joe:rust (fresh) | rg --no-ignore -l "ignore-wasm" tests | wc -l
88
[17:40] Joe:rust (fresh) | rg --no-ignore -l "ignore-emscripten" tests | wc -l
207
--- after
[17:39] Joe:rust (needs-subprocess-thread) | rg --no-ignore -l "ignore-sgx" tests | wc -l
36
[17:39] Joe:rust (needs-subprocess-thread) | rg --no-ignore -l "ignore-wasm" tests | wc -l
38
[17:39] Joe:rust (needs-subprocess-thread) | rg --no-ignore -l "ignore-emscripten" tests | wc -l
61Review advice
- Best reviewed commit-by-commit.
- Non-trivial test changes (not mechanically simple replacements) are split into individual commits to help with review.
- I could split some test changes out into another PR, but I found that I needed to change some tests to
needs-threads, some toneeds-subprocess, and some needed to use both, so they might conflict and become very annoying.
r? @ghost (need to run try jobs)
try-job: x86_64-msvc-1 try-job: i686-msvc-1 try-job: i686-mingw try-job: x86_64-mingw-1 try-job: x86_64-apple-1 try-job: aarch64-apple try-job: aarch64-gnu try-job: test-various try-job: armhf-gnu
This comment was marked as outdated.
Implement Implement needs-subprocess directive, and cleanup a bunch of tests to use `needs-{subprocess,threads}needs-subprocess directive, and cleanup a bunch of tests to use needs-{subprocess,threads}
| @@ -4,8 +4,6 @@ |
|---|
| // these platforms also. |
| //@ ignore-windows |
| //@ ignore-wasm32 no processes |
| //@ ignore-sgx no processes |
| use std::process::Command; |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test is actually exercising the use import suggestion for the Unix CommandExt as an example
| #![allow(non_upper_case_globals)] |
|---|
| //@ ignore-emscripten no threads |
| //@ check-pass |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another closure type inference test, does not need to run
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
This comment has been minimized.
This comment was marked as outdated.
This comment was marked as outdated.
Comment on lines +492 to +501
| pub fn has_subprocess_support(&self) -> bool { | |
|---|---|
| // FIXME(#135928): compiletest is always a **host** tool. Building and running an | |
| // capability detection executable against the **target** is not trivial. The short term | |
| // solution here is to hard-code some targets to allow/deny, unfortunately. | |
| let unsupported_target = self.target_cfg().env == "sgx" | |
| | | matches!(self.target_cfg().arch.as_str(), "wasm32" |
| | | self.target_cfg().os == "emscripten"; |
| !unsupported_target | |
| } |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like it, but it is what it is, at least it's centralized here.
bors added a commit to rust-lang-ci/rust that referenced this pull request
…
Implement needs-subprocess directive, and cleanup a bunch of tests to use needs-{subprocess,threads}
Summary
Closes rust-lang#128295.
- Implements
//@ needs-subprocessdirective in compiletest as requested in rust-lang#128295. However, compiletest is a host tool, so we can't just try to spawn process because that spawns the process on host, not the target, under cross-compilation scenarios.- The short-term solution is to add Yet Another list of allow-list targets.
- The long-term solution is to first check if a
$targetsupports std, then try to run a binary to do run-time capability detection on the target. But that is tricky because you have to build-and-run a binary for the target. - This PR picks the short-term solution, because the long-term solution is highly non-trivial, and it's already an improvement over individual
ignore-*s all over the place. - Opened an issue about the long-term solution in rust-lang#135928.
- Documents
//@ needs-subprocessin rustc-dev-guide. - Replace
ignore-{wasm,wasm32,emscripten,sgx}withneeds-{subprocess,threads}where suitable in tests. - Some drive-by test changes as I was trying to figure out if I could use
needs-{subprocess,threads}and found some bits needlessly distracting.
Count of tests that use ignore-{wasm,wasm32,emscripten,sgx} before and after this PR:
| State | ignore-sgx |
ignore-wasm |
ignore-emscripten |
|---|---|---|---|
| Before this PR | 96 | 88 | 207 |
| After this PR | 36 | 38 | 61 |
Commands used to find out locally
--- before
[17:40] Joe:rust (fresh) | rg --no-ignore -l "ignore-sgx" tests | wc -l
96
[17:40] Joe:rust (fresh) | rg --no-ignore -l "ignore-wasm" tests | wc -l
88
[17:40] Joe:rust (fresh) | rg --no-ignore -l "ignore-emscripten" tests | wc -l
207
--- after
[17:39] Joe:rust (needs-subprocess-thread) | rg --no-ignore -l "ignore-sgx" tests | wc -l
36
[17:39] Joe:rust (needs-subprocess-thread) | rg --no-ignore -l "ignore-wasm" tests | wc -l
38
[17:39] Joe:rust (needs-subprocess-thread) | rg --no-ignore -l "ignore-emscripten" tests | wc -l
61Review advice
- Best reviewed commit-by-commit.
- Non-trivial test changes (not mechanically simple replacements) are split into individual commits to help with review.
- I could split some test changes out into another PR, but I found that I needed to change some tests to
needs-threads, some toneeds-subprocess, and some needed to use both, so they might conflict and become very annoying.
r? @ghost (need to run try jobs)
try-job: x86_64-msvc-1 try-job: i686-msvc-1 try-job: i686-mingw try-job: x86_64-mingw-1 try-job: x86_64-apple-1 try-job: aarch64-apple try-job: aarch64-gnu try-job: test-various try-job: armhf-gnu
This comment was marked as resolved.
This comment has been minimized.
This comment was marked as resolved.
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
I updated the tests manually and unfortunately there isn't really an easy way to just yolo replace, sorry for the ton of test changes.
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
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
| @@ -18,9 +18,9 @@ |
|---|
| //@ ignore-android FIXME #17520 |
| //@ ignore-openbsd no support for libbacktrace without filename |
| //@ ignore-wasm no backtrace support |
| //@ ignore-emscripten no panic or subprocess support |
| //@ ignore-sgx no subprocess support |
| //@ ignore-emscripten no panic support |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't this also "no backtrace support"? It's a panic=abort target anyway, and this is not an unwind test
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I wasn't sure about that one, which is why I opened Cleanup "no panic support" tests #135923. I have no idea what "no panic support" actually means, so I didn't modify those ones in this PR. I suspect it's actually needs-unwind, but I wasn't 100% sure so...
📌 Commit 071ad37 has been approved by oli-obk
It is now in the queue for this repository.
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
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request
…r=oli-obk
Implement needs-subprocess directive, and cleanup a bunch of tests to use needs-{subprocess,threads}
Summary
Closes rust-lang#128295.
- Implements
//@ needs-subprocessdirective in compiletest as requested in rust-lang#128295. However, compiletest is a host tool, so we can't just try to spawn process because that spawns the process on host, not the target, under cross-compilation scenarios.- The short-term solution is to add Yet Another list of allow-list targets.
- The long-term solution is to first check if a
$targetsupports std, then try to run a binary to do run-time capability detection on the target. But that is tricky because you have to build-and-run a binary for the target. - This PR picks the short-term solution, because the long-term solution is highly non-trivial, and it's already an improvement over individual
ignore-*s all over the place. - Opened an issue about the long-term solution in rust-lang#135928.
- Documents
//@ needs-subprocessin rustc-dev-guide. - Replace
ignore-{wasm,wasm32,emscripten,sgx}withneeds-{subprocess,threads}where suitable in tests. - Some drive-by test changes as I was trying to figure out if I could use
needs-{subprocess,threads}and found some bits needlessly distracting.
Count of tests that use ignore-{wasm,wasm32,emscripten,sgx} before and after this PR:
| State | ignore-sgx |
ignore-wasm |
ignore-emscripten |
|---|---|---|---|
| Before this PR | 96 | 88 | 207 |
| After this PR | 36 | 38 | 61 |
Commands used to find out locally
--- before
[17:40] Joe:rust (fresh) | rg --no-ignore -l "ignore-sgx" tests | wc -l
96
[17:40] Joe:rust (fresh) | rg --no-ignore -l "ignore-wasm" tests | wc -l
88
[17:40] Joe:rust (fresh) | rg --no-ignore -l "ignore-emscripten" tests | wc -l
207
--- after
[17:39] Joe:rust (needs-subprocess-thread) | rg --no-ignore -l "ignore-sgx" tests | wc -l
36
[17:39] Joe:rust (needs-subprocess-thread) | rg --no-ignore -l "ignore-wasm" tests | wc -l
38
[17:39] Joe:rust (needs-subprocess-thread) | rg --no-ignore -l "ignore-emscripten" tests | wc -l
61Review advice
- Best reviewed commit-by-commit.
- Non-trivial test changes (not mechanically simple replacements) are split into individual commits to help with review. Their individual commit messages give some basic description of the changes.
- I could split some test changes out into another PR, but I found that I needed to change some tests to
needs-threads, some toneeds-subprocess, and some needed to use both, so they might conflict and become very annoying.
r? @ghost (need to run try jobs)
try-job: x86_64-msvc-1 try-job: i686-msvc-1 try-job: i686-mingw try-job: x86_64-mingw-1 try-job: x86_64-apple-1 try-job: aarch64-apple try-job: aarch64-gnu try-job: test-various try-job: armhf-gnu
bors added a commit to rust-lang-ci/rust that referenced this pull request
…iaskrgr
Rollup of 7 pull requests
Successful merges:
- rust-lang#135295 (Check empty SIMD vector in inline asm)
- rust-lang#135873 (coverage: Prepare for upcoming changes to counter creation)
- rust-lang#135926 (Implement
needs-subprocessdirective, and cleanup a bunch of tests to useneeds-{subprocess,threads}) - rust-lang#135950 (Tidy Python improvements)
- rust-lang#135956 (Make
Vec::pop_ifa bit more presentable) - rust-lang#135966 ([AIX] Allow different sized load and store in
tests/assembly/powerpc64-struct-abi.rs) - rust-lang#135983 (Doc difference between extend and extend_from_slice)
r? @ghost
@rustbot modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request
…iaskrgr
Rollup of 6 pull requests
Successful merges:
- rust-lang#135873 (coverage: Prepare for upcoming changes to counter creation)
- rust-lang#135926 (Implement
needs-subprocessdirective, and cleanup a bunch of tests to useneeds-{subprocess,threads}) - rust-lang#135950 (Tidy Python improvements)
- rust-lang#135956 (Make
Vec::pop_ifa bit more presentable) - rust-lang#135966 ([AIX] Allow different sized load and store in
tests/assembly/powerpc64-struct-abi.rs) - rust-lang#135983 (Doc difference between extend and extend_from_slice)
r? @ghost
@rustbot modify labels: rollup
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request
Rollup merge of rust-lang#135926 - jieyouxu:needs-subprocess-thread, r=oli-obk
Implement needs-subprocess directive, and cleanup a bunch of tests to use needs-{subprocess,threads}
Summary
Closes rust-lang#128295.
- Implements
//@ needs-subprocessdirective in compiletest as requested in rust-lang#128295. However, compiletest is a host tool, so we can't just try to spawn process because that spawns the process on host, not the target, under cross-compilation scenarios.- The short-term solution is to add Yet Another list of allow-list targets.
- The long-term solution is to first check if a
$targetsupports std, then try to run a binary to do run-time capability detection on the target. But that is tricky because you have to build-and-run a binary for the target. - This PR picks the short-term solution, because the long-term solution is highly non-trivial, and it's already an improvement over individual
ignore-*s all over the place. - Opened an issue about the long-term solution in rust-lang#135928.
- Documents
//@ needs-subprocessin rustc-dev-guide. - Replace
ignore-{wasm,wasm32,emscripten,sgx}withneeds-{subprocess,threads}where suitable in tests. - Some drive-by test changes as I was trying to figure out if I could use
needs-{subprocess,threads}and found some bits needlessly distracting.
Count of tests that use ignore-{wasm,wasm32,emscripten,sgx} before and after this PR:
| State | ignore-sgx |
ignore-wasm |
ignore-emscripten |
|---|---|---|---|
| Before this PR | 96 | 88 | 207 |
| After this PR | 36 | 38 | 61 |
Commands used to find out locally
--- before
[17:40] Joe:rust (fresh) | rg --no-ignore -l "ignore-sgx" tests | wc -l
96
[17:40] Joe:rust (fresh) | rg --no-ignore -l "ignore-wasm" tests | wc -l
88
[17:40] Joe:rust (fresh) | rg --no-ignore -l "ignore-emscripten" tests | wc -l
207
--- after
[17:39] Joe:rust (needs-subprocess-thread) | rg --no-ignore -l "ignore-sgx" tests | wc -l
36
[17:39] Joe:rust (needs-subprocess-thread) | rg --no-ignore -l "ignore-wasm" tests | wc -l
38
[17:39] Joe:rust (needs-subprocess-thread) | rg --no-ignore -l "ignore-emscripten" tests | wc -l
61Review advice
- Best reviewed commit-by-commit.
- Non-trivial test changes (not mechanically simple replacements) are split into individual commits to help with review. Their individual commit messages give some basic description of the changes.
- I could split some test changes out into another PR, but I found that I needed to change some tests to
needs-threads, some toneeds-subprocess, and some needed to use both, so they might conflict and become very annoying.
r? @ghost (need to run try jobs)
try-job: x86_64-msvc-1 try-job: i686-msvc-1 try-job: i686-mingw try-job: x86_64-mingw-1 try-job: x86_64-apple-1 try-job: aarch64-apple try-job: aarch64-gnu try-job: test-various try-job: armhf-gnu
bors added a commit to rust-lang-ci/rust that referenced this pull request
Remove redundant //@ ignore-{wasm,wasm32,emscripten} in tests
Follow-up to rust-lang#135926.
When //@ needs-{unwind,subprocess} are the suitable capability guards.
Resolves rust-lang#135923.
r? @ghost
try-job: test-various
bors added a commit to rust-lang-ci/rust that referenced this pull request
Remove generic //@ ignore-{wasm,wasm32,emscripten} in tests
Follow-up to rust-lang#135926.
In favor of capability-based guards //@ needs-{unwind,subprocess}.
Resolves rust-lang#135923.
r? @ghost
try-job: test-various
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request
Remove generic //@ ignore-{wasm,wasm32,emscripten} in tests
Follow-up to rust-lang#135926.
In favor of capability-based guards //@ needs-{unwind,subprocess}.
Resolves rust-lang#135923.
r? @ghost
try-job: test-various
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request
Remove generic //@ ignore-{wasm,wasm32,emscripten} in tests
Follow-up to rust-lang#135926.
In favor of capability-based guards //@ needs-{unwind,subprocess}.
Resolves rust-lang#135923.
r? @ghost
try-job: test-various
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request
Rollup merge of rust-lang#136476 - jieyouxu:panic-panic-panic, r=lcnr
Remove generic //@ ignore-{wasm,wasm32,emscripten} in tests
Follow-up to rust-lang#135926.
In favor of capability-based guards //@ needs-{unwind,subprocess}.
Resolves rust-lang#135923.
r? @ghost
try-job: test-various
Labels
Area: The compiletest test runner
Area: rustc-dev-guide
Area: The testsuite used to check the correctness of rustc
Area: The tidy tool
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Relevant to the compiler team, which will review and decide on the PR/issue.