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.

Sign up for GitHub

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

Jump to bottom

std: move stdio to sys #136780

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

std: move stdio to sys #136780

wants to merge 1 commit into from
+77 −150

Conversation

joboet
Copy link
Member

@joboet joboet commented Feb 9, 2025

As per #117276, this moves the platform definitions of Stdout and friends into sys. This PR also unifies the UNIX and Hermit implementations and moves the __rust_print_err function needed by libunwind on SGX into the dedicated module for such helper functions.

@rustbot
Copy link
Collaborator

rustbot commented Feb 9, 2025

r? @Amanieu

rustbot has assigned @Amanieu.
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 O-hermit Operating System: Hermit O-SGX Target: SGX O-solid Operating System: SOLID O-unix Operating system: Unix-like O-wasi Operating system: Wasi, Webassembly System Interface O-wasm Target: WASM (WebAssembly), http://webassembly.org/ O-windows Operating system: Windows S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Feb 9, 2025
@joboet joboet mentioned this pull request Feb 9, 2025
Open
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@thaliaarchi thaliaarchi mentioned this pull request Feb 11, 2025
Draft
thaliaarchi
thaliaarchi reviewed Feb 11, 2025
View reviewed changes
library/std/src/sys/stdio/unix_like.rs
@@ -89,7 +96,7 @@ impl io::Write for Stderr {
}

pub fn is_ebadf(err: &io::Error) -> bool {
err.raw_os_error() == Some(libc::EBADF as i32)
err.raw_os_error() == Some(EBADF as i32)
}

pub const STDIN_BUF_SIZE: usize = crate::sys::io::DEFAULT_BUF_SIZE;
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you mean to change Hermit from pub const STDIN_BUF_SIZE: usize = 128; to DEFAULT_BUF_SIZE?

pub const DEFAULT_BUF_SIZE: usize = if cfg!(target_os = "espidf") { 512 } else { 8 * 1024 };

Copy link
Member Author

Choose a reason for hiding this comment

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

No, I didn't. Whoops! 😄 Though maybe that isn't too bad a thing anyway, @mkroening?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, that sounds sensible to me, thanks! :) 👍

thaliaarchi
thaliaarchi approved these changes Feb 11, 2025
View reviewed changes
Copy link
Contributor

@thaliaarchi thaliaarchi left a comment

Choose a reason for hiding this comment

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

I'm not an official reviewer, but having been working in this area recently, this looks good to me.

@bors
Copy link
Contributor

bors commented Feb 15, 2025

☔ The latest upstream changes (presumably #137046) made this pull request unmergeable. Please resolve the merge conflicts.

@bors
Copy link
Contributor

bors commented Feb 17, 2025

☔ The latest upstream changes (presumably #137163) made this pull request unmergeable. Please resolve the merge conflicts.

@Amanieu
Copy link
Member

Amanieu commented Feb 18, 2025

@bors r+

@bors
Copy link
Contributor

bors commented Feb 18, 2025

📌 Commit a1a2a46 has been approved by Amanieu

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 Feb 18, 2025
@matthiaskrgr matthiaskrgr mentioned this pull request Feb 19, 2025
Closed
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 19, 2025
@bors
Auto merge of rust-lang#137283 - matthiaskrgr:rollup-nm296fd, r=matth…
29210cb 
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#120580 (Add `MAX_LEN_UTF8` and `MAX_LEN_UTF16` Constants)
 - rust-lang#132268 (Impl TryFrom<Vec<u8>> for String)
 - rust-lang#136093 (Match Ergonomics 2024: update old-edition behavior of feature gates)
 - rust-lang#136344 (Suggest replacing `.` with `::` in more error diagnostics.)
 - rust-lang#136690 (Use more explicit and reliable ptr select in sort impls)
 - rust-lang#136815 (CI: Stop /msys64/bin from being prepended to PATH in msys2 shell)
 - rust-lang#136923 (Lint `#[must_use]` attributes applied to methods in trait impls)
 - rust-lang#137155 (Organize `OsString`/`OsStr` shims)
 - rust-lang#137225 (vectorcall ABI: require SSE2)

Failed merges:

 - rust-lang#136780 (std: move stdio to `sys`)

r? `@ghost`
`@rustbot` modify labels: rollup
@thaliaarchi
Copy link
Contributor

The merge conflict is with #120580.

@bors
Copy link
Contributor

bors commented Feb 20, 2025

☔ The latest upstream changes (presumably #137290) made this pull request unmergeable. Please resolve the merge conflicts.

@bors bors removed the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Feb 20, 2025
@bors bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Feb 20, 2025
@thaliaarchi
Copy link
Contributor

Hey, gentle nudge :). I'd like this to be merged so I can rebase my PRs onto it. Would you mind resolving the conflicts so it can get back into the queue?

@joboet
std: move stdio to sys
ee7fded 
As per rust-lang#117276, this moves the platform definitions of `Stdout` and friends into `sys`. This PR also unifies the UNIX and Hermit implementations and moves the `__rust_print_err` function needed by libunwind on SGX into the dedicated module for such helper functions.
@joboet
Copy link
Member Author

joboet commented Feb 21, 2025

@bors r=@Amanieu

@bors
Copy link
Contributor

bors commented Feb 21, 2025

📌 Commit ee7fded has been approved by Amanieu

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 Feb 21, 2025
@bors
Copy link
Contributor

bors commented Feb 21, 2025

☔ The latest upstream changes (presumably #137397) made this pull request unmergeable. Please resolve the merge conflicts.

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 21, 2025
@Amanieu
Copy link
Member

Amanieu commented Feb 22, 2025

Since this is frequently generating conflicts

@bors p=1

@thaliaarchi
Copy link
Contributor

Sorry for the most recent conflict. I had opened a batch of PRs which I thought would land slower than this. I'm now holding off on any further conflicts from my work.

@thaliaarchi thaliaarchi mentioned this pull request Feb 25, 2025
Open
@thaliaarchi
Copy link
Contributor

After opening #137650, I have some more feedback:

I think sys::stdio::unix_like from this PR should still be named unix, since Hermit is more alike for stdio than different. The long term plan for Hermit is to move from definitions in hermit-abi to libc, and the relevant symbols for stdio are already in libc. Also, naming it unix matches sys::alloc::unix, which is used for Unix, WASI, and TEEOS.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mkroening mkroening mkroening left review comments

@thaliaarchi thaliaarchi thaliaarchi approved these changes

Assignees

@Amanieu Amanieu

Labels
O-hermit Operating System: Hermit O-SGX Target: SGX O-solid Operating System: SOLID O-unix Operating system: Unix-like O-wasi Operating system: Wasi, Webassembly System Interface O-wasm Target: WASM (WebAssembly), http://webassembly.org/ O-windows Operating system: Windows S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@joboet @rustbot @rust-log-analyzer @bors @Amanieu @thaliaarchi @mkroening