Put dependencies behind features

[Uriopass]

"Don't pay for what you don't use"
Bzip2 is good, but packaging a full bzip2 encoder/decoder even if you don't use compression is a bit cumbersome.
Same goes for encryption for example.

I suggest putting theses behind conditional features.
Cargo's default features can be use to keep backward's compatibility.

[avl]

I 100% agree with you. Quite early I had a request for compression and encryption support. Mostly for fun, I decided to implement it. It is a slightly strange thing to have integrated into savefile though - there's no reason it couldn't be a separate crate.

What do you think, should I make it into an optional feature (perhaps not even enabled by default), or make it into an entirely separate crate?

Speaking of dependencies, Savefile has quite a few. I depend on IndexMap, for instance, so that savefile can have a default-implementation for IndexMap. Maybe all such dependencies should also be behind feature flags (but enabled by default)?

[avl]

I've put bzip2 and ring (encryption-library) behind optional non-default feature flags, and published version 0.9.1

I hope this resolves your issue. If not, just reopen this, or file another issue. Have a great day!

[Uriopass]

Well you've already done it, but yes behind a feature flag is good :)

Speaking of dependencies, Savefile has quite a few. I depend on IndexMap, for instance, so that savefile can have a default-implementation for IndexMap. Maybe all such dependencies should also be behind feature flags (but enabled by default)?

Definitely! I thought you used indexmap internally but if it's just to implement the traits on it it should probably be behind a feature flag.
Just imagine the most basic usecase: ser/de a basic struct, what dependency would it need ?

[avl]

Yeah, I think basically only byteorder is an actual dependency of savefile. All the other dependencies are only to offer implementations of the Savefile-traits for their datatypes.

This whole thing is slightly unsatisfactory. For a user of savefile, they have to use the same version of indexmap, arrayvec, parking_lot, or they won't be able to serialize/deserialize those types. And because of rust orphan rules (which exist for extremely good reasons!), there's no way for anyone to implement the Savefile-traits for these outside the Savefile-crate (except within indexmap, arrayvec etc).

I've been thinking about what could be done. There are a few options:

1. Add support for multiple versions of 3rd party crates. This sounds tricky
2. Abandon the savefile-traits, and somehow try to utilize serde-traits (effectively making savefile a sort of serde-extension).
3. Approach 3rd-party crates and ask if they would like to ship Savefile-support.
4. Try to select a resonable (recent) version of each 3rd-party crate to support, and hope this is good enough for all Savefile-users.

I'm not sure if alternative 1 is even possible. Number 2 would be a very major change, I don't think that's realistic. Alternative 3 is not reasonable for a relatively obscure library like savefile.

So I think alternative 4 is the most reasonable right now. Hopefully, fundamental crates like IndexMap, arrayvec etc, won't see much churn in the future.

One more thing, I just realized the 'rand' dependency is really only used when 'ring' dependency is enabled. Reopening until I've fixed this and also the other dependencies.

I'm thinking IndexMap is such a popular crate, it might make sense to have support for it enabled by default?

[Uriopass]

Serde provides support for remote crates to bypass the orphan rule, but it's a bit unwieldy:
https://serde.rs/remote-derive.html

I think your analysis is good and the 4th option (which is what you have right now) is the good one, although they should be feature gated (the goal of this issue).

As for the default support, I'm not really sure what is best for the ecosystem:

1. Enabled by default runs the risk of compiling support for nothing.
2. Disabled by default hinders discoverability. However people don't expect all crates under the sun to be implemented by default, and people know about cargo features for the most part.

[avl]

Thanks for the link about remote-derive. Very interesting.

I agree with everything that you're writing.

I think the drawbacks of accidentally compiling in support for IndexMap, even though it isn't being used, are quite limited. It shouldn't affect the final binary. It should hardly affect compile times, except for the first compile.

Experienced developers with constraints on dependencies should know to check if there are default features which could be disabled.

I think the defaults should be such that they are suitable to the widest group of users, with some preference for beginners (who are less likely to know to check features). And since I feel that the drawbacks of unnecessarily adding support for unused crates is quite small, I'm presently leaning towards including support for IndexMap etc by default.

But I'm willing to listen to more arguments! :-)

[avl]

Ok, I've now published 0.10.1, which makes all runtime-dependencies except "byteorder" optional.

I suppose someone might want 'no_std' support, but that would require removing support for a very large number of data structures, I'm not sure there is much demand for a convenience-oriented serialization library for 'no_std'-targets.