Accept TOML format in rust-toolchain #2438
Conversation
|
Not sure about the macOS CI failure as I can't see the logs 🤔 |
|
Builder probably flaked, just run it again
…On Fri, 24 Jul 2020, 10:32 ebroto, ***@***.***> wrote:
Not sure about the macOS CI failure as I can't see the logs 🤔
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#2438 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AADZ7XSFKR7ASWFNOICTL43R5C3BPANCNFSM4PGEQPKA>
.
|
There was a problem hiding this comment.
Over-all I'm very impressed. The single main concern I have is that I think it'd be beneficial to make channel optional so that people can use this new format to specify components/targets without constraining the channel of the toolchain. With that, a bit of refactoring would be appropriate since the result of finding an override configuration might be that there's no toolchain override (though that might come from elsewhere) but there might be components/targets to install.
In addition, the integration tests could be tweaked to be more correct, less prone to risk of silently becoming worthless in the future, and possibly quicker to run, with the tweaks I mention in comments.
This is an excellent first pass. What do you think about my 'optional channel' point?
- Make channel optional in the TOML toolchain file. - Avoid exposing OverrideCfg outside the `config` module. - Rework tests to avoid installing multiple toolchains unnecessarily and improve test naming to express better the intent. - Restore unrelated extra whitespace.
|
I think making the channel optional is a great idea, thanks for the suggestion! I've also tweaked the integration tests with your insights. I'll leave some comments to clarify some decisions I made. |
There was a problem hiding this comment.
I think this is looking pretty good now. @rbtcollins Do you have time to skim?
When trying to detect the format of the toolchain override file, let serde do all the work. If the file does not parse as TOML, fallback to the legacy format, but keep the TOML error. If the first line does not happen to be a valid toolchain, show also the TOML error so that TOML syntax errors are reported.
66: Pin toolchain to specific nightly version r=stlankes a=jschwe This allows us to specify a specific version and needed components in rust-toolchain that rustup can then automatically install when running e.g. `cargo build`. This needs a release of rustup that contains rust-lang/rustup#2438. The current version does not contain this yet, so this is PR is currently blocked (and also not tested). Benefits: - Pinning a specific compiler version ensures that building "always works". New nightly toolchains often can't build older versions of the rusty-hermit project - Required toolchain components are automatically installed by rustup. Co-authored-by: Jonathan Schwender <schwenderjonathan@gmail.com>
Closes #2350