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.

weihanglo · 2025-10-18T12:45:20Z

src/cargo/ops/cargo_install.rs

+                .unwrap_or_default();
+            self.gctx.shell().warn(format!(
+                "\
+using non-default toolchain{maybe_toolchain} overridden by {source}


we now prefer annotate-snippets style diagnostics. See the linked pull requests in #15944 for references.

(I guess the "use cargo +stable install …" can be rendered under a help:)

Using warn is fine if its a single message but yes, this should be two messages as written, a warn and help.

Please tell me if I used the new API incorrectly.

weihanglo · 2025-10-18T12:53:02Z

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]


We prefer tests for a behavior change on its own commit first, so that in the next fix impl commit we can see the behavior change through test assertion/snapshot diff.

Note that with what was said, each commit should pass tests. As the last commit has no test changes, I'm assuming that instead you added all tests as if feature is implemented and put them in the test commit.

See https://doc.crates.io/contrib/process/working-on-cargo.html#submitting-a-pull-request for more details

epage · 2025-10-20T21:19:41Z

tests/testsuite/install.rs

 use cargo_test_support::install::{assert_has_installed_exe, assert_has_not_installed_exe, exe};
 use cargo_test_support::paths;

-fn pkg(name: &str, vers: &str) {


For myself, I'm not too much of a fan of us reusing like this as it hides in the test what is happening and gets annoying when you just need a little customization. Just directly publish the needed package inside of your test.

epage · 2025-10-20T21:23:09Z

tests/testsuite/rustup.rs

+[DOWNLOADING] crates ...
+[DOWNLOADED] foo v0.0.1 (registry `dummy-registry`)
+[INSTALLING] foo v0.0.1
+[WARNING] using non-default toolchain `test-toolchain` overridden by env


When the new rustup is released, to avoid it causing annoyances in our tests, I suspect we want to update

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

Line 1377 in 40076ea

fn test_env(mut self) -> Self {

to loop through RUSTUP_* and env_remove them from the binary being executed

epage · 2025-10-20T21:25:02Z

tests/testsuite/rustup.rs

    // The toolchain rustc needs to call the real rustc. In order to do that,
    // it needs to restore or clear the RUSTUP environment variables so that
    // if rustup is installed, it will call the correct rustc.
+    let rustup_toolchain_source_setup = match std::env::var_os("RUSTUP_TOOLCHAIN_SOURCE") {


I don't think we should be doing anything based on the rustup we are running in

See also #16131 (comment)

epage · 2025-10-20T21:27:03Z

tests/testsuite/rustup.rs

+/// Arguments
+///
+/// - `proxy_calls_cargo`: if true, the cargo proxy calls the cargo under test;
+///    otherwise, the cargo proxy calls an executable that panics immediately
+/// - `env_setup`: environment variable setup the proxy should perform
+fn simulated_rustup_environment(proxy_calls_cargo: bool, env_setup: &str) -> RustupEnvironment {


These make the tests much harder to read and not thrilled about injecting code directly into things.

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

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

src/cargo/ops/cargo_install.rs

+                .map(|toolchain| format!(" `{toolchain}`"))
+                .unwrap_or_default();
+            let report = &[Level::WARNING
+                .primary_title(format!(


Not too obvious but we are limiting ourselves to secondary_titles for now

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

src/cargo/ops/cargo_install.rs

+                    "using non-default toolchain{maybe_toolchain} overridden by {source}"
+                ))
+                .element(Level::HELP.message(format!(
+                    "Use `cargo +stable install` if you meant to use the stable toolchain."


Please change Use to use. Our messages should be lower case

I really wish we had [+default](#t-rustup > Could we support `+default` toolchain? @ 💬) to suggest here.

If its purely for diagnostic purposes, we could run rustup show active-toolchain, parse the output, and show that toolchain here. If we ever want to force its use, it needs to be a more strictly defined programmatic interface.

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"),

smoelius added 2 commits October 17, 2025 16:48

Set RUSTUP_TOOLCHAIN_SOURCE in rustup tests

ff31445

Move pkg to testsuite/utils

4c7f9d0

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

smoelius added 2 commits October 19, 2025 05:45

Implement test for 11036 cargo changes

9353e04

Implement 11036 cargo changes

c64e01a

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

	// 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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

smoelius commented Oct 18, 2025 •

edited

Loading