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

cargo: add a "failpoints" feature #37

Merged
merged 2 commits into from
Jun 13, 2019
Merged

Conversation

lucab
Copy link
Member

@lucab lucab commented Jun 6, 2019

Breaking: this changes the library behavior, disabling code
generation (in macro) by default.

It introduces a failpoints Cargo feature that consumers
should only enable where relevant (e.g. in testing environments).

Transitive consumers of this library can enable this feature too,
with the special name fail/failpoints (this requires a recent
cargo binary).

Closes: #35

Breaking: this changes the library behavior, disabling code
generation (in macro) by default.

It introduces a `failpoints` Cargo
feature that consumers should only enable where relevant (e.g.
in testing environments).

Transitive consumers of this library can enable this feature too,
with the special name `fail/failpoints` (this requires a recent
cargo binary).

Signed-off-by: Luca Bruno <luca.bruno@coreos.com>
@lucab
Copy link
Member Author

lucab commented Jun 6, 2019

/cc @brson @BusyJay for review

@@ -18,8 +18,11 @@ lazy_static = "1.2"
rand = "0.6"

[features]
no_fail = []
failpoints = []
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps make failpoints as the default feature? Dependencies not wanting failpoints could still opt-out with default-features = false.

Suggested change
failpoints = []
default = ["failpoints"]
failpoints = []

Copy link
Member Author

Choose a reason for hiding this comment

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

I understand the sentiment behind this suggestion, but unfortunately that will nullify this PR. There is no way for cargo to disable default features across transitive dependencies (by design). It does provide a way to enable them though, via the crate/feature syntax.

This library features should be handled by chaining upward the opt-in switch, and leaving the final decision to the top consumer.

I acknowledge this is a breaking change, but at the same time I do believe failpoints are an instrumentation feature mostly meant for testing/debugging mode (i.e. not normally present in release-mode binary)

Copy link
Member

Choose a reason for hiding this comment

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

How about naming it activation?

/cc @brson @siddontang @overvenus PTAL

Copy link
Member

Choose a reason for hiding this comment

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

@kennytm Do you have any suggestions?

@BusyJay
Copy link
Member

BusyJay commented Jun 10, 2019

You need to update all the CI configuration files to run tests with failpoints feature.

@lucab
Copy link
Member Author

lucab commented Jun 11, 2019

An overall update:

  • edited both CI configurations to test with and without the feature
  • with regard to naming, I'm following API guidelines and sticking to something obviously related to the crate-name, and non-creative
  • with regard to feature enablement, default-features makes it opt-out again so I don't think it's tenable here

A middle-ground solution could be to make this cfg(any(debug_assertions, feature = "failpoints")), so that code is always generated in debug/testing mode (i.e. cargo test), but it's strictly opt-in in release mode (i.e. cargo build --release).
I'm not huge a fan of this as it introduces more complexity and troublesome sharp edges (think of cargo test --release, see tikv/rust-prometheus#246), but at least it wouldn't need changes in most consumers' CI. Another drawback is that it won't be anymore possible to disable for debug builds (i.e. cargo build).

@BusyJay
Copy link
Member

BusyJay commented Jun 12, 2019

I'm OK with overall changes. Well done @lucab! The only thing I concerns about is the feature name, but I don't have strong opinions.

Copy link
Member

@overvenus overvenus left a comment

Choose a reason for hiding this comment

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

I have long wanted this!

@BusyJay BusyJay merged commit cb35903 into tikv:master Jun 13, 2019
@lucab
Copy link
Member Author

lucab commented Jun 13, 2019

@BusyJay @overvenus thanks for reviewing and merging this! Would you mind a git tag and cargo publish for 0.3.0? That would also close #31.

@lucab lucab deleted the ups/opt-in-feature branch June 13, 2019 08:03
@BusyJay
Copy link
Member

BusyJay commented Jun 13, 2019

Yes, I will publish this once #32 is merged. Maybe in 24 hours.

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.

cargo feature should be opt-in, not opt-out
4 participants