Skip to content

dirfd: initial quick and dirty implementation for unix #139514

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

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

Conversation

Qelxiros
Copy link

@Qelxiros Qelxiros commented Apr 8, 2025

Tracking issue: #120426

As per this comment, this issue needs someone to start work on an implementation, so I've implemented a couple functions for UNIX. There's a lot more work to be done here (most of the feature), so I'd love some guidance on what needs fixing in this PR and any notes on how to proceed. Thanks!

@rustbot
Copy link
Collaborator

rustbot commented Apr 8, 2025

r? @workingjubilee

rustbot has assigned @workingjubilee.
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-libs Relevant to the library team, which will review and decide on the PR/issue. labels Apr 8, 2025
@rust-log-analyzer
Copy link
Collaborator

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

error: an `#[unstable]` annotation here has no effect
    --> library/std/src/fs.rs:1375:1
     |
1375 | #[unstable(feature = "dirfd", issue = "120426")]
     | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     |
     = note: see issue #55436 <https://github.com/rust-lang/rust/issues/55436> for more information
     = note: `#[deny(ineffective_unstable_trait_impl)]` on by default

For more information about this error, try `rustc --explain E0412`.
[RUSTC-TIMING] std test:false 2.577
error: could not compile `std` (lib) due to 3 previous errors
Build completed unsuccessfully in 0:01:11

@jieyouxu
Copy link
Member

r? libs

@rustbot rustbot assigned tgross35 and unassigned workingjubilee Apr 28, 2025
@@ -1353,6 +1360,25 @@ impl Seek for Arc<File> {
}
}

#[unstable(feature = "dirfd", issue = "120426")]
Copy link
Contributor

Choose a reason for hiding this comment

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

This goes on each of the functions, not the impl block

Comment on lines +1365 to +1372
/// Opens a file relative to this directory.
pub fn open<P: AsRef<Path>>(&self, path: P) -> io::Result<File> {
self.inner.open(path).map(|f| File { inner: f })
}
/// Opens a file relative to this directory with the specified options.
pub fn open_with<P: AsRef<Path>>(&self, path: P, opts: &OpenOptions) -> io::Result<File> {
self.inner.open_with(path, &opts.0).map(|f| File { inner: f })
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please add examples here, as well as tests? Tests can go in std/src/fs/tests.rs.

@@ -116,6 +116,13 @@ pub struct File {
inner: fs_imp::File,
}

#[unstable(feature = "dirfd", issue = "120426")]
#[cfg(target_family = "unix")]
Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't/can't have platform-gated config in fs, tidy would probably complain about this if CI got that far. It's okay to stub out less popular platforms for now with FIXMEs, but I think the initial implementation should come with Windows support. There are some guidelines at #120426 (comment), feel free to ask questions on Zulip if you get stuck.

Comment on lines +301 to +306

// pub fn create_dir<P: AsRef<Path>>(&self, path: P) -> Result<()>
// pub fn rename<P: AsRef<Path>, Q: AsRef<Path>>(&self, from: P, to_dir: &Self, to: Q) -> Result<()>
// pub fn remove_file<P: AsRef<Path>>(&self, path: P) -> Result<()>
// pub fn remove_dir<P: AsRef<Path>>(&self, path: P) -> Result<()>
// pub fn symlink<P: AsRef<Path>, Q: AsRef<Path>>(&self, original: P, link: Q)
Copy link
Contributor

Choose a reason for hiding this comment

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

It's probably better to delete this, instead of possibly forgetting to remove them later.

@@ -116,6 +116,13 @@ pub struct File {
inner: fs_imp::File,
}

#[unstable(feature = "dirfd", issue = "120426")]
#[cfg(target_family = "unix")]
/// An object providing access to a directory on the filesystem.
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you elaborate the documentation here a bit? At least make it clear that this is a handle, and what happens on drop (similar to File).

@rustbot rustbot 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 May 1, 2025

// dirfd isn't supported everywhere
#[cfg(not(any(
miri,
Copy link
Member

Choose a reason for hiding this comment

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

we handle such missing platforms by returning errors instead of not having the methods exist at all

@bors
Copy link
Collaborator

bors commented May 3, 2025

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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
Development

Successfully merging this pull request may close these issues.

8 participants