-
Notifications
You must be signed in to change notification settings - Fork 57
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
Tidy up features and dependencies #438
Conversation
Thanks for this contribution, this is something really interesting that I want to get merged. I'm currently traveling, I'll look closely at that next week |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, quite the cleanup, many thanks!
Learned about the ?
operator, looking forward to other reviewers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the PR, thanks a lot for the cleanup. I've left some minor requests
.github/workflows/tests.yml
Outdated
rustup target add wasm32-unknown-unknown | ||
- uses: dtolnay/rust-toolchain@38b70195107dddab2c7bbd522bcf763bac00963b # stable | ||
- uses: Swatinem/rust-cache@f0deed1e0edfc6a9be95417288c0e1099b1eeec3 # v2.7.7 | ||
- uses: taiki-e/install-action@99b53876daf89877c0c7291dfd5033c8c0988612 # cargo-hack |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
@dave-tucker on top of my previous comments, could you also take a look at the |
This contains a number of small changes to the way that the project uses features. 1. Dependencies are specified using `default-features=false` and only the required features of dependencies are enabled. This avoids changes in downstream dependencies default feature-set bringing in new dependencies that we don't need. 2. I've audited all dependencies to ensure that just those required are included in each feature 3. All *-native-tls and *-rustls-tls features have been removed and are replaced with a single feature for each type of TLS. This correctly enables the right features in dependent packages but only if they have been selected by another feature (i.e cosign). Signed-off-by: Dave Tucker <[email protected]>
This uses cargo-hack to test all possible feature combinations. Since at present this is ~800 combinations, cargo check has been split to operation in parallel. Single features are tested in isolation (with a few exceptions) All features combinations are tested with `native-tls` All features combinations are tested with `rustls-tls` This generates a lot of compiler warnings for dead-code since some modules expose methods that are only accessed when other features are enabled. Given the combination of features is so large, cargo test is run twice only on `full,native-tls` and `full,rustls-tls`. Signed-off-by: Dave Tucker <[email protected]>
This adds check-features, check-features-native-tls and check-features-rustls-tls targets to the Makefile to allow local testing of features. Given that these tests take a long time to run, they've been added as 3 seperate targets. Most issues are likely to be caught in CI, but this allows a contributor to easily reproduce and fix these issues in their own development environment. The targets also require that the user has installed cargo-hack, but the error message from cargo when this is missing is pretty informative. Signed-off-by: Dave Tucker <[email protected]>
e02b679
to
75d8eef
Compare
This checks in a .taplo.yml file that was used to format the Cargo.toml files with the recent changes and adds a check in CI to ensure that all files are formatted the same way. In addition, it changes all remaining uses of installing rust using rustup directly to instead use the dtolnay/rust-toolchain action. Signed-off-by: Dave Tucker <[email protected]>
75d8eef
to
d1243f8
Compare
@flavio all comments addressed and Makefile updated as requested. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for having done all the requested changes
Summary
While identifying the root cause of #436 I started looking into the features offered by
sigstore
such that I might avoid the dependency ontough
entirely - it's marked as optional.I attempted to use
cargo-hack --every-feature
to test whether the features compiled on their own.Many features did not. You can reproduce this with:
I then tried
cargo-hack --feature-powerset
to test all combinations of features but it died due to OOM - too many feature combinations.As such, I took a pass at trying to clean up the feature (and dependency) situation as follows:
Dependencies are specified using
default-features=false
and onlythe required features of dependencies are enabled. This avoids
changes in downstream dependencies default feature-set bringing in
new dependencies that we don't need.
I've audited all dependencies to ensure that just those required are
included in each feature
All *-native-tls and *-rustls-tls features have been removed and are
replaced with a single feature for each type of TLS. This correctly
enables the right features in dependent packages but only if they
have been selected by another feature (i.e cosign).
This uses the
?
operator added in Rust 1.60 but giventhis is over 2 years and 27 stable versions ago I doubt this will be an
issue.
With these changes it was possible to add run
cargo check
in CI for:*-tls
feature to compilenative-tls
rustls-tls
Then to run
cargo test
against--features full,native-tls
and--features full,rustls-tls
to ensure that both supported TLS options are properly tested.Release Note
full-native-tls
,fulcio-native-tls
,rekor-native-tls
,cosign-native-tls
,mock-client-native-tls
,sigstore-trust-root-native-tls
,registry-native-tls
,full-rustls-tls
,fulcio-rustls-tls
,rekor-rustls-tls
,cosign-rustls-tls
,mock-client-rustls-tls
,sigstore-trust-root-rustls-tls
,registry-rustls-tls
. These features have been replaced withnative-tls
andrustls-tls
features. To migrate, simply replacefull-native-tls
withfull, native-tls
in yourCargo.toml
Documentation
No