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

Add ability to specify rust language version #1863

Closed
wants to merge 1 commit into from

Conversation

e-nomem
Copy link

@e-nomem e-nomem commented Apr 4, 2021

This PR is probably a breaking change
Previously, using hooks written in rust only required a system installation of Rust and cargo, now it instead requires a system installation of rustup. rustup is the preferred method of installing rust so most users are probably using it already, but some installation methods will install Rust and cargo alone.

A quick background on rustup

When rustc, cargo, et al. are installed via rustup, the binary that is invoked is actually rustup itself. It then does the job of figuring out which toolchain to use and invokes the correct matching binary. It has some interesting behavior when making this decision:

  • If a toolchain is explicitly selected on the cli (e.g. cargo +stable install...) it will use the specified toolchain or error if it is not installed.
  • If a toolchain is selected via an override (there are a few different methods, but the relevant one here is the presence of a rust-toolchain file in the hook repo), it will use the specified toolchain or download it if it is not installed.
  • If no toolchain is selected explicitly or via an override, it will use the default toolchain if one exists, or error out.

This means that, as it is currently implemented, using pre-commit with rust hooks that specify overrides cause these override toolchains to be installed into the system installation, where they accumulate and live forever. This is actually what I was trying to fix originally in this PR, full support for specifying the language version turned out to be a beneficial side-effect.

Design

For backwards compatibility, C.DEFAULT is still the default version for the rust language, and it implies the use of the system default toolchain.

All public entrypoints into the rust language api start by resolving the actual toolchain version to be used, maintaining the precedence of pre-commit config > override file > default/system. If we actually resolve a version that is not C.DEFAULT, we set the env var RUSTUP_HOME to point at what is essentially our virtual environment. By resolving the version up front ourselves, we can always be explicit about which toolchain we are using and remove the ambiguity caused by rustup trying to resolve overrides.

rustup expects to be in charge of it's own data, so this PR has all the toolchains installed side-by-side into a top-level rustup directory within a prefix. Each individual hook is still installed into the appropriate rustenv-{version} directory. This data is all self contained within a prefix so all the downloaded toolchains can be properly discarded via pre-commit clean or pre-commit gc.

TODO

  • Tests

@asottile
Copy link
Member

asottile commented Apr 4, 2021

in the future it would be better to start a discussion in an issue first about a large feature rather than a PR (as it saves you whatever work you've done so far if incorrect). let me know if you want me to review what you have so far, otherwise I will wait for tests

@e-nomem
Copy link
Author

e-nomem commented Apr 4, 2021

Yep, I have some thoughts around some other improvements for rust language support that I planned to write up for discussion, but this particular part seemed a bit more important and straightforward. Also, while starting a discussion first may save me time by avoiding false starts, I'm also cognizant that it saves you time reviewing them as well 😅 , so I'll be sure to do that next time.

I'll get the tests updated later today, but if you have any thoughts around the general approach I've taken here, please let me know.

@asottile
Copy link
Member

hmm ok I think I'd like the behaviour to better match the other 1st class languages

that is:

  • a default_language_version implementation is added, this detects if rustc is available globally and returns system if it is, default if it isn't
  • if no rustc is available but there is a global rustup, default provisions a stable rustc inside the environment directory
  • if no global rustc or rustup are available provision a temporary rustup environment which bootstraps a stable rustc inside the environment directory
  • if language_version is specified, follow the same pattern except install that particular rustc

the node / ruby languages work ~mostly like this so those would be the ones to follow -- and if rust doesn't require a runtime (I think it does iirc?) then the rustc temporary installations can be deleted after the compilation (I forget specifically the requirements here, and don't have the time at the moment to do the research myself)

Holzhaus added a commit to Holzhaus/pre-commit that referenced this pull request Sep 30, 2022
This adds 1st-class language support for Rust in pre-commit. I tried to
implement the behavior as described here:
pre-commit#1863 (comment)
Holzhaus added a commit to Holzhaus/pre-commit that referenced this pull request Oct 2, 2022
This adds 1st-class language support for Rust in pre-commit. I tried to
implement the behavior as described here:
pre-commit#1863 (comment)
Holzhaus added a commit to Holzhaus/pre-commit that referenced this pull request Oct 2, 2022
This adds 1st-class language support for Rust in pre-commit. I tried to
implement the behavior as described here:
pre-commit#1863 (comment)
Holzhaus added a commit to Holzhaus/pre-commit that referenced this pull request Oct 2, 2022
This adds 1st-class language support for Rust in pre-commit. I tried to
implement the behavior as described here:
pre-commit#1863 (comment)
@Holzhaus
Copy link
Contributor

Holzhaus commented Oct 4, 2022

Superseeded by #2534.

@asottile asottile closed this Oct 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants