Repository: rust-lang/cargo (rust)

Issue:
Cargo appends paths to `PATH` while building without checking if those paths already exist
### Problem

`cargo` appears to be appending paths to the `PATH` environment variable without checking if those paths are already present in it, causing unneeded rebuilds in some situations. I would expect cargo to not add to the path if it doesn't have to.

### Steps

This issue can be reduced to a single file.
1. Create a new cargo project.
2. Enter the following in `main.rs`:
```rust
use std::process::Command;

fn main() {
    for path in std::env::split_paths(&std::env::var("PATH").unwrap()) {
        println!("{}", path.to_string_lossy());
    }

    if std::env::var("EXIT").is_ok() {
        return;
    }

    Command::new("cargo")
        .arg("run")
        .env("EXIT", "1")
        .status()
        .unwrap();
}
```
3. Inspect the printed `PATH` variables. The beginning of the path of the first cargo invocation should be:
```
C:\\Users\\natha\\OneDrive\\Desktop\\cargo-path-duplicate\\target\\debug\\deps
C:\\Users\\natha\\OneDrive\\Desktop\\cargo-path-duplicate\\target\\debug
C:\\Users\\natha\\.rustup\\toolchains\\stable-x86_64-pc-windows-msvc\\lib\\rustlib\\x86_64-pc-windows-msvc\\lib
...rest of PATH
```
While the nested cargo invocation should be:
```
C:\\Users\\natha\\OneDrive\\Desktop\\cargo-path-duplicate\\target\\debug\\deps
C:\\Users\\natha\\OneDrive\\Desktop\\cargo-path-duplicate\\target\\debug
C:\\Users\\natha\\.rustup\\toolchains\\stable-x86_64-pc-windows-msvc\\lib\\rustlib\\x86_64-pc-windows-msvc\\lib
C:\\Users\\natha\\OneDrive\\Desktop\\cargo-path-duplicate\\target\\debug\\deps
C:\\Users\\natha\\OneDrive\\Desktop\\cargo-path-duplicate\\target\\debug
C:\\Users\\natha\\.rustup\\toolchains\\stable-x86_64-pc-windows-msvc\\lib\\rustlib\\x86_64-pc-windows-msvc\\lib
...rest of PATH
```
As can be seen, cargo has added the same 3 paths again to the path, making crates that watch the PATH variable for changes be rebuilt.

### Possible Solution(s)

I'm not sure where cargo inserts these paths, but perhaps just avoiding adding a path if the path occurs within the first 3 paths is sufficient? Another solution could be a hashset over all paths in the `PATH`, but that seems excessive.

### Notes

I'm using an [xtask](https://github.com/matklad/cargo-xtask) setup to run `cargo build`, while running clippy through cargo directly. Switching between xtask and cargo causes a decent number of crates to be rebuilt. This is because `cc`, which a few of my crates use, emits a `cargo:rerun-if-env-changed` directive for `PATH`, which is dirty between cargo and xtask. I would expect a `cargo build` run from within xtask and a `cargo build` run outside of xtask to not need to rebuild each other's artifacts.

As an alternative, I could just add a new command to my xtask setup for clippy. However, I would prefer if I didn't have to wrap each cargo build-like command and `cargo install`-ed subcommand that I want to use.

### Version

```text
cargo 1.79.0 (ffa9cf99a 2024-06-03)
release: 1.79.0
commit-hash: ffa9cf99a594e59032757403d4c780b46dc2c43a
commit-date: 2024-06-03
host: x86_64-pc-windows-msvc
libgit2: 1.7.2 (sys:0.18.3 vendored)
libcurl: 8.6.0-DEV (sys:0.4.72+curl-8.6.0 vendored ssl:Schannel)
os: Windows 10.0.22631 (Windows 11 Professional) [64-bit]
```
```


Pull request:
fix: avoid inserting duplicate `dylib_path_envvar` when calling `cargo run` recursively
### What does this PR try to resolve?

If the current program started by `cargo run` recursively call into `cargo run`, the second `cargo run` will insert  `search_path`  into `dylib_path_envvar` again.

Fixes #14194

### How should we test and review this PR?

The first commit adds the test to reflect the issue. The first call to `cargo run` stores the dylib search path env var to a file. Subsequent calls verify that env var remains the same.

The second commit fixes the behavior by checking if env vars in `search_path` are a prefix of the slice of env vars in `dylib_path_envvar`.



Patch:
diff --git a/Cargo.lock b/Cargo.lock
index 5f83785bb..1c2640bf0 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -1600,9 +1600,9 @@ dependencies = [
 
 [[package]]
 name = "gix-path"
-version = "0.10.11"
+version = "0.10.9"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "ebfc4febd088abdcbc9f1246896e57e37b7a34f6909840045a1767c6dafac7af"
+checksum = "8d23d5bbda31344d8abc8de7c075b3cf26e5873feba7c4a15d916bce67382bd9"
 dependencies = [
  "bstr",
  "gix-trace",
@@ -1776,9 +1776,9 @@ dependencies = [
 
 [[package]]
 name = "gix-trace"
-version = "0.1.10"
+version = "0.1.9"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "6cae0e8661c3ff92688ce1c8b8058b3efb312aba9492bbe93661a21705ab431b"
+checksum = "f924267408915fddcd558e3f37295cc7d6a3e50f8bd8b606cee0808c3915157e"
 
 [[package]]
 name = "gix-transport"
diff --git a/src/cargo/core/compiler/compilation.rs b/src/cargo/core/compiler/compilation.rs
index 8ec625d77..5ecf77d4c 100644
--- a/src/cargo/core/compiler/compilation.rs
+++ b/src/cargo/core/compiler/compilation.rs
@@ -323,7 +323,11 @@ impl<'gctx> Compilation<'gctx> {
 
         let dylib_path = paths::dylib_path();
         let dylib_path_is_empty = dylib_path.is_empty();
-        search_path.extend(dylib_path.into_iter());
+        if dylib_path.starts_with(&search_path) {
+            search_path = dylib_path;
+        } else {
+            search_path.extend(dylib_path.into_iter());
+        }
         if cfg!(target_os = "macos") && dylib_path_is_empty {
             // These are the defaults when DYLD_FALLBACK_LIBRARY_PATH isn't
             // set or set to an empty string. Since Cargo is explicitly setting
diff --git a/src/cargo/core/resolver/features.rs b/src/cargo/core/resolver/features.rs
index 29d90957a..b7f857224 100644
--- a/src/cargo/core/resolver/features.rs
+++ b/src/cargo/core/resolver/features.rs
@@ -319,30 +319,8 @@ impl ResolvedFeatures {
         pkg_id: PackageId,
         features_for: FeaturesFor,
     ) -> Vec<InternedString> {
-        if let Some(res) = self.activated_features_unverified(pkg_id, features_for) {
-            res
-        } else {
-            panic!(
-                "did not find features for ({pkg_id:?}, {features_for:?}) within activated_features:\n{:#?}",
-                self.activated_features.keys()
-            )
-        }
-    }
-
-    /// Variant of `activated_features` that returns `None` if this is
-    /// not a valid pkg_id/is_build combination. Used in places which do
-    /// not know which packages are activated (like `cargo clean`).
-    pub fn activated_features_unverified(
-        &self,
-        pkg_id: PackageId,
-        features_for: FeaturesFor,
-    ) -> Option<Vec<InternedString>> {
-        let fk = features_for.apply_opts(&self.opts);
-        if let Some(fs) = self.activated_features.get(&(pkg_id, fk)) {
-            Some(fs.iter().cloned().collect())
-        } else {
-            None
-        }
+        self.activated_features_int(pkg_id, features_for)
+            .expect("activated_features for invalid package")
     }
 
     /// Returns if the given dependency should be included.
@@ -362,6 +340,30 @@ impl ResolvedFeatures {
             .unwrap_or(false)
     }
 
+    /// Variant of `activated_features` that returns `None` if this is
+    /// not a valid pkg_id/is_build combination. Used in places which do
+    /// not know which packages are activated (like `cargo clean`).
+    pub fn activated_features_unverified(
+        &self,
+        pkg_id: PackageId,
+        features_for: FeaturesFor,
+    ) -> Option<Vec<InternedString>> {
+        self.activated_features_int(pkg_id, features_for).ok()
+    }
+
+    fn activated_features_int(
+        &self,
+        pkg_id: PackageId,
+        features_for: FeaturesFor,
+    ) -> CargoResult<Vec<InternedString>> {
+        let fk = features_for.apply_opts(&self.opts);
+        if let Some(fs) = self.activated_features.get(&(pkg_id, fk)) {
+            Ok(fs.iter().cloned().collect())
+        } else {
+            bail!("features did not find {:?} {:?}", pkg_id, fk)
+        }
+    }
+
     /// Compares the result against the original resolver behavior.
     ///
     /// Used by `cargo fix --edition` to display any differences.


Test patch:
diff --git a/tests/testsuite/artifact_dep.rs b/tests/testsuite/artifact_dep.rs
index f0b32ebb2..c4d1f9ce1 100644
--- a/tests/testsuite/artifact_dep.rs
+++ b/tests/testsuite/artifact_dep.rs
@@ -1573,7 +1573,7 @@ fn artifact_dep_target_specified() {
         .masquerade_as_nightly_cargo(&["bindeps"])
         .with_stdout_data("")
         .with_stderr_data(r#"...
-[..]did not find features for (PackageId { name: "bindep", version: "0.0.0", source: "[..]" }, NormalOrDev) within activated_features:[..]
+[..]activated_features for invalid package: features did not find PackageId { name: "bindep", version: "0.0.0", source: "[..]" } NormalOrDev[..]
 ...
 "#)
         .with_status(101)
diff --git a/tests/testsuite/build.rs b/tests/testsuite/build.rs
index bb13046e5..b7c876154 100644
--- a/tests/testsuite/build.rs
+++ b/tests/testsuite/build.rs
@@ -2154,6 +2154,66 @@ fn crate_library_path_env_var() {
     setenv_for_removing_empty_component(p.cargo("run")).run();
 }
 
+// See https://github.com/rust-lang/cargo/issues/14194
+#[cargo_test]
+fn issue_14194_deduplicate_library_path_env_var() {
+    let p = project()
+        .file(
+            "src/main.rs",
+            &format!(
+                r#"
+                    use std::process::Command;
+                    fn main() {{
+                        let level: i32 = std::env::args().nth(1).unwrap().parse().unwrap();
+                        let txt = "var.txt";
+                        let lib_path = std::env::var("{}").unwrap();
+
+                        // Make sure we really have something in dylib search path.
+                        let count = std::env::split_paths(&lib_path).count();
+                        assert!(count > 0);
+
+                        if level >= 3 {{
+                            std::fs::write(txt, &lib_path).unwrap();
+                        }} else {{
+                            let prev_lib_path = std::fs::read_to_string(txt).unwrap();
+                            // Ensure no duplicate insertion to dylib search paths
+                            // when calling `cargo run` recursively.
+                            assert_eq!(lib_path, prev_lib_path);
+                        }}
+
+                        if level == 0 {{
+                            return;
+                        }}
+                        
+                        let _  = Command::new(std::env!("CARGO"))
+                        .arg("run")
+                        .arg("--")
+                        .arg((level - 1).to_string())
+                        .status()
+                        .unwrap();
+                    }}
+                "#,
+                dylib_path_envvar(),
+            ),
+        )
+        .build();
+
+    setenv_for_removing_empty_component(p.cargo("run -- 3"))
+        .with_stderr_data(str![[r#"
+[COMPILING] foo v0.0.1 ([ROOT]/foo)
+[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s
+[RUNNING] `target/debug/foo[EXE] 3`
+[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s
+[RUNNING] `target/debug/foo[EXE] 2`
+[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s
+[RUNNING] `target/debug/foo[EXE] 1`
+[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s
+[RUNNING] `target/debug/foo[EXE] 0`
+
+"#]])
+        .run();
+}
+
 // Regression test for #4277
 #[cargo_test]
 fn build_with_fake_libc_not_loading() {