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

[hampuslidin]

Problem

Whenever a crate containing multiple binaries, where some of them are behind a feature gate, performing a cargo install <crate-name> will cause a rebuild every time, instead of ignoring the rebuild. This is prevalent for nushell, which has several binaries behind the extra feature, but I was able to reproduce this in a simple project, as described below.

Steps

1. Run cargo new --bin cargo_rebuild_bug && cd cargo_rebuild_bug.
2. Add file src/optional/optional.rs:

fn main() {
    println!("This is an optional binary");
}

3. Modify Cargo.toml:

[package]
name = "cargo_rebuild_bug"
version = "0.1.0"
authors = ["An Author <an.author@example.com>"]
edition = "2018"

[features]
optional = []

[[bin]]
name = "optional"
path = "src/optional/optional.rs"
required-features = ["optional"]

[[bin]]
name = "cargo_rebuild_bug"
path = "src/main.rs"

4. Since local builds will reinstall every time, since you will need to run cargo install --path ., I modified the cargo crate source code to investigate what cargo think is different in comparison to the previously stored tracking information. I applied the following git patch:

diff --git a/src/cargo/ops/common_for_install_and_uninstall.rs b/src/cargo/ops/common_for_install_and_uninstall.rs
index 9f0c5dd20..5cb05beb3 100644
--- a/src/cargo/ops/common_for_install_and_uninstall.rs
+++ b/src/cargo/ops/common_for_install_and_uninstall.rs
@@ -163,6 +163,7 @@ impl InstallTracker {
         _rustc: &str,
     ) -> CargoResult<(Freshness, BTreeMap<String, Option<PackageId>>)> {
         let exes = exe_names(pkg, &opts.filter);
+        println!("exes: {:#?}\n", exes);
         // Check if any tracked exe's are already installed.
         let duplicates = self.find_duplicates(dst, &exes);
         if force || duplicates.is_empty() {
@@ -220,6 +221,15 @@ impl InstallTracker {
             if matching_duplicates.iter().all(is_up_to_date) {
                 Ok((Freshness::Fresh, duplicates))
             } else {
+                matching_duplicates.iter().for_each(|dupe_pkg_id| {
+                    let info = self
+                        .v2
+                        .installs
+                        .get(dupe_pkg_id)
+                        .expect("dupes must be in sync");
+
+                    println!("Dupe {} exes: {:#?}\n", dupe_pkg_id, info.bins);
+                });
                 Ok((Freshness::Dirty, duplicates))
             }
         } else {

5. Run cargo run -- install nu in the modified cargo crate. The first time I run it, it will install the nu package as usual, but if I run it a second time after that, it rebuilds again and receives the following output:

❯ cargo run -- install nu
    Blocking waiting for file lock on build directory
   Compiling cargo v0.49.0 (/Users/lidin/Documents/Projekt/Github)
    Finished dev [unoptimized + debuginfo] target(s) in 30.90s
     Running `target/debug/cargo install nu`
    Updating crates.io index
exes: {
    "nu",
    "nu_plugin_core_fetch",
    "nu_plugin_core_inc",
    "nu_plugin_core_match",
    "nu_plugin_core_post",
    "nu_plugin_core_ps",
    "nu_plugin_core_sys",
    "nu_plugin_core_textview",
    "nu_plugin_extra_binaryview",
    "nu_plugin_extra_s3",
    "nu_plugin_extra_start",
    "nu_plugin_extra_tree",
}

Dupe nu v0.19.0 exes: {
    "nu",
    "nu_plugin_core_fetch",
    "nu_plugin_core_inc",
    "nu_plugin_core_match",
    "nu_plugin_core_post",
    "nu_plugin_core_ps",
    "nu_plugin_core_sys",
    "nu_plugin_core_textview",
}

Dupe nu v0.19.0 exes: {
    "nu",
    "nu_plugin_core_fetch",
    "nu_plugin_core_inc",
    "nu_plugin_core_match",
    "nu_plugin_core_post",
    "nu_plugin_core_ps",
    "nu_plugin_core_sys",
    "nu_plugin_core_textview",
}

Dupe nu v0.19.0 exes: {
    "nu",
    "nu_plugin_core_fetch",
    "nu_plugin_core_inc",
    "nu_plugin_core_match",
    "nu_plugin_core_post",
    "nu_plugin_core_ps",
    "nu_plugin_core_sys",
    "nu_plugin_core_textview",
}

Dupe nu v0.19.0 exes: {
    "nu",
    "nu_plugin_core_fetch",
    "nu_plugin_core_inc",
    "nu_plugin_core_match",
    "nu_plugin_core_post",
    "nu_plugin_core_ps",
    "nu_plugin_core_sys",
    "nu_plugin_core_textview",
}

Dupe nu v0.19.0 exes: {
    "nu",
    "nu_plugin_core_fetch",
    "nu_plugin_core_inc",
    "nu_plugin_core_match",
    "nu_plugin_core_post",
    "nu_plugin_core_ps",
    "nu_plugin_core_sys",
    "nu_plugin_core_textview",
}

Dupe nu v0.19.0 exes: {
    "nu",
    "nu_plugin_core_fetch",
    "nu_plugin_core_inc",
    "nu_plugin_core_match",
    "nu_plugin_core_post",
    "nu_plugin_core_ps",
    "nu_plugin_core_sys",
    "nu_plugin_core_textview",
}

Dupe nu v0.19.0 exes: {
    "nu",
    "nu_plugin_core_fetch",
    "nu_plugin_core_inc",
    "nu_plugin_core_match",
    "nu_plugin_core_post",
    "nu_plugin_core_ps",
    "nu_plugin_core_sys",
    "nu_plugin_core_textview",
}

Dupe nu v0.19.0 exes: {
    "nu",
    "nu_plugin_core_fetch",
    "nu_plugin_core_inc",
    "nu_plugin_core_match",
    "nu_plugin_core_post",
    "nu_plugin_core_ps",
    "nu_plugin_core_sys",
    "nu_plugin_core_textview",
}

  Installing nu v0.19.0
   Compiling libc v0.2.77
   Compiling proc-macro2 v1.0.21
   Compiling autocfg v1.0.1
...

It seems like when exe_names in check_upgrade in common_for_install_and_uninstall.rs generates the binary names, it includes all binaries, even the ones behind the feature gate.

6. Running cargo run -- install --path /path/to/cargo_rebuild_bug also reveals the extraneous package names:

❯ cargo run -- install --path ../cargo_rebuild_bug
    Finished dev [unoptimized + debuginfo] target(s) in 0.08s
     Running `target/debug/cargo install --path ../cargo_rebuild_bug`
exes: {
    "cargo_rebuild_bug",
    "optional",
}
  Installing cargo_rebuild_bug v0.1.0 (/Users/lidin/Documents/Projekt/Github/../cargo_rebuild_bug)
    Finished release [optimized] target(s) in 0.03s
exes: {
    "cargo_rebuild_bug",
    "optional",
}
   Replacing /Users/lidin/.cargo/bin/cargo_rebuild_bug
    Replaced package `cargo_rebuild_bug v0.1.0 (/Users/lidin/Documents/Projekt/cargo_rebuild_bug)` with `cargo_rebuild_bug v0.1.0 (/Users/lidin/Documents/Projekt/Github/../cargo_rebuild_bug)` (executable `cargo_rebuild_bug`)

Possible Solution(s)

exe_names should not only check if the target is a bin (for when the CompileFilter is Default), but also if it should be included given the current feature flags passed to the install subcommand. This is at least the only change I think is needed for cargo to realize that the packages are the same.

Or, this is a misconfiguration on nushells part, and there is perhaps a better way to specify binaries behind feature gates?

I could give a go implementing the first suggestion and create a PR for it, but I have not worked in this project before, so I thought I'd first create this issue. :)

Notes

Output of cargo version:

cargo 1.46.0 (149022b1d 2020-07-17)

[hampuslidin]

A rough POC for a bug fix can be found in this gist.

[ehuss]

Yes, I think your analysis is correct, exe_names should not include targets that are skipped due to features. It might be a little tricky to fix. resolve_ws_with_opts can trigger downloads, which is a drawback. If it does go down that route, the resolution should be shared with check_yanked_install. I'm trying to think of some way to avoid that. It could just use the features passed on the command-line, but that would not work for dep_name/feat_name syntax. 😦 Another option is to add a new field to InstallInfo that specifies which bins/examples were specified on the command-line, and use that to determine which were installed. Backwards compatibility might be a little tricky there.

[hampuslidin]

Hmm, I read you. My POC was only for checking if checking for activated features was enough to solve the problem, but as you say there are likely better ways to get the information. Do tell if I can help out with this!