Recursive tool invocations should invoke the proxy, not the tool dire… #812

brson · Nov 15, 2016

…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

michaelwoerister · Nov 15, 2016

Nice! :D

brson · Nov 16, 2016

Pushed a test commit to try to understand what's going on on the bots.

brson · Nov 17, 2016

OK, I think I've got the test written correctly.

brson · Nov 17, 2016

Legit failures on windows.

brson · Nov 18, 2016

Well, toolchain link seems pretty broken on Windows. I can't get it to work manually. Windows doesn't seem to understand the directory junction. I'm not sure why the rest of the link tests are working.

brson · Nov 18, 2016

I think I'm just going to clean this up and merge the fix for unix, and figure out the windows junction problems later.

bors · Nov 21, 2016

☔️ The latest upstream changes (presumably #822) made this pull request unmergeable. Please resolve the merge conflicts.

brson · Nov 22, 2016

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 fs::metadata does not follow windows directory junctions as I expected, so the "does this toolchain exist" test always failed, then there was the junction bug @Diggsey fixed, and finally, I had to hack up the fallback code to copy cargo.exe to its own directory before running it - otherwise the windows process lookup order just always runs the rustc.exe next to it and doesn't bother searching the path.

So there's some hacks here.

r? @alexcrichton

brson · Nov 22, 2016

brson reviewed Nov 22, 2016

View changes

brson · Nov 22, 2016

src/rustup-cli/proxy_mode.rs

    } else {
-        env::args_os().take(1).chain(env::args_os().skip(2)).collect()
+        env::args_os().skip(2).collect()


These were incidental changes to improve error reporting in the proxies.

brson · Nov 22, 2016

brson reviewed Nov 22, 2016

View changes

brson · Nov 22, 2016

src/rustup/command.rs

@@ -1,8 +1,6 @@
-use std::env;


This file is again all just incidental stuff.

brson · Nov 22, 2016

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.

alexcrichton · Nov 22, 2016

alexcrichton reviewed Nov 22, 2016

View changes

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...

alexcrichton · Nov 22, 2016

src/rustup/toolchain.rs

+        } else {
+            false
+        };
+        utils::is_directory(&self.path) || is_symlink


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()?

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.

@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.

brson · Nov 22, 2016

Failure seems bogus.

brson · Nov 22, 2016

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...

That's what it should do, yes, and it used to, and this patch makes it do that again.

brson · Nov 22, 2016

The fallback is when rustup uses the nightly (fallback) cargo against a custom toolchain that doesn't contain cargo.

michaelwoerister · Nov 23, 2016

🎉

alexcrichton · Dec 13, 2016

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 cargo is invoked it no longer has the rustc bin directory in PATH. This means that when Cargo during the tests compiles a dynamically linked binary (dynamically against libstd) the binary is unable to actually find the standard library.

I'm... not entirely sure how to fix this, but I'll try to figure something out.

MoSal · Feb 28, 2017

This line causes a compile failure in latest nightly. s/&args/args fixes it.

brson force-pushed the brson:proxy-fix branch from 8cbe1de to c8dec07 Nov 17, 2016

brson referenced this pull request Nov 18, 2016
Closed
rustup-init x86_64-pc-windows-gnu reparse links created in msys2 shell corrupted #488

Diggsey referenced this pull request Nov 19, 2016
Merged
Canonicalize the path and then strip off the `\\?\` prefix before cre… #823

brson force-pushed the brson:proxy-fix branch from 0d82b65 to bb64297 Nov 22, 2016

brson force-pushed the brson:proxy-fix branch from bb64297 to fa8a05d Nov 22, 2016

brson force-pushed the brson:proxy-fix branch from fa8a05d to 87d9452 Nov 22, 2016

brson force-pushed the brson:proxy-fix branch from 87d9452 to a7cba0c Nov 22, 2016

brson merged commit 4d38610 into rust-lang:master Nov 23, 2016
1 of 2 checks passed

1 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

bors referenced this pull request Nov 23, 2016
Merged
Rename ~/.multirust to ~/.rustup #830

alexcrichton referenced this pull request Dec 13, 2016
Closed
`cargo test` incompatible with latest rustup #3394

brson referenced this pull request May 5, 2017
Merged
Add the rust lib dir (containing std-<hash>.dll) to the path on windows #1093

rust-lang/rustup.rs

Join GitHub today

Recursive tool invocations should invoke the proxy, not the tool dire… #812

Conversation

brson commented Nov 15, 2016

This comment has been minimized.

michaelwoerister commented Nov 15, 2016

This comment has been minimized.

brson commented Nov 16, 2016

brson force-pushed the brson:proxy-fix branch from 8cbe1de to c8dec07 Nov 17, 2016

This comment has been minimized.

brson commented Nov 17, 2016

This comment has been minimized.

brson commented Nov 17, 2016

This comment has been minimized.

brson commented Nov 18, 2016

brson referenced this pull request Nov 18, 2016

This comment has been minimized.

brson commented Nov 18, 2016

Diggsey referenced this pull request Nov 19, 2016

This comment has been minimized.

bors commented Nov 21, 2016

brson force-pushed the brson:proxy-fix branch from 0d82b65 to bb64297 Nov 22, 2016

This comment has been minimized.

brson commented Nov 22, 2016

brson reviewed Nov 22, 2016 View changes

This comment has been minimized.

brson reviewed Nov 22, 2016 View changes

This comment has been minimized.

brson force-pushed the brson:proxy-fix branch from bb64297 to fa8a05d Nov 22, 2016

This comment has been minimized.

brson commented Nov 22, 2016 • edited

brson force-pushed the brson:proxy-fix branch from fa8a05d to 87d9452 Nov 22, 2016

brson force-pushed the brson:proxy-fix branch from 87d9452 to a7cba0c Nov 22, 2016

alexcrichton reviewed Nov 22, 2016 View changes

alexcrichton left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Diggsey Nov 22, 2016 • edited

This comment has been minimized.

brson commented Nov 22, 2016

This comment has been minimized.

brson commented Nov 22, 2016

This comment has been minimized.

brson commented Nov 22, 2016

Hide details View details brson merged commit 4d38610 into rust-lang:master Nov 23, 2016 1 of 2 checks passed

1 of 2 checks passed

bors referenced this pull request Nov 23, 2016

This comment has been minimized.

michaelwoerister commented Nov 23, 2016

This comment has been minimized.

alexcrichton commented Dec 13, 2016

alexcrichton referenced this pull request Dec 13, 2016

This comment has been minimized.

MoSal commented on src/rustup/command.rs in a7cba0c Feb 28, 2017

brson referenced this pull request May 5, 2017

brson force-pushed the brson:proxy-fix branch from `8cbe1de` to `c8dec07` Nov 17, 2016

brson force-pushed the brson:proxy-fix branch from `0d82b65` to `bb64297` Nov 22, 2016

brson reviewed Nov 22, 2016

View changes

brson reviewed Nov 22, 2016

View changes

brson force-pushed the brson:proxy-fix branch from `bb64297` to `fa8a05d` Nov 22, 2016

brson commented Nov 22, 2016 •

edited

brson force-pushed the brson:proxy-fix branch from `fa8a05d` to `87d9452` Nov 22, 2016

brson force-pushed the brson:proxy-fix branch from `87d9452` to `a7cba0c` Nov 22, 2016

alexcrichton reviewed Nov 22, 2016

View changes

Diggsey Nov 22, 2016 •

edited

brson merged commit `4d38610` into rust-lang:master Nov 23, 2016
1 of 2 checks passed

MoSal commented on `src/rustup/command.rs` in `a7cba0c` Feb 28, 2017