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

Point PATH to toolchain/bin on Windows #402

Merged
merged 1 commit into from May 17, 2016

Conversation

Projects
None yet
4 participants
@SpaceManiac
Copy link
Contributor

SpaceManiac commented May 6, 2016

Yes, this again. On Windows, LD_LIBRARY_PATH and DYLD_LIBRARY_PATH are not obeyed, and .dlls are searched for in the PATH (and located in the bin directory).

This becomes relevant when attempting to link to a rustc_private crate such as libsyntax.

Prior art: brson/multirust#99

@brson

This comment has been minimized.

Copy link
Contributor

brson commented May 6, 2016

This env_var::set_path function prepends the path, which (as you mention in the multirust PR) will cause cargo to invoke rustc directly. This would break current and future features where rustup expects to intercept calls to rustc.

Related to this. Right now rustup requires either PATH to be set or RUSTC to be set in the environment, to ~/.cargo/bin, in order for cargo to find rustc. Normally this is set up by the installer but there are scenarios where you might want to run rustup without. I'm not sure this is the best thing to do, and rustup in proxy mode might instead set the PATH to include ~/.cargo/bin to ensure cargo finds the right one (it used to).

What if we prepended ~/.cargo/bin to the PATH and appended $toolchain/bin (on all platforms, for consistency)? With a comment like

Prepend $CARGO_HOME/bin so that cargo finds the rustc proxy even if it's not in PATH. Append $toolchain/bin so that, on Windows, build scripts that invoke plugins can find the rustc dylibs.

(I see that last time I wanted to prepend this path instead, but don't recall the reasoning).

cc @Diggsey I've tweaked, and broke and unbroke these env vars a lot. Seeing if you have opinions.

@brson

This comment has been minimized.

Copy link
Contributor

brson commented May 6, 2016

Or alternately we might append both, but in that order. By appending we could let users influence the binary lookup with PATH, by prepending we make a stronger effort to force cargo to find the right rustc.

@Diggsey

This comment has been minimized.

Copy link
Contributor

Diggsey commented May 6, 2016

I can't think of a better option. Appending both paths but in the correct order seems like the closest we're going to get.

@Diggsey

This comment has been minimized.

Copy link
Contributor

Diggsey commented May 6, 2016

(if the wrong rustc is on the PATH earlier, then that's a configuration error anyway, and rustc wouldn't be proxied correctly to start with, so I'm not too worried about supporting that usecase, whereas I am interested in supporting an intentional override via PATH)

@SpaceManiac

This comment has been minimized.

Copy link
Contributor Author

SpaceManiac commented May 9, 2016

I'm of course fine with whatever you think the correct behavior for the rest of the system is, as long as $toolchain/bin ends up somewhere in PATH.

@brson

This comment has been minimized.

Copy link
Contributor

brson commented May 10, 2016

@SpaceManiac let's rename set_path to prepend_path and add another method append_path, then use that to first append utils::cargo_home, then $toolchain/bin.

@SpaceManiac SpaceManiac force-pushed the SpaceManiac:windows-path branch from e001269 to d32d044 May 12, 2016

@SpaceManiac

This comment has been minimized.

Copy link
Contributor Author

SpaceManiac commented May 12, 2016

There's a second pass, append_path is a little awkward (especially in the error handling) due to its need to work with a list of paths, but it should be functional.

@brson brson force-pushed the rust-lang:master branch 2 times, most recently from 1acdeb2 to 9830d33 May 12, 2016

@brson

This comment has been minimized.

Copy link
Contributor

brson commented May 12, 2016

lgtm. Thanks. Just waiting for the build to pass.

@oli-obk

This comment has been minimized.

Copy link
Contributor

oli-obk commented May 14, 2016

The appveyor build was cancelled, can someone restart it?

@SpaceManiac SpaceManiac force-pushed the SpaceManiac:windows-path branch from d32d044 to 82165fb May 16, 2016

@brson brson merged commit 1a335ec into rust-lang:master May 17, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@brson

This comment has been minimized.

Copy link
Contributor

brson commented May 17, 2016

Thanks @SpaceManiac!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.