Skip to content

Conversation

@Saplyn
Copy link

@Saplyn Saplyn commented Nov 7, 2025

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.

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

epage commented Nov 7, 2025

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
Copy link
Author

Saplyn commented Nov 8, 2025

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

@Saplyn Saplyn force-pushed the master branch 4 times, most recently from 2d2adc0 to 8a99fa6 Compare November 13, 2025 20:36
@Saplyn Saplyn marked this pull request as ready for review November 14, 2025 08:51
@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 14, 2025
@rustbot
Copy link
Collaborator

rustbot commented Nov 14, 2025

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

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() {
Copy link
Contributor

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::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).

Copy link
Author

Choose a reason for hiding this comment

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

  1. I've changed to Package::new() and it now uses dumb-registery
  2. It's now using rust_host() to detect host and the test ignores windows
  3. I could confirm the test fails before the second commit and passes after, both privously and now
  4. It's now moved into features (features::dont_demand_not_required_dep)
  5. 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.

Copy link
Author

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-cargo-targets Area: selection and definition of targets (lib, bins, examples, tests, benches) S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

cargo test requires windows-only dependency on linux

4 participants