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

Allow specifying a custom toolchain in rust-toolchain? #2458

Closed
anp opened this issue Aug 14, 2020 · 6 comments · Fixed by #2678
Closed

Allow specifying a custom toolchain in rust-toolchain? #2458

anp opened this issue Aug 14, 2020 · 6 comments · Fixed by #2678

Comments

@anp
Copy link
Member

anp commented Aug 14, 2020

A project at work uses prebuilt toolchain binaries from internal infrastructure. We currently provide instructions on how to link that location with rustup for use with cargo and other tools. Ideally we'd be able to specify that relationship in version control and have rustup 'just work' when run within our directory tree.

I see that TOML support in the rust-toolchain file was recently merged (super exciting for other reasons too!) and I'm wondering whether it could be used to specify a "local" custom toolchain for a repository. Maybe something like this?

[toolchain]
path = "prebuilts/.../bin"
@kinnison
Copy link
Contributor

Nominally this is semi-supported by the rustup toolchain link ... pathways. However to make this particular use-case function would require the creation of a temporary toolchain whose path is not inside the rustup toolchain paths, not certain how complex that would be.

kinnison added a commit to kinnison/rustup that referenced this issue Aug 29, 2020
Signed-off-by: Daniel Silverstone (WSL2) <dsilvers@digital-scurf.org>
kinnison added a commit to kinnison/rustup that referenced this issue Aug 29, 2020
Signed-off-by: Daniel Silverstone (WSL2) <dsilvers@digital-scurf.org>
kinnison added a commit to kinnison/rustup that referenced this issue Sep 5, 2020
Signed-off-by: Daniel Silverstone (WSL2) <dsilvers@digital-scurf.org>
kinnison added a commit to kinnison/rustup that referenced this issue Nov 17, 2020
Signed-off-by: Daniel Silverstone <dsilvers@digital-scurf.org>
@jonhoo
Copy link
Contributor

jonhoo commented Feb 16, 2021

This would come in extremely handy for me as well, though for a fairly different use-case. In my case, I have a larger build system that sometimes needs to "fiddle some bits" before cargo/rustc gets to run. Currently, the way that works is that the user has to invoke the "main" build system, which then internally invokes cargo. This works, but means that tools that aren't aware of the larger build system (like rust-analyzer) do not invoke the build system, which may lead to new dependencies not being available and such.

One way I've tried to hack around this problem is to tell users to install a custom system-wide toolchain that's really just a thing wrapper around the default toolchain, and which invokes the "fiddle" part of the main build system when necessary before forwarding to cargo. That approach works ok, but it a) requires that users know about and install this system-wide toolchain, and b) it's now a non-local configuration for building projects, which can make debugging build problems harder.

But, with this feature, we could do much better. The first time you run the build system, it could set up a rust-toolchain for the project that points at a project-local toolchain that's the aforementioned wrapper. Then, when the user later runs cargo, rustc, or any other command, they'll automatically be routed through the wrapper script (and thus through the main build system) without having to do any special or system-wide setup!

@kinnison
Copy link
Contributor

@jonhoo That's a really interesting use-case for this approach. I was wondering what value a path based rust-toolchain file might be wrt. checked in files; but as part of a wider build system that makes a huge amount of sense. If you wanted to adopt the WIP PR I have and take it further toward something mergeable then I'd be super-grateful. I trust you to find the corner cases I've not thought of yet.

For now, you could have your build system set up the 'fiddling' toolchain itself and pop a rust-toolchain file down for that as part of the startup of the build system, to save your users having to know what to do.

It also sounds like perhaps having a path based rustup set override might need to be part of this, but I'm not certain and wouldn't want to conflate that with this change.

@jonhoo
Copy link
Contributor

jonhoo commented Feb 17, 2021

@kinnison I'd be happy to pick up the PR! Could you give me a rough idea of the state-of-affairs, next steps, and remaining/unresolved bits?

For now, you could have your build system set up the 'fiddling' toolchain itself and pop a rust-toolchain file down for that as part of the startup of the build system, to save your users having to know what to do.

This gets tricky quickly, since IIUC the toolchains are all global (well, user-global). But the user might have tons of packages checked out locally, potentially using different versions of the build system that may want to use slightly different wrappers. I could have the build system generate a randomly named toolchain, but then we're generating garbage in the user's configuration over time, which doesn't sound right either. That's why I'm so excited by the prospect of a project-local toolchain.

It also sounds like perhaps having a path based rustup set override might need to be part of this, but I'm not certain and wouldn't want to conflate that with this change.

I'm not sure why that would be necessary? Is creating the rust-toolchain file not sufficient for that? Or are you saying to ensure that the toolchain gets picked up "deeper down" as well. If the build system had to generate user-global overrides for each package, I'd worry about that list getting very long over time.

@kinnison
Copy link
Contributor

I think your use-case does indeed warrant the rust-toolchain approach so let's just consider that from now on. Thank you for clarifying things a little more for me.

So the work I did is in #2471 and in that, Jubilee had a really good point that currently rust-toolchain is defined as US-ASCII (though we effectively ignore that when trying for a toml parse). In #2653 a contributor is working toward supporting a rust-toolchain.toml file which would lift my concern around knowing file content encoding since that would very much be defined to be utf-8. Obviously this means that the path cannot contain non-unicode which is a bit of a pain, but likely only for a small number of people.

In my PR, I talked about thinking that cargo +/path/to/toolchain build was a valuable thing to support, but on further reflection I think encouraging that kind of difficult-to-track-down-potential-problems use-case is not ideal, so perhaps restricting the path form entirely to the toml variant is, in fact, a better bet.

Given that, I'd suggest that a good step would be (a) to remove support for the single-line rust-toolchain variant from the patch (which is in the ephemeral-linked-toolchain branch in https://github.com/kinnison/rustup if you want to clone from there) and then (b) consider how to construct some appropriate test cases. Given your past with that particular part of the codebase I have no fear that you'll be good at that.

Once that is all done, it'll be a case of updating the rustup book to explain the functionality, and we'll be good for review. I am happy to close my draft PR in favour of one you open, should you feel you still want to get this done.

(Of course, if you want, you can ignore the work I already did, and just build from fresh if you have a better idea of how to go about it, I'm very not precious about the WIP code I hacked together 😁 )

@jonhoo
Copy link
Contributor

jonhoo commented Feb 23, 2021

Thanks! PR up in #2678 :)

rbtcollins pushed a commit to rbtcollins/rustup.rs that referenced this issue Apr 27, 2021
Signed-off-by: Daniel Silverstone <dsilvers@digital-scurf.org>
rbtcollins pushed a commit to rbtcollins/rustup.rs that referenced this issue Jul 11, 2021
Signed-off-by: Daniel Silverstone <dsilvers@digital-scurf.org>
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.

3 participants