Implement `needs-subprocess` directive, and cleanup a bunch of tests to use `needs-{subprocess,threads}` #135926

jieyouxu · 2025-01-23T09:52:02Z

Summary

Implements //@ needs-subprocess directive 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 $target supports 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-subprocess in rustc-dev-guide.
Replace ignore-{wasm,wasm32,emscripten,sgx} with needs-{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 to needs-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

rustbot · 2025-01-23T09:52:13Z

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

…<try> Implement `needs-subprocess` directive, and cleanup a bunch of tests to use `needs-{subprocess,threads} ### Summary - Implements `//@ needs-subprocess` directive in compiletest. - Documents `//@ needs-subprocess` in rustc-dev-guide. - Replace `ignore-{wasm,wasm32,emscripten,sgx}` with `needs-{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 | <details> <summary>Commands used to find out locally</summary> ``` --- 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 ``` </details> ### Review 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 to `needs-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

jieyouxu · 2025-01-23T09:58:10Z

tests/ui/issues/issue-39175.rs

@@ -4,8 +4,6 @@
 // these platforms also.

 //@ ignore-windows
-//@ ignore-wasm32 no processes
-//@ ignore-sgx no processes

 use std::process::Command;


This test is actually exercising the use import suggestion for the Unix CommandExt as an example

jieyouxu · 2025-01-23T09:58:59Z

tests/ui/issues/issue-2190-1.rs

-#![allow(non_upper_case_globals)]
-
-//@ ignore-emscripten no threads
+//@ check-pass


Another closure type inference test, does not need to run

src/tools/compiletest/src/header/needs.rs

jieyouxu · 2025-01-23T11:18:55Z

src/tools/compiletest/src/common.rs

+    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" | "wasm64")
+            || self.target_cfg().os == "emscripten";
+        !unsupported_target
+    }


I don't like it, but it is what it is, at least it's centralized here.

…<try> Implement `needs-subprocess` directive, and cleanup a bunch of tests to use `needs-{subprocess,threads}` ### Summary Closes rust-lang#128295. - Implements `//@ needs-subprocess` directive 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 `$target` supports 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-subprocess` in rustc-dev-guide. - Replace `ignore-{wasm,wasm32,emscripten,sgx}` with `needs-{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 | <details> <summary>Commands used to find out locally</summary> ``` --- 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 ``` </details> ### Review 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 to `needs-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

rustbot · 2025-01-23T16:54:37Z

The rustc-dev-guide subtree was changed. If this PR only touches the dev guide consider submitting a PR directly to rust-lang/rustc-dev-guide otherwise thank you for updating the dev guide with your changes.

cc @BoxyUwU, @jieyouxu, @Kobzol

jieyouxu · 2025-01-23T16:55:27Z

Try-job came back green (only difference was a comment).

Maybe r? @oli-obk (originally randomly rolled, or reroll)

@rustbot ready

jieyouxu · 2025-01-23T16:57:53Z

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.

tests/ui/array-slice-vec/bounds-check-no-overflow.rs

jieyouxu · 2025-01-24T06:23:44Z

@rustbot review

oli-obk · 2025-01-24T07:39:26Z

tests/ui/panics/runtime-switch.rs

@@ -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


isn't this also "no backtrace support"? It's a panic=abort target anyway, and this is not an unwind test

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

oli-obk · 2025-01-24T07:44:49Z

@bors r+

bors · 2025-01-24T07:44:51Z

📌 Commit 071ad37 has been approved by oli-obk

It is now in the queue for this repository.

…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-subprocess` directive 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 `$target` supports 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-subprocess` in rustc-dev-guide. - Replace `ignore-{wasm,wasm32,emscripten,sgx}` with `needs-{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 | <details> <summary>Commands used to find out locally</summary> ``` --- 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 ``` </details> ### 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 to `needs-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

…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-subprocess` directive, and cleanup a bunch of tests to use `needs-{subprocess,threads}`) - rust-lang#135950 (Tidy Python improvements) - rust-lang#135956 (Make `Vec::pop_if` a 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

…iaskrgr Rollup of 6 pull requests Successful merges: - rust-lang#135873 (coverage: Prepare for upcoming changes to counter creation) - rust-lang#135926 (Implement `needs-subprocess` directive, and cleanup a bunch of tests to use `needs-{subprocess,threads}`) - rust-lang#135950 (Tidy Python improvements) - rust-lang#135956 (Make `Vec::pop_if` a 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

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-subprocess` directive 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 `$target` supports 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-subprocess` in rustc-dev-guide. - Replace `ignore-{wasm,wasm32,emscripten,sgx}` with `needs-{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 | <details> <summary>Commands used to find out locally</summary> ``` --- 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 ``` </details> ### 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 to `needs-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.

Sign in to view

rustbot assigned oli-obk Jan 23, 2025

jieyouxu unassigned oli-obk Jan 23, 2025

This comment was marked as outdated.

Sign in to view

jieyouxu changed the title ~~Implement needs-subprocess directive, and cleanup a bunch of tests to use `needs-{subprocess,threads}~~ Implement needs-subprocess directive, and cleanup a bunch of tests to use needs-{subprocess,threads} Jan 23, 2025

jieyouxu commented Jan 23, 2025

View reviewed changes

jieyouxu added S-waiting-on-author 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 Jan 23, 2025

jieyouxu mentioned this pull request Jan 23, 2025

tests: Port translation to rmake.rs #135818

Open

fmease reviewed Jan 23, 2025

View reviewed changes

src/tools/compiletest/src/header/needs.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

This comment was marked as outdated.

Sign in to view

jieyouxu force-pushed the needs-subprocess-thread branch from 3a5effe to b3f4819 Compare January 23, 2025 11:16

This comment was marked as outdated.

Sign in to view

jieyouxu commented Jan 23, 2025

View reviewed changes

This comment was marked as resolved.

Sign in to view

This comment has been minimized.

Sign in to view

This comment was marked as resolved.

Sign in to view

jieyouxu force-pushed the needs-subprocess-thread branch from b3f4819 to b471895 Compare January 23, 2025 12:42

tests: use needs-threads instead of ignore-emscripten

071ad37

jieyouxu force-pushed the needs-subprocess-thread branch from b471895 to 071ad37 Compare January 23, 2025 12:51

jieyouxu marked this pull request as ready for review January 23, 2025 16:54

rustbot assigned oli-obk Jan 23, 2025

rustbot added S-waiting-on-review 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 Jan 23, 2025

oli-obk reviewed Jan 23, 2025

View reviewed changes

tests/ui/array-slice-vec/bounds-check-no-overflow.rs Show resolved Hide resolved

jieyouxu added S-waiting-on-author 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 Jan 23, 2025

rustbot added S-waiting-on-review 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 Jan 24, 2025

oli-obk reviewed Jan 24, 2025

View reviewed changes

oli-obk approved these changes Jan 24, 2025

View reviewed changes

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 Jan 24, 2025

matthiaskrgr mentioned this pull request Jan 24, 2025

Rollup of 7 pull requests #135986

Closed

matthiaskrgr mentioned this pull request Jan 24, 2025

Rollup of 6 pull requests #136000

Merged

bors merged commit e96bb6a into rust-lang:master Jan 24, 2025
6 checks passed

rustbot added this to the 1.86.0 milestone Jan 24, 2025

jieyouxu deleted the needs-subprocess-thread branch January 25, 2025 03:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement `needs-subprocess` directive, and cleanup a bunch of tests to use `needs-{subprocess,threads}` #135926

Implement `needs-subprocess` directive, and cleanup a bunch of tests to use `needs-{subprocess,threads}` #135926

jieyouxu commented Jan 23, 2025 •

edited

Loading

This comment was marked as resolved.

rustbot commented Jan 23, 2025

This comment was marked as outdated.

This comment was marked as outdated.

jieyouxu Jan 23, 2025 •

edited

Loading

jieyouxu Jan 23, 2025

This comment has been minimized.

This comment was marked as outdated.

This comment was marked as outdated.

jieyouxu Jan 23, 2025

This comment was marked as resolved.

This comment has been minimized.

This comment was marked as resolved.

rustbot commented Jan 23, 2025

jieyouxu commented Jan 23, 2025

jieyouxu commented Jan 23, 2025

jieyouxu commented Jan 24, 2025

oli-obk Jan 24, 2025

jieyouxu Jan 24, 2025 •

edited

Loading

oli-obk commented Jan 24, 2025

bors commented Jan 24, 2025

Implement needs-subprocess directive, and cleanup a bunch of tests to use needs-{subprocess,threads} #135926

Implement needs-subprocess directive, and cleanup a bunch of tests to use needs-{subprocess,threads} #135926

Conversation

jieyouxu commented Jan 23, 2025 • edited Loading

Summary

Review advice

This comment was marked as resolved.

rustbot commented Jan 23, 2025

This comment was marked as outdated.

This comment was marked as outdated.

jieyouxu Jan 23, 2025 • edited Loading

Choose a reason for hiding this comment

jieyouxu Jan 23, 2025

Choose a reason for hiding this comment

This comment has been minimized.

This comment was marked as outdated.

This comment was marked as outdated.

jieyouxu Jan 23, 2025

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment has been minimized.

This comment was marked as resolved.

rustbot commented Jan 23, 2025

jieyouxu commented Jan 23, 2025

jieyouxu commented Jan 23, 2025

jieyouxu commented Jan 24, 2025

oli-obk Jan 24, 2025

Choose a reason for hiding this comment

jieyouxu Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

oli-obk commented Jan 24, 2025

bors commented Jan 24, 2025

Implement `needs-subprocess` directive, and cleanup a bunch of tests to use `needs-{subprocess,threads}` #135926

Implement `needs-subprocess` directive, and cleanup a bunch of tests to use `needs-{subprocess,threads}` #135926

jieyouxu commented Jan 23, 2025 •

edited

Loading

jieyouxu Jan 23, 2025 •

edited

Loading

jieyouxu Jan 24, 2025 •

edited

Loading