Skip to content

[giflib] Changed library linkage to static.#5784

Merged
Rastaban merged 2 commits intomicrosoft:masterfrom
qis:dev/qis/giflib
Mar 27, 2019
Merged

[giflib] Changed library linkage to static.#5784
Rastaban merged 2 commits intomicrosoft:masterfrom
qis:dev/qis/giflib

Conversation

@qis
Copy link
Copy Markdown
Contributor

@qis qis commented Mar 23, 2019

The CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS flag prevents giflib from being built with a /GL and /LTCG toolchain.

Tested with this:
https://github.com/qis/toolchains/blob/075a467b671cc08b3f94c1f37ef503bc1627f2fe/windows.cmake

Please note that the portfile.cmake file does not explicitly set CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS, but it is set in giflib-5.1.4/CMakeLists.txt line 22 when BUILD_SHARED_LIBS is enabled.

I have found no other support for shared libraries on Windows in the giflib port. All functions are hard-coded as extern or don't have any linkage specifier at all.

@cenit
Copy link
Copy Markdown
Contributor

cenit commented Mar 23, 2019

I am a little bit against all this static linking you are pushing "everywhere".
It is not something I'd support, because static linking is not always an option. Also, reducing opportunities is not something in my mindset.
What is refraining you from just using your own triplet, instead of forcing the option loosing to everyone? In this case also it is even more disturbing, since the author itself was using this mechanism to create a shared library and we were not even applying it with a patch. I can't understand.

@qis
Copy link
Copy Markdown
Contributor Author

qis commented Mar 23, 2019

@cenit As I understand it from this discussion, WINDOWS_EXPORT_ALL_SYMBOLS is a hack with multiple drawbacks:

  • 65535 symbol limit
  • code bloat
  • reduced performance
  • incompatible with some optimizations

It would be interesting to know what is true and how many users (and potential users) of vcpkg need /LTCG support or are not OK with this hack.

@cenit
Copy link
Copy Markdown
Contributor

cenit commented Mar 23, 2019

The main question is: why should we block interested user from exploiting a dynamic link? Just create your own triplet, with static linking for libraries and dynamic for crt, and you should be ok, without limiting others. Or not? Interested users can already do it, without impacting people that does not need it or that require dynamic linking.
For code bloat and whatsoever, with gcc and shared options do something similar from the beginning (since the dll export concept and function visibility is something not native there), and nobody ever complained. Of course it is bloated with respect to selected export, but if it is the author choice (again in this case especially they are doing so) who are we to block it?!??

@qis
Copy link
Copy Markdown
Contributor Author

qis commented Mar 23, 2019

Yes, good points. I'm not sure what the exact differences to the Linux ELF model but I'm pretty sure that they are substantial.

By declaring BUILD_SHARED_LIBS the vcpkg already makes a decision. The authors do not declare their libraries as add_library(... SHARED ...), but chose to support this flag (which is not on by default).

Given the drawbacks of WINDOWS_EXPORT_ALL_SYMBOLS I'm not convinced that the authors actually know what they're doing. It could be that they do. Especially when it's a C library like in this case. But I doubt it.

@cenit
Copy link
Copy Markdown
Contributor

cenit commented Mar 24, 2019

On the contrary, since it is a c library, the symbol proliferation due to templates is not present. And I think the author may know better than us their product...
Again, as I already said, I don’t see any valid point on your side. No strong reason to remove dynamic linking to everybody since with a proper triplet you can have your necessities satisfied without limiting others. My vote would be to revert all your “static linking” recent commits...

@qis
Copy link
Copy Markdown
Contributor Author

qis commented Mar 24, 2019

@cenit While it would be reasonable to revert the tiff commit, I'd suggest you talk to other vcpkg team members before you revert the changes to the C++ libraries cpr and fmt.

@ras0219-msft What is your take on it?

@cenit
Copy link
Copy Markdown
Contributor

cenit commented Mar 24, 2019

Of course I can accept community decision, also this one. It is just a trend that I don’t like and I wanted to explain my position before it was too late. Another point: For now no big deal about licenses. But what about if you made only static a library whose licensing required me to open up the source code if static linked? vcpkg is used a lot also in private companies and there might be some serious concern about using it more, if you have to constantly check if libraries become only static, without any message, one day out of the blue, just because you wanted to update...

@qis
Copy link
Copy Markdown
Contributor Author

qis commented Mar 24, 2019

@cenit Since we have strict no (L)GPL license requirements, I personally wouldn't have touched those. But this is a very good point.

May I suggest adding some guidelines regarding this and similar issues to the documentation? Perhaps in the docs/maintainers/vcpkg_check_linkage.md file? I did not intend to step on anybody's toes.

@cenit
Copy link
Copy Markdown
Contributor

cenit commented Mar 24, 2019

Yep, I noticed no license damage for now. Not sure if it was intended or by chance, so I raised up my objection and wanted to make it clear, again, before it was too late. I agree with you that maybe some documentation or guideline or whatever might just be enough

@ras0219-msft
Copy link
Copy Markdown
Contributor

@cenit @qis Thanks for opening this discussion!

After considering the significant downsides of WINDOWS_EXPORT_ALL_SYMBOLS, we do not consider it valid shared library support on windows unless the author of the library explicitly wants to opt into it (and even in that case, we are largely against it).

This means for libraries like giflib where we are providing the cmake file, the lack of a .def file and a lack of __declspec() declarations means that the library simply does not support shared builds for windows. A hack that sometimes works is insufficiently high quality for vcpkg to declare support for that configuration.

The above said, it is important to enable users to opt into advanced or custom scenarios assuming they are fully aware of the downsides. In this case, it is as simple as removing the vcpkg_check_linkage() line and adding a -DCMAKE_WINDOWS_EXPORT_ALL_SYMBOLS=ON declaration to the vcpkg_install_cmake() command.

@Rastaban Rastaban merged commit d41a499 into microsoft:master Mar 27, 2019
@cenit
Copy link
Copy Markdown
Contributor

cenit commented Mar 27, 2019

Understood. Just to have it even more clear, does this mean that moving on every library in which we explicitly enable the CMake flag WINDOWS_EXPORT_ALL_SYMBOLS that receives an update should be cleaned up from it?

@ras0219-msft
Copy link
Copy Markdown
Contributor

Yes, for every library that we are providing buildsystem replacements for, we should avoid/remove uses of WINDOWS_EXPORT_ALL_SYMBOLS. If you'd like to do the grep+PR that would be awesome; otherwise we'll put it on our backlog to address once we've sufficiently drained the current PR queue.

I'll note that it (WINDOWS_EXPORT_ALL_SYMBOLS) only "activates" if you ask for a shared build, so we don't even necessarily need to remove it from the build scripts -- we just need to add vcpkg_check_linkage(ONLY_STATIC_LIBRARY) to the top.

@cenit
Copy link
Copy Markdown
Contributor

cenit commented Mar 28, 2019

You mean this one 8feeb8d ?
I did it mechanically few hours ago, while thinking for some real algorithms. It was a gift trying to convince you to review that PR, so that we can merge it...

I removed any reference to that symbol also from vcpkg-provided CMakeLists.txt, only a couple of patches adding it to original CMakeLists.txt are left.
Tomorrow another gift will land. It just keeps giving... 🤣

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