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

Command for "install stuff from rust-toolchain.toml" #2686

Closed
1 task done
Tracked by #3635
jmaargh opened this issue Mar 1, 2021 · 17 comments · Fixed by #3983
Closed
1 task done
Tracked by #3635

Command for "install stuff from rust-toolchain.toml" #2686

jmaargh opened this issue Mar 1, 2021 · 17 comments · Fixed by #3983
Assignees
Milestone

Comments

@jmaargh
Copy link

jmaargh commented Mar 1, 2021

Apologies if this already exists, but I couldn't find it in the docs or shell help. Or if there is an alternate workflow for this.

I love rust-toolchain.toml, it's a great idea. It would be even more useful if there were a command such as rustup override install, which would look at the current rust-toolchain.toml and install the toolchain and components specified there. It would be super useful both when exploring projects for the first time, but also when using disposable container-based development environments.

I'm unopinionated on the exact syntax of this command. Also happy to help implement this if the maintainers want it.

Tasks

Preview Give feedback
  1. enhancement
    rami3l
@rbtcollins
Copy link
Contributor

See #1397

@jmaargh
Copy link
Author

jmaargh commented Mar 2, 2021

I feel like there's value in keeping this issue different from #1397 for a couple of reasons:

  1. The main point of rustup show should not force-install the default toolchain if it is not installed. #1397 is to disable a certain behaviour, this is to add that behaviour somewhere else (something that was discussed in rustup show should not force-install the default toolchain if it is not installed. #1397, but isn't the main point)

  2. It's not clear to me from rustup show should not force-install the default toolchain if it is not installed. #1397 whether what's being discussed is installing just the toolchain per-se, or also the components that can be set in a rust-toolchain.toml file. This issue is suggesting ensuring that everything specified in rust-toolchain.toml is installed and available.

@kinnison
Copy link
Contributor

I think that ensuring rust-toolchain{,.toml} is properly honoured is an all-or-nothing thing - it couldn't "just" be the toolchain itself, no matter what.

You're right that #1397 is about ensuring that we don't over-indulge in autoinstallation, and I think this is a good home for how we then permit manual triggering of what was previously automatic.

I continue to think something like rustup toolchain ensure [PATH] seems like a good approach, but further straw men for discussion would be good.

@jmaargh
Copy link
Author

jmaargh commented Mar 12, 2021

I'm easy about the syntax. rustup toolchain ensure [path] works for me.

I was thinking re-using rustup toolchain install, so:

  • rustup toolchain install [toolchain] works as currently
  • rustup toolchain install [path to rust-toolchain.toml] installs according to the specified file
  • rustup toolchian install [path to directory] finds the rust-toolchain.toml that would apply to that directory (if any) and asks for confirmation before installing, with the confirmation prompt specifying exactly which file and that you could have specified a toolchain instead.
  • rustup toolchain install acts as rustup toolchian install .

@kinnison
Copy link
Contributor

Given all the other arguments rustup toolchain install can be given, I'd favour a separate verb.

@rbtcollins
Copy link
Contributor

There's a request to permit this during rustup-init as well; I've marked them as dup because I think its basically the same work... getting to a oneline for rustup not being installed is doable via:
rustup-init -y --no-update-default-toolchain && rustup toolchain ensure . (or whatever design we end up settling on).

@eminence
Copy link

I'd also like to have rustup-init be able to read a rust-toolchain file to figure out what toolchain to install. In my specific use case, I'm install rust (via rustup-init) as part of my CI scripts. Currently I'm using:

rustup-init.exe --no-modify-path --default-toolchain stable --profile minimal -y

Since my rust-toolchain file lists channel = "1.59", rustup will end up installing the stable channel (during initial install), and then later installing 1.59 (during the build).

I could resolve this by manually keeping my CI scripts in-sync with my rust-toolchain file, but I'd prefer to just manage rust-toolchain. (Another possible solution to my problem might to have something like --profile empty as suggested in #2970. This would let rustup-init do some very minimal work and let the normal toolchain file mechanisms do the right stuff)

@rbtcollins
Copy link
Contributor

@eminence just use --default-toolchain none then ?

@eminence
Copy link

Ah, sorry, I didn't know that was possible! I didn't see it listed in --help, It'll give it a try!

@jmaargh
Copy link
Author

jmaargh commented Mar 31, 2022

Given someone has actually started trying to implement this functionality, should we agree on syntax?

Based on the above, people seem to be keen for a new verb and have been using ensure (and I'm not able to think of one I prefer right now), so let's move ahead with that and not bikeshed (it can easily be changed last minute if needed).

As for exact syntax, based on my suggestion above I'd suggest:

  • rustup toolchain ensure [path to rust-toolchain.toml] installs according to the specified file
  • rustup toolchian ensure [path to directory] finds the rust-toolchain.toml that would apply to that directory (if any) and asks for confirmation before installing, with the confirmation prompt specifying exactly which file it's found.
  • rustup toolchain ensure acts as rustup toolchian ensure .

I'd also suggest a confirmation prompt by default, listing the path to the file it's using and what it's about to install, with a -y|--yes flag to skip the confirmation. Not a strong opinion on this one, if people don't like the confirmation then just do without.

Are people happy with this? Anything we want to change?

@lopopolo
Copy link

  • rustup toolchain ensure [path to rust-toolchain.toml] installs according to the specified file

I want to chime in with a big yes for this. One reason I've been holding off on moving from the legacy rust-toolchain file to rust-toolchain.toml is because I rarely want CI to install all of the components that are nice to work by default in local development scenarios.

If I could stash a CI-specific toolchain file in .github/rust-toolchain-clippy.toml or similar, I'd be able to rustup toolchain ensure .github/rust-toolchain-clippy.toml, that would let me use the new format and still use more minimal toolchain files in CI environments.

On a similar note, some of my projects use e.g. nightly rustdoc for building crate docs or nightly rustfmt which I invoke like:

rustup run --install nightly cargo doc --workspace
rustup run --install nightly cargo fmt -- --color=auto

Unfortunately this invocation doesn't let me specify components to install on the nightly toolchain and makes it more difficult to onboard new contributors to local development.

Is something like rustup toolchain ensure path/to/toolchain.toml run ... something we can consider here too?

@eminence
Copy link

eminence commented Mar 31, 2022

just use --default-toolchain none then ?

Fantastic, works as expected. Thank you!

@rbtcollins
Copy link
Contributor

@lopopolo at least in my view, the toolchain file shouldn't be used to specify components for developers: there is plenty of legitimate variance (e.g. docs for windows users are expensive in terms of performance, docs for unix machines are not very useful when the developer is using a remote system (e.g. vscode remote or intellij remote) to develop, and their browser is not local.

The toolchain is a very blunt hammer, and as such one should be minimalist with its use, which lines up with your desire to only install some components in CI.

Using it for trivial setups like 'stable' or '1.54' won't - ever IMO - work well.

The rust-version entry in cargo.toml is a much better fit for developer experience: it ensures a minimum version, not an exact version.

I think its quite certain we will do an active verb to install a toolchain compatible with the local directory, coupled to the work to stop e.g. 'rustup show' implicitly installing things.

Having that optionally accept a toolchain file seems reasonable too, but I don't expect that to resolve the tension folk are feeling when they are using toolchain files today.

@jmaargh
Copy link
Author

jmaargh commented Nov 9, 2023

Any thoughts on the plan from this post? I'd be happy to work on a PR if this is agreed (modulo bikeshedding on the exact subcommand name, which can be changed very easily).

Twey added a commit to Twey/linera-protocol that referenced this issue Nov 16, 2023
Moves the nightly toolchain we use for linting out of a GitHub action
and into a dedicated `rust-toolchain.lint.toml` file.

rustup doesn't currently support specifying a toolchain file, though
[it will soon](rust-lang/rustup#2686), so we
hack around it by moving the toolchain file back and forth as part of
the GitHub action for Rust.
Twey added a commit to linera-io/linera-protocol that referenced this issue Nov 17, 2023
* Codify linting toolchain in a toolchain file

Moves the nightly toolchain we use for linting out of a GitHub action
and into a dedicated linting `rust-toolchain.toml` file.

rustup doesn't currently support specifying a toolchain file, though
[it will soon](rust-lang/rustup#2686), so we
hack around it by keeping the toolchain files in separate directories.

* Add a Nix devShell for the linting toolchain
@djc
Copy link
Contributor

djc commented Nov 20, 2023

@jmaargh I think adding an ensure command like that would make sense, yes.

Twey added a commit to Twey/linera-protocol that referenced this issue Nov 20, 2023
Moves the nightly toolchain we use for linting out of a GitHub action
and into a dedicated `toolchains/lint/rust-toolchain.toml` file.

rustup doesn't currently support specifying a toolchain file, though
[it will soon](rust-lang/rustup#2686), so we
hack around it by moving the toolchain file back and forth as part of
the GitHub action for Rust.
@jmaargh
Copy link
Author

jmaargh commented Nov 20, 2023

@djc Are you a maintainer? I don't want to start working until maintainers have agreed in principle.

@rami3l
Copy link
Member

rami3l commented Nov 21, 2023

@djc Are you a maintainer? I don't want to start working until maintainers have agreed in principle.

Yes (and so am I). Please go ahead with it! 🙏

Twey added a commit to Twey/linera-protocol that referenced this issue Nov 21, 2023
Moves the nightly toolchain we use for linting out of a GitHub action
and into a dedicated `toolchains/lint/rust-toolchain.toml` file.

rustup doesn't currently support specifying a toolchain file, though
[it will soon](rust-lang/rustup#2686), so we
hack around it by moving the toolchain file back and forth as part of
the GitHub action for Rust.
Twey added a commit to Twey/linera-protocol that referenced this issue Nov 21, 2023
Moves the nightly toolchain we use for linting out of a GitHub action
and into a dedicated `toolchains/lint/rust-toolchain.toml` file.

rustup doesn't currently support specifying a toolchain file, though
[it will soon](rust-lang/rustup#2686), so we
hack around it by moving the toolchain file back and forth as part of
the GitHub action for Rust.
Twey added a commit to Twey/linera-protocol that referenced this issue Nov 21, 2023
Moves the nightly toolchain we use for linting out of a GitHub action
and into a dedicated `toolchains/lint/rust-toolchain.toml` file.

rustup doesn't currently support specifying a toolchain file, though
[it will soon](rust-lang/rustup#2686), so we
hack around it by moving the toolchain file back and forth as part of
the GitHub action for Rust.
Twey added a commit to linera-io/linera-protocol that referenced this issue Nov 22, 2023
* Codify linting toolchain in a toolchain file

Moves the nightly toolchain we use for linting out of a GitHub action
and into a dedicated `toolchains/lint/rust-toolchain.toml` file.

rustup doesn't currently support specifying a toolchain file, though
[it will soon](rust-lang/rustup#2686), so we
hack around it by moving the toolchain file back and forth as part of
the GitHub action for Rust.

* Update code for recent clippy

* Update code for recent rustfmt

* Add a Nix devShell for the linting toolchain
@rami3l rami3l modified the milestones: 1.25.0, 1.28.0 Jan 17, 2024
@rami3l rami3l self-assigned this Aug 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants