Skip to content

fix: reset $CARGO if the running program is real cargo[.exe] #15208

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.

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

Merged
merged 2 commits into from
Feb 28, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
20 changes: 18 additions & 2 deletions src/cargo/util/context/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -493,9 +493,25 @@ impl GlobalContext {
paths::resolve_executable(&argv0)
}

// Determines whether `path` is a cargo binary.
// See: https://github.com/rust-lang/cargo/issues/15099#issuecomment-2666737150
fn is_cargo(path: &Path) -> bool {
path.file_stem() == Some(OsStr::new("cargo"))
}

let from_current_exe = from_current_exe();
if from_current_exe.as_deref().is_ok_and(is_cargo) {
return from_current_exe;
}

let from_argv = from_argv();
if from_argv.as_deref().is_ok_and(is_cargo) {
return from_argv;
}

let exe = from_env()
.or_else(|_| from_current_exe())
.or_else(|_| from_argv())
.or(from_current_exe)
.or(from_argv)
.context("couldn't get the path to cargo executable")?;
Ok(exe)
})
Expand Down
89 changes: 88 additions & 1 deletion tests/testsuite/cargo_command.rs
Original file line number Diff line number Diff line change
Expand Up @@ -391,10 +391,13 @@ fn cargo_subcommand_env() {
.canonicalize()
.unwrap();
let envtest_bin = envtest_bin.to_str().unwrap();
// Previously, `$CARGO` would be left at `envtest_bin`. However, with the
Copy link
Member

Choose a reason for hiding this comment

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

Seeing this test change and I wonder if using $CARGO as a Cargo wrapper is a supported use case, and we don't want to reset it.

Maybe we can detect iff the current $CARGO is cargo[.exe] and then we reset?

Copy link
Member

Choose a reason for hiding this comment

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

cc @epage as you proposed the current solution

Copy link
Contributor Author

@smoelius smoelius Feb 23, 2025

Choose a reason for hiding this comment

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

I have no problem with implementing this if @epage agrees.

Copy link
Contributor

Choose a reason for hiding this comment

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

CARGO was originally not intended for purposes of being a wrapper. That people could do it was a byproduct of the implementation that was done but it wasn't intentional. I wouldn't worry too much about it.

Copy link
Member

Choose a reason for hiding this comment

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

That is fair. Thanks Ed.

// fix for #15099, `$CARGO` is now overwritten with the path to the current
// exe when it is detected to be a cargo binary.
cargo_process("envtest")
.env("PATH", &path)
.env(cargo::CARGO_ENV, &envtest_bin)
.with_stdout_data(format!("{}\n", envtest_bin).raw().raw())
.with_stdout_data(format!("{}\n", cargo.display()).raw())
.run();
}

Expand Down Expand Up @@ -570,3 +573,87 @@ fn full_did_you_mean() {
"#]])
.run();
}

#[cargo_test]
fn overwrite_cargo_environment_variable() {
// If passed arguments `arg1 arg2 ...`, this program runs them as a command.
// If passed no arguments, this program simply prints `$CARGO`.
let p = project()
.file("Cargo.toml", &basic_manifest("foo", "1.0.0"))
.file(
"src/main.rs",
r#"
fn main() {
let mut args = std::env::args().skip(1);
if let Some(arg1) = args.next() {
let status = std::process::Command::new(arg1)
.args(args)
.status()
.unwrap();
assert!(status.success());
} else {
eprintln!("{}", std::env::var("CARGO").unwrap());
}
}
"#,
)
.build();

// Create two other cargo binaries in the project root, one with the wrong
// name and one with the right name.
let cargo_exe = cargo_test_support::cargo_exe();
let wrong_name_path = p
.root()
.join(format!("wrong_name{}", env::consts::EXE_SUFFIX));
let other_cargo_path = p.root().join(cargo_exe.file_name().unwrap());
std::fs::hard_link(&cargo_exe, &wrong_name_path).unwrap();
std::fs::hard_link(&cargo_exe, &other_cargo_path).unwrap();

// The output of each of the following commands should be `path-to-cargo`:
// ```
// cargo run
// cargo run -- cargo run
// cargo run -- wrong_name run
// ```

let cargo = cargo_exe.display().to_string();
let wrong_name = wrong_name_path.display().to_string();
let stderr_cargo = format!(
"{}[EXE]\n",
cargo_exe
.canonicalize()
.unwrap()
.with_extension("")
.to_str()
.unwrap()
);

for cmd in [
"run",
&format!("run -- {cargo} run"),
&format!("run -- {wrong_name} run"),
] {
p.cargo(cmd).with_stderr_contains(&stderr_cargo).run();
}

// The output of the following command should be `path-to-other-cargo`:
// ```
// cargo run -- other_cargo run
// ```

let other_cargo = other_cargo_path.display().to_string();
let stderr_other_cargo = format!(
"{}[EXE]\n",
other_cargo_path
.canonicalize()
.unwrap()
.with_extension("")
.to_str()
.unwrap()
.replace(p.root().parent().unwrap().to_str().unwrap(), "[ROOT]")
);

p.cargo(&format!("run -- {other_cargo} run"))
.with_stderr_contains(stderr_other_cargo)
.run();
}
22 changes: 11 additions & 11 deletions tests/testsuite/test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3909,22 +3909,22 @@ test env_test ... ok
.run();

// Check that `cargo test` propagates the environment's $CARGO
let rustc = cargo_util::paths::resolve_executable("rustc".as_ref())
.unwrap()
.canonicalize()
.unwrap();
let stderr_rustc = format!(
let cargo_exe = cargo_test_support::cargo_exe();
let other_cargo_path = p.root().join(cargo_exe.file_name().unwrap());
std::fs::hard_link(&cargo_exe, &other_cargo_path).unwrap();
let stderr_other_cargo = format!(
"{}[EXE]",
rustc
other_cargo_path
.canonicalize()
.unwrap()
.with_extension("")
.to_str()
.unwrap()
.replace(rustc_host, "[HOST_TARGET]")
.replace(p.root().parent().unwrap().to_str().unwrap(), "[ROOT]")
);
p.cargo("test --lib -- --nocapture")
// we use rustc since $CARGO is only used if it points to a path that exists
.env(cargo::CARGO_ENV, rustc)
.with_stderr_contains(stderr_rustc)
p.process(other_cargo_path)
.args(&["test", "--lib", "--", "--nocapture"])
.with_stderr_contains(stderr_other_cargo)
.with_stdout_data(str![[r#"
...
test env_test ... ok
Expand Down