Add path override to rust-toolchain.toml

[jonhoo]

This PR implements path-based toolchain overrides from rust-toolchain.toml.

Specifically, it allows rust-toolchain.toml to specify:

[toolchain]
path = "./ephemeral/toolchain/"

Which will cause rustc to invoke ephemeral/toolchain/bin/rustc, and
similarly for all other parts of the toolchain. Relative paths are resolved
relative to the .toml file (as opposed to the current working directory).

Replaces #2471.
Fixes #2458.

Unresolved questions

• What should the name of a path toolchain be? This PR currently uses the last
  component of the path, but there's also a decent argument that it should
  instead be the whole path.

[anp]

What should the name of a path toolchain be?

Maybe it should be configurable via a required option? Or maybe it should be the name of the directory that hosts the rust-toolchain file rather than the path of the toolchain itself? The toolchain file in my use case is likely to be an immediate child of a meaningfully named directory.

[kinnison]

There's a few tweaks I'd like to see, but over-all I think this is excellent.

[kinnison]

The ideas presented for the name of the toolchain are interesting.

In some senses I like the idea of the basename of the toolchain path since that'd often be semi-useful; but so would calling it after the directory containing the rust-toolchain.toml

I think that the name has to be either the path to the toolchain or else the path to the rust-toolchain.toml because ultimately it gets put into RUSTUP_TOOLCHAIN in the environment which will then be used subsequently by proxies. I think we possibly don't verify that in the test suite very well, beyond setting it to verify the outer proxy uses it.

It's possible that cargo won't be bitten by this, but I can't guarantee that rls or rust-analyzer won't be. At minimum I'd like an assertion that you've done a reasonable amount of testing for this, or that you're confident it's covered by something in the existing test suite.

[rbtcollins]

I think the toolchain name must be like linkable toolchains no? Not stable, not 1.3.0, etc. Rob

…

On Tue, 23 Feb 2021, 20:35 Daniel Silverstone, ***@***.***> wrote: The ideas presented for the name of the toolchain are interesting. In some senses I like the idea of the basename of the toolchain path since that'd often be semi-useful; but so would calling it after the directory containing the rust-toolchain.toml I think that the name has to be either the path to the toolchain or else the path to the rust-toolchain.toml because ultimately it gets put into RUSTUP_TOOLCHAIN in the environment which will then be used subsequently by proxies. I think we possibly don't verify that in the test suite very well, beyond setting it to verify the outer proxy uses it. It's possible that cargo won't be bitten by this, but I can't guarantee that rls or rust-analyzer won't be. At minimum I'd like an assertion that you've done a reasonable amount of testing for this, or that you're confident it's covered by something in the existing test suite. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#2678 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADZ7XWLGBYMJ53FTZBTR53TAP7RXANCNFSM4YBVGXRA> .

[jonhoo]

The ideas presented for the name of the toolchain are interesting.

In some senses I like the idea of the basename of the toolchain path since that'd often be semi-useful; but so would calling it after the directory containing the rust-toolchain.toml

I think the current naming scheme (basename of the toolchain path) is indeed the right way to go. My reasoning here is that while the basename of the dir that holds rust-toolchain probably has a "good" name, it's not specific to the toolchain. It would mean that my toolchain gets named after my project rather than the toolchain I'm pointing that project at. If I have a project foobar whose rust-toolchain.toml points to path = "/my/toolchains/special-nightly", it makes more sense to me for the toolchain to be named "special-nightly" (as it currently is) than "foobar".

I think that the name has to be either the path to the toolchain or else the path to the rust-toolchain.toml because ultimately it gets put into RUSTUP_TOOLCHAIN in the environment which will then be used subsequently by proxies. I think we possibly don't verify that in the test suite very well, beyond setting it to verify the outer proxy uses it.

The tests did check the toolchain name indirectly through error messages, but I've added an explicit test for rustup show active-toolchain.

It's possible that cargo won't be bitten by this, but I can't guarantee that rls or rust-analyzer won't be. At minimum I'd like an assertion that you've done a reasonable amount of testing for this, or that you're confident it's covered by something in the existing test suite.

I'm not sure I follow exactly what you're asking for here? What problem are you anticipating?
Maybe it'd be good to loop in @matklad here who knows the ecosystem well.

I think the toolchain name must be like linkable toolchains no? Not stable, not 1.3.0, etc.

Hmm, why is that?

[kinnison]

What I'm imagining is that a program is run by a proxy (cargo, rls, etc) and then that calls rustc or cargo or somesuch). The subordinate invocation of a rustup proxy uses the RUSTUP_TOOLCHAIN environment variable set by the outer invocation to determine what toolchain to run, assuming I'm reading the setup in Cfg properly.

[jonhoo]

Ohh, I see what you mean. Yeah, I think you're right then that the toolchain name needs to be the full path and needs to be settable through RUSTUP_TOOLCHAIN. I'll fix that.

[jonhoo]

Pushed a fix that should address that use-case. I also tested it with cargo --call-rustc for proxying, but not sure if that's what you were after?

[jonhoo]

I have no idea what's going on with the Windows paths here: https://github.com/rust-lang/rustup/pull/2678/checks?check_run_id=1966265648#step:14:516

The failure appears to be that Windows just doesn't handle path_a.join(path_b) when path_b is a relative path starting with .., but maybe I'm missing something? I don't have a Windows computer available to me, so debugging it is a pain since I have to go through CI. Anyone care to take a look?

[jonhoo]

Only tangentially related, but I wonder if it'd make sense for rustup to use camino for UTF-8 paths...

[matklad]

Drive by comment, might be completely irrelevant.

"\\\\?\\D:\\a\\rustup\\rustup\\target\\x86_64-pc-windows-msvc in the CI output looks weird. That's an UNC windows path, and those are known to cause all kinds of problems. It most likely comes from fs::canonicalize call somewhere, and it's better to avoid it. For exacple, to get an absolute path the better pattern is usually current_dir().join(&path) rather than path.canonicalize(). See also https://github.com/rust-lang/cargo/blob/1f6c6bd5e7bbdf596f7e88e6db347af5268ab113/src/doc/contrib/src/architecture/files.md#filesystems

[jonhoo]

Good instinct! Yes, looks like rustup does canonicalize the path to the rust-toolchain file here:

rustup/src/config.rs

Line 560 in fc2f6b1

let dir = utils::canonicalize_path(dir, notify);

Which then produces a \\?\ style path, which in turn doesn't work when joined with a ..\ path as used in the relative path override test. I've ignored the test for now with a comment to that effect.

@kinnison Do you know why we canonicalize the path in that location? Is it necessary? Might fix up the other tests that are currently ignored due to UNC as well.

[kinnison]

That particular canonicalisation is for the override finding. It'd not be unreasonable to maintain the uncanonicalised copy there for the purpose of better joining later I guess.

Interestingly that means that on Windows, all override paths are stored as UNC paths in the settings file.

So long as you maintain a parallel d.parent() chain for the uncanonicalised path you should be able to pass that in when constructing the toolchain override file.

[jonhoo]

I guess my question is more why we canonicalize the path in that location? If that didn't canonicalize, then I think everything would just work. Is there a reason why canonicalization is needed? From what I can tell, it's always been there since the commit in which we first got support for a toml file:

rustup/src/rustup/settings.rs

Line 77 in d1ae0eb

let dir = utils::canonicalize_path(dir_unresolved, ntfy!(&notify_handler));

Carrying around the uncanonicalized path too is possible, though it'd make the traversal to find the file hairier, and would make OverrideReason::ToolchainFile more complicated.

[jonhoo]

@kinnison Gentle bump on ^

[kinnison]

By my understanding, and I'll admit I'd need to re-study this part of the codebase to remember, the canonicalisation is because on at least some platforms there may be things like symlinks involved in CWD et al, and the goal would be to ensure that stored overrides use the canonical path so that they're not confused by such things. We could possibly make the calling of the canonicalisation functions conditional on not(windows) but I'd like to understand what that might do. I'm not normally a Windows based developer so we could do with someone who uses rustup on Windows to have a looksee at how that might affect matters. @rbtcollins Do you have time to have a poke at this?

[jonhoo]

I feel like the real answer here is probably to canonicalize the paths only when they are exposed (like printed or written out into a file), and not in internal representations.

[kinnison]

We also will need to canonicalise for comparison but yes that could work.

[jonhoo]

Pushed a commit that makes that change, so let's see how it goes!

[jonhoo]

That seems to have worked — it helped that I only changed the canonicalization of the toolchain file path, which doesn't appear in that many places.

[jonhoo]

@kinnison Gentle ping again (please do let me know if I shouldn't ping you explicitly — I figured your notification inbox may be quite busy with auto-notifications, so thought an explicit mention might add some signal).

[rbtcollins]

Pinging us is fine - we're stuck at the moment - not enough contributors to respond quickly, which puts people off becoming contributors :/. I will try to get to this soonish if Daniel doesn't.

[kinnison]

I apologise for the delay, I was testing all this under Linux, WSL, and Windows.

It all seems to behave very well.