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

remove_dir_all: use fallback implementation on Miri #94749

Merged
merged 2 commits into from
Mar 20, 2022

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Mar 8, 2022

Fixes rust-lang/miri#1966

The new implementation requires openat, unlinkat, and fdopendir. These cannot easily be shimmed in Miri since libstd does not expose APIs corresponding to them. So for now it is probably easiest to just use the fallback code in Miri. Nobody should run Miri as root anyway...

@rust-highfive
Copy link
Collaborator

r? @kennytm

(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 Mar 8, 2022
@cuviper
Copy link
Member

cuviper commented Mar 9, 2022

Can you add a user-facing note about this in the std::fs::remove_dir_all docs?

@RalfJung
Copy link
Member Author

RalfJung commented Mar 9, 2022

Sure... not what I expected (I don't think we have other such notes for Miri), but why not.

@@ -1480,14 +1480,14 @@ pub fn chroot(dir: &Path) -> io::Result<()> {

pub use remove_dir_impl::remove_dir_all;

// Fallback for REDOX and ESP-IDF
#[cfg(any(target_os = "redox", target_os = "espidf"))]
// Fallback for REDOX and ESP-IDF (and Miri)
Copy link
Member Author

Choose a reason for hiding this comment

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

FWIW the docs also do not seem to mention ESP-IDF (whatever that is)

@RalfJung
Copy link
Member Author

@cuviper since you left a comment earlier -- can I assign this PR to you, or should I try to find another reviewer?

@cuviper
Copy link
Member

cuviper commented Mar 19, 2022

Sure, LGTM.

r? @cuviper
@bors r+ rollup

@bors
Copy link
Contributor

bors commented Mar 19, 2022

📌 Commit 28eb06b has been approved by cuviper

@rust-highfive rust-highfive assigned cuviper and unassigned kennytm Mar 19, 2022
@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 Mar 19, 2022
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 19, 2022
…iper

remove_dir_all: use fallback implementation on Miri

Fixes rust-lang/miri#1966

The new implementation requires `openat`, `unlinkat`, and `fdopendir`. These cannot easily be shimmed in Miri since libstd does not expose APIs corresponding to them. So for now it is probably easiest to just use the fallback code in Miri. Nobody should run Miri as root anyway...
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 20, 2022
…askrgr

Rollup of 5 pull requests

Successful merges:

 - rust-lang#94749 (remove_dir_all: use fallback implementation on Miri)
 - rust-lang#94948 (Fix diagnostics for `#![feature(deprecated_suggestion)]`)
 - rust-lang#94989 (Add Stream alias for AsyncIterator)
 - rust-lang#95108 (Give more details in `Display` for `hir::Target`)
 - rust-lang#95110 (Provide more useful documentation of conversion methods)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit acb7ed1 into rust-lang:master Mar 20, 2022
@rustbot rustbot added this to the 1.61.0 milestone Mar 20, 2022
bors added a commit to rust-lang/miri that referenced this pull request Mar 20, 2022
@RalfJung RalfJung deleted the remove-dir-all-miri branch March 20, 2022 18:29
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bring back read_dir and remove_dir_all support
6 participants