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

Move all sys::ext modules to os #84200

Merged
merged 9 commits into from
May 5, 2021
Merged

Move all sys::ext modules to os #84200

merged 9 commits into from
May 5, 2021

Conversation

CDirkx
Copy link
Contributor

@CDirkx CDirkx commented Apr 14, 2021

This PR moves all sys::ext modules to os, centralizing the location of all os code and simplifying the dependencies between os and sys.

Because this also removes all uses cfg_if! on publicly exported items, where after #81969 there were still a few left, this should properly work around rust-lang/rust-analyzer#6038.

@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 14, 2021
@rust-highfive
Copy link
Collaborator

r? @m-ou-se

(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 14, 2021
Comment on lines +143 to +121
#[cfg(not(doc))]
#[stable(feature = "os", since = "1.0.0")]
pub use imp::*;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The stability attributes on these re-exports don't affect the stability of the exported items.

@rust-log-analyzer

This comment has been minimized.

@CDirkx
Copy link
Contributor Author

CDirkx commented Apr 14, 2021

Not really sure about the error, building for x86_64-unknown-linux-gnu works for me locally.

@m-ou-se
Copy link
Member

m-ou-se commented Apr 16, 2021

Not really sure about the error, building for x86_64-unknown-linux-gnu works for me locally.

It only works if the directory library/std/src/os/imp exists, since pub mod linux; is trying to load library/std/src/os/imp/../linux/mod.rs. Git doesn't track empty directories, so you have this directory locally in your working directory, but it's not part of the git tree and doesn't exist after a clean checkout like in CI.

@m-ou-se
Copy link
Member

m-ou-se commented Apr 16, 2021

(You can add #[path = "."] to the inline modules, and remove all the ../ from the paths.)

@CDirkx
Copy link
Contributor Author

CDirkx commented Apr 18, 2021

Oh I didn't know that could be done, thanks for the suggestion!

Copy link
Member

@m-ou-se m-ou-se left a comment

Choose a reason for hiding this comment

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

This took some time to review, mostly because the current code (before this change) is a bit hard to follow, which this PR nicely cleans up. Thanks for doing this!

I only have one question:

Comment on lines 2 to 1
#![allow(missing_docs)]
#![unstable(feature = "hermit_platform", issue = "none")]
Copy link
Member

Choose a reason for hiding this comment

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

Why did this change?

Copy link
Contributor Author

@CDirkx CDirkx Apr 21, 2021

Choose a reason for hiding this comment

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

Oh good catch, that is separate change I was working on that should be excluded from this PR.

(I noticed that hermit currently exports its extensions as unix, even though it is not target_os = "unix" and only contains OsStringExt (which might be confusing for users expecting e.g. sys::os::unix::fs::FileExt). I spoke with someone working on Hermit about whether this was intentional; and he said that currently there is no way for hermit to fully replicate everything in os::unix and that the plan is to instead move to os::hermit.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Excluded 👍🏻

@CDirkx
Copy link
Contributor Author

CDirkx commented Apr 21, 2021

This took some time to review, mostly because the the current code (before this change) is a bit hard to follow

Fully agreed that the code was hard to follow, it took quite some time untangling it all.
Thanks for all the reviews you have been doing!

Copy link
Member

@m-ou-se m-ou-se left a comment

Choose a reason for hiding this comment

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

This will fail to merge once your other PR (#84119) is merged into thet main branch, which should happen soon. So I'm waiting with the r+ until the rebase of this one.

@m-ou-se m-ou-se 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 Apr 21, 2021
@bors
Copy link
Contributor

bors commented Apr 22, 2021

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

@CDirkx
Copy link
Contributor Author

CDirkx commented Apr 22, 2021

Rebased and resolved the merge conflicts

@m-ou-se
Copy link
Member

m-ou-se commented Apr 22, 2021

@bors r+ rollup=iffy

@bors
Copy link
Contributor

bors commented Apr 22, 2021

📌 Commit 76e03ac8fdc1b9840e0dc906f9d7c10b2a99bb8e has been approved by m-ou-se

@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 Apr 22, 2021
@bors
Copy link
Contributor

bors commented Apr 22, 2021

⌛ Testing commit 76e03ac8fdc1b9840e0dc906f9d7c10b2a99bb8e with merge 694f1b7949b1bb1edb51e84f939aac981ad85326...

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Apr 22, 2021

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 22, 2021
@bors
Copy link
Contributor

bors commented May 3, 2021

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

@m-ou-se
Copy link
Member

m-ou-se commented May 5, 2021

Thanks again :)

@bors r+

@bors
Copy link
Contributor

bors commented May 5, 2021

📌 Commit 2173d8d has been approved by m-ou-se

@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 May 5, 2021
@bors
Copy link
Contributor

bors commented May 5, 2021

⌛ Testing commit 2173d8d with merge 342db70...

@bors
Copy link
Contributor

bors commented May 5, 2021

☀️ Test successful - checks-actions
Approved by: m-ou-se
Pushing 342db70 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 5, 2021
@bors bors merged commit 342db70 into rust-lang:master May 5, 2021
@rustbot rustbot added this to the 1.54.0 milestone May 5, 2021
@CDirkx CDirkx deleted the os branch May 5, 2021 18:51
flip1995 pushed a commit to flip1995/rust that referenced this pull request May 6, 2021
Move all `sys::ext` modules to `os`

This PR moves all `sys::ext` modules to `os`, centralizing the location of all `os` code and simplifying the dependencies between `os` and `sys`.

Because this also removes all uses `cfg_if!` on publicly exported items, where after rust-lang#81969 there were still a few left, this should properly work around rust-lang/rust-analyzer#6038.

`@rustbot` label: +T-libs-impl
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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-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