Warn about `CARGO` environment variable in cargo proxy #4175

smoelius · 2025-02-03T20:05:30Z

Addresses rust-lang/clippy#15099

To recount the problem (copied from rust-lang/cargo#15099 (comment)):

The cargo proxy runs, selects the default cargo, and sets the CARGO environment variable to default toolchain's cargo.
That cargo invocations runs some other code, which invokes cargo +<toolchain> <subcommand>.
<subcommand> reads the CARGO environment variable and runs the default toolchain's cargo rather than <toolchain>'s cargo.

This PR addresses the problem by warning when the CARGO environment variable does not match what cargo +<toolchain> <subcommand> will run.

This PR is currently organized as eight commits:

The first commit applies the fix.
The next five commits add infrastructure to support the seventh commit.
The seventh commit adds a test to address all scenarios in $CARGO passed to external subcommands can point to the wrong cargo if already set cargo#15099 (comment).
The eighth commit is a small bug fix to Config::cmd.
The ninth commit incorporates feedback from $CARGO passed to external subcommands can point to the wrong cargo if already set cargo#15099 (comment) and onward.

Nits are welcome.

smoelius · 2025-02-28T17:08:48Z

The issue was resolved in the Cargo repository, making this PR moot.

However, I humbly consider some of the PR's changes to be improvements to the testing infrastructure. For example, I think 6059cb1 (Do not append EXE_SUFFIX in Config::cmd) is a genuine bugfix

Should I submit any of the commits as separate PRs?

djc · 2025-02-28T17:48:11Z

Should I submit any of the commits as separate PRs?

Yes, please!

rami3l · 2025-03-01T09:57:54Z

@smoelius Thanks for the work! I'm interested in extracted PRs as well.

PS: Following the previous discussion, I'm now converting this PR to draft to prevent accidental merges.

Previously, `Config::cmd` would always append an `EXE_SUFFIX`. However, this would produce an invalid path if the suffix was already present. Extracted from rust-lang#4175

`SanitizedOutput`s have `String` `stdout` and `stderr` fields. This makes `SanitizedOutput`s easier to display than `Output`s. This commit adds a `TryFrom<Output>` implementation for `SanitizedOutput`. In this way, a test can run a command to produce an `Output, and from that `Output` produce a `SanitizedOutput`. Extracted from rust-lang#4175

The function `Config::run_subprocess` takes a program name and a list of arguments, constructs a command from them, and then runs the command. The function this commit adds is similar, but it accepts an already constructed command. Extracted from rust-lang#4175

Currently, when the mocked cargo command is passed `--recursive-cargo-subcommand`, it runs: ``` cargo-foo --recursive-cargo ``` However, cargo subcommands are normally passed their subcommand name as the first argument. Thus, one would expect the following to be run: ``` cargo-foo foo --recursive-cargo ``` This commit changes the mocked cargo command to do the latter. It also adds a check to ensure that the "subcommand name as first argument" behavior is respected. This is, admittedly, a rather pedantic change. Extracted from rust-lang#4175

`SanitizedOutput`s have `String` `stdout` and `stderr` fields. This makes `SanitizedOutput`s easier to display than `Output`s. This commit adds a `TryFrom<Output>` implementation for `SanitizedOutput`. In this way, a test can run a command to produce an `Output`, and from that `Output` produce a `SanitizedOutput`. Extracted from rust-lang#4175

`SanitizedOutput`s have `String` `stdout` and `stderr` fields. This makes `SanitizedOutput`s easier to display than `Output`s. This commit adds a `TryFrom<Output>` implementation for `SanitizedOutput`. In this way, a test can run a command to produce an `Output`, and from that `Output` produce a `SanitizedOutput`. Extracted from #4175

Currently, when the mocked cargo command is passed `--recursive-cargo-subcommand`, it runs: ``` cargo-foo --recursive-cargo ``` However, cargo subcommands are normally passed their subcommand name as the first argument. Thus, one would expect the following to be run: ``` cargo-foo foo --recursive-cargo ``` This commit changes the mocked cargo command to do the latter. It also adds a check to ensure that the "subcommand name as first argument" behavior is respected. This is, admittedly, a rather pedantic change. Extracted from #4175

Previously, `Config::cmd` would always append an `EXE_SUFFIX`. However, this would produce an invalid path if the suffix was already present. Extracted from #4175

Currently, when the mocked cargo command is passed `--recursive-cargo-subcommand`, it runs: ``` cargo-foo --recursive-cargo ``` However, cargo subcommands are normally passed their subcommand name as the first argument. Thus, one would expect the following to be run: ``` cargo-foo foo --recursive-cargo ``` This commit changes the mocked cargo command to do the latter. It also adds a check to ensure that the "subcommand name as first argument" behavior is respected. This is, admittedly, a rather pedantic change. Extracted from #4175

smoelius added 7 commits February 3, 2025 11:16

Fix rust-lang/clippy#15099

856ec19

Add TryFrom<Output> for SanitizedOutput

6b991f5

Add Config::run_subprocess_cmd

710d59f

Add clitools::create_cargo_foo

a0b204e

Have mocked cargo better adhere to cargo conventions

e831496

Add --recursive-cargo-without-toolchain option to mocked cargo

e80b0ba

Add test for rust-lang/clippy#15099 fix

2d94925

smoelius marked this pull request as draft February 3, 2025 20:54

Do not append EXE_SUFFIX in Config::cmd

6059cb1

smoelius marked this pull request as ready for review February 4, 2025 01:11

smoelius mentioned this pull request Feb 4, 2025

$CARGO passed to external subcommands can point to the wrong cargo if already set rust-lang/cargo#15099

Closed

Warn but don't clear CARGO env var

5d5776c

smoelius changed the title ~~Clear CARGO environment variable in cargo proxy~~ Warn about CARGO environment variable in cargo proxy Feb 6, 2025

rami3l marked this pull request as draft March 1, 2025 09:57

This was referenced Mar 1, 2025

Do not append EXE_SUFFIX in Config::cmd #4200

Merged

Add TryFrom<Output> for SanitizedOutput #4201

Merged

Add Config::run_subprocess_cmd #4202

Closed

Have mocked cargo better adhere to cargo conventions #4203

Merged

smoelius closed this Mar 1, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Warn about `CARGO` environment variable in cargo proxy #4175

Warn about `CARGO` environment variable in cargo proxy #4175

smoelius commented Feb 3, 2025 •

edited

Loading

smoelius commented Feb 28, 2025

djc commented Feb 28, 2025

rami3l commented Mar 1, 2025 •

edited

Loading

Warn about CARGO environment variable in cargo proxy #4175

Warn about CARGO environment variable in cargo proxy #4175

Conversation

smoelius commented Feb 3, 2025 • edited Loading

smoelius commented Feb 28, 2025

djc commented Feb 28, 2025

rami3l commented Mar 1, 2025 • edited Loading

Warn about `CARGO` environment variable in cargo proxy #4175

Warn about `CARGO` environment variable in cargo proxy #4175

smoelius commented Feb 3, 2025 •

edited

Loading

rami3l commented Mar 1, 2025 •

edited

Loading