Skip to content

Add async-fs wrappers #70

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 2 commits into
base: main
Choose a base branch
from
Open

Conversation

Lynnesbian
Copy link

This pull request adds wrappers for async-fs types (used by smol), similar to #40. The wrappers are kept behind the optional async-fs feature.

@andrewhickman
Copy link
Owner

Thanks for the PR, I think this functionality would be a great addition.

I kinda regret adding the tokio support to the main crate under a feature flag, because it means if there is ever a tokio 2.0, I'd probably need to bump the major version for all users to support it. I wonder if it might be possible to include async-fs in a new crate (e.g. async-fs-err to avoid this issue). One annoyance with this approach is that the error types in this crate are not exported, so they couldn't be shared between the two crates. Maybe we should just duplicate the errors module in both crates

@Lynnesbian
Copy link
Author

Hmm, so it appears the CI failed because earlier Rust versions don't let you have a dependency named async-fs that depends on a crate named async-fs. The suggested workaround for this is usually to use the dep: prefix, which was added in Rust 1.60, so we can't use it.

The only other solutions would be to rename the async-fs feature (maybe to something like async-fs-support or async-fs-2, given that async-fs is currently at version two?), or to rename the async-fs dependency by using the package specifier.

@Lynnesbian
Copy link
Author

I wonder if it might be possible to include async-fs in a new crate (e.g. async-fs-err to avoid this issue). One annoyance with this approach is that the error types in this crate are not exported, so they couldn't be shared between the two crates. Maybe we should just duplicate the errors module in both crates

If you wanted to go down that path, perhaps the errors could be factored out into a separate crate, and included transitively, creating something like...

fs-err (parent crate)
|- fs-err-types (error types & handling)
|- fs-err-std (std::fs)
|- fs-err-tokio (tokio)
|- fs-err-async-fs (async-fs)

Users would depend on fs-err with whatever features they wanted, which would pull in the transitive crates. std could be made a default feature.

It would probably be a fair bit of work, and would obviously involve a breaking version change, but it could make the crate more flexible?

@Lynnesbian
Copy link
Author

OK, I've thought about this plan a bit more, and I have a basic outline for how it would work:

  • fs-err becomes a "shell crate" that doesn't do anything on its own, but exports the std, tokio, and async-std implementations
  • fs-err-types contains publicly exported types for errors (ErrorKind, Error, etc), as well as unsealed traits for things like PathExt
  • fs-err-std, fs-err-tokio, and fs-err-async-fs contain the actual code that handles those file system modules, and return errors imported from fs-err-types

There are a few advantages to doing things this way:

  • Anyone can write a new fs-err crate - Given some asynchronous file system module foo, someone else can write fs-err-foo, have it depend on fs-err-types, and publish and maintain it independently
  • Split builds - fs-err builds pretty quickly, so this is of admittedly limited relevance, but splitting the crates allows Cargo to parallelise their compilation
  • More flexible MSRV - The std module can maintain its MSRV of Rust 1.40, while foo can have an MSRV of 1.70. This would allow fs-err-foo to use more recent Rust features without impacting the MSRV of the "main" implementation
  • In the case of a theoretical Tokio 2, fs-err-tokio could be removed from fs-err, but remain accessible on crates.io, while a new fs-err-tokio2 is added. This would allow people who need support for the older Tokio version to directly depend on fs-err-tokio, while the default is upgraded. With the current layout, fs-err would either need two separate Tokio features that can't both be enabled at the same time, which would cause some issues, or Tokio 1 support would need to be dropped

...but there are also some disadvantages:

  • The sealed and pub(crate) traits have to be made public. As mentioned, this allows others to make their own fs-err-foo crates, but it might mean revealing crate internals that are meant to be private, and will end causing churn if the previously sealed traits are ever extended
  • It's a fairly big restructuring of how things are currently
  • Workspaces can be a little less ergonomic to work with than singular crates. For example, Cargo currently lacks support for publishing multiple crates simultaneously, although this is being worked on

Whether you want to do this or not depends on how you feel about making the error types public and "unsealing" traits like PathExt. If you think this workspace structure is the way to go, I can draft a PR implementing it. If you don't, no worries, and I can rename the async-std feature to fix the compile error on 1.40, or I can create a new fs-err-async-fs crate and duplicate the current error module into it.

@andrewhickman
Copy link
Owner

In the case of a theoretical Tokio 2, fs-err-tokio could be removed from fs-err, but remain accessible on crates.io, while a new fs-err-tokio2 is added

Wouldn't this have the same issue of requiring a version bump on the main fs-err crate?

I'm wondering if unsealing the extension traits might cause breakage too. I'm comfortable relying on documentation to stop external users implementing them, but I think it would be possible for the versions of fs-err-types and the consuming crates to get out of sync. For example, you update a dependency which uses fs-err-std which transitively updates fs-err-types, but you are also using fs-err-tokio which now fails to compile because it doesn't implement all the methods in the new version of fs-err-types.

For now I prefer just exposing fs-err, fs-err-async-fs, fs-err-tokio as three independent crates. The error type could be moved into a shared crate later on, but I'd want to figure out a nicer API first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants