Release 0.4.22 breaks --all-features CI testing

[bromls]

A recent change in the log 0.4.22 release, #627, breaks CI for any project that depends on the log crate and uses cargo --all-features for testing.

The tracing crate has equivalent "max-level" features and chooses to resolve them by defining a priority: https://docs.rs/tracing/latest/src/tracing/level_filters.rs.html

It can be confusing that cargo features are additive, but that is how the capability has been designed. Though discouraged, rust documentation does allow for compilation errors on truly mutually-exclusive features. However, this arguably should not be introduced in a point release.

[Thomasdezeeuw]

Though discouraged, rust documentation does allow for compilation errors on truly mutually-exclusive features.

The problem is really in the way the log crates uses features as they don't add, but rather remove functionality, so yes there are very much mutually exclusive, see #481 for more.

However, this arguably should not be introduced in a point release.

The --all-features flag shouldn't apply to any of your crate's dependencies. Are you testing the log crate in your CI? Otherwise this shouldn't really affect anyone uses log's features correctly.

[sbromling]

feel free to close

[bastibl]

I have a larger project that sets release_max_level_info and some binaries for benchmarking/performance measurements in a different crate that set release_max_level_off. This also does not work anymore, and I see no obvious workaround. I always saw the way that this can be configured as a great idea, but now it is broken for no obvious reasons. Is this not considered to be a valid use-case?

[Thomasdezeeuw]

I have a larger project that sets release_max_level_info and some binaries for benchmarking/performance measurements in a different crate that set release_max_level_off. This also does not work anymore, and I see no obvious workaround.

This by design really. The problem is that the log crate can not determine which feature/filter should be used. Which means you either get more logs than you expect (info instead of off) or no logs at all (off instead info). In the past this lead to surprising results, hence we want to do something about this.

I always saw the way that this can be configured as a great idea, but now it is broken for no obvious reasons. Is this not considered to be a valid use-case?

I didn't consider your specific use case. Is there any chance you can move the setting of the features to the (main) binary and benchmarks? I think some of the following might work.

[bin]
required-features = ["log/release_max_level_info"]

[bench]
required-features = ["log/release_max_level_off"]

A little off topic, but are you sure you want to change the logging at all for your benchmarks? Because it means the compiled code will be different in your benchmarks compared to your binary you're actually running. You're effectively no longer testing the code you're running.

[bastibl]

Rust features are additive. So if feature A is enabled that means that one gets at least feature A and not A and only A. While it might be unexpected for some, this is just how it works and not an issue of this crate.

Before the recent changes, this crate was perfectly inline with Rust's feature semantics: If I set release_max_level_info, it is guaranteed that Trace and Debug are disabled. It does not imply that Info etc will definitely be compiled in. So one could be more restrictive adding more features. I always regarded this as a feature and a nice way to use Rust features.

For whatever reason, this is now declared to be a problem. And previous, perfectly fine use-cases result in a compile error.

The current release assumes that anybody, who puts a release_max_level_info somewhere would assume that this guarantees that Info etc are compiled in. I agree, if one is new to the language and has misconceptions about features, one might expect this. But then the problem are the misconceptions (which will anyway bite you at some point in time) and not an issue of the log crate.

In my opinion, the current release is against feature semantics in Rust. I don't get the rational behind this decision and hope you reconsider these changes.

[KodrAus]

The max level features we use in log are a historical misuse of Cargo features that makes --all-features builds unusable. They should only be used by an end-user binary, which should only define a single one (or two if using the release variants as well). @bastibl the case you've described as intentional and desirable is a situation others have ended up in entirely accidentally where a library they aren't in control of is setting a max level they're not aware of. Once that happens you're also stuck. Really, we shouldn't be controlling log levels via Cargo features, but that's the scheme we currently have. We've introduced this check to try manage the scalability of this scheme which becomes very opaque very quickly.

[bastibl]

If you believe Rust features are the wrong way, that's fine. Then one could switch to build.rs + environment variables. (Even though I think its worse, since magic environment variables cannot be discovered with Rust tooling/IDEs.)

One could also rename the features to something like disable_<level> to make semantics clearer.

But breaking a perfectly fine system for no reason, prohibiting the use of Rust features the way they are designed and used throughout the ecosystem?

[Thomasdezeeuw]

If you believe Rust features are the wrong way, that's fine.

I think you're misunderstanding what @KodrAus and I have been saying. The Rust features are fine. Log's usage of it is not. I.e. log is the problem. We know this and I think pretty much everybody agrees it's bad, but we don't want to break this, it's something to improve for v1 though.

But breaking a perfectly fine system for no reason, prohibiting the use of Rust features the way they are designed and used throughout the ecosystem?

See comments above, it's wasn't and isn't a perfectly fine system.

The goal of the disallowing multiple filter feature is because Log usage of features is wrong. This leads to surprising results w.r.t. things being not logged, see @KodrAus comment above.

@bastibl did you try the solution I suggested in #635 (comment)?

[bastibl]

I fully understand what you are doing and why you are doing it. I just believe that using features and disallowing additive features by raising a compile error is the worst of both worlds. Anyway, I see you're fond of your current solution and are not up for discussion.

The solution I found is switching to tracing. It can be used as a drop-in replacement: https://docs.rs/tracing/latest/tracing/#for-log-users So I'm fine as long as you don't pitch your solution to the tracing developers :-)

[Thomasdezeeuw]

Anyway, I see you're fond of your current solution and are not up for discussion.

We're not fond of the solution... I still feel like we're having a miscommunication here.