Recursive tool invocations should invoke the proxy, not the tool dire… #812
Conversation
|
Nice! |
|
Pushed a test commit to try to understand what's going on on the bots. |
|
OK, I think I've got the test written correctly. |
|
Legit failures on windows. |
|
Well, |
|
I think I'm just going to clean this up and merge the fix for unix, and figure out the windows junction problems later. |
|
☔ The latest upstream changes (presumably #822) made this pull request unmergeable. Please resolve the merge conflicts. |
|
OK, I've got a variation that works on windows now. Turned out to be 3 bugs preventing linked toolchains from working properly on windows! First, std's So there's some hacks here. |
| } else { | ||
| env::args_os().take(1).chain(env::args_os().skip(2)).collect() | ||
| env::args_os().skip(2).collect() |
There was a problem hiding this comment.
These were incidental changes to improve error reporting in the proxies.
| @@ -1,8 +1,6 @@ | |||
| use std::env; | |||
There was a problem hiding this comment.
This file is again all just incidental stuff.
|
Well, this doesn't pass the test suite... Edit: oh, I updated my toolchain and the test suite no longer passes because of assumptions it contained about the relative path between test exes and rustup-init.exe, so the test failures are on my end. |
…ctly The way the proxy was setting up the PATH variable contained two bugs: first, that it allowed the toolchain path to precede the value of CARGO_HOME/bin; but second that it didn't add CARGO_HOME/bin to the path at all. The result was that when e.g. cargo execs rustc, it was directly execing the toolchain rustc. Now it execs the proxy, assuming that CARGO_HOME/bin is set up correctly, and guaranteeing not to run the toolchain tool directly. Fixes rust-lang#809
There was a problem hiding this comment.
Seems ok to me, to be honest I'm not really sure what's going on here though. If it fixes things them I'm fine merging.
I guess I always figured that rustup proxies always had the highest priority in PATH and then from those bins we'd delegate further, but maybe that's naive? I also don't understand what a fallback is...
| } else { | ||
| false | ||
| }; | ||
| utils::is_directory(&self.path) || is_symlink |
There was a problem hiding this comment.
Hm I'm somewhat dubious about this change. Perhaps the error is that the call to fs::metadata returns false for is_directory? Couldn't function just be self.path.exists()?
There was a problem hiding this comment.
Er to elaborate, this is fine, but I feel like this is just jumping through too many hoops to conclude that something is a directory when all we're interested in is if a path exists or not.
There was a problem hiding this comment.
@alexcrichton The reason we care that it's a directory is because sometimes extra files get created (think thumbs.db or various dot files) and it's better if rustup doesn't confuse them for toolchains.
|
Failure seems bogus. |
That's what it should do, yes, and it used to, and this patch makes it do that again. |
|
The fallback is when rustup uses the nightly (fallback) cargo against a custom toolchain that doesn't contain cargo. |
|
🎉 |
|
Just logging this because it happened, but I don't think this is something we should change. I believe this commit broke Cargo's test suite unfortunately. Specifically when I'm... not entirely sure how to fix this, but I'll try to figure something out. |
…ctly
The way the proxy was setting up the PATH variable contained two bugs:
first, that it allowed the toolchain path to precede the value of CARGO_HOME/bin;
but second that it didn't add CARGO_HOME/bin to the path at all. The result
was that when e.g. cargo execs rustc, it was directly execing the toolchain
rustc.
Now it execs the proxy, assuming that CARGO_HOME/bin is set up correctly,
and guaranteeing not to run the toolchain tool directly.
Fixes #809
r? @Diggsey cc @japaric @michaelwoerister