-
Notifications
You must be signed in to change notification settings - Fork 189
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
base: main
Are you sure you want to change the base?
Conversation
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.
93029ca
to
b031dee
Compare
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 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. 🙂 |
Is there any particular reason for making the dependency optional? |
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? |
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. |
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.