Skip to content

Make Windows directory layout uniform with everything else #29500

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

Merged
merged 2 commits into from
Nov 3, 2015

Conversation

vadimcn
Copy link
Contributor

@vadimcn vadimcn commented Nov 1, 2015

According to a recent discussion on IRC, there's no good reason for Windows builds to store target libraries under bin, when on every other platform they are under lib.

This might be a [breaking-change] for some users. I am pretty sure VisualRust has that path hard-coded somewhere.

r? @brson

@nagisa
Copy link
Member

nagisa commented Nov 1, 2015

How does this interact with library search? In my memory libraries on windows would be put alongside the executable, so meddling with library search paths would be unnecessary.

@retep998
Copy link
Member

retep998 commented Nov 1, 2015

@nagisa I believe this does not change the location of the DLLs for rustc itself, but rather changes the location of rustlib which is the target specific libraries.

@vadimcn
Copy link
Contributor Author

vadimcn commented Nov 1, 2015

Yah, dlls are still under bin

@@ -295,7 +294,7 @@ fn find_libdir(sysroot: &Path) -> String {

#[cfg(windows)]
fn find_libdir(_sysroot: &Path) -> String {
"bin".to_string()
"lib".to_string()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think with this change we could actually remove the #[cfg] from this function altogether perhaps?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to do lib32/lib64 probing on Windows? I suppose there's no harm in that though...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nah not as far as I know at least, it'd basically just be a noop there

@alexcrichton
Copy link
Member

r+ from me, thanks @vadimcn!

@vadimcn
Copy link
Contributor Author

vadimcn commented Nov 2, 2015

cc @vosen

@vadimcn
Copy link
Contributor Author

vadimcn commented Nov 2, 2015

@bors: r=alexcrichton 8cf50bc

bors added a commit that referenced this pull request Nov 3, 2015
According to a recent [discussion on IRC](https://botbot.me/mozilla/rust-tools/2015-10-27/?msg=52887517&page=2), there's no good reason for Windows builds to store target libraries under `bin`, when on every other platform they are under `lib`.

This might be a [breaking-change] for some users.  I am pretty sure VisualRust has that path hard-coded somewhere.

r? @brson
@bors
Copy link
Collaborator

bors commented Nov 3, 2015

⌛ Testing commit 8cf50bc with merge 749625a...

@bors bors merged commit 8cf50bc into rust-lang:master Nov 3, 2015
@brson
Copy link
Contributor

brson commented Nov 4, 2015

I wish I had seen this before it landed. I do not fully understand the fallout offhand.

@brson
Copy link
Contributor

brson commented Nov 4, 2015

My main concern was how this would affect rust-installer/rustup, but after a quick look I think they will not be impacted.

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.

6 participants