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

Disable Ctrl-C handling on WASM #123788

Merged
merged 2 commits into from
Apr 11, 2024
Merged

Conversation

bjorn3
Copy link
Member

@bjorn3 bjorn3 commented Apr 11, 2024

WASM fundamentally doesn't support signals. If WASI ever gets support for notifying the guest process of a Ctrl-C that happened, this would have to be done through the guest process polling for the signal, which will require thread support in WASI too to be compatible with the api provided by the ctrlc crate.

@rustbot
Copy link
Collaborator

rustbot commented Apr 11, 2024

r? @lcnr

rustbot has assigned @lcnr.
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

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 11, 2024
@rust-log-analyzer

This comment has been minimized.

bjorn3 added 2 commits April 11, 2024 12:35
WASM fundamentally doesn't support signals. If WASI ever gets support
for notifying the guest process of a Ctrl-C that happened, this would
have to be done through the guest process polling for the signal, which
will require thread support in WASI too to be compatible with the api
provided by the ctrlc crate.
It incorrectly filters out non-wasm dependencies too.
@bjorn3 bjorn3 force-pushed the disable_ctrlc_on_wasi branch from 26291ce to 8b0b88a Compare April 11, 2024 12:35
@rustbot rustbot added 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 Apr 11, 2024
@rustbot
Copy link
Collaborator

rustbot commented Apr 11, 2024

The list of allowed third-party dependencies may have been modified! You must ensure that any new dependencies have compatible licenses before merging.

cc @davidtwco, @wesleywiser

@lcnr
Copy link
Contributor

lcnr commented Apr 11, 2024

this seems reasonable to me, I don't know much about wasm though 🤷 cc @alexcrichton maybe? What's the impact of simply using ctrlc in wasm? do you get compilation failure/linker errors or is it simply a noop?

r=me after CI

@bjorn3
Copy link
Member Author

bjorn3 commented Apr 11, 2024

CI failure is because of a tidy bug. It checks if a target dependent dependency applies to wasm and if so ignores it. This check however also triggers if it doesn't just apply to wasm but also to other targets. So for example cfg(not(windows)) would also be ignored as wasm is matched by cfg(not(windows)). Or in this case cfg(not(target_family = "wasm"))] is considered to be matched by wasm as target_family = "wasm" is not explicitly set in the check. I've pushed a commit to remove the filtering entirely. It only filtered out two deps anyway. At least one of which is incorrect and I think the other one is also incorrect.

@bjorn3
Copy link
Member Author

bjorn3 commented Apr 11, 2024

What's the impact of simply using ctrlc in wasm? do you get compilation failure/linker errors or is it simply a noop?

I got a compilation failure when compiling the ctrlc crate.

The errors in question
error[E0432]: unresolved import `platform::Signal`
  --> /home/gh-bjorn3/.cargo/registry/src/index.crates.io-6f17d22bba15001f/ctrlc-3.4.4/src/lib.rs:53:9
   |
53 | pub use platform::Signal;
   |         ^^^^^^^^^^^^^^^^ no `Signal` in `platform`

error[E0412]: cannot find type `Error` in module `platform`
  --> /home/gh-bjorn3/.cargo/registry/src/index.crates.io-6f17d22bba15001f/ctrlc-3.4.4/src/error.rs:25:21
   |
25 | impl From<platform::Error> for Error {
   |                     ^^^^^ not found in `platform`
   |
help: consider importing one of these items
   |
1  + use core::error::Error;
   |
1  + use core::fmt::Error;
   |
1  + use crate::Error;
   |
1  + use crate::error::fmt::Error;
   |
     and 3 other candidates
help: if you import `Error`, refer to it directly
   |
25 - impl From<platform::Error> for Error {
25 + impl From<Error> for Error {
   |

error[E0412]: cannot find type `Error` in module `platform`
  --> /home/gh-bjorn3/.cargo/registry/src/index.crates.io-6f17d22bba15001f/ctrlc-3.4.4/src/error.rs:26:26
   |
26 |     fn from(e: platform::Error) -> Error {
   |                          ^^^^^ not found in `platform`
   |
help: consider importing one of these items
   |
1  + use core::error::Error;
   |
1  + use core::fmt::Error;
   |
1  + use crate::Error;
   |
1  + use crate::error::fmt::Error;
   |
     and 3 other candidates
help: if you import `Error`, refer to it directly
   |
26 -     fn from(e: platform::Error) -> Error {
26 +     fn from(e: Error) -> Error {
   |

error[E0433]: failed to resolve: could not find `Error` in `platform`
  --> /home/gh-bjorn3/.cargo/registry/src/index.crates.io-6f17d22bba15001f/ctrlc-3.4.4/src/error.rs:28:27
   |
28 |         if e == platform::Error::EEXIST {
   |                           ^^^^^ could not find `Error` in `platform`
   |
help: consider importing one of these items
   |
1  + use core::error::Error;
   |
1  + use core::fmt::Error;
   |
1  + use crate::Error;
   |
1  + use crate::error::fmt::Error;
   |
     and 3 other candidates
help: if you import `Error`, refer to it directly
   |
28 -         if e == platform::Error::EEXIST {
28 +         if e == Error::EEXIST {
   |

error[E0412]: cannot find type `Signal` in module `platform`
  --> /home/gh-bjorn3/.cargo/registry/src/index.crates.io-6f17d22bba15001f/ctrlc-3.4.4/src/signal.rs:22:21
   |
22 |     Other(platform::Signal),
   |                     ^^^^^^ not found in `platform`

error[E0425]: cannot find function `init_os_handler` in module `platform`
   --> /home/gh-bjorn3/.cargo/registry/src/index.crates.io-6f17d22bba15001f/ctrlc-3.4.4/src/lib.rs:134:25
    |
134 |         match platform::init_os_handler(overwrite) {
    |                         ^^^^^^^^^^^^^^^ not found in `platform`

error[E0425]: cannot find function `block_ctrl_c` in module `platform`
   --> /home/gh-bjorn3/.cargo/registry/src/index.crates.io-6f17d22bba15001f/ctrlc-3.4.4/src/lib.rs:146:27
    |
146 |                 platform::block_ctrl_c().expect("Critical system error while waiting for Ctrl-C");
    |                           ^^^^^^^^^^^^ not found in `platform`

Some errors have detailed explanations: E0412, E0425, E0432, E0433.
For more information about an error, try `rustc --explain E0412`.
error: could not compile `ctrlc` (lib) due to 7 previous errors

@@ -668,27 +670,7 @@ fn check_permitted_dependencies(
let mut deps = HashSet::new();
for to_check in restricted_dependency_crates {
let to_check = pkg_from_name(metadata, to_check);
use cargo_platform::Cfg;
use std::str::FromStr;
// We don't expect the compiler to ever run on wasm32, so strip
Copy link
Member

Choose a reason for hiding this comment

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

😈

Copy link
Member

@Noratrieb Noratrieb left a comment

Choose a reason for hiding this comment

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

r=me

@bjorn3
Copy link
Member Author

bjorn3 commented Apr 11, 2024

@bors r=Nilstrieb,lcnr

@bors
Copy link
Contributor

bors commented Apr 11, 2024

📌 Commit 8b0b88a has been approved by Nilstrieb,lcnr

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 11, 2024
@bors
Copy link
Contributor

bors commented Apr 11, 2024

⌛ Testing commit 8b0b88a with merge 72fe8a0...

@klensy
Copy link
Contributor

klensy commented Apr 11, 2024

cargo-platform became dead dep?

@alexcrichton
Copy link
Member

Looks good to me as well 👍

@bjorn3
Copy link
Member Author

bjorn3 commented Apr 11, 2024

Yes, the cargo-platform dependency can be removed. I've opened #123796 for this as this PR is already being tested by bors.

@bors
Copy link
Contributor

bors commented Apr 11, 2024

☀️ Test successful - checks-actions
Approved by: Nilstrieb,lcnr
Pushing 72fe8a0 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 11, 2024
@bors bors merged commit 72fe8a0 into rust-lang:master Apr 11, 2024
12 checks passed
@rustbot rustbot added this to the 1.79.0 milestone Apr 11, 2024
@bjorn3 bjorn3 deleted the disable_ctrlc_on_wasi branch April 11, 2024 16:44
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (72fe8a0): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

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

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)
3.5% [2.5%, 4.7%] 3
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 3.5% [2.5%, 4.7%] 3

Cycles

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)
6.3% [2.4%, 8.9%] 6
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Binary size

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

Bootstrap: 676.108s -> 675.554s (-0.08%)
Artifact size: 316.01 MiB -> 316.02 MiB (0.00%)

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Apr 12, 2024
…ubby789

Remove unused cargo-platform dependency from tidy

Noticed in rust-lang#123788 (comment)
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Apr 12, 2024
Rollup merge of rust-lang#123796 - bjorn3:remove_cargo_platform, r=clubby789

Remove unused cargo-platform dependency from tidy

Noticed in rust-lang#123788 (comment)
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-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants