Skip to content
New issue

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

#

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

Already on GitHub? # to your account

Jump to bottom

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

Merged
merged 2 commits into from
Feb 28, 2025
Merged

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

merged 2 commits into from
Feb 28, 2025

Conversation

smoelius
Copy link
Contributor

Overwrite $CARGO when the current exe is detected to be a cargo binary.

From #15099 (comment):

opt-in behavior (set by cargo-the-bin):

  1. from_current_exe if its cargo[EXE_SUFFIX]
  2. from_argv if its cargo[EXE_SUFFIX]
  3. from_env
  4. from_current_exe
  5. from_argv

r? @epage

@rustbot rustbot added A-configuration Area: cargo config files and env vars S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 20, 2025
@smoelius
Copy link
Contributor Author

I'll fix the errors shortly.

@smoelius smoelius marked this pull request as draft February 20, 2025 11:45
weihanglo
weihanglo reviewed Feb 20, 2025
View reviewed changes
Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

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

Thanks for the patch!

Please allow me time to test against the dynamic linker use case.

@weihanglo weihanglo changed the title Fix #15099 fix: reset $CARGO if the running program is real cargo[.exe] Feb 20, 2025
@smoelius smoelius force-pushed the fix-15099 branch 2 times, most recently from 9c65e60 to 2c7e098 Compare February 20, 2025 18:14
@smoelius
Copy link
Contributor Author

Just because I am curious, is the reason for the recent CI failures known?

@epage
Copy link
Contributor

epage commented Feb 21, 2025

They were fixed in #15211

@smoelius
Copy link
Contributor Author

Oh, interesting! Thank you for explaining!

@smoelius smoelius force-pushed the fix-15099 branch 2 times, most recently from 0a8a248 to 0ce3f9e Compare February 21, 2025 18:12
@smoelius
Fix rust-lang#15099
44970a8 
Overwrite `$CARGO` when the current exe is detected to be a cargo
binary.

See: rust-lang#15099 (comment)
@smoelius smoelius marked this pull request as ready for review February 21, 2025 21:04
@smoelius
Copy link
Contributor Author

smoelius commented Feb 21, 2025 •
edited
Loading

If I'm interpreting the results right, path normalization is applied to stderr but not stdout?

  • EDIT: I did some experiments, and that seems to be how snapbox works?

Anyway, the PR should be ready for review.

weihanglo
weihanglo reviewed Feb 23, 2025
View reviewed changes
Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

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

Did manual tests for ld wrapper from what Joo's has described in #10113. This seems working!

One thing I am not sure is this characteristic:

  • We don't fix this issue for when users have an alternative name for the Cargo binary (or on some platforms, an alternative symlink name)

Should we document our heuristic rule in the CARGO env doc? I feel like it might be excessive and don't want to wait for a perfect doc to merge this fix.

weihanglo
weihanglo reviewed Feb 23, 2025
View reviewed changes
tests/testsuite/cargo_command.rs
@@ -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 •
edited
Loading

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.

@smoelius
Copy link
Contributor Author

One thing I am not sure is this characteristic:

  • We don't fix this issue for when users have an alternative name for the Cargo binary (or on some platforms, an alternative symlink name)

Should we document our heuristic rule in the CARGO env doc? I feel like it might be excessive and don't want to wait for a perfect doc to merge this fix.

I would just point out that the current description of each variable is light on details: https://doc.rust-lang.org/cargo/reference/environment-variables.html#environment-variables-cargo-sets-for-crates

I think there some benefit to this, as it makes the list easy to scan.

@weihanglo weihanglo added A-environment-variables Area: environment variables A-custom-subcommands Area: custom 3rd party subcommand plugins labels Feb 24, 2025
weihanglo
weihanglo approved these changes Feb 28, 2025
View reviewed changes
Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

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

Thanks for working with us for this fix!

@weihanglo weihanglo added this pull request to the merge queue Feb 28, 2025
Merged via the queue into rust-lang:master with commit 58cfd96 Feb 28, 2025
21 checks passed
@weihanglo weihanglo linked an issue Feb 28, 2025 that may be closed by this pull request
$CARGO passed to external subcommands can point to the wrong cargo if already set #15099
Closed
@weihanglo weihanglo mentioned this pull request Feb 28, 2025
Closed
@smoelius smoelius mentioned this pull request Feb 28, 2025
Closed
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@epage epage epage left review comments

@weihanglo weihanglo weihanglo approved these changes

Assignees

@epage epage

Labels
A-configuration Area: cargo config files and env vars A-custom-subcommands Area: custom 3rd party subcommand plugins A-environment-variables Area: environment variables S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

$CARGO passed to external subcommands can point to the wrong cargo if already set
4 participants
@smoelius @epage @weihanglo @rustbot