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

Reuse modules on hermit #84521

Merged
merged 1 commit into from
Apr 27, 2021
Merged

Reuse modules on hermit #84521

merged 1 commit into from
Apr 27, 2021

Conversation

CDirkx
Copy link
Contributor

@CDirkx CDirkx commented Apr 24, 2021

Reuse the following modules on hermit:

  • unix::path (contents identical)
  • unsupported::io (contents identical)
  • unsupported::thread_local_key (contents functionally identical, only changes are the panic error messages)

@rustbot label: +T-libs-impl

@rustbot rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Apr 24, 2021
@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 24, 2021
@CDirkx
Copy link
Contributor Author

CDirkx commented Apr 24, 2021

hermit::cmath could also be reused from another platform, which is addressed in #84522.

pub mod io;
pub mod memchr;
pub mod mutex;
pub mod net;
pub mod os;
#[path = "../unix/path.rs"]
Copy link
Member

Choose a reason for hiding this comment

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

Hm, I'm a bit worried that in theory people changing the unix path.rs will not expect it to also get picked up on other platforms. It does seem pretty unlikely to get changed, but maybe we can at least add a comment to that file that it's also used by other platforms?

@Mark-Simulacrum
Copy link
Member

Modulo path.rs being reused this seems fine -- curious to hear @m-ou-se if you have thoughts as you've done some recent work on the sys/ files with the mutex stuff if that's a real concern

@m-ou-se
Copy link
Member

m-ou-se commented Apr 27, 2021

That path.rs is already used in the same way in several other places:

$ rg unix/path
wasm/mod.rs
32:#[path = "../unix/path.rs"]

unsupported/mod.rs
13:#[path = "../unix/path.rs"]

wasi/mod.rs
37:#[path = "../unix/path.rs"]

We could maybe move it out of unix/ and have some other place for this. But I'm not worried about adding another platform to this list right now.

@Mark-Simulacrum
Copy link
Member

In that case, @bors r+

Seems good to potentially move it out but it's probably a common pattern and the sys stuff doesn't change much outside additions or refactors.

@bors
Copy link
Contributor

bors commented Apr 27, 2021

📌 Commit 36e9382 has been approved by Mark-Simulacrum

@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 27, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 27, 2021
Rollup of 5 pull requests

Successful merges:

 - rust-lang#84132 (Ignore nonstandard lldb version strings in compiletest)
 - rust-lang#84521 (Reuse modules on `hermit`)
 - rust-lang#84563 (Update backtrace to 0.3.57)
 - rust-lang#84610 (Update Clippy)
 - rust-lang#84613 (move representability checks to rustc_ty_utils)

Failed merges:

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

CDirkx commented Apr 27, 2021

Looking around sys and sys_common code I think shared code falls into these categories:

  • Stub implementations for unsupported operations: sys\unsupported
  • Fallback code and default values, e.g. register_dtor_fallback, io::DEFAULT_BUF_SIZE, realloc_fallback: some still in sys_common, planned to be moved to sys\common introduced recently in Move std::sys_common::alloc to new module std::sys::common #82492
  • Code shared across unix based systems (such as all the unix os variants and targets like redox and vxworks before they were fully integrated with unix): sys\unix
  • Code shared across non-windows platforms that isn't necessarily unix specific, e.g. path, os_str_bytes, cmath: some of those are in sys\unix, os_str_bytes lives in sys_common with a #[cfg(not(windows))]
  • POSIX code, basically only sys_common::net used on both unix and windows but none of the other platforms.

Not sure what to do about those last two, I would like to move them out of sys_common (see #84187). Some options are:

  1. Leave it as is, so code in sys/unix and sys_common.
  2. Introduce sys/posix and sys/nonwindows?, not ideal in my opinion as now a platforms code can be spread out over a lot of locations.
  3. Introduce sys/posix and keep sys/unix as the canonical non-windows location
  4. Keep sys/unix as the canonical non-windows location (so also move os_str_bytes to unix), move sys_common::net to unix and just include it on windows (or vice versa)
  5. Expand the definition of sys/common. I had initially intended common to only contain code that could be shared on all platforms, so no #[cfg] necesarry, but that doesn't have to be the case.
  6. Introduce something like sys/shared, similar to common, but allowing #[cfg], might be very confusing for new code, having to figure out the differences between sys_common, common and shared and potentially unsupported.

Of these I personally think option 4 would be the best way forward, maybe with a comment in the unix files noting that they are indeed shared on other platforms. @m-ou-se what are your thoughts?

@bors bors merged commit e7be5dd into rust-lang:master Apr 27, 2021
@rustbot rustbot added this to the 1.53.0 milestone Apr 27, 2021
@CDirkx CDirkx deleted the hermit-dedup branch April 27, 2021 20:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants