Skip to content
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

feat(rustup-mode): install the active toolchain by default on rustup toolchain install #3983

Merged
merged 4 commits into from
Aug 6, 2024

Conversation

rami3l
Copy link
Member

@rami3l rami3l commented Aug 6, 2024

Closes #2686 by extending the existing rustup toolchain install and rustup install commands, as discussed in #3635 (comment):

What would be the semantics here? Would it only install if there is a rust-toolchain.toml file or would it install any missing default toolchain?

My understanding is that it is expected to do anything rustc --version currently does before actually running that command on the corresponding rustc, it's just that everything will become explicit now. So yes, this includes (and isn't limited to) installing the toolchain specified in the rust-toolchain.toml file.

And presumably the rustup install alias.

Yes! As the two subcommands has been unified with #3596, that looks like a less painful move indeed. A single change for these two places!

TLDR: This PR makes rustup toolchain install and rustup install install the active toolchain, i.e. behave like the hypothetical rustup toolchain ensure command, when no toolchain is specified.

@rami3l rami3l requested a review from djc August 6, 2024 12:46
@rami3l rami3l force-pushed the feat/ensure-toolchain-install branch 2 times, most recently from c8da02e to f9bff67 Compare August 6, 2024 12:57
@rami3l rami3l marked this pull request as ready for review August 6, 2024 12:58
@rami3l rami3l force-pushed the feat/ensure-toolchain-install branch from f9bff67 to cf4dd59 Compare August 6, 2024 13:01
@rami3l rami3l force-pushed the feat/ensure-toolchain-install branch from cf4dd59 to 261b851 Compare August 6, 2024 13:05
@djc
Copy link
Contributor

djc commented Aug 6, 2024

I think conceptually it would be a good next step to break find_or_install_active_toolchain() into find and install parts.

@rami3l
Copy link
Member Author

rami3l commented Aug 6, 2024

I think conceptually it would be a good next step to break find_or_install_active_toolchain() into find and install parts.

@djc The whole story is actually more complex than that. We already have find_active_toolchain() but it doesn't equal to find_or_install_active_toolchain() minus ensure_installed() or something like that. Currently find_or_install_active_toolchain() doesn't even call find_active_toolchain(), it's a real mess that happens to get this job done.

My plan is that when removing implicit installations altogether, this new call site of that find_or_install_active_toolchain() will be the only one, and thus I can start inlining/refactoring it so that all the verbose logs and/or sanity checks can hopefully be included.

@rami3l rami3l added this pull request to the merge queue Aug 6, 2024
Merged via the queue into rust-lang:master with commit 5b5ec92 Aug 6, 2024
27 checks passed
@rami3l rami3l deleted the feat/ensure-toolchain-install branch August 6, 2024 13:54
@ChrisDenton
Copy link
Member

Is there documentation of the new rustup toolchain install behaviour?

@weihanglo
Copy link
Member

While not really in the same situation, here is the background of why Cargo moved away from implicit cargo install to cargo install --path .: rust-lang/cargo#5327.

@ChrisDenton
Copy link
Member

Hm, I think rustup is kinda in the exact opposite position. As that issue says, you (almost) never want to cargo install the crate in the current directory but you (almost) always want to rustup install the toolchain in the crate's rust-toolchain file if you're working on that crate.

@rami3l
Copy link
Member Author

rami3l commented Aug 7, 2024

Is there documentation of the new rustup toolchain install behaviour?

@ChrisDenton This PR has already changed the --help output, would you mind sharing what you have in mind?

@ChrisDenton
Copy link
Member

I was thinking more of online documentation. Although maybe that should wait until a release. Something along the lines of advice for people using CI. Because presumably we want to guide people towards using this rather than relying on auto install as a side effect of other commands.

@rami3l
Copy link
Member Author

rami3l commented Aug 8, 2024

I was thinking more of online documentation. Although maybe that should wait until a release. Something along the lines of advice for people using CI. Because presumably we want to guide people towards using this rather than relying on auto install as a side effect of other commands.

@ChrisDenton The plan is to get rid of implicit installations immediately (#3985), and I promise that will be mentioned in highlight when we cut a new release 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Command for "install stuff from rust-toolchain.toml"
4 participants