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

Make the dependency filetime optional #277

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

chris-morgan
Copy link

filetime is in the same boat as xattr: only used if you’re unpacking tarballs to disk, and already disableable at runtime (preserve_mtime, unpack_xattrs). An excellent candidate for making optional.

This is a very slightly breaking change: anyone using tar with default features disabled (to lose xattr) will now also lose filetime. I’d consider that narrowly acceptable in the circumstances of the similarity of the two optional dependencies.

Or, “the outworking of making libc optional in order to reach zero
dependencies, even though you almost certainly depend on libc by some
other path already”.

This one is rather more extreme, but I was curious and it happened, and
frankly I think the end result is pretty decent, so that if I were
maintaining this crate I’d ship it. There are plenty of people who won’t
use Builder functionality, being able to turn it off is a fairly trivial
nice thing. And yeah, I just like being able to have no dependencies
sometimes.

Along the way I noticed wasm32 has a big ol’ unimplemented!() on
Header::fill_platform_from. I should learn what WASI does for a file
system. Probably there should be a simple fallback implementation that
only knows about files, directories and symlinks.
- cfg(target_arch = "wasm32") would panic if you tried to do any std::fs
  Builder stuff, which was fine for wasm32-unknown-unknown and
  wasm32-unknown-emscripten, but not good for wasm32-wasi, which has a
  file system.

  (Might still want a specialised implementation for wasm32-wasi:
  std::os::wasi::fs::FileTypeExt is currently unstable, but offers
  is_block_device, is_character_device, is_socket_dgram and
  is_socket_stream. But this one is good to start with.)

- cfg(not(any(unix, windows, target_arch = "wasm32"))) would fail to
  compile (… unless you disabled the builder feature, since my previous
  commit).

Now it all works pretty decently.
@chris-morgan
Copy link
Author

I should have put the code down while I still had a chance.

I’ve pushed two more commits, one making Builder optional in order to futilely seek zero dependencies, and one to unbreak Builder on cfg(not(any(unix, windows))) platforms, e.g. wasm32-wasi should work now instead of panicking (untested), and anything else that’s also not wasm32 should work instead of not compiling. (When I flip the unix and not(unix) on fill_platform_from, the tests still pass; not sure how much weight to put on that, though—clearly nothing’s testing fine mode preservation.) Details in commit messages. This branch name is now silly. Such is life.

Up to you what you do with this. If you decide you don’t like splitting out an optional builder feature, I hope you at least consider the fill_platform_from patch (with the very minor changes required to disentangle it).

This was fun, even if you don’t like it all. 🙂

@alexcrichton
Copy link
Owner

Is there any particular reason for making the dependency optional?

@chris-morgan
Copy link
Author

It’s just because major use cases don’t actually use filetime (e.g. I’m using tar and reading from the tarball without unpacking), so I think it’s nice to remove it from the dependency tree proactively rather than having to compile it and then wait until later in compilation or linking to confirm that it wasn’t actually used. (Mildly related: that’s been a pet peeve of mine in license-scraping tools: they don’t recognise when nothing actually gets used from a crate, like this.) It’s the principle of the thing, keeping things lean deliberately so that the compiler can definitely avoid needing to compile unused dependencies. Otherwise, why is xattrs marked as optional?

@glandium
Copy link

Also, filetime now pulls in windows-sys, which is a large dependency that is still a moving target, so many crates also depend on different versions of it.

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.

3 participants