-
Notifications
You must be signed in to change notification settings - Fork 981
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
68aafbb
87afccd
92b2ba4
da1223a
282ede1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -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); | ||||||||||||||||||
} | ||||||||||||||||||
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
So I think we want it to handle the Lines 19 to 26 in 061cf78
I am happy to be corrected if I am misunderstanding. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What happens if run There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Within the To me, it seems like we should only inherit There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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
If I run with just There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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()); | ||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think both are in that list. I updated the list. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As mentioned in rust-lang/cargo#11036, There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||||||||||||||||||
} | ||||||||||||||||||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh,
RUSTUP_TOOLCHAIN
is documented as something the user can set; I didn't realize that rustup sets it on child processes butrustup/src/toolchain.rs
Line 184 in 061cf78