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

specify required components in rust-toolchain file #2350

Closed
matthiaskrgr opened this issue May 29, 2020 · 11 comments · Fixed by #2438
Closed

specify required components in rust-toolchain file #2350

matthiaskrgr opened this issue May 29, 2020 · 11 comments · Fixed by #2438

Comments

@matthiaskrgr
Copy link
Member

I wanted to pin clippy to specific nightly version (which will be updated every time the subtree of the rustc repo is synced into the clippy standalone repo) to make sure a cargo build will always be able to build clippy.

I wanted to use a rust-toolchain file but it turns out, rustup only downloads default components for the specified version.
Clippy needs the rustc-dev component but there is no way to tell rustup to install that via rust-toolchain rendering it pretty useless for that use case. :(

It would be useful to be able to specify required components in the roolchain file.

@matthiaskrgr
Copy link
Member Author

It seem that rustup only parses the first line of the rust-toolchain file currently, so this might only be a matter of parsing additional lines and backwards-compatibility is already taken care of...
But I haven't looked at the corresponding rustup code yet to verify.

@kinnison
Copy link
Contributor

I can confirm that currently rustup reads one line from the file. So adding further content would be possible.

One option, however, could be to simply say that if the first line has any of [, , #, or = in it, then the file is a toml file which we can make extensible, otherwise it's a one-line override. It's important that we continue to support one-line overrides because we need to be compatible with all the documentation out there; but we probably don't need to make the new format "backward compatible" with the old format.

@kinnison
Copy link
Contributor

kinnison commented Jul 6, 2020

I am prepared to mentor someone through working out how to do this.

Find me here or on the #wg-rustup Channel on the discord server if you are interested in helping.

@ebroto
Copy link
Member

ebroto commented Jul 6, 2020

I am interested in working on this :)

@kinnison
Copy link
Contributor

kinnison commented Jul 6, 2020

@ebroto that's wonderful news. If you want to chat about it, please prod me on discord, otherwise feel free to outline a plan here first. I think we need to hash out how this will work before it's sensible to attempt any kind of implementation.

@Nemo157
Copy link
Member

Nemo157 commented Jul 13, 2020

As a quite drastic extension to this (that I think is probably out of scope, but maybe worth discussing); it can be useful to have different components coming from different toolchains. I noticed this in the context of rust-lang/rust (which isn't using a rust-toolchain file yet, but I think I've seen discussion proposing rearrangement of the source tree that would allow it): rust-lang/rust#74274.

@kinnison
Copy link
Contributor

rustup doesn't support heterogenous toolchains at this time, and right now I'm not sure we'd want to. I don't think we'll be ruling this out since we can include a version in the toml but we won't be aiming at it for now. It'll be interesting to think about it in the future though.

@jyn514
Copy link
Member

jyn514 commented Jul 13, 2020

feel free to outline a plan here first. I think we need to hash out how this will work before it's sensible to attempt any kind of implementation.

One option, however, could be to simply say that if the first line has any of [, , #, or = in it, then the file is a toml file which we can make extensible, otherwise it's a one-line override.

we probably don't need to make the new format "backward compatible" with the old format.

Since the new format doesn't have to worry about backwards compatibility, we can make things really easy on rustup. How about the following:

[rustup]
toolchain = "1.44.0"
components = ["rustfmt", ...]

If it starts with the literal line [rustup], then treat it as toml; otherwise treat it as the current one-line format. We can always expand the toml keys recognized in the future if we want to implement more features like @Nemo157 suggested. I'm open to bike-shedding on the names of the keys, toolchain and components seem pretty reasonably though.

@kinnison
Copy link
Contributor

I would like at minimum for there to be a moderately comprehensible error, and "[rustup]" is an invalid channel name so makes it pretty clear something unexpected is going on if someone has an old rustup and encounters a new rust-toolchain file. So this is a reasonable approach too. @ebroto Do you feel that @jyn514's point about not needing significant backward compatibility is reasonable? If so, then you can simplify your approach if you were still trying to support both rust-toolchain and some putative rustup.toml

@ebroto
Copy link
Member

ebroto commented Jul 14, 2020

Since there was no clear consensus in discord about having a rustup.toml file I already went for this solution. The section is named toolchain though, and the property is named channel, but this can be easily changed.

Requiring the section header to be in the first line would help to have a clear error when the file is intended to be TOML but there is a syntax error (e.g. you forget to put the channel name inside quotes) as there would be no fallback to the old format, but on the other hand it would not work if you have a simple comment in the first line. This makes me think that supporting two files could be more maintainable in the end, and probably clearer for the users. Thoughts?

@kinnison
Copy link
Contributor

I want a reasonable clear error for the user of old rustup encountering a new rust-toolchain file, because there are distros out there with rustup in them and there's a risk they'll not update quickly enough. However, error messages caused by malformed new-format rust-toolchain are fine and should be reported cleanly without worrying about trying to fall back.

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.

5 participants