fix: update `resolve_all_features()` to filter pkg deps by Saplyn · Pull Request #16221 · rust-lang/cargo

Saplyn · 2025-11-07T03:25:39Z

What does this PR try to resolve?

Before, resolve_all_features() isn't filtering dependencies, as it uses the set returned by Resolve::deps() directly which is mostly unfiltered. This PR fixes it by having the set goes through PackageSet::filter_deps()'s filter.

Note that this patch changed resolve_all_features()'s function signature as well as made PackageSet::filter_deps() public.

Close #15399

How to test and review this PR?

As described in #15399, cargo test --frozen should not panic (doesn't try to download unneeded deps) when does the following:

git clone https://github.com/hickory-dns/hickory-dns
cd hickory-dns
git checkout v0.25.1
mkdir .cargo_home
CARGO_HOME=.cargo_home cargo fetch --locked --target=x86_64-unknown-linux-gnu
CARGO_HOME=.cargo_home cargo test --frozen -p hickory-resolver

More discussion on Zulip chat.

epage · 2025-11-07T22:08:25Z

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)

A test will be needed for this
It is really helpful if you create a test in its own commit that shows the panic and then put this commit on top that removes the panic. Then the diff between these two commits shows how behavior changed. This allows you to verify the test is testing the right thing and gives a good demonstration of the fix

Saplyn · 2025-11-08T06:58:17Z

Thanks! I'll look into how to write a test for this pr and try implementing it before making it ready for review

rustbot · 2025-11-14T08:51:04Z

r? @epage

rustbot has assigned @epage.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

ehuss · 2025-12-10T01:43:04Z

tests/testsuite/resolve_optional_dependency.rs

+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() {


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::new as 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 the rustc_host() function if you need a target for cargo 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 check instead of other commands (--examples would be needed in this case).

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.

(I guess I should also @epage)

ehuss

Thanks! Small request for the test, but otherwise I think this should be fine.

View changes since this review

ehuss · 2026-01-20T18:28:31Z

tests/testsuite/features.rs

+default = ["feat"]
+feat = ["dep:win-only"]
+
+[target.'cfg(windows)'.dependencies]


This test can be made to run on any target by using cfg(false) here instead of windows.

Thanks, I shall fix this right now

Saplyn · 2026-01-20T20:38:29Z

Hi @ehuss, I've changed my solution as requested (from cfg(windows) to cfg(false)).

ehuss

Thanks!

View changes since this review

weihanglo · 2026-01-21T22:14:13Z

tests/testsuite/features.rs

+    p.cargo(&format!("fetch --target={host}")).run();
+    p.cargo(&format!("check --target={host} --examples --frozen"))


Nit: can use --target host-tuple maybe?

Update cargo submodule 14 commits in 85eff7c80277b57f78b11e28d14154ab12fcf643..efcd9f58636c1990393d495159045d9c35e43b8f 2026-01-15 16:18:08 +0000 to 2026-01-23 13:50:59 +0000 - chore(deps): update cargo-semver-checks to v0.46.0 (rust-lang/cargo#16548) - Increase cache_lock test timeout (rust-lang/cargo#16545) - iTerm now supports OSC 9;4 (terminal window progress bar) (rust-lang/cargo#16506) - chore: Updated compiler errors for Rust 1.93 (rust-lang/cargo#16543) - test(build-std): adjust snapshot (rust-lang/cargo#16539) - chore: bump to 0.96.0 (rust-lang/cargo#16538) - fix: update `resolve_all_features()` to filter pkg deps (rust-lang/cargo#16221) - fix: show implicit_minimum_version_req emitted source once per package (rust-lang/cargo#16535) - fix: `--remap-path-scope` stabilized in 1.95-nightly (rust-lang/cargo#16536) - feat(lints): Add non_kebab_case_bin lint (rust-lang/cargo#16524) - fix(rm): Suggest table flags when none are specified (rust-lang/cargo#16533) - fix(patch): clean up patch-related error messages (rust-lang/cargo#16498) - Store artifact deps in build unit dir (rust-lang/cargo#16519) - refactor(timings): reuse timing metric collection logic between `--timings` and `-Zbuild-analysis` (rust-lang/cargo#16497)

rustbot added the A-cargo-targets Area: selection and definition of targets (lib, bins, examples, tests, benches) label Nov 7, 2025

Saplyn force-pushed the master branch 4 times, most recently from 2d2adc0 to 8a99fa6 Compare November 13, 2025 20:36

Saplyn marked this pull request as ready for review November 14, 2025 08:51

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 14, 2025

rustbot assigned epage Nov 14, 2025

ehuss reviewed Dec 10, 2025

View reviewed changes

Saplyn force-pushed the master branch from 8a99fa6 to 82aa714 Compare December 12, 2025 03:57

ehuss reviewed Jan 20, 2026

View reviewed changes

ehuss assigned ehuss and unassigned epage Jan 20, 2026

Saplyn added 2 commits January 21, 2026 03:28

test: add tests for optional dependency resolution check

5190ae7

fix: update resolve_all_features() to filter pkg deps

a120d52

Saplyn force-pushed the master branch from 7232f37 to 5190ae7 Compare January 20, 2026 19:47

ehuss approved these changes Jan 21, 2026

View reviewed changes

ehuss added this pull request to the merge queue Jan 21, 2026

weihanglo reviewed Jan 21, 2026

View reviewed changes

Merged via the queue into rust-lang:master with commit 7a34e4f Jan 21, 2026
29 checks passed

rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 21, 2026

weihanglo mentioned this pull request Jan 23, 2026

Update cargo submodule rust-lang/rust#151544

Merged

rustbot added this to the 1.95.0 milestone Jan 24, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: update `resolve_all_features()` to filter pkg deps#16221

fix: update `resolve_all_features()` to filter pkg deps#16221
ehuss merged 2 commits intorust-lang:masterfrom
Saplyn:master

Saplyn commented Nov 7, 2025

Uh oh!

epage commented Nov 7, 2025

Uh oh!

Saplyn commented Nov 8, 2025

Uh oh!

rustbot commented Nov 14, 2025

Uh oh!

ehuss Dec 10, 2025

Uh oh!

Saplyn Dec 12, 2025

Uh oh!

Saplyn Dec 12, 2025

Uh oh!

ehuss left a comment •

edited by rustbot

Loading

Uh oh!

ehuss Jan 20, 2026

Uh oh!

Saplyn Jan 20, 2026

Uh oh!

Saplyn commented Jan 20, 2026 •

edited

Loading

Uh oh!

ehuss left a comment •

edited by rustbot

Loading

Uh oh!

weihanglo Jan 21, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

		p.cargo(&format!("fetch --target={host}")).run();
		p.cargo(&format!("check --target={host} --examples --frozen"))

Conversation

Saplyn commented Nov 7, 2025

What does this PR try to resolve?

How to test and review this PR?

Uh oh!

epage commented Nov 7, 2025

Uh oh!

Saplyn commented Nov 8, 2025

Uh oh!

rustbot commented Nov 14, 2025

Uh oh!

ehuss Dec 10, 2025

Choose a reason for hiding this comment

Uh oh!

Saplyn Dec 12, 2025

Choose a reason for hiding this comment

Uh oh!

Saplyn Dec 12, 2025

Choose a reason for hiding this comment

Uh oh!

ehuss left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ehuss Jan 20, 2026

Choose a reason for hiding this comment

Uh oh!

Saplyn Jan 20, 2026

Choose a reason for hiding this comment

Uh oh!

Saplyn commented Jan 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ehuss left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weihanglo Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ehuss left a comment •

edited by rustbot

Loading

Saplyn commented Jan 20, 2026 •

edited

Loading

ehuss left a comment •

edited by rustbot

Loading