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 all dependencies optional #100

Merged
merged 11 commits into from Nov 3, 2018

Conversation

KodrAus
Copy link
Collaborator

@KodrAus KodrAus commented Aug 14, 2018

Except for log.

Closes #71
Closes #72

This PR refactors the internals of the fmt module so we can optionally exclude dependencies and their supported features. The goal of making things optional is to improve compile times and reduce artifact sizes so rather than trying to shim the same public API we exclude swathes of API that are tied to specific dependencies when those dependencies aren't available.

By default all current dependencies are included. Since env_logger usually lives at the top of a dependency graph, if an application disables default features then it should be unlikely that another dependency adds them back in.

This is still in a bit of a strawman phase, there might be other approaches we'll want to consider.

@KodrAus
Copy link
Collaborator Author

KodrAus commented Aug 15, 2018

A potential source of breakage here is if folks are currently using no_default_features with env_logger to exclude regex. With this change that will now also exclude styling and timestamps.

@KodrAus
Copy link
Collaborator Author

KodrAus commented Aug 15, 2018

Possibly also worth noting that libraries using env_logger as a dev-dependency won't cause the default features to be activated if you happen to depend on them.

@KodrAus KodrAus changed the title [WIP] Make all dependencies optional Make all dependencies optional Aug 21, 2018
@KodrAus
Copy link
Collaborator Author

KodrAus commented Aug 22, 2018

cc @briansmith

This makes a pretty big impact on compile-times. On my machine locally for release builds I'm waiting ~18s with default features, and ~1.5s without them. Artifact size is also a bit smaller.

I'll do a proper review of the APIs that are available and unavailable depending on features, but this seems worth following through for folks that care about compile times and don't care about the format.

What do you think?

@fbernier
Copy link

fbernier commented Aug 24, 2018

Thank you for this. The community needs more of these PRs. (Thanks to the library author too)

@KodrAus
Copy link
Collaborator Author

KodrAus commented Aug 26, 2018

I did a quick run through source on GitHub and came up with a few dependencies that would see a change in their default format following this PR:

I'm starting to lean towards pushing this and #82 together as a minor version bump, so 0.6.0 where we can start that version with clear semantics about how changes to the default format will be handled. I'll open up an issue to get some feedback.

@KodrAus
Copy link
Collaborator Author

KodrAus commented Aug 26, 2018

I've added a simple test harness as a local crate that runs tests for all feature permutations. The number of permutations will increase exponentially with the number of optional features so in the future we might need a different strategy for maintaining confidence that everything plays nicely.

@KodrAus KodrAus mentioned this pull request Aug 27, 2018
@KodrAus KodrAus merged commit 15ed5c7 into rust-cli:master Nov 3, 2018
@KodrAus KodrAus deleted the feat/optional-dependencies branch November 3, 2018 05:53
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.

None yet

2 participants