Skip to content

Don't activate default features of env_logger #33

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

Merged
merged 1 commit into from
Sep 6, 2019

Conversation

badboy
Copy link
Contributor

@badboy badboy commented Sep 5, 2019

env_logger comes with several optional-but-default-activated features,
including a dependency on regex.
There's no need for android_logger to require that. Deactivating default
features allows users of android_logger to define the features they
need.

We're using regex in our own library, but with [recent changes](https://github.com/rust-lang/regex/pull/613) we were able to reduce our library size by 30% or so. However, now upgrading to the latest android_logger pulls in the whole of regex again, without a way to prevent that. If android_logger just won't ask for the default features our own library is free to specify the final feature set (of course only as long as no _other_ library also pulls in regex with default features).

@Nercury Nercury self-requested a review September 5, 2019 13:37
Copy link
Collaborator

@Nercury Nercury left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! This looks like a sane thing to do if it reduces the binary size.

This dependency was probably brought in by fancier log filter. I need to check if it works as expected with this change, add a feature to disable it if it does not work for some reason.

@Nercury
Copy link
Collaborator

Nercury commented Sep 6, 2019

Since this breaks backwards compatibility, would make this crate work differently than default env_logger, would need a new major version and at least a note in readme, let's do it this way:

  1. Expose a feature for this crate named regex, that will forward regex to env_logger's regex:
[features]
default = ["regex"]
regex = ["env_logger/regex"]
  1. Remove the current change that adds line default-features = false.
  2. Add default-features = false for this crate in your project.

@badboy badboy force-pushed the dont-depend-on-regex branch from 998b842 to 0c404d5 Compare September 6, 2019 07:00
@Nercury
Copy link
Collaborator

Nercury commented Sep 6, 2019

Sorry, looks like we need to leave default-features = false on dependencies.env_logger for this scheme to work, otherwise the regex will still get picked up.

@badboy
Copy link
Contributor Author

badboy commented Sep 6, 2019

Sorry, looks like we need to leave default-features = false on dependencies.env_logger for this scheme to work, otherwise the regex will still get picked up.

Oh, true. I should not send PRs before my first coffee in the morning :D

@badboy
Copy link
Contributor Author

badboy commented Sep 6, 2019

These are the default features of env_logger:

default = ["termcolor", "atty", "humantime", "regex"]

So default-features=false and enabling regex will still be different from that.
Hmpf. I'll spend a bit to think about that. Maybe the solution here is something else (and for now I'm fine with depending on an old version of android_logger and in the future I might even replace it due to some other internal code)

@Nercury
Copy link
Collaborator

Nercury commented Sep 6, 2019

This change is good overall, because we certainly don't need other env_logger features such as colored terminal output or humantime. The only thing we re-use is the filter.

This allows to deactivate the feature if wanted.

env_logger comes with several optional-but-default-activated features,
including a dependency on regex. Not everyone likes to pull in regex.
Putting this behind a feature allows users of android_logger to use it
with default-features=false and thus removing the regex dependency.
@badboy badboy force-pushed the dont-depend-on-regex branch from 0c404d5 to 1db5caf Compare September 6, 2019 07:42
@badboy
Copy link
Contributor Author

badboy commented Sep 6, 2019

Well, then...

@Nercury Nercury merged commit 21a26ad into rust-mobile:master Sep 6, 2019
@badboy badboy deleted the dont-depend-on-regex branch August 25, 2020 14:52
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.

2 participants