fix(install): filter out binaries not included by selected features by bcully · Pull Request #17544 · rust-lang/cargo

bcully · 2026-10-01T06:17:14Z

What does this PR try to resolve?

Cargo install was computing all possible binaries for a package regardless of the requested features, and would conclude that the installed package was out-of-date if it hadn't installed all binaries, even if the requested features wouldn't actually produce any of the "missing" binaries.

For example, I found this bug because every time I ran cargo install jj-cli it would rebuild the package. This was because jj-cli can build some optional test/example binaries, but those aren't part of the default feature.

How to test and review this PR?

I've added a new test, and also manually checked cargo install jj-cli doesn't rebuild the package on the second install.

rustbot · 2026-10-02T15:31:22Z

Thanks for the pull request, and welcome! The Rust Project has assigned @weihanglo (or someone else) to review your changes, you should hear from them (or someone else) within the next two weeks.

Please see the contribution instructions and our LLM policy for more information.

Why was this reviewer chosen?

The reviewer was selected based on:

Owners of files modified in this PR: @Muscraft, @epage, @weihanglo
@Muscraft, @epage, @weihanglo expanded to Muscraft, epage, weihanglo
Random selection from Muscraft, epage, weihanglo

epage · 2026-10-06T14:21:04Z

r? @Muscraft

Muscraft

Thanks for working on this, and sorry for the long delay in reviewing it. I am slightly unfamiliar with this part of the codebase and have been trying to wrap my brain around these changes.

I think this is a good start, but using only the command-line features for this runs into the issues with dep_name/feat_name that @ehuss brought up in the original issue. From what I can tell, the reason using only the command-line features won't work is that it doesn't take into account features from things like the [dependencies] table, whereas resolve_all_features does. I am not quite sure how we should go about getting a "real" resolve in this case, but it might be good to look into whether there is a way to reuse a resolve from another part of cargo.

View changes since this review

Muscraft · 2026-10-12T23:12:00Z

+    let check_install = |arg_line, expected_bins| {
+        cargo_process(arg_line).run();
+        validate_trackers("foo", "1.0.0", expected_bins);
+
+        cargo_process(arg_line)
+            .with_stderr_data(str![[r#"
+...
+[IGNORED] package `foo v1.0.0` is already installed, use --force to override
+...
+"#]])
+            .run();
+    };


Having the same str![[r#""#]] inside the closure can break the automatic updating of snapshot tests, as far as I remember. It would be better if each had its own.

Do you mean, make check_install take three arguments, like

check_install("install --no-default-features foo", &["foo"], str![[r#" ... [IGNORED] package `foo v1.0.0` is already installed, use --force to override ... "#]]); check_install("install foo", &["foo", "foo-feat"], str![[r#" ... [IGNORED] package `foo v1.0.0` is already installed, use --force to override ... "#]]);

Is there any way to avoid this repetition? str![[]] seems to want only a literal.

Muscraft · 2026-10-12T23:23:01Z

+        [[bin]]
+        name = "foo-e2"
+        path = "src/bin/foo-e2.rs"
+        required-features = ["e2"]


We should probably have a test case for dep_name/feat_name somewhere

I'll try to get one going. My first attempt produced confusing errors, where the features I defined in "dep" didn't seem to be visible in "foo".

I just missed that you have to separately declare features with Package::feature. I've added coverage for dep/feat and dep?/feat. Maybe this also helps to clarify the confusion I have about what extra resolution we need for dep/feature syntax?

Muscraft · 2026-10-12T23:40:10Z

 }
+
+#[cargo_test]
+fn check_upgrade_respects_features() {


We prefer it if new tests are in their own commit before any code changes so that it is easier to see the effect of the code changes. The C-TEST heading on @epage's PR Style Guidelines describes the workflow.

Muscraft · 2026-10-12T23:45:09Z

+            if weak {
+                None
+            } else {
+                Some(dep_name.as_str())


Using only dep_name here throws away the feature portion of dep/feature (/feature)

I mentioned below, I'm afraid I don't understand the problem. It seemed to me that only the "dep" part would potentially trigger a new binary in the install target.

bcully · 2026-10-16T04:09:51Z

Thanks for the review. I'll do some of the restructuring (make a passing test for the current behaviour first) and see if I can get a test case for dep/featurename as well (I tested it manually but ran into some issues adding a test case the first time). I assume that it is fine to force-push this PR to rewrite its history based on the style comment.

But, I have to admit, I still do not understand

From what I can tell, the reason using only the command-line features won't work is that it doesn't take into account features from things like the [dependencies] table, whereas resolve_all_features does.

Would it be possible to draw up a concrete example of the problem? In my mind, the binaries produced by cargo install depend only on the features selected in the package to be installed. The selected features may get augmented by some implicit dep/feature syntax, which I account for, but my imagination is failing beyond that. Along the same lines, I didn't understand

Using only dep_name here throws away the feature portion of dep/feature (/feature)

As I read it, only the dep part was going to affect which binaries would be selected for installation. So it would be very helpful if you could illustrate some concrete dep/feature topology that this PR isn't handling, just because I'm having trouble picturing it myself.

Demonstrate the existing cargo install behaviour when some binaries are not produced by the features used for the build. In this case, install of an already-installed package that doesn't provide all possible binaries will rebuild and reinstall, even if the second install won't produce any additional binaries. References rust-lang#8703

Cargo install was computing all possible binaries for a package regardless of the requested features, and would conclude that the installed package was out-of-date if it hadn't installed all binaries, even if the requested features wouldn't actually produce any of the "missing" binaries. Fixes rust-lang#8703

rustbot · 2026-10-17T17:07:16Z

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

rustbot added the Command-install label Oct 1, 2026

bcully mentioned this pull request Oct 1, 2026

cargo rebuilds every time when installing a crate with binaries behind feature gates #8703

Open

bcully marked this pull request as ready for review October 2, 2026 15:31

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 2, 2026

rustbot assigned weihanglo Oct 2, 2026

rustbot assigned Muscraft and unassigned weihanglo Oct 6, 2026

Muscraft reviewed Oct 12, 2026

View reviewed changes

bcully force-pushed the push-rqlrwyyvpyzu branch from 98397f8 to f3ba730 Compare October 16, 2026 05:31

This comment has been minimized.

Sign in to view

bcully added 2 commits October 17, 2026 10:04

bcully force-pushed the push-rqlrwyyvpyzu branch from f3ba730 to 3912fb4 Compare October 17, 2026 17:07

Conversation

bcully commented Oct 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR try to resolve?

How to test and review this PR?

Uh oh!

rustbot commented Oct 2, 2026

Uh oh!

epage commented Oct 6, 2026

Uh oh!

Muscraft 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bcully commented Oct 16, 2026

Uh oh!

This comment has been minimized.

rustbot commented Oct 17, 2026

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

bcully commented Oct 1, 2026 •

edited

Loading

Muscraft left a comment •

edited by rustbot

Loading