Warn when installing with a non-default toolchain #16131

smoelius · 2025-10-18T09:36:35Z

This PR implements the cargo changes for #11036. The rustup changes were implemented in rust-lang/rustup#4518.

The PR is currently organized as four commits:

The first expands the rustup tests to set RUSTUP_TOOLCHAIN_SOURCE. This change is not strictly necessary, but it improves the rustup tests' fidelity.
The second moves the pkg function to a location where the next commit can use it.
The third implements a test to check the fourth commit's fix.
The fourth warns when cargo install is invoked with a non-default toolchain, as indicated by RUSTUP_TOOLCHAIN_SOURCE.

In the third commit, two significant changes were made to simulated_rustup_environment:

Previously, the constructed proxy would call an always-panicking executable whenever it was asked to run cargo. Now, the proxy can be made to run the cargo under test.
The proxy can now be made to set additional environment variables (e.g., RUSTUP_TOOLCHAIN_SOURCE) before calling the proxied tool.

The PR is currently marked as draft because rust-lang/rustup#4518 has not yet been released. Technically, this PR could be merged before then. But testing it with a fixed rustup would seem to make sense.

Nits are welcome.

cc: @epage

rustbot · 2025-10-18T09:36:40Z

r? @weihanglo

rustbot has assigned @weihanglo.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

weihanglo

EDIT: The force push was to make Clippy happy.

Feel free to force-push and re-organize your commits during the iterations.

src/cargo/ops/cargo_install.rs

tests/testsuite/rustup.rs

tests/testsuite/install.rs

tests/testsuite/rustup.rs

epage · 2025-10-20T21:29:36Z

tests/testsuite/rustup.rs

+/// Performs a `cargo install` with a non-default toolchain in a simulated
+/// rustup environment. The purpose is to verify the warning that is emitted.
+#[cargo_test]
+fn cargo_install_with_non_default_toolchain() {


We should likely have cargo install tests for

Rustup that doesn't set RUSTUP_TOOLCHAIN_SOURCE

For each value of RUSTUP_TOOLCHAIN_SOURCE

For an unknown value of RUSTUP_TOOLCHAIN_SOURCE (handling future cases where new values are added)

epage · 2025-10-20T21:32:17Z

src/cargo/ops/cargo_install.rs

+            && !matches!(
+                source,
+                RustupToolchainSource::Default | RustupToolchainSource::CommandLine


Downside to matches! is we can miss it if new values are added. For now, that is not likely to happen as this is all for one feature. That might not always be the case.

I would

Move the &str -> RustupToolchainSource to a function on get_rustup_toolchain_source (shouldn't touch gctx)

Have a function on RustupToolchainSource that lts is ask the question we are asking here and uses a match with every case enumerated

I don't think I follow what you're asking for.

Could you give me some pseudo-code? Or could you point to a function that uses this pattern?

Move the &str -> RustupToolchainSource to a function on get_rustup_toolchain_source (shouldn't touch gctx)

That has happened

Have a function on RustupToolchainSource that lts is ask the question we are asking here and uses a match with every case enumerated

impl RustupToolchainSource { fn is_current_dir_override(&self) -> bool { match self { } } }

(name is not prescriptive, just the first thing I could think of for what might describe what we care about)

src/cargo/ops/cargo_install.rs

epage · 2025-10-20T21:39:24Z

src/cargo/ops/cargo_install.rs


+    fn get_rustup_toolchain_source(&self) -> Option<RustupToolchainSource> {
+        self.gctx
+            .get_env("RUSTUP_TOOLCHAIN_SOURCE")


My gut reaction is we should make an exception and read from std::env. That seems matched with

cargo/src/cargo/util/mod.rs

Lines 75 to 78 in 40076ea

// ALLOWED: `RUSTUP_HOME` should only be read from process env, otherwise

// other tools may point to executables from incompatible distributions.

#[allow(clippy::disallowed_methods)]

std::env::var_os("RUSTUP_HOME").is_some()

but then we have this

cargo/src/cargo/util/rustc.rs

Lines 362 to 363 in 40076ea

gctx.get_env("RUSTUP_HOME"),

gctx.get_env("RUSTUP_TOOLCHAIN"),

My gut reaction is we should make an exception and read from std::env. That seems matched with...but then we have this...

I'm not seeing the rationale for using std::env or gctx.get_env(..).

Regardless, I will do whichever you prefer.

gctx.get_env is to allow us to read from .cargo/config.tomls [env] table which should not be setting RUSTUP variables.

smoelius · 2025-11-06T23:41:13Z

Just FYI, I know it looks like only some of the comments have been addressed. I am still hoping to get feedback on #16131 (comment) and #16131 (comment).

smoelius · 2025-12-28T21:33:31Z

I resolved comments that I thought I implemented uncontroversially. I hope that was okay.

EDIT: I also hid a few of my own comments that were old.

epage · 2025-12-30T16:43:50Z

Please clean up your commits for how they should be reviewed and merged, rather than for how it was developed.

epage · 2025-12-30T16:44:32Z

crates/cargo-test-support/src/lib.rs

+            .env_remove("RUSTUP_HOME")
+            .env_remove("RUSTUP_TOOLCHAIN")
+            .env_remove("RUSTUP_TOOLCHAIN_SOURCE")


should we loop over all RUSTUP_* like we do for cargo?

epage · 2025-12-30T16:45:07Z

tests/testsuite/jobserver.rs

    p.process(make)
        .env("PATH", path)
        .env("CARGO", cargo_exe())
+        .env("RUSTUP_HOME", env!("REAL_RUSTUP_HOME"))


Why do we need REAL_RUSTUP_HOME rather than just reading RUSTUP_HOME?

Some of the tests seem to require something in the real rustup installation. I haven't tried to figure out what, though.

I can understand forwarding RUSTUP_HOME. What I'm wondering about is REAL_RUSTUP_HOME. We should be able to read RUSTUP_HOME (ideally at runtime, not compile time) and forward that to the test. The p.process call is stripping RUSTUP_HOME but the process we are currently running in should still have it set if it is set (it isn't always)

tests/testsuite/rustup.rs

weihanglo · 2025-12-31T00:19:22Z

tests/testsuite/rustup.rs

nit: Can we have some explanation in the commit message, so the commit standalone tells why we need this? It is hard to tell whether this is a refactor or a bugfix for the testsuite.

Are you asking for one comment per commit? Or one comment describing the entire PR in one of the commits? Either way, I am happy to oblige.

I reviewed the code commit by commit, so was specifically asking the first commit. Not a blocker though. Thanks!

weihanglo · 2025-12-31T00:30:42Z

tests/testsuite/rustup.rs

+    cargo_install_with_toolchain_source(Some("invalid"), false);
+}
+
+fn cargo_install_with_toolchain_source(source: Option<&str>, should_warn: bool) {


Instead of passing boolean we can make the argument impl snapbox::IntoData so that every test has its own snapshot. What do you think?

What would the impl snapbox::IntoData be? Could you give me an example?

He's asking to pass in stderr rather than creating it within the test

For more context:

it makes the test easier to evaluate what it does

we prefer using snapshots (str![]), rather than hard coded values, wherever possible so we can easily update all tests when unrelated formatting changes

weihanglo · 2025-12-31T00:35:58Z

src/cargo/ops/cargo_install.rs

    }
 }

+enum RustupToolchainSource {


As far as I know, rustup hasn't yet claimed those values are stable. At least the doc doesn't say anything more than the environment key https://github.com/rust-lang/rustup/blob/de2b979bd4569cacfabf501bb58464c9139b0d6d/doc/user-guide/src/environment-variables.md?plain=1#L75.

Could we have a link to the rustup source code if we want to make an assumption on that?

Should we check with the rustup team to ensure these value strings are stable?

I am happy to get confirmation from the Rustup team that the five values are stable, but would you feel more comfortable if they were listed directly in the documentation?

Yeah, thanks. But actually if we have #16131 (comment) maybe it is not really necessary?

weihanglo · 2025-12-31T00:39:50Z

src/cargo/ops/cargo_install.rs

+                "cli" => Some(RustupToolchainSource::CommandLine),
+                "path-override" => Some(RustupToolchainSource::OverrideDB),
+                "toolchain-file" => Some(RustupToolchainSource::ToolchainFile),
+                _ => None,


For forward compatibility, should we have RustupToolchainSource::Other(String) and render it verbatim?

epage · 2025-12-31T13:18:57Z

build.rs

+    #[allow(clippy::disallowed_methods)]
+    let real_rustc = std::env::var("RUSTC").unwrap();
+    println!("cargo:rustc-env=REAL_RUSTC={real_rustc}");


Similar to REAL_RUSTUP_HOME, we shouldn't need a REAL_RUSTC because RUSTC (if it is set which it isn't guaranteed to be) is not cleared within the process that can also read REAL_RUSTC

Oh, cargo sets it on the build script and we are forwarding it along.

While a lot simpler of a solution, I'm not too thrilled with this because

say we could get rid of our build script for production reasons (which would be ideal), we'd then have production builds running a build script for tests

we have to use env! which I assume makes caching more difficult because it has an absolute path in it that can change, particularly for CI

@weihanglo your thoughts?

epage · 2025-12-31T13:27:15Z

tests/testsuite/rustup.rs

Add RustupEnvironmentBuilder

When doing a refactor commit, particularly when the diff is more difficult to read (indentation level, moving), please separate out any logical changes into a separate commit

epage · 2025-12-31T13:28:01Z

tests/testsuite/rustup.rs

Set RUSTUP_TOOLCHAIN_SOURCE in rustup tests

Why is this needed?

We need to be able to work with old rustup that doesn't set this, so it is probably good for us to have some existing tests that leave it unset

epage · 2025-12-31T13:30:27Z

tests/testsuite/rustup.rs

+fn pkg(name: &str, vers: &str) {
+    Package::new(name, vers)
+        .file("src/main.rs", "fn main() {{}}")
+        .publish();
+}


imo this abstraction does not carry enough value to outweigh what it is hiding. We should instead just have these calls in the tests

Looks like I had same the same thing before (#16131 (comment)) but it got resolved

epage · 2025-12-31T13:33:31Z

crates/cargo-test-support/src/lib.rs

+        for (k, _v) in env::vars() {
+            if k.starts_with("RUSTUP_") {
+                self = self.env_remove(&k);
+            }
+        }


This seems like a change that is independent of implementing this warning and should be in a commit beforehand

epage · 2025-12-31T13:35:34Z

src/cargo/ops/cargo_install.rs

+            .ok()
+            .and_then(|val| match val {


I feel like the non-combinator approach would be much more readable here

let source = std::env::var_os("RUSTUP_TOOLCHAIN_SOURCE").ok()?; let source = match source { }; Some(source)

epage · 2025-12-31T13:39:21Z

src/cargo/ops/cargo_install.rs

+        {
+            let maybe_toolchain = self
+                .gctx
+                .get_env("RUSTUP_TOOLCHAIN")


ditto for get_env

rustbot assigned weihanglo Oct 18, 2025

rustbot added Command-install S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 18, 2025

smoelius marked this pull request as draft October 18, 2025 09:36

rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 18, 2025

smoelius force-pushed the second-half-of-11036 branch from 2157847 to 9006944 Compare October 18, 2025 09:52

weihanglo reviewed Oct 18, 2025

View reviewed changes

src/cargo/ops/cargo_install.rs Outdated Show resolved Hide resolved

tests/testsuite/rustup.rs Show resolved Hide resolved

smoelius force-pushed the second-half-of-11036 branch from 5f2fbb4 to c64e01a Compare October 19, 2025 09:49

smoelius mentioned this pull request Oct 19, 2025

Set RUSTUP_TOOLCHAIN_SOURCE rust-lang/rustup#4518

Merged

weihanglo mentioned this pull request Oct 19, 2025

Expand RUSTUP_TOOLCHAIN_SOURCE's documentation rust-lang/rustup#4549

Merged

epage reviewed Oct 20, 2025

View reviewed changes

tests/testsuite/install.rs Show resolved Hide resolved

epage reviewed Oct 20, 2025

View reviewed changes

tests/testsuite/rustup.rs Outdated Show resolved Hide resolved

epage reviewed Oct 20, 2025

View reviewed changes

tests/testsuite/rustup.rs Outdated Show resolved Hide resolved

epage reviewed Oct 20, 2025

View reviewed changes

tests/testsuite/rustup.rs Outdated Show resolved Hide resolved

epage reviewed Oct 20, 2025

View reviewed changes

src/cargo/ops/cargo_install.rs Outdated Show resolved Hide resolved

epage reviewed Oct 20, 2025

View reviewed changes

src/cargo/ops/cargo_install.rs Outdated Show resolved Hide resolved

epage reviewed Oct 20, 2025

View reviewed changes

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. label Nov 6, 2025

smoelius force-pushed the second-half-of-11036 branch from c64e01a to f0f222f Compare November 6, 2025 23:07

rustbot added A-infrastructure Area: infrastructure around the cargo repo, ci, releases, etc. A-testing-cargo-itself Area: cargo's tests labels Dec 28, 2025

smoelius force-pushed the second-half-of-11036 branch from f75ccb8 to a386ddd Compare December 28, 2025 21:21

epage reviewed Dec 30, 2025

View reviewed changes

tests/testsuite/rustup.rs Show resolved Hide resolved

Determine real rustc path in build script

b3d8899

smoelius force-pushed the second-half-of-11036 branch from a386ddd to 989ec0e Compare December 31, 2025 00:15

smoelius added 4 commits December 30, 2025 19:24

Add RustupEnvironmentBuilder

62f37d9

Set RUSTUP_TOOLCHAIN_SOURCE in rustup tests

ab44119

Implement tests for 11036 cargo changes

c1e51dc

Implement 11036 cargo changes

71b7a8f

smoelius force-pushed the second-half-of-11036 branch from 989ec0e to 71b7a8f Compare December 31, 2025 00:24

weihanglo reviewed Dec 31, 2025

View reviewed changes

epage reviewed Dec 31, 2025

View reviewed changes

src/cargo/ops/cargo_install.rs

{

let maybe_toolchain = self

.gctx

.get_env("RUSTUP_TOOLCHAIN")

Copy link

Contributor

epage Dec 31, 2025

Choose a reason for hiding this comment

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

ditto for get_env

	// ALLOWED: `RUSTUP_HOME` should only be read from process env, otherwise
	// other tools may point to executables from incompatible distributions.
	#[allow(clippy::disallowed_methods)]
	std::env::var_os("RUSTUP_HOME").is_some()

	gctx.get_env("RUSTUP_HOME"),
	gctx.get_env("RUSTUP_TOOLCHAIN"),

Warn when installing with a non-default toolchain #16131

Are you sure you want to change the base?

Warn when installing with a non-default toolchain #16131

Conversation

smoelius commented Oct 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Oct 18, 2025

Uh oh!

weihanglo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

smoelius commented Nov 6, 2025

Uh oh!

smoelius commented Dec 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

epage commented Dec 30, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

smoelius commented Oct 18, 2025 •

edited

Loading

smoelius commented Dec 28, 2025 •

edited

Loading