Make cargo forward pre-existing CARGO if set #11285

jonhoo · 2022-10-25T19:39:51Z

Currently, Cargo will always set $CARGO to point to what it detects its own path to be (using std::env::current_exe). Unfortunately, this runs into trouble when Cargo is used as a library, or when current_exe is not actually the binary itself (e.g., when invoked through Valgrind or ld.so), since $CARGO will not point at something that can be used as cargo. This, in turn, means that users can't currently rely on $CARGO to do the right thing, and will sometimes have to invoke cargo directly from $PATH instead, which may not reflect the cargo that's currently in use.

This patch makes Cargo re-use the existing value of $CARGO if it's already set in the environment. For Cargo subcommands, this will mean that the initial invocation of cargo in cargo foo will set $CARGO, and then Cargo-as-a-library inside of cargo-foo will inherit that (correct) value instead of overwriting it with the incorrect value cargo-foo. For other execution environments that do not have cargo in their call stack, it gives them the opportunity to set a working value for $CARGO.

One note about the implementation of this is that the test suite now needs to override $CARGO explicitly so that the user's $CARGO does not interfere with the contents of the tests. It could remove $CARGO instead, but overriding it seemed less error-prone.

Fixes #10119.
Fixes #10113.

rust-highfive · 2022-10-25T19:39:54Z

r? @weihanglo

(rust-highfive has picked a reviewer for you, use r? to override)

Currently, Cargo will always set `$CARGO` to point to what it detects its own path to be (using `std::env::current_exe`). Unfortunately, this runs into trouble when Cargo is used as a library, or when `current_exe` is not actually the binary itself (e.g., when invoked through Valgrind or `ld.so`), since `$CARGO` will not point at something that can be used as `cargo`. This, in turn, means that users can't currently rely on `$CARGO` to do the right thing, and will sometimes have to invoke `cargo` directly from `$PATH` instead, which may not reflect the `cargo` that's currently in use. This patch makes Cargo re-use the existing value of `$CARGO` if it's already set in the environment. For Cargo subcommands, this will mean that the initial invocation of `cargo` in `cargo foo` will set `$CARGO`, and then Cargo-as-a-library inside of `cargo-foo` will inherit that (correct) value instead of overwriting it with the incorrect value `cargo-foo`. For other execution environments that do not have `cargo` in their call stack, it gives them the opportunity to set a working value for `$CARGO`. One note about the implementation of this is that the test suite now needs to override `$CARGO` explicitly so that the _user's_ `$CARGO` does not interfere with the contents of the tests. It _could_ remove `$CARGO` instead, but overriding it seemed less error-prone. Fixes rust-lang#10119. Fixes rust-lang#10113.

jonhoo · 2022-10-25T20:52:01Z

I hate Windows paths.. Pushed a test change that should hopefully fix the issue.

jonhoo · 2022-10-25T23:18:04Z

Huzzah, looks like that did it!

arlosi · 2022-10-27T14:51:00Z

src/doc/src/reference/environment-variables.md

@@ -23,6 +23,12 @@ system:
 * `CARGO_TARGET_DIR` — Location of where to place all generated artifacts,
  relative to the current working directory. See [`build.target-dir`] to set
  via config.
+* `CARGO` - If set, Cargo will forward this value instead of setting it
+  to its own auto-detected path when it builds crates and when it
+  executes build scripts and external subcommands. This value is not


Should we include that the value, if set, should be the path to the cargo binary performing the build?

I don't think that's necessary. It may also be confusing phrasing for tools that use Cargo as a library but do not otherwise have a cargo binary to point to (e.g., because they're not cargo subcommands).

weihanglo

Looks good. Thank!

weihanglo · 2022-11-02T12:38:24Z

@bors r+

bors · 2022-11-02T12:38:26Z

📌 Commit 724a197 has been approved by weihanglo

It is now in the queue for this repository.

bors · 2022-11-02T12:38:33Z

⌛ Testing commit 724a197 with merge 352175f...

bors · 2022-11-02T13:27:22Z

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing 352175f to master...

14 commits in 7e484fc1a766f56dbc95380f45719698e0c82749..810cbad9a123ad4ee0a55a96171b8f8478ff1c03 2022-10-27 15:20:57 +0000 to 2022-11-02 21:04:31 +0000 - Update curl-sys (rust-lang/cargo#11326) - Mention fix on build script deadlock (rust-lang/cargo#11325) - Make cargo forward pre-existing CARGO if set (rust-lang/cargo#11285) - Clean up workspace dependencies after cargo remove (rust-lang/cargo#11242) - Update the outdated link for rust-semverver (rust-lang/cargo#11322) - Fix broken link to compilation entry point (rust-lang/cargo#11317) - Only remove fingerprints and build script artifacts of the requested package (rust-lang/cargo#10621) - Newer anyhow features are required (rust-lang/cargo#11316) - Clean stale git temp files (rust-lang/cargo#11308) - Report crate size on package and publish (rust-lang/cargo#11270) - add a note that some warnings (and/or errors) can be auto-fixed (rust-lang/cargo#10989) - Update libcurl (rust-lang/cargo#11307) - artifact deps shoud works when target field specified coexists with `optional = true` (rust-lang/cargo#11183) - Fix singular verb in tests page (rust-lang/cargo#11300)

Update cargo 14 commits in 7e484fc1a766f56dbc95380f45719698e0c82749..810cbad9a123ad4ee0a55a96171b8f8478ff1c03 2022-10-27 15:20:57 +0000 to 2022-11-02 21:04:31 +0000 - Update curl-sys (rust-lang/cargo#11326) - Mention fix on build script deadlock (rust-lang/cargo#11325) - Make cargo forward pre-existing CARGO if set (rust-lang/cargo#11285) - Clean up workspace dependencies after cargo remove (rust-lang/cargo#11242) - Update the outdated link for rust-semverver (rust-lang/cargo#11322) - Fix broken link to compilation entry point (rust-lang/cargo#11317) - Only remove fingerprints and build script artifacts of the requested package (rust-lang/cargo#10621) - Newer anyhow features are required (rust-lang/cargo#11316) - Clean stale git temp files (rust-lang/cargo#11308) - Report crate size on package and publish (rust-lang/cargo#11270) - add a note that some warnings (and/or errors) can be auto-fixed (rust-lang/cargo#10989) - Update libcurl (rust-lang/cargo#11307) - artifact deps shoud works when target field specified coexists with `optional = true` (rust-lang/cargo#11183) - Fix singular verb in tests page (rust-lang/cargo#11300) r? `@ghost`

20 commits in 7e484fc1a766f56dbc95380f45719698e0c82749..9286a1beba5b28b115bad67de2ae91fb1c61eb0b 2022-10-27 15:20:57 +0000 to 2022-11-04 06:41:49 +0000 - chore: Upgrade dependencies (rust-lang/cargo#11328) - Clean more aggressively in CI (rust-lang/cargo#11335) - Remove remove_dir_all (rust-lang/cargo#11333) - test(publish): Cover more wait-for-publish cases (rust-lang/cargo#11327) - Revert rust-lang/cargo#11183 (rust-lang/cargo#11331) - fix(semver-check): adapt to a different error for variant not covered (rust-lang/cargo#11332) - Update curl-sys (rust-lang/cargo#11326) - Mention fix on build script deadlock (rust-lang/cargo#11325) - Make cargo forward pre-existing CARGO if set (rust-lang/cargo#11285) - Clean up workspace dependencies after cargo remove (rust-lang/cargo#11242) - Update the outdated link for rust-semverver (rust-lang/cargo#11322) - Fix broken link to compilation entry point (rust-lang/cargo#11317) - Only remove fingerprints and build script artifacts of the requested package (rust-lang/cargo#10621) - Newer anyhow features are required (rust-lang/cargo#11316) - Clean stale git temp files (rust-lang/cargo#11308) - Report crate size on package and publish (rust-lang/cargo#11270) - add a note that some warnings (and/or errors) can be auto-fixed (rust-lang/cargo#10989) - Update libcurl (rust-lang/cargo#11307) - artifact deps shoud works when target field specified coexists with `optional = true` (rust-lang/cargo#11183) - Fix singular verb in tests page (rust-lang/cargo#11300)

Update cargo 20 commits in 7e484fc1a766f56dbc95380f45719698e0c82749..9286a1beba5b28b115bad67de2ae91fb1c61eb0b 2022-10-27 15:20:57 +0000 to 2022-11-04 06:41:49 +0000 - chore: Upgrade dependencies (rust-lang/cargo#11328) - Clean more aggressively in CI (rust-lang/cargo#11335) - Remove remove_dir_all (rust-lang/cargo#11333) - test(publish): Cover more wait-for-publish cases (rust-lang/cargo#11327) - Revert rust-lang/cargo#11183 (rust-lang/cargo#11331) - fix(semver-check): adapt to a different error for variant not covered (rust-lang/cargo#11332) - Update curl-sys (rust-lang/cargo#11326) - Mention fix on build script deadlock (rust-lang/cargo#11325) - Make cargo forward pre-existing CARGO if set (rust-lang/cargo#11285) - Clean up workspace dependencies after cargo remove (rust-lang/cargo#11242) - Update the outdated link for rust-semverver (rust-lang/cargo#11322) - Fix broken link to compilation entry point (rust-lang/cargo#11317) - Only remove fingerprints and build script artifacts of the requested package (rust-lang/cargo#10621) - Newer anyhow features are required (rust-lang/cargo#11316) - Clean stale git temp files (rust-lang/cargo#11308) - Report crate size on package and publish (rust-lang/cargo#11270) - add a note that some warnings (and/or errors) can be auto-fixed (rust-lang/cargo#10989) - Update libcurl (rust-lang/cargo#11307) - artifact deps shoud works when target field specified coexists with `optional = true` (rust-lang/cargo#11183) - Fix singular verb in tests page (rust-lang/cargo#11300) r? `@ghost`

rust-highfive assigned weihanglo Oct 25, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 25, 2022

jonhoo mentioned this pull request Oct 25, 2022

CARGO is not Cargo when build script is invoked through cargo used as a library #10119

Closed

jonhoo force-pushed the cargo-env branch from 9b41df9 to 724a197 Compare October 25, 2022 20:51

arlosi reviewed Oct 27, 2022

View reviewed changes

weihanglo approved these changes Nov 2, 2022

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 2, 2022

bors merged commit 352175f into rust-lang:master Nov 2, 2022

bors mentioned this pull request Nov 2, 2022

Implement RFC 3139: alternative registry authentication support #10592

Merged

weihanglo mentioned this pull request Nov 2, 2022

Update cargo rust-lang/rust#103860

Merged

jonhoo deleted the cargo-env branch November 3, 2022 18:58

weihanglo mentioned this pull request Nov 4, 2022

Update cargo rust-lang/rust#103992

Merged

Muscraft mentioned this pull request Nov 21, 2022

fix: Make auto-fix note work with clippy #11399

Merged

ehuss added this to the 1.67.0 milestone Mar 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make cargo forward pre-existing CARGO if set #11285

Make cargo forward pre-existing CARGO if set #11285

jonhoo commented Oct 25, 2022

rust-highfive commented Oct 25, 2022

jonhoo commented Oct 25, 2022

jonhoo commented Oct 25, 2022

arlosi Oct 27, 2022

jonhoo Oct 31, 2022

weihanglo left a comment

weihanglo commented Nov 2, 2022

bors commented Nov 2, 2022

bors commented Nov 2, 2022

bors commented Nov 2, 2022

Make cargo forward pre-existing CARGO if set #11285

Make cargo forward pre-existing CARGO if set #11285

Conversation

jonhoo commented Oct 25, 2022

rust-highfive commented Oct 25, 2022

jonhoo commented Oct 25, 2022

jonhoo commented Oct 25, 2022

arlosi Oct 27, 2022

Choose a reason for hiding this comment

jonhoo Oct 31, 2022

Choose a reason for hiding this comment

weihanglo left a comment

Choose a reason for hiding this comment

weihanglo commented Nov 2, 2022

bors commented Nov 2, 2022

bors commented Nov 2, 2022

bors commented Nov 2, 2022