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

Teach rustup to override the toolchain from a version file #1172

Merged
merged 4 commits into from
Jun 24, 2017

Conversation

brson
Copy link
Contributor

@brson brson commented Jun 19, 2017

This adds a .rust-version file similar to rbenv's .ruby-version.

The form of toolchain supported here is limited to channel names,
explicit versions, and optional archive dates.

From the readme:

The version file

rustup directory overrides are a local configuration, stored in
$RUSTUP_HOME. Some projects though find themselves 'pinned' to a
specific release of Rust and want this information reflected in their
source repository. This is most often the case for nightly-only
software that pins to a revision from the release archives.

In these cases the toolchain can be named in the project's directory
in a file called .rust-version, the content of which the name of a
single rustup toolchain, and which is suitable to check in to source
control.

The toolchains named in this file have a more restricted form than
rustup toolchains generally, and may only contain the names of the
three release channels, 'stable', 'beta', 'nightly', Rust version
numbers, like '1.0.0', and optionally an archive date, like
'nightly-2017-01-01'. They may not name custom toolchains, nor
host-specific toolchains.

The version file has lower precedence than all other methods of
specifying the toolchain.


Some observations and questions about the design:

The file is called .rust-version with precedent from rbenv. The contents
of this file though would better be described as a "rust release channel",
or "rustup toolchain". A better name might be .rustup-toolchain though
the acceptable values in the file are a subset of valid toolchain names.

One might reuse the .cargo directory for this, putting the file in
e.g. .cargo/rust-version. This seems reasonable, and would establish
a strong precent for tools reusing .cargo; it would go against the
rbenv precedent. It would also go against the existing rustup
precedent where rustup stores it's global state in its own ~/.rustup
folder - but note that I have some small desire to move ~/.rustup
into ~/.cargo/rustup as a simplification.

Opinions about the above?

This doesn't allow specifying the host triple, nor does it allow
custom toolchains.

The search semantics are based off of the current working directory, though
one might feasibly expect them to be based off of the Cargo.toml directory.
That is cargo --manifest=foo/Cargo.toml will not search in foo. Existing
overrides have this limitation as well.

Fixes #460

cc @SimonSapin @Diggsey @alexcrichton @yazaddaruvala

@Diggsey
Copy link
Contributor

Diggsey commented Jun 19, 2017

This looks great to me.

I don't have particularly strong opinions about where the override should go, other than setting a precedent for storing stuff in .cargo seems... nice, and also avoids cluttering the root directory even further.

It does make me wonder whether it's worth in the future having rustup follow a similar configuration hierarchy to cargo.

@alexcrichton
Copy link
Member

A general rule of thumb with Cargo is that you shouldn't version control .cargo, and I think this'd want to be in version control, right? If that's the case I'd recommend avoiding .cargo/* but a dedicated file sounds ok.

This adds a .rust-version file similar to rbenv's .ruby-version.

The form of toolchain supported here is limited to channel names,
explicit versions, and optional archive dates.
@steveklabnik
Copy link
Member

I like the idea of naming it .rust-toolchain. I'd keep it in the project root for the reasons @alexcrichton mentioned above

README.md Outdated

The version file has lower precedence than all other methods of
specifying the toolchain.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just clarifying, given that the file has lower precedence (i.e. it might not be obvious when it is not used), does rustup show work as expected?

Is it worth re-stating:

To see the active toolchain use rustup show.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, rustup show will indicate that .rust-version is in play. I will update the docs as you suggest.

@yazaddaruvala
Copy link

Just for the sake of having said it, my personal preference would have been toml or json with only one property, just incase some valid future usecase arises to add other parts of the toolchain triple into .rust-version, or something we haven't thought of yet.

However, I understand that you may also want the code to be opinionated so it is harder to abuse this late or reduce feature requests which request to abuse this.

Having said that, this look really good, thank you!

@SimonSapin
Copy link
Contributor

Looks good. I’d even go with rust-version over .rust-version. There is no reason this file should to be hidden.

@nox
Copy link

nox commented Jun 20, 2017

I second @SimonSapin's comment, let's not repeat the error of Travis' taste for hidden files.

@metajack
Copy link

I think making this lowest precedence is going to leave people confused when they set an override some other way, and then try to use a .rust-version file. Others systems I'm aware of that use files have close ones taking precedence. Ie, local .bashrc overrides /etc/bashrc, etc. The closest analog I can think of right now is Apache's .htaccess, support for which must be separately enabled in the main config file. It's a very common problem that confuses many.

@SimonSapin
Copy link
Contributor

When compiling in /foo/bar, maybe the precedence should be:

  • Command line flag (e.g. cargo +nightly)
  • rustup override for /foo/bar/
  • /foo/bar/.rust-version
  • rustup override for /foo/
  • /foo/.rust-version
  • rustup override for /
  • /.rust-version

@brson
Copy link
Contributor Author

brson commented Jun 22, 2017

However, I understand that you may also want the code to be opinionated so it is harder to abuse this late or reduce feature requests which request to abuse this.

Yeah, that is my main reasoning, but also parsing toml is presumably more expensive than loading the string, and this is on a path that is supposed to be fast (though is increasingly bloated). If there is ever strong demand for yet more local rustup config, we can always add it. Just more stuff...

@nox The precedent for dotfiles here is pretty well established, by rbenv, by git, not travis particularly.

That said, I also think having a hidden file control what toolchain is invoked is pretty magical, and if we don't use the name 'rust-version', following rbenv's precedent, then we might as well abandon all precedent and just name the file 'rust-toolchain'.

Interleaving the 'override' / toolchain-file search is will require some major refactoring, but I'll look into it. I do think that arrangement sounds more difficult to explain in the documentation, though perhaps it's more intuitive.

@brson
Copy link
Contributor Author

brson commented Jun 23, 2017

I'm still feeling bikesheddy about the name. Possibilities:

  • rust-version
  • rust-toolchain
  • rustup-toolchain

@steveklabnik likes rust-toolchain which I had not considered before, since I consider toolchains, at least as they are known in rustup, a rustup concept, not a rust concept, but rust-toolchain is fine with me.

@brson
Copy link
Contributor Author

brson commented Jun 23, 2017

I'm inclined to drop the dot from the name.

@Mark-Simulacrum
Copy link
Member

I'm against rustup-toolchain since we might eventually have different toolchains or something like it for rustup. It leads me to believe that I can select a rustup version; also, this is something that should ideally be toolchain multiplexer agnostic -- some future rustreallyup will want to reuse these.

I think rust-toolchain makes the most sense, since we're not only versioning Rust itself, but all of its dependencies (rustdoc, rls, clippy in the future, etc.).

I'd go without the dot. There's no need to hide what version of Rust you use.

@est31
Copy link
Member

est31 commented Jun 23, 2017

I do know of project maintainers which want to keep their top directory clean and small (not me), so having a hidden file as option will certainly help. I guess it'd be okay if rust-toolchain remains the default endorsed method. We should probably error if both hidden and non hidden files exist. I also support rust-toolchain over something rustup specific as the version pinning might be used by different tools in the future. There are already similar usage patterns with gitignore, which increasingly gets used for non git purposes (like by ripgrep for example).

@metajack
Copy link

I do know of project maintainers which want to keep their top directory clean

Sounds like a minor annoyance to discourage you from using it. Or alternatively, a very obvious sign that you are doing something special. Both seem like features here.

@brson
Copy link
Contributor Author

brson commented Jun 23, 2017

Ok now the file is called 'rust-toolchain' and the precedence is as described by @SimonSapin / @metajack, where the closest override to the cwd wins.

@brson brson force-pushed the version-file branch 4 times, most recently from 091ee47 to 2f657df Compare June 23, 2017 01:16
Also interleave the directory override and rust-toolchain directory walk.
@brson
Copy link
Contributor Author

brson commented Jun 23, 2017

@bors r+

@brson
Copy link
Contributor Author

brson commented Jun 23, 2017

@bors r+

@brson
Copy link
Contributor Author

brson commented Jun 24, 2017

@bors asleep at the wheel.

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.

10 participants