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.

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

Disallow RUSTUP_HOME in the [env] table. #12101

Merged
merged 2 commits into from May 7, 2023

Conversation

ehuss
Copy link
Contributor

@ehuss ehuss commented May 7, 2023

This adds a check to prevent RUSTUP_HOME from being set in the [env] config table under the same reasoning as was done in #11590. Cargo will likely behave incorrectly if this key is set in the config since it will not change the home used by the outer cargo itself.

This is a breaking change, though I think it is unlikely to be used in practice. When cargo is executed via a rustup proxy, the proxy sets RUSTUP_HOME which overrides the [env] table entry. It may be feasible that someone is invoking cargo directly without the rustup wrappers, and then using this to steer the rustc invocations to a different rustup location, but I'm not sure that's a use case we need to be supporting.

This is being added as a further assurance for #11590 to make sure the environment is configured as expected.

We could potentially add other Rustup env vars to reject, but I'm not sure I want to delve into analyzing all the possible reasons or interactions for each one.

@rustbot
Copy link
Collaborator

rustbot commented May 7, 2023

r? @weihanglo

(rustbot has picked a reviewer for you, use r? to override)

@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 May 7, 2023
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!

Do you think it's worth having an in-code explanation about why they are banned?

@ehuss
Copy link
Contributor Author

ehuss commented May 7, 2023

Sure! Added a comment.

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! The comments are fantastic!

@weihanglo
Copy link
Member

@bors r+

@bors
Copy link
Collaborator

bors commented May 7, 2023

📌 Commit 2fce10d has been approved by weihanglo

It is now in the queue for this repository.

@bors 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 May 7, 2023
@bors
Copy link
Collaborator

bors commented May 7, 2023

⌛ Testing commit 2fce10d with merge 1490d10...

@bors
Copy link
Collaborator

bors commented May 7, 2023

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing 1490d10 to master...

@bors bors merged commit 1490d10 into rust-lang:master May 7, 2023
18 checks passed
@rbtcollins
Copy link
Contributor

@ehuss RUSTUP_TOOLCHAIN seems like one to ban too ? (That was actually the one I expected from #11917 :P.

@ehuss
Copy link
Contributor Author

ehuss commented May 8, 2023

Sure, I went ahead and posted #12107.

bors added a commit that referenced this pull request May 8, 2023
Disallow RUSTUP_TOOLCHAIN in the [env] table.

This adds RUSTUP_TOOLCHAIN to the list of disallowed keys in the `[env]` table. This is intended to help prevent confusion and broken environments, along with the same reasoning as #12101 and #11590.
bors added a commit to rust-lang-ci/rust that referenced this pull request May 10, 2023
Update cargo

10 commits in 569b648b5831ae8a515e90c80843a5287c3304ef..26b73d15a68fb94579f6d3590585ec0e9d81d3d5
2023-05-05 15:49:44 +0000 to 2023-05-09 20:28:03 +0000
- Update the semver-check script to be able to run in any directory. (rust-lang/cargo#12117)
- Semver: Note that it is not a breaking change to make an unsafe function safe (rust-lang/cargo#12116)
- Add more documentation for artifact-dependencies. (rust-lang/cargo#12110)
- changelog: move registry query fixes to the right place (rust-lang/cargo#12086)
- Disallow RUSTUP_TOOLCHAIN in the [env] table. (rust-lang/cargo#12107)
- Disallow RUSTUP_HOME in the [env] table. (rust-lang/cargo#12101)
- Fix redacting tokens in http debug. (rust-lang/cargo#12095)
- Fix self_signed_should_fail for macOS. (rust-lang/cargo#12097)
- Update git2 (rust-lang/cargo#12096)
- do not try an exponential number of package names (rust-lang/cargo#12083)

r? `@ghost`
RalfJung pushed a commit to RalfJung/miri that referenced this pull request May 10, 2023
Update cargo

10 commits in 569b648b5831ae8a515e90c80843a5287c3304ef..26b73d15a68fb94579f6d3590585ec0e9d81d3d5
2023-05-05 15:49:44 +0000 to 2023-05-09 20:28:03 +0000
- Update the semver-check script to be able to run in any directory. (rust-lang/cargo#12117)
- Semver: Note that it is not a breaking change to make an unsafe function safe (rust-lang/cargo#12116)
- Add more documentation for artifact-dependencies. (rust-lang/cargo#12110)
- changelog: move registry query fixes to the right place (rust-lang/cargo#12086)
- Disallow RUSTUP_TOOLCHAIN in the [env] table. (rust-lang/cargo#12107)
- Disallow RUSTUP_HOME in the [env] table. (rust-lang/cargo#12101)
- Fix redacting tokens in http debug. (rust-lang/cargo#12095)
- Fix self_signed_should_fail for macOS. (rust-lang/cargo#12097)
- Update git2 (rust-lang/cargo#12096)
- do not try an exponential number of package names (rust-lang/cargo#12083)

r? `@ghost`
@ehuss ehuss added this to the 1.71.0 milestone May 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-configuration Area: cargo config files and env vars S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants