Skip to content

Set RUSTUP_TOOLCHAIN_SOURCE #4518

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.

Sign up for GitHub

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
+134 −5
Open

Set RUSTUP_TOOLCHAIN_SOURCE #4518

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
68aafbb
Set `RUSTUP_TOOLCHAIN_SOURCE`
smoelius Oct 6, 2025
87afccd
Make Clippy happy
smoelius Oct 6, 2025
92b2ba4
Make Clippy even happier
smoelius Oct 6, 2025
da1223a
Eliminate overlap with `RUSTUP_TOOLCHAIN`
smoelius Oct 7, 2025
282ede1
Describe `RUSTUP_TOOLCHAIN_SOURCE` in environment-variables.md
smoelius Oct 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 8 additions & 5 deletions src/cli/proxy_mode.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,9 @@ use anyhow::Result;
use crate::{
cli::{common::set_globals, job, self_update},
command::run_command_for_dir,
config::ActiveReason,
process::Process,
toolchain::ResolvableLocalToolchainName,
toolchain::{ResolvableLocalToolchainName, maybe_set_env_source},
};

#[tracing::instrument(level = "trace", skip(process))]
Expand All @@ -24,6 +25,7 @@ pub async fn main(arg0: &str, current_dir: PathBuf, process: &Process) -> Result
.filter(|arg| arg.starts_with('+'))
.map(|name| ResolvableLocalToolchainName::try_from(&name.as_ref()[1..]))
.transpose()?;
let toolchain_specified = toolchain.is_some();

// Build command args now while we know whether or not to skip arg 1.
let cmd_args: Vec<_> = process
Expand All @@ -32,9 +34,10 @@ pub async fn main(arg0: &str, current_dir: PathBuf, process: &Process) -> Result
.collect();

let cfg = set_globals(current_dir, true, process)?;
let cmd = cfg
.resolve_local_toolchain(toolchain)
.await?
.command(arg0)?;
let toolchain = cfg.resolve_local_toolchain(toolchain).await?;
let mut cmd = toolchain.command(arg0)?;
if toolchain_specified {
maybe_set_env_source(&mut cmd, || Some(ActiveReason::CommandLine));
}
run_command_for_dir(cmd, arg0, &cmd_args)
}
13 changes: 13 additions & 0 deletions src/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,19 @@ pub(crate) enum ActiveReason {
ToolchainFile(PathBuf),
}

impl ActiveReason {
/// Format `ActiveReason` for setting the `RUSTUP_TOOLCHAIN_SOURCE` environment variable.
pub fn to_source(&self) -> &str {
match self {
Self::Default => "default",
Self::Environment => "env",
Self::CommandLine => "cli",
Self::OverrideDB(_) => "override",
Self::ToolchainFile(_) => "config",
Comment on lines +109 to +113
Copy link
Contributor

Choose a reason for hiding this comment

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

This is redundant, because the latter component is RUSTUP_TOOLCHAIN. I'll fix this PR in the next few hours.

Huh, RUSTUP_TOOLCHAIN is documented as something the user can set; I didn't realize that rustup sets it on child processes but

rustup/src/toolchain.rs

Line 184 in 061cf78

cmd.env("RUSTUP_TOOLCHAIN", format!("{}", self.name));

}
}
}

impl Display for ActiveReason {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> std::result::Result<(), fmt::Error> {
match self {
Expand Down
8 changes: 8 additions & 0 deletions src/test/mock_bin_src.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,14 @@ fn main() {
let mut out = io::stderr();
writeln!(out, "{}", std::env::current_exe().unwrap().display()).unwrap();
}
Some("--echo-rustup-toolchain-source") => {
let mut out = io::stderr();
if let Ok(rustup_toolchain_source) = std::env::var("RUSTUP_TOOLCHAIN_SOURCE") {
writeln!(out, "{rustup_toolchain_source}").unwrap();
} else {
panic!("RUSTUP_TOOLCHAIN_SOURCE environment variable not set");
}
}
arg => panic!("bad mock proxy commandline: {:?}", arg),
}
}
Expand Down
21 changes: 21 additions & 0 deletions src/toolchain.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,14 @@ impl<'a> Toolchain<'a> {

env_var::inc("RUST_RECURSION_COUNT", cmd, self.cfg.process);

maybe_set_env_source(cmd, || {
if let Ok(Some((_, reason))) = self.cfg.active_toolchain() {
Some(reason)
} else {
None
}
});

cmd.env("RUSTUP_TOOLCHAIN", format!("{}", self.name));
cmd.env("RUSTUP_HOME", &self.cfg.rustup_dir);
}
Expand Down Expand Up @@ -605,3 +613,16 @@ impl<'a> Toolchain<'a> {
.collect())
}
}

/// Set the `RUSTUP_TOOLCHAIN_SOURCE` environment variable if not already set.
///
/// `RUSTUP_TOOLCHAIN_SOURCE` indicates how the toolchain was determined. The environment
/// variable could have been set in proxy_mode.rs, in which case it should not be changed.
Comment on lines +617 to +620
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason we let the original through rather than override it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that's the behavior we want.

In my installation, all the proxies link to rustup:

lrwxrwxrwx 1 smoelius smoelius       32 Jan 31  2025 cargo -> /home/smoelius/.cargo/bin/rustup

So I think we want it to handle the + args and we should just let those through:

rustup/src/cli/proxy_mode.rs

Lines 19 to 26 in 061cf78

// Check for a + toolchain specifier
let arg1 = args.next();
let toolchain = arg1
.as_ref()
.map(|arg| arg.to_string_lossy())
.filter(|arg| arg.starts_with('+'))
.map(|name| ResolvableLocalToolchainName::try_from(&name.as_ref()[1..]))
.transpose()?;

I am happy to be corrected if I am misunderstanding.

Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if run cargo +beta test and one of my tests invokes cargo doc within a directory that has a rustup-toolchain.toml for nightly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

RUSTUP_TOOLCHAIN_SOURCE should be set to cli. But I don't think I understand the point of your question.

Copy link
Contributor

Choose a reason for hiding this comment

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

Within the cargo doc call, the source of the toolchain is config but RUSTUP_TOOLCHAIN_SOURCE will incorrectly report back cli because the outer most rustup call had it set on the CLI.

To me, it seems like we should only inherit RUSTUP_TOOLCHAIN_SOURCE if we are inheriting the parent's toolchain.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Within the cargo doc call, the source of the toolchain is config

I don't mean to question you but are you sure that's right?

I just tested in a configuration like you described with this program:

//! ```
//! let stdout = std::process::Command::new("env").output().unwrap().stdout;
//! std::fs::write("env.txt", stdout).unwrap();
//! ```

#[test]
fn test() {
    std::process::Command::new("cargo").arg("doc").status().unwrap();
}

And here is what I saw written to the env.txt file:

RUSTUP_HOME=/home/smoelius/.rustup
RUSTUP_TOOLCHAIN=beta-x86_64-unknown-linux-gnu
RUSTUP_TOOLCHAIN_SOURCE=cli

If I run with just cargo test instead of cargo +beta test, I observe what you describe.

Copy link
Contributor

Choose a reason for hiding this comment

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

If I run with just cargo test instead of cargo +beta test, I observe what you describe.

So my exact example didn't work but the principle still applies.

pub(crate) fn maybe_set_env_source(cmd: &mut Command, f: impl FnOnce() -> Option<ActiveReason>) {
if env::var_os("RUSTUP_TOOLCHAIN_SOURCE").is_some() {
return;
}
if let Some(reason) = f() {
cmd.env("RUSTUP_TOOLCHAIN_SOURCE", reason.to_source());
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume you'll need to update https://github.com/rust-lang/rustup/blob/main/doc/user-guide/src/environment-variables.md

Copy link
Contributor

Choose a reason for hiding this comment

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

Or is that only env variables that rustup reads?

Are the ones it sets defined anywhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think both are in that list. I updated the list.

Copy link
Contributor

Choose a reason for hiding this comment

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

As mentioned in rust-lang/cargo#11036, RUSTUP_TOOLCHAIN_SOURCE could be taken as pointing to something inside the toolchain or pointing to what caused the toolchain to be set, not just the kind of toolchain source.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I take your point. But I thought we wanted to let the rustup team decide on the variable name. So I have tried to remain agnostic on that front.

}
}
82 changes: 82 additions & 0 deletions tests/suite/cli_rustup.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3038,6 +3038,88 @@ error: no active toolchain
.is_ok();
}

#[tokio::test]
async fn rustup_toolchain_source_cli() {
let cx = CliTestContext::new(Scenario::SimpleV2).await;
cx.config
.expect(&["rustup", "install", "nightly"])
.await
.is_ok();
cx.config
.expect(["cargo", "+nightly", "--echo-rustup-toolchain-source"])
.await
.with_stderr(snapbox::str![[r#"
...
cli

"#]]);
}

#[tokio::test]
async fn rustup_toolchain_source_env() {
let cx = CliTestContext::new(Scenario::SimpleV2).await;
cx.config
.expect_with_env(
["cargo", "--echo-rustup-toolchain-source"],
[("RUSTUP_TOOLCHAIN", "nightly")],
)
.await
.with_stderr(snapbox::str![[r#"
...
env

"#]]);
}

#[tokio::test]
async fn rustup_toolchain_source_override() {
let cx = CliTestContext::new(Scenario::SimpleV2).await;
cx.config
.expect(["rustup", "override", "set", "nightly"])
.await
.is_ok();
cx.config
.expect(["cargo", "--echo-rustup-toolchain-source"])
.await
.with_stderr(snapbox::str![[r#"
...
override

"#]]);
}

#[tokio::test]
async fn rustup_toolchain_source_config() {
let cx = CliTestContext::new(Scenario::SimpleV2).await;
let toolchain_file = cx.config.current_dir().join("rust-toolchain.toml");
raw::write_file(&toolchain_file, "[toolchain]\nchannel='nightly'").unwrap();
cx.config
.expect(["cargo", "--echo-rustup-toolchain-source"])
.await
.with_stderr(snapbox::str![[r#"
...
config

"#]]);
}

#[tokio::test]
async fn rustup_toolchain_source_default() {
let cx = CliTestContext::new(Scenario::SimpleV2).await;
cx.config
.expect(&["rustup", "default", "stable"])
.await
.is_ok();
cx.config
.expect(["cargo", "--echo-rustup-toolchain-source"])
.await
.with_stderr(snapbox::str![[r#"
...
default

"#]]);
}

#[tokio::test]
async fn directory_override_doesnt_need_to_exist_unless_it_is_selected() {
let cx = CliTestContext::new(Scenario::SimpleV2).await;
Expand Down