-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
x.py: run rustup toolchain link
in setup
#89212
Conversation
src/bootstrap/setup.rs
Outdated
let build = TargetSelection::from_user(&env!("BUILD_TRIPLE")); | ||
let stage_path = ["build", build.rustc_target_arg(), "stage1"].join("/"); | ||
let toolchain_link = | ||
Command::new("rustup").args(&["toolchain", "link", "stage1", &stage_path[..]]).output(); |
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.
I just realized this will overwrite any existing toolchain - that seems kind of not great, it means we'll delete any existing toolchain with this name. @kinnison @rbtcollins is that intentional that it will silently overwrite?
I guess to avoid that we could explicitly run rustup toolchain list
and see if it already has stage1
.
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.
Usually this command is executed by the user, so like most Unix-y commands, dispatches as silently and swiftly as possible, and alerting someone is a sign of failure.
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.
Hmm - but ln -s
(which you said in the original issue is basically what toolchain link
does) gives an error if the file already exists:
$ ln -s x config.toml
ln: failed to create symbolic link 'config.toml': File exists
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.
Reasonable~
But the other reason I mentioned links is due to problems due to OS control over when files may be edited/clobbered/removed, which has less to do with the particular link.
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.
Since we have to create the directory in the first place, couldn't we just not link if it already exists?
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.
Since we have to create the directory in the first place, couldn't we just not link if it already exists?
That doesn't seem great - that won't warn or setup the toolchain if the user has already run x.py build
but not created a rustup toolchain.
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.
I think this is why @Mark-Simulacrum suggested using a stamp file. We could also extend this by adding configuration for it in the config.toml
; e.g. toolchain-overwrite
or something where the user can specify if they want to overwrite the toolchain every time they run x.py setup
or not, and by default ./x.py setup
will only do it once.
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.
I really don't think this needs additional configuration. We can just check if rustup toolchain list
contains stage1
.
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.
Have we considered the possible failure mode of a distro-installed Rustup that is later overshadowed by Rustup's normal /home/user/.{cargo,rustup}
install process? Possibly that doesn't matter: maybe such a case would make one expect possible overshadowing.
src/bootstrap/setup.rs
Outdated
println!("WARNING: `rustup`failed to link stage 1 build to `stage1` toolchain"); | ||
println!( | ||
"To manually link stage 1 build to `stage1` tool chain, run:\n | ||
`rustup toolchain link stage1 build/{}/stage1`", | ||
build.rustc_target_arg() |
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.
So... I could have missed something about the previous code, but at this point in the program, what proof do we have that the user has Rustup on their system that justifies emitting this message all "WARNING!" style, telling them about a binary that possibly isn't installed on their system? It is entirely possible that the user is bootstrapping Rust on their system for the first time, Rustup is not available for this target, and we should be careful to neither alarm nor distract them. Something gentler and softer-voiced seems appropriate.
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.
If rustup is installed:
Linking `stage1` toolchain
If not:
`rustup` is not installed
Unable to link `stage1` toolchain
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.
I think it would be fine to only warn if the profile isn't "user"
(which is what distros should be using); for everyone else they would find rustup really useful for rust-lang/rust even if they wouldn't use it normally.
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.
It's mostly about framing: gently observing that they could do so is fine, as long as it doesn't appear on every invocation after, if they decide they don't want to use it.
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.
@workingjubilee yup - this is only running once when you first run setup
, it's never repeated.
src/bootstrap/setup.rs
Outdated
let build = TargetSelection::from_user(&env!("BUILD_TRIPLE")); | ||
let stage_path = ["build", build.rustc_target_arg(), "stage1"].join("/"); | ||
let toolchain_link = | ||
Command::new("rustup").args(&["toolchain", "link", "stage1", &stage_path[..]]).output(); |
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.
Usually this command is executed by the user, so like most Unix-y commands, dispatches as silently and swiftly as possible, and alerting someone is a sign of failure.
I'm not sure how this could go wrong. If there's a distro-installed rustup, it could run |
I more meant user surprisal due to the overshadowing and the linked toolchain "disappearing", if that mattered. |
I clarified offline - the way this can go wrong is:
Given that in this (somewhat convoluted) scenario they'll be able to easily rerun |
This comment has been minimized.
This comment has been minimized.
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.
Looks pretty good to me :) I just have a couple last nits.
@Mark-Simulacrum do you want to take a look before I approve? No problem if not, this seems pretty low risk.
src/bootstrap/setup.rs
Outdated
} else { | ||
println!( | ||
"Added `stage1` rustup toolchain; try `cargo +stage1 build` on a separate rust project to run a newly-built toolchain" | ||
); | ||
} | ||
} else { | ||
println!( | ||
"`stage1` toolchain already linked; not attempting to link `stage1` toolchain", | ||
); | ||
} | ||
} else { | ||
println!( | ||
"`rustup` failed to list current toolchains; not attempting to link `stage1` toolchain" | ||
); | ||
} | ||
} else if profile != Profile::User { | ||
println!("`rustup` is not installed; cannot link `stage1` toolchain"); | ||
} |
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.
This long list of else
blocks makes it hard to tell which error corresponds with which check IMO - could you instead use a separate functions and early returns so it's more clear? Something like this:
maybe_create_rustup_toolchain();
// ...
}
fn maybe_create_rustup_toolchain() {
if !rustup_installed || !stage_dir_exists {
// ...
return
}
// ...
}
No, happy to let you own the review here. Can always adjust later in future PRs if it becomes necessary :) |
@bors r+ This is awesome, thank you! ❤️ |
📌 Commit 91f77f7b8850d10b263fc6f35ed1781933c9eb37 has been approved by |
@bors r- Please squash the commits here to keep history a little cleaner. |
Fixed types Add checks for rustup and if toolchain is linked Fortified rustup/directory checks; made other suggested changes Added check for output status Remove output of rustup from console Made suggested change Deleted confusing comment Fixed compiler error; removed extra declaration Refactored to smaller components; made suggested changes Automate toolchain linking for stage 1 builds
91f77f7
to
adbb608
Compare
@bors r+ rollup |
📌 Commit adbb608 has been approved by |
x.py: run `rustup toolchain link` in setup Addresses rust-lang#89206 r? `@jyn514`
…laumeGomez Rollup of 8 pull requests Successful merges: - rust-lang#87260 (Libgccjit codegen) - rust-lang#89212 (x.py: run `rustup toolchain link` in setup) - rust-lang#89233 (Hide `<...> defined here` note if the source is not available) - rust-lang#89235 (make junit output more consistent with default format) - rust-lang#89255 (Fix incorrect disambiguation suggestion for associated items) - rust-lang#89276 (Fix the population of the `union.impls` field) - rust-lang#89283 (Add regression test for issue rust-lang#83564) - rust-lang#89318 (rustc_session: Remove lint store from `Session`) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
…etup, r=Mark-Simulacrum Fix linking stage1 toolchain in `./x.py setup` Closes [92319](rust-lang#92319) Fix linking stage1 toolchain in `./x.py setup`. I guess this can be considered a follow up to rust-lang#89212 by `@Sl1mb0.` We create 2 directories and 1 file that are required by rustup to [link a custom toolchain from path](https://github.com/rust-lang/rustup/blob/5225e87a5d974ab5f1626bcb2a7b43f76ab883f0/src/toolchain.rs#L479-L497). cc `@jyn514` and `@Mark-Simulacrum` as they were active in rust-lang#89206
…etup, r=Mark-Simulacrum Fix linking stage1 toolchain in `./x.py setup` Closes [92319](rust-lang#92319) Fix linking stage1 toolchain in `./x.py setup`. I guess this can be considered a follow up to rust-lang#89212 by ``@Sl1mb0.`` We create 2 directories and 1 file that are required by rustup to [link a custom toolchain from path](https://github.com/rust-lang/rustup/blob/5225e87a5d974ab5f1626bcb2a7b43f76ab883f0/src/toolchain.rs#L479-L497). cc ``@jyn514`` and ``@Mark-Simulacrum`` as they were active in rust-lang#89206
…etup, r=Mark-Simulacrum Fix linking stage1 toolchain in `./x.py setup` Closes [92319](rust-lang#92319) Fix linking stage1 toolchain in `./x.py setup`. I guess this can be considered a follow up to rust-lang#89212 by ```@Sl1mb0.``` We create 2 directories and 1 file that are required by rustup to [link a custom toolchain from path](https://github.com/rust-lang/rustup/blob/5225e87a5d974ab5f1626bcb2a7b43f76ab883f0/src/toolchain.rs#L479-L497). cc ```@jyn514``` and ```@Mark-Simulacrum``` as they were active in rust-lang#89206
…etup, r=Mark-Simulacrum Fix linking stage1 toolchain in `./x.py setup` Closes [92319](rust-lang#92319) Fix linking stage1 toolchain in `./x.py setup`. I guess this can be considered a follow up to rust-lang#89212 by ````@Sl1mb0.```` We create 2 directories and 1 file that are required by rustup to [link a custom toolchain from path](https://github.com/rust-lang/rustup/blob/5225e87a5d974ab5f1626bcb2a7b43f76ab883f0/src/toolchain.rs#L479-L497). cc ````@jyn514```` and ````@Mark-Simulacrum```` as they were active in rust-lang#89206
…etup, r=Mark-Simulacrum Fix linking stage1 toolchain in `./x.py setup` Closes [92319](rust-lang#92319) Fix linking stage1 toolchain in `./x.py setup`. I guess this can be considered a follow up to rust-lang#89212 by `````@Sl1mb0.````` We create 2 directories and 1 file that are required by rustup to [link a custom toolchain from path](https://github.com/rust-lang/rustup/blob/5225e87a5d974ab5f1626bcb2a7b43f76ab883f0/src/toolchain.rs#L479-L497). cc `````@jyn514````` and `````@Mark-Simulacrum````` as they were active in rust-lang#89206
Addresses #89206
r? @jyn514