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

Change name when outputting staticlibs on Windows #29520

Merged
merged 2 commits into from
Jan 21, 2016

Conversation

retep998
Copy link
Member

@retep998 retep998 commented Nov 2, 2015

I'm not sure if this was the best way to go about it, but it seems to work.

Fixes #29508

r? @alexcrichton

@retep998
Copy link
Member Author

retep998 commented Nov 2, 2015

Example showing rustc --crate-type=dylib,staticlib where foo.rs is built with this PR and bar.rs is built with a regular nightly.

@alexcrichton
Copy link
Member

I'm a little worried about this being a breaking change to existing tooling, for example this will break Cargo and presumably any existing scripts and such used on Windows today, such as some tests we have as well.

I sympathize though in that the "best name" here is probably libfoo.lib, but I want to be sure that we ward off breakage ahead of time.

@retep998
Copy link
Member Author

retep998 commented Nov 2, 2015

I'm not really sure what's a good way to handle the transition. Would Cargo have to first be modified to handle both cases? I could also change this PR so it only affects -msvc and not all of Windows, which wouldn't break all the people using msys and mingw.

I'll look into fixing up the tests in the meantime though.

@alexcrichton
Copy link
Member

Ideally on the Cargo side of things it would support both names to run with older and future compilers, but it would unfortunately still break all older Cargo instances working with newer compilers (which currently works today).

I never really fully understood these naming conventions for MinGW/MSYS, there certainly seem to be a lot of libfoo.a libraries lying around but I also vaguely remember discovering at some point that -lfoo to the linker will also pick up foo.lib. Regardless, at bare minimum, creating a staticlib with the name foo and then running gcc with -lfoo should continue to work in MinGW. If it works with either libfoo.a or foo.lib then it probably doesn't matter too much if we special-case MinGW.

@retep998
Copy link
Member Author

retep998 commented Nov 2, 2015

With -msvc, when creating a dylib it'll produce a foo.lib import library. Since .a is wrong for -msvc this means that when creating a staticlib the only choice is libfoo.lib, otherwise it would conflict with the dylib's import library. Do note that this means users would have to pass libfoo to the linker, they wouldn't be able to just pass foo, but they already can't do that with the existing wrong choice of libfoo.a so there's no worries about breaking that here.

For -gnu, when creating dylibs, no import library is produced. This means that we have several choices, libfoo.a which is what we currently use, foo.lib which would work with -lfoo but would be oddly inconsistent from the -msvc version which has the extra lib prefix, and libfoo.lib which would match -msvc but break -lfoo. So for -gnu I'm leaning towards sticking with libfoo.a since it doesn't cause inconsistencies and wouldn't break anything.

@alexcrichton
Copy link
Member

cc @vadimcn, @brson, curious on your thoughts here as well.

I'm fine leaving libfoo.a for MinGW and the method by which you pass libraries to link.exe is different enough that I'd also be fine with libfoo.lib being the name there. While a breaking change, I think it's one for the better and unlikely to break much in practice right now.

@vadimcn
Copy link
Contributor

vadimcn commented Nov 4, 2015

My first reaction is that I don't like the direction this takes us in terms of increasing the rift between -gnu and -msvc builds. We are gating too many things on the same flag: exception ABI, the linker used (what else?). Now it would also be the output names of the staticlibs. If we continue like this, we'll end up with two sets of everything on Windows forever...

Would it be feasible to tweak names of the import libraries instead?

@retep998
Copy link
Member Author

retep998 commented Nov 4, 2015

It is possible to tweak the name of the import library that is created.
link.exe /IMPLIB:somename
As for what it would be named, I don't know.

I went with the libfoo.lib for static libraries and foo.lib for dynamic libraries since that is the convention Microsoft follows for their libraries like libucrt.lib vs ucrt.lib and libvcruntime.lib vs vcruntime.lib.

@vadimcn
Copy link
Contributor

vadimcn commented Nov 4, 2015

What I don't like about lib<name>.lib is that name of the library is no longer the same as that of the crate, which makes -windows-msvc an odd-ball platform. Everywhere else, if you compile crate foo as a staticlib, you get a library you can link with -lfoo, but here it'd be -llibfoo.
I wouldn't mind it so much if this transformation applied to all platforms.

@retep998
Copy link
Member Author

retep998 commented Nov 4, 2015

-windows-msvc is already an odd-ball platform in that -lfoo never worked anyway since that's a gnu-style flag that doesn't work with link.exe. At the moment users have to either rename the resulting library or pass it explicitly as link.exe libfoo.a, both of which are far from ideal. Changing it to libfoo.lib would at the very least simplify it to link.exe libfoo and it wouldn't confuse Windows users that might not be familiar with the .a extension.

Any transformation which applies to all platforms would just cause a significant amount of breakage. I'd prefer if this only affected Windows platforms to minimize that.

@vadimcn
Copy link
Contributor

vadimcn commented Nov 4, 2015

Any transformation which applies to all platforms would just cause a significant amount of breakage. I'd prefer if this only affected Windows platforms to minimize that.

Yeah, I am not really proposing doing that, but hypothetically that would be one way to fix the problem and keep all platforms consistent.

Since implibs exist only on Windows, IMO it makes more sense to rename them, not normal libs.

We should also consider changing the -gnu toolchain to output libs in the <name>.lib format as well, since that's native to Windows and both link and ld understand it.

@retep998
Copy link
Member Author

retep998 commented Nov 4, 2015

What naming convention do you propose for the import library?

@vadimcn
Copy link
Contributor

vadimcn commented Nov 4, 2015

How about one of these?:
<name>.implib
<name>.imp.lib
<name>.dll.lib

@retep998
Copy link
Member Author

retep998 commented Nov 4, 2015

Of those options, I feel least opposed to <name>.dll.lib.

@retep998 retep998 force-pushed the staticlib-naming-fiasco branch from 6d59d98 to 6f28456 Compare December 5, 2015 01:56
@retep998
Copy link
Member Author

retep998 commented Dec 5, 2015

I've updated the PR to now make static libraries foo.lib on Windows always while the import library for Rust DLLs was renamed to foo.dll.lib. I'm still building the code and will comment further once testing confirms it works.

@retep998 retep998 force-pushed the staticlib-naming-fiasco branch from 6f28456 to bb63b15 Compare December 5, 2015 03:34
@retep998
Copy link
Member Author

retep998 commented Dec 5, 2015

Okay, it seems to be working now.

Feel free to bikeshed some more if you think this isn't the way to do it.

@retep998 retep998 force-pushed the staticlib-naming-fiasco branch from bb63b15 to 3e315d0 Compare December 5, 2015 03:36
@retep998
Copy link
Member Author

retep998 commented Dec 5, 2015

Image of how the output looks now when creating both a staticlib and a dylib.

@vadimcn
Copy link
Contributor

vadimcn commented Dec 5, 2015

lgtm

@retep998
Copy link
Member Author

retep998 commented Dec 5, 2015

Now to fix all the tests that expect things to have a different name...

@alexcrichton
Copy link
Member

Tagging this with T-tools to ensure this comes up during triage, I fear there are ramifications here that need to be handled beyond "just renaming artifacts" and I'd just want to make sure to get a few extra opinions.

@alexcrichton
Copy link
Member

We talked about this in the tools triage meeting today and the conclusion was to move forward with this. @retep998 feel free to ping when the tests are passing.

@retep998 retep998 force-pushed the staticlib-naming-fiasco branch 2 times, most recently from 9d55090 to 1188ae0 Compare January 15, 2016 20:12
@retep998
Copy link
Member Author

I have tests passing for x86_64-pc-windows-msvc. I am now working on x86_64-pc-windows-gnu.

libfoo.a -> foo.lib
In order to not cause conflicts, changes the DLL import library name
foo.lib -> foo.dll.lib

Fixes rust-lang#29508

Because this changes output filenames this is a [breaking-change]

Signed-off-by: Peter Atashian <retep998@gmail.com>
Signed-off-by: Peter Atashian <retep998@gmail.com>
@retep998 retep998 force-pushed the staticlib-naming-fiasco branch from 51f8131 to 1b0064e Compare January 16, 2016 17:36
@retep998
Copy link
Member Author

@alexcrichton Tests are passing on both targets locally. I've rebased and squashed the commits nicely. r?


fn build_dylib(&mut self, out_filename: &Path) {
self.cmd.arg("/DLL");
let mut arg: OsString = "/IMPLIB:".into();
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a comment here explaining why this flag is passed?

@alexcrichton
Copy link
Member

As I mentioned above as well I'm still worried that this patch will break Cargo, and after reviewing some of the related code I can confirm that it will indeed break Cargo on basically any MSVC build with a staticlib somewhere along the way.

I think that we'll need to figure out how to mitigate that breakage before we land this patch to the compiler.

retep998 added a commit to retep998/cargo that referenced this pull request Jan 20, 2016
Required for rust-lang/rust#29520

Signed-off-by: Peter Atashian <retep998@gmail.com>
bors added a commit to rust-lang/cargo that referenced this pull request Jan 21, 2016
@alexcrichton
Copy link
Member

Ok, with rust-lang/cargo#2299 proposed I'll just want to hold off on merging this until after the release, then this should ride the trains normally into the stable release just fine.

@alexcrichton
Copy link
Member

@bors: r+ 1b0064e

@bors
Copy link
Contributor

bors commented Jan 21, 2016

⌛ Testing commit 1b0064e with merge 34b4e66...

bors added a commit that referenced this pull request Jan 21, 2016
I'm not sure if this was the best way to go about it, but it seems to work.

Fixes #29508

r? @alexcrichton
@bors bors merged commit 1b0064e into rust-lang:master Jan 21, 2016
@nagisa nagisa added the relnotes Marks issues that should be documented in the release notes of the next release. label Jan 22, 2016
alexcrichton added a commit to alexcrichton/cargo that referenced this pull request Jan 25, 2016
A few small tweaks to tests were necessary to accomodate rust-lang/rust#29520,
and a few changes were made to the code to account for that as well.
bors added a commit to rust-lang/cargo that referenced this pull request Jan 25, 2016
A few small tweaks to tests were necessary to accomodate rust-lang/rust#29520,
and a few changes were made to the code to account for that as well.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes Marks issues that should be documented in the release notes of the next release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants