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

Disable regex "perf" feature #181

Closed
wants to merge 1 commit into from
Closed

Disable regex "perf" feature #181

wants to merge 1 commit into from

Conversation

link2xt
Copy link

@link2xt link2xt commented Oct 18, 2020

Users can still enable it if needed.

Normally "env_logger" is used as a dev dependency, but because
-Z features=dev_dep is not stabilized yet[1], "perf" feature leaks into
build dependencies and makes it impossible to disable "perf" feature,
except by disabling "env_logger".

[1] rust-lang/cargo#7916

Users can still enable it if needed.

Normally "env_logger" is used as a dev dependency, but because
`-Z features=dev_dep` is not stabilized yet[1], "perf" feature leaks into
build dependencies and makes it impossible to disable "perf" feature,
except by disabling "env_logger".

[1] rust-lang/cargo#7916
@link2xt
Copy link
Author

link2xt commented Oct 19, 2020

For rationale behind this, see rust-lang/regex#721

@jplatte
Copy link
Contributor

jplatte commented Oct 19, 2020

Hi, and thanks for the PR! I can understand the wish to use env_logger with regex filters but without the regex perf feature, but

Normally "env_logger" is used as a dev dependency

The most common use I've seen is as a regular dependency in an application, so we definitely shouldn't just change the default feature set here.

To make this possible in a backwards-compatible manner, please adjust your PR to

  • Rename the regex crate to regex1 in Cargo.toml
  • Remove the perf feature from the regex dependency
  • Introduce a regex Cargo feature that enables regex1/perf so all existing code that activates env_logger/regex gets the same feature set as before
  • Introduce a regex_lite / regex_noperf / whatever feature that just enables the dependency, without the perf feature

@link2xt
Copy link
Author

link2xt commented Oct 20, 2020

We have a https://github.com/deltachat/deltachat-core-rust application, which depends on pretty_env_logger, which in turn depends on env_logger with default features. If we introduce regex_lite, then pretty_env_logger and all similar libraries will have to switch to using regex_lite, and even then with everyone using regex_lite, it is possible that a full-blown version of regex with perf features is compiled because some other dependency in the application or the application depends on it.

Is backwards compatibility worth all these complications? How likely is it that someone used env_logger and had no other dependencies that depend on regex/perf, and will notice when env_logger becomes slower and smaller?

@jplatte
Copy link
Contributor

jplatte commented Oct 20, 2020

We have a deltachat/deltachat-core-rust application, which depends on pretty_env_logger, which in turn depends on env_logger with default features.

Then make a PR to have pretty_env_logger pass through the relevant cargo features rather than always activating them. It's not a lot of work.

and even then with everyone using regex_lite, it is possible that a full-blown version of regex with perf features is compiled because some other dependency in the application or the application depends on it.

This is true even with your PR as-is.

Is backwards compatibility worth all these complications?

Yes.

@jplatte
Copy link
Contributor

jplatte commented Oct 20, 2020

  • Rename the regex crate to regex1 in Cargo.toml
  • Remove the perf feature from the regex dependency
  • Introduce a regex Cargo feature that enables regex1/perf so all existing code that activates env_logger/regex gets the same feature set as before
  • Introduce a regex_lite / regex_noperf / whatever feature that just enables the dependency, without the perf feature

By the way, I've realized that updating this PR to be backwards-compatible is probably even easier than going through that list and then replacing all uses of regex in the code by regex1 to fix the resulting compile errors. You should be able to rename it back in lib.rs with

// renamed in Cargo.toml to not conflict with the regex feature
extern crate regex1 as regex;

@mkroman
Copy link

mkroman commented Jan 11, 2021

It appears the -Z features stabilization was merged 6 days ago 🎉

@jplatte
Copy link
Contributor

jplatte commented Jan 11, 2021

Since it doesn't look like this PR is going anywhere, I'm going to close it. Feel free to open an issue about the same thing; I'm still open to fixing this in a backwards-compatible way (though am not going to work on the fix myself).

@jplatte jplatte closed this Jan 11, 2021
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

3 participants