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
Merged

Point PATH to toolchain/bin on Windows #402

merged 1 commit into from
May 17, 2016

Conversation

SpaceManiac
Copy link
Contributor

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
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
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
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
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
Copy link
Contributor Author

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
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
Copy link
Contributor Author

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 master branch 2 times, most recently from 1acdeb2 to 9830d33 Compare May 12, 2016 14:08
@brson
Copy link
Contributor

brson commented May 12, 2016

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

@oli-obk
Copy link
Contributor

oli-obk commented May 14, 2016

The appveyor build was cancelled, can someone restart it?

@brson brson merged commit 1a335ec into rust-lang:master May 17, 2016
@brson
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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants