fs: add support for DirBuilder and DirBuilderExt #2524

shkurski · 2020-05-12T07:46:06Z

Motivation

tokio doesn't have an asynchronous version of std::fs::DirBuilder yet.
This PR adds support for std::fs::DirBuilder and implementation for the std::os::unix::fs::DirBuilderExt trait.

Solution

The initial idea was to implement a thin wrapper around an internally
held std::fs::DirBuilder instance. This, however, didn't work due to
std::fs::DirBuilder not having Copy/Clone traits implemented, which
was necessary for constructing an instance to move-capture it into a
closure.

Instead, we have mirrored the std::fs::DirBuilder configuration by storing
the recursive and (unix-only) mode parameters locally, which were then
used to construct the std::fs::DirBuilder instance on-the-fly.

The (unix-only) DirBuilderExt trait has been implemented in-place
within the dir_builder crate. Despite it might have looked better to
have it separated somewhere in ../fs/os/unix, I haven't found a clean
and idiomatic way to do so (due to Rust's "orphan rules"). I'd be glad
to hear the propositions in case someone sees a way for this to be
further simplified.

Edit: force-pushed immediately after the initial commit was made.
Hope it was quick enough to not break any local repo's.

Fixes: #2369

The initial idea was to implement a thin wrapper around an internally held `std::fs::DirBuilder` instance. This, however, didn't work due to `std::fs::DirBuilder` not having a Copy/Clone traits implemented, which are necessary for constructing an instance to move-capture it into a closure. Instead, we mirror `std::fs::DirBuilder` configuration by storing the `recursive` and (unix-only) `mode` parameters locally, which are then used to construct an `std::fs::DirBuilder` instance on-the-fly. The (unix-only) `DirBuilderExt` trait has been implemented in-place within the `dir_builder` create. Despite it might have looked better to have it separated somewhere in `../fs/os/unix`, I haven't found a clean and idiomatic way to do so (due to Rust's "orphan rules"). I'd be glad to hear the propositions in case someone sees a way for this to be further simplified. Fixes: tokio-rs#2369

Darksonn

Thank you for the PR!

For the same reason as described in #2515, we should define our own extension trait instead of using the one from std.

Besides that I have a few documentation comments, and some comments on the use of #[cfg(unix)]. These are nothing major, and the PR looks pretty good in general 👍

Darksonn · 2020-05-12T14:45:26Z

tokio/src/fs/dir_builder.rs

+/// A builder for creating directories in various manners.
+///
+/// Additional Unix-specific options are available via importing the
+/// [os::unix::fs::DirBuilderExt] trait.


Can we change the link text to [`DirBuilderExt`]? Note that the link is broken.

Darksonn · 2020-05-12T14:48:19Z

tokio/src/fs/dir_builder.rs

+/// [os::unix::fs::DirBuilderExt]: ../os/unix/fs/trait.DirBuilderExt.html
+/// [std::fs::DirBuilder]: https://doc.rust-lang.org/std/fs/struct.DirBuilder.html


This should use intra-rustdoc links, so that broken links are automatically detected.

Suggested change

/// [os::unix::fs::DirBuilderExt]: ../os/unix/fs/trait.DirBuilderExt.html

/// [std::fs::DirBuilder]: https://doc.rust-lang.org/std/fs/struct.DirBuilder.html

/// [os::unix::fs::DirBuilderExt]: crate::os::unix::fs::DirBuilderExt

/// [std::fs::DirBuilder]: std::fs::DirBuilder

Darksonn · 2020-05-12T15:00:25Z

tokio/src/fs/dir_builder.rs

+        #[cfg(not(unix))]
+        let builder = DirBuilder { recursive: false };
+
+        #[cfg(unix)]
+        let builder = DirBuilder {


We can just use the Default impl here.

pub fn new() -> Self { Default::default() }

Thank you, switched to the Default impl.

tokio/src/fs/dir_builder.rs

Darksonn · 2020-05-12T15:02:35Z

tokio/src/macros/cfg.rs

+macro_rules! cfg_unix {
+    ($($item:item)*) => {
+        $(
+            #[cfg(unix)]
+            $item
+        )*
+    }
+}


We use #[cfg(unix)] everywhere else in the codebase. Maybe adding this cfg_* makes sense, but I would rather want it in its own PR.

Darksonn · 2020-05-12T15:05:57Z

tokio/src/fs/dir_builder.rs

+/// Additional Unix-specific options are available via importing the
+/// [os::unix::fs::DirBuilderExt] trait.
+///
+/// This is a specialized version of [`std::fs::DirBuilder`] for usage from


Suggested change

/// This is a specialized version of [`std::fs::DirBuilder`] for usage from

/// This is a specialized version of [`std::fs::DirBuilder`] for usage on

Thanks! Fixed

This commit removes the previously implemented std::os::unix::fs::DirBuilderExt trait from tokio::fs::DirBuilder. Additionally, the `cfg_unix` macro has been dropped and the code has been simplified based on the review.

shkurski · 2020-05-12T22:13:07Z

For the same reason as described in #2515, we should define our own extension trait instead of using the one from std.

Thanks for the review 👍
I've implemented our own extension trait and addressed your comments.

Darksonn

This looks good to me. 👍

Darksonn

I was going through the list of PRs and noticed this link.

Darksonn · 2020-05-16T14:34:19Z

tokio/src/fs/os/unix/dir_builder_ext.rs

+/// Unix-specific extensions to [`DirBuilderExt`].
+///
+/// [crate::fs::os::unix::DirBuilderExt]: DirBuilderExt


This link seems off. Shouldn't it link to DirBuilder?

The initial idea was to implement a thin wrapper around an internally held `std::fs::DirBuilder` instance. This, however, didn't work due to `std::fs::DirBuilder` not having a Copy/Clone traits implemented, which are necessary for constructing an instance to move-capture it into a closure. Instead, we mirror `std::fs::DirBuilder` configuration by storing the `recursive` and (unix-only) `mode` parameters locally, which are then used to construct an `std::fs::DirBuilder` instance on-the-fly. This commit also mirrors the (unix-only) DirBuilderExt trait from std. Fixes: tokio-rs#2369

- docs: misc improvements (#2572, #2658, #2663, #2656, #2647, #2630, #2487, #2621, #2624, #2600, #2623, #2622, #2577, #2569, #2589, #2575, #2540, #2564, #2567, #2520, #2521, #2572, #2493) - rt: allow calls to `block_on` inside calls to `block_in_place` that are themselves inside `block_on` (#2645) - net: fix non-portable behavior when dropping `TcpStream` `OwnedWriteHalf` (#2597) - io: improve stack usage by allocating large buffers on directly on the heap (#2634) - io: fix unsound pin projection in `AsyncReadExt::read_buf` and `AsyncWriteExt::write_buf` (#2612) - io: fix unnecessary zeroing for `AsyncRead` implementors (#2525) - io: Fix `BufReader` not correctly forwarding `poll_write_buf` (#2654) - coop: returning `Poll::Pending` no longer decrements the task budget (#2549) - io: little-endian variants of `AsyncReadExt` and `AsyncWriteExt` methods (#1915) - io: fix panic in `AsyncReadExt::read_line` (#2541) - task: add [`tracing`] instrumentation to spawned tasks (#2655) - sync: allow unsized types in `Mutex` and `RwLock` (via `default` constructors) (#2615) - net: add `ToSocketAddrs` implementation for `&[SocketAddr]` (#2604) - fs: add `OpenOptionsExt` for `OpenOptions` (#2515) - fs: add `DirBuilder` (#2524) [`tracing`]: https://crates.io/crates/tracing Signed-off-by: Eliza Weisman <[email protected]>

# 0.2.22 (July 2!, 2020) ### Fixes - docs: misc improvements (#2572, #2658, #2663, #2656, #2647, #2630, #2487, #2621, #2624, #2600, #2623, #2622, #2577, #2569, #2589, #2575, #2540, #2564, #2567, #2520, #2521, #2493) - rt: allow calls to `block_on` inside calls to `block_in_place` that are themselves inside `block_on` (#2645) - net: fix non-portable behavior when dropping `TcpStream` `OwnedWriteHalf` (#2597) - io: improve stack usage by allocating large buffers on directly on the heap (#2634) - io: fix unsound pin projection in `AsyncReadExt::read_buf` and `AsyncWriteExt::write_buf` (#2612) - io: fix unnecessary zeroing for `AsyncRead` implementors (#2525) - io: Fix `BufReader` not correctly forwarding `poll_write_buf` (#2654) - io: fix panic in `AsyncReadExt::read_line` (#2541) ### Changes - coop: returning `Poll::Pending` no longer decrements the task budget (#2549) ### Added - io: little-endian variants of `AsyncReadExt` and `AsyncWriteExt` methods (#1915) - task: add [`tracing`] instrumentation to spawned tasks (#2655) - sync: allow unsized types in `Mutex` and `RwLock` (via `default` constructors) (#2615) - net: add `ToSocketAddrs` implementation for `&[SocketAddr]` (#2604) - fs: add `OpenOptionsExt` for `OpenOptions` (#2515) - fs: add `DirBuilder` (#2524) [`tracing`]: https://crates.io/crates/tracing Signed-off-by: Eliza Weisman <[email protected]>

shkurski force-pushed the fs-dir-builder branch from cf2ccdd to 0321531 Compare May 12, 2020 07:48

Darksonn reviewed May 12, 2020

View reviewed changes

Darksonn added A-tokio Area: The main tokio crate C-enhancement Category: A PR with an enhancement or bugfix. M-fs Module: tokio/fs labels May 12, 2020

shkurski added 2 commits May 13, 2020 00:47

fix documentation errors

e73a9b2

remove DirBuilderExt trait implementation and simplify code

384d185

This commit removes the previously implemented std::os::unix::fs::DirBuilderExt trait from tokio::fs::DirBuilder. Additionally, the `cfg_unix` macro has been dropped and the code has been simplified based on the review.

implement tokio::fs::os::unix::DirBuilderExt trait

4d137fa

shkurski force-pushed the fs-dir-builder branch from 1f64f39 to 4d137fa Compare May 12, 2020 22:15

Darksonn approved these changes May 13, 2020

View reviewed changes

poke ci

22bc3e9

Darksonn reviewed May 16, 2020

View reviewed changes

Fix DirBuilderExt link

aeb8bcf

Darksonn merged commit 8fda5f1 into tokio-rs:master May 21, 2020

Darksonn mentioned this pull request Jun 30, 2020

OS specific File APIs #2636

Open

hawkw mentioned this pull request Jul 20, 2020

chore: prepare to release 0.2.22 #2672

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fs: add support for DirBuilder and DirBuilderExt #2524

fs: add support for DirBuilder and DirBuilderExt #2524

shkurski commented May 12, 2020 •

edited

Loading

Darksonn left a comment

Darksonn May 12, 2020

shkurski May 12, 2020

Darksonn May 12, 2020

shkurski May 12, 2020

Darksonn May 12, 2020

shkurski May 12, 2020

Darksonn May 12, 2020

shkurski May 12, 2020

Darksonn May 12, 2020

shkurski May 12, 2020

shkurski commented May 12, 2020 •

edited

Loading

Darksonn left a comment

Darksonn left a comment

Darksonn May 16, 2020

		/// [os::unix::fs::DirBuilderExt]: ../os/unix/fs/trait.DirBuilderExt.html
		/// [std::fs::DirBuilder]: https://doc.rust-lang.org/std/fs/struct.DirBuilder.html

	/// This is a specialized version of [`std::fs::DirBuilder`] for usage from
	/// This is a specialized version of [`std::fs::DirBuilder`] for usage on

fs: add support for DirBuilder and DirBuilderExt #2524

fs: add support for DirBuilder and DirBuilderExt #2524

Conversation

shkurski commented May 12, 2020 • edited Loading

Motivation

Solution

Darksonn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shkurski commented May 12, 2020 • edited Loading

Darksonn left a comment

Choose a reason for hiding this comment

Darksonn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shkurski commented May 12, 2020 •

edited

Loading

shkurski commented May 12, 2020 •

edited

Loading