Add default features for time and compression #28
Conversation
This makes the `chrono` and `flate2` dependencies optional, adding a `time` and `compression` feature for each. These features are enabled by default, but can be disabled by using `default-features = false` in the Cargo.toml dependency definition, allowing library users to opt out of these dependencies if they don't need them. This should reduce compile times and binary size. In my testing on a release build I saved 1.3 seconds of compilation time (although this is very CPU dependent) and 66 KiB of binary size by dropping these features from a project that depends on file-rotate. `cfg` attributes have been added to tests to support testing with features disabled, and I've added additional test runs in the rust.yml GitHub Action to cover these cases. If you want me to add additional cargo check runs for these cases let me know and I'll do that as well. The example and doc tests only work with the default features enabled, but I think that's fine. I have not bumped the library version in Cargo.toml as I believe that should be left to the maintainer. However, note that a major version bump is needed (e.g. 0.7.6 -> 0.8.0) as this change is breaking for anyone already using `default-features = false` (even though there are currently no features someone may still be doing this for some reason). I've seen other projects get hit by this, and affected library users typically call for a cargo yank.
|
Looks good! |
|
Ah, I see why the checks failed: I missed that one of the tests only runs in |
There was a problem hiding this comment.
I do not like peppering the code with #[cfg(...)]. Like @Ploppz mentioned, we could separate the various use-cases out into different modules so we only need a #[cfg(...)] on mod ... declarations. In that case we can implement a no-op/panicking modules for time/compression when either or both are feature disabled.
I'll approve this code now for practical purposes, however, a cleanup is required to make this code more legible and maintainable.
| @@ -411,7 +415,7 @@ impl<S: SuffixScheme> FileRotate<S> { | |||
| path: P, | |||
| suffix_scheme: S, | |||
| content_limit: ContentLimit, | |||
| compression: Compression, | |||
| #[cfg(feature = "compression")] compression: Compression, | |||
There was a problem hiding this comment.
Not a fan of making the API contingent upon features; it makes swapping between features in a project using this library harder because the user will need to modify code each time. I suggest we instead panic if this argument is something that's not Compression::None when compression is disabled.
There was a problem hiding this comment.
I agree that feature-gating args out of a parameter list is unclean. However, I'm not a fan of API that looks correct and smells correct but panics at runtime; that seems like it would just move the developer pain from compile-time to runtime and be more difficult to catch.
Some other ideas:
- Instead of panicking if an API user tries to enable compression without the feature we no-op. This is still misleading, but at least it doesn't cause unexpected panics.
- A builder pattern where we have feature-gated extension traits that add more setter functions. This would still require API users to make code changes when they enable/disable features, but it's limited to adding/removing a
useon the extension trait. I've seen other popular libraries do this: as an arbitrary example, here's a trait of mac-specific builder functions that winit only implements for that specific platform.
Also, I'd hate to make you feel pressured to merge a PR you dislike so please don't feel like you need to merge this now just to clean it up later. If there's a specific way you want to see this implemented I'm happy to alter the PR. Or if this isn't something you want to consider right now that's also fine: I'm not in any rush.
There was a problem hiding this comment.
No-op sounds like a good solution. Ideally with a warning message printed at runtime (or compile time, if possible, but no compile/runtime errors) so we don't get bug reports here and users can easily identify what's wrong.
There was a problem hiding this comment.
Let me present an argument for panicking. The panic will happen only on creation of FileRotate, and I can only really see this object being constructed in the initialization (or very early) in the application when it runs. So any simple test of the application should show this - manual or automatic integration tests.
Here is one example from a popular library, not really of a panic but of silently leaving an application non-functioning due to a programmer error: If you use actix-web to build a web server, and you make a mistake in a request handler's parameters asking for app data of a type that has not been registered, it will not warn you at compile time... but your API will always return some 5xx http error saying that it's wrongly configured. Has it ever been a problem for me as a user of actix-web? No, because it makes all my tests fail and it's easily fixed.
There was a problem hiding this comment.
That said, a builder pattern sounds like a good solution as well. And it doesn't have to be through an extension trait IMO but maybe just a feature gated function on the builder? I agree that changing the number of parameters with #[cfg] can lead to confusion, but what do you think about feature gating builder functions, @kstrafe ?
| @@ -411,7 +415,7 @@ impl<S: SuffixScheme> FileRotate<S> { | |||
| path: P, | |||
| suffix_scheme: S, | |||
| content_limit: ContentLimit, | |||
| compression: Compression, | |||
| #[cfg(feature = "compression")] compression: Compression, | |||
| #[cfg(unix)] mode: Option<u32>, | |||
There was a problem hiding this comment.
Unrelated to this commit, but this code forces cross-platform code that uses this library to specify #[cfg(unix)]. I do not think this design is good. @Ploppz
This makes the
chronoandflate2dependencies optional, adding atimeandcompressionfeature for each. These features are enabled by default, but can be disabled by usingdefault-features = falsein the Cargo.toml dependency definition, allowing library users to opt out of these dependencies if they don't need them.This should slightly reduce compile times and measurably reduce binary size. In my testing on a release build I saved 72.5 KiB of binary size by dropping these features from a project that depends on file-rotate, but doesn't need compression or time-based log rotation. Using fat LTO also does not solve the size bloat: it just reduces it down to 66 KiB. These filesize savings may not sound like much, but size bloat adds up quickly across multiple dependencies.
cfgattributes have been added to tests to support testing with features disabled, and I've added additional test runs in the rust.yml GitHub Action to cover these cases. The example and doc tests only work with the default features enabled, but I think that's fine. I've tested locally and the tests pass for all four possible feature enablement configurations. If you want me to add additional cargo check runs for these cases let me know and I'll do that as well, but I think that may just slow down the GitHub Action CI time unnecessarily.I have not bumped the library version in Cargo.toml as I believe that should be left to the maintainer. However, note that a major version bump is needed (e.g. 0.7.6 -> 0.8.0) as this change is breaking for anyone already using
default-features = false(even though there are currently no features someone may still be doing this). I've seen other projects get hit by this, and affected library users typically call for a cargo yank.Also, I must apologize because I only noticed after I made this change that you already changed the chrono dependency from optional to mandatory last year as part of #24. If optional dependencies aren't something you're interested in merging I understand, but I figure I might as well open the PR seeing as it's done.