-
Notifications
You must be signed in to change notification settings - Fork 2.7k
fix: update resolve_all_features() to filter pkg deps
#16221
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
base: master
Are you sure you want to change the base?
Conversation
|
Not sure I'm in a good place to give feedback on the change itself at this time but wanted to highlight PR expectations (https://doc.crates.io/contrib/process/working-on-cargo.html#submitting-a-pull-request)
|
|
Thanks! I'll look into how to write a test for this pr and try implementing it before making it ready for review |
2d2adc0 to
8a99fa6
Compare
| use cargo_test_support::project; | ||
|
|
||
| #[cargo_test(ignore_windows = "test windows only dependency on unix systems")] | ||
| fn cargo_test_should_not_demand_not_required_dep() { |
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.
Just some quick feedback on this test:
- Tests should not access the network or crates.io. Registries must be defined locally. See the calls to
Package::newas examples. - Tests should not hard-code target names. If possible try to model this without things that depend on the target (like maybe
cfg(false)). See also therustc_host()function if you need a target forcargo fetch. - The first commit adding a test should show the failing behavior. That way we can see and verify what the change in behavior is in the second commit.
- Generally we don't create new modules to house a single test. Since this is essentially checking the behavior of dealing with features, I would probably just put it in the features module.
- For performance reasons, we tend to prefer using
cargo checkinstead of other commands (--exampleswould be needed in this case).
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've changed to
Package::new()and it now uses dumb-registery - It's now using
rust_host()to detect host and the test ignores windows - I could confirm the test fails before the second commit and passes after, both privously and now
- It's now moved into
features(features::dont_demand_not_required_dep) - I've now changed to
cargo check --examples
Sorry for force pushing into my own repo. I don't know how to keep all history there while keeping 2 commits (test, then fix) in order.
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 guess I should also @epage)
What does this PR try to resolve?
Before,
resolve_all_features()isn't filtering dependencies, as it uses the set returned byResolve::deps()directly which is mostly unfiltered. This PR fixes it by having the set goes throughPackageSet::filter_deps()'s filter.Note that this patch changed
resolve_all_features()'s function signature as well as madePackageSet::filter_deps()public.Close #15399
How to test and review this PR?
As described in #15399,
cargo test --frozenshould not panic (doesn't try to download unneeded deps) when does the following:More discussion on Zulip chat.