Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: avoid inserting duplicate dylib_path_envvar when calling cargo run recursively #14464

Merged
merged 2 commits into from
Oct 8, 2024
Merged

fix: avoid inserting duplicate dylib_path_envvar when calling cargo run recursively #14464

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e888c13
test: Add test for issue-14194
linyihai Aug 28, 2024
54dbc2b
fix: Avoid inserting `search_path` again.
linyihai Aug 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion src/cargo/core/compiler/compilation.rs
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

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

The reason this approach didn't work is pretty much due to #10358. When Cargo computes fingerprint for a Unit of work, it asks env::var which doesn't contain LIBRARY_PATH or PATH because that is set by Cargo. rerun-if-env-changed only captures environment variables set by users via shell like MY_VAR=1 cargo build. Variables added by Cargo only available when consulting the Command struct of the build script execution via Command::get_envs (like what has done in f91dc78).

As a consequence, if you open the fingerprint JSON file of the build script execution (normally at target/debug/.fingerprint/foo-559e30894cda9925/run-build-script-build-script-build.json), you'll see

{
  "rustc": 11851488446658826000,
  // ...
  "local": [
    {
      "RerunIfEnvChanged": {
        "var": "DYLD_FALLBACK_LIBRARY_PATH",
        "val": null
      }
    }
  ],
  "rustflags": [],
  // ...
}

which shows that Cargo failed to capture the initial library search path set by itself.

There is another way to observe this bug: set the LEVEL env var in the test to a higher number like 5. Any subsequent Cargo invocation after the first one should be fresh. Before doing this verification you need to change std::env!("LEVEL") to std::env::var!("LEVEL") because the former will be captured by rustc dep-info, which trigger a rebuild.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before doing this verification you need to change std::env!("LEVEL") to std::env::var!("LEVEL") because the former will be captured by rustc dep-info, which trigger a rebuild.

I didn't expect such a difference.

I decide to to get the "LEVEL" from the command line argument.

Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
60 changes: 60 additions & 0 deletions tests/testsuite/build.rs
View file
Open in desktop
weihanglo marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
linyihai marked this conversation as resolved.
Show resolved Hide resolved
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() {
Expand Down