-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Add option to change the library naming scheme #15012
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A thing to decide before merging is whether we should keep the current scheme (
.a
on Windows) or switch to the platform default (.dll
for libs and.dll.lib
for import libs).
This isn't the platform default at all, though? We're just making up our own new convention that nobody uses, aren't we? Isn't it .lib
alone.
As far as I can tell this entire feature despite being cross platform basically exists solely to cater to windows users specifically. It should be a fatal error to set this property on Linux (or at least to change it from the default).
The purpose of setting it is really just to make users happy who insist that .a
should mean mingw-gcc+ucrt while .lib
should mean MSVC+ucrt. I'm not necessarily averse to making those users happy but it feels like a leaky abstraction if it affects other platforms too.
Making this change by default seems like it would break existing users who rely on the mingw linker search pattern support for detecting libraries by name rather than filepath (and which currently supports both lib*.a
and *.lib
, doesn't it?)
This just copies the way rustc does it. Which is the most reasonable way of making it work in a cross platform way. |
Actually no, for example on macOS you might want to use .so instead of .dylib sometimes. |
79030ff
to
88741ce
Compare
Now tests pass, the clang ones are the same as in master. Questions still to answer:
|
Doesn't cygwin install libraries with cyg as the prefix? Also, iOS is different from macOS in that it only uses -dynamiclib; I am not sure if that means that shared_module uses .dylib on iOS, and that there should be different naming conventions, because I am not sure what's the rationale for this feature. Is there an issue here on GitHub describing the problem you're trying to solve? The documentation is extremely scant in both the what and the why departments.
Maybe if you forbid both_libraries or default_library=both. But again, difficult to answer without knowing why you need this in the first place. |
I'll take your word for it. In that case it is still a logical error to set this option on a Linux host machine, and I believe we must reflect that by making it a setup-time fatal error to attempt to set it in that configuration. ... I am nonetheless surprised to hear that macOS users need to select this at setup time. I could hear an application itself being picky, but in that case I'd expect them to set name_suffix directly and not let end users set the "wrong" values. |
The problem is that some Windows developers really want to have the "platform standard" naming scheme where static libraries have the suffix You can't really know the correct extension in advance, either, there are people who use MinGW to build libs that will be consumed by MSVC projects. It's all terribly complicated. A reasonable compromise is to do what Rust does and make the import library have the extension '.dll.lib`. But that would be a backwards incompatible change. Ergo this MR. |
Ok, then I think for this feature to be complete:
|
We can add new option value later, but those two seem a bit inadequate. Should the "platform" one do |
88741ce
to
358cb5c
Compare
Changed to align the recommended way. Is this enough for Windows devs or will someone still insist on having plain |
The conversion from "platform" to dictionary key should be in a separate function. I would change Also it's not clear to me why it's ok to change the default, I will trust you :) but maybe the implications of the change should be in the release notes? |
An open question is whether this should be changed now or later. The new option is "better" in the sense that is more native to each platform. Maybe we can add this now and change the default in 1.11 or something like that? |
74e1a87
to
3516276
Compare
I don't think we can reasonably change the default until we do a 2.0, the change is going to break some project somewhere, and if an upstream really wants to rely on the new setting they can add it to their |
3516276
to
0b8137b
Compare
0b8137b
to
1652342
Compare
Made |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
1652342
to
86a3452
Compare
With this you can choose the library naming scheme regardless of the platform used.
A thing to decide before merging is whether we should keep the current scheme (
.a
on Windows) or switch to the platform default (.dll
for libs and.dll.lib
for import libs).