fix feature flag for frame-system-benchmarking#14617
fix feature flag for frame-system-benchmarking#14617xlc wants to merge 1 commit intoparitytech:masterfrom
Conversation
| frame-system = { version = "4.0.0-dev", default-features = false, path = "../../system" } | ||
| frame-benchmarking = { version = "4.0.0-dev", default-features = false, path = "../../benchmarking", features = ["runtime-benchmarks"] } | ||
| frame-support = { version = "4.0.0-dev", default-features = false, path = "../../support", features = ["runtime-benchmarks"] } | ||
| frame-system = { version = "4.0.0-dev", default-features = false, path = "../../system", features = ["runtime-benchmarks"] } |
There was a problem hiding this comment.
A required feature flag doesn't make sense.
Rust feature flags are additive, and this crate only does benchmarking. So without the flag the crate just does nothing (which is fine).
We cannot ever enable runtime-benchmarks or try-runtime by default. It causes all kinds of issues by being propagated downwards. There is a script in the CI to prevent this as well.
There was a problem hiding this comment.
I don't 100% get what you mean. Crates can be optional. You shouldn't include/enable this if it is not meant for runtime benchmarks. If any downstream crates are getting issues because of this, that means it shouldn't include this at all.
There was a problem hiding this comment.
That means we are shifting the problem from not accidentally enabling the feature to not accidentally enabling this crate?
I think just never enabling runtime-benchmarks and try-runtime by default is a pretty simple rule, that does not need any exceptions. But yea, it should also be possible to optionally enable the dependency.
There was a problem hiding this comment.
Include an empty crate doesn't make a lot of sense to me. Why do you ever want to do that?
I think just never enabling runtime-benchmarks and try-runtime by default is a pretty simple rule, that does not need any exceptions.
I don't agree with this. Pallets specifically built for runtime benchmarks (i.e. this one) can have runtime-benchmarks enabled by default.
On another note, I don't like implicit rules. If it is not sufficiently discussed, and documented, they don't exist to me.
2 participants
A required feature flag doesn't make sense.