-
Notifications
You must be signed in to change notification settings - Fork 317
Do not require dependencies for static linking when exiv2 was built as shared #2872
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
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
This is actually to be determined by the client linking to exiv2, not at exiv2 build time, no?
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.
Defining this based on whether the client is building shared libs is also wrong though, as it really depends on whether:
It's possible to want to link your own project as shared, but use one specific dependency via static linkage, e.g. I might want to distribute my own project that bundles a static exiv2 but links to a shared libcurl / OpenSSL for security-based platform policy reasons.
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.
Thanks for the feedback. So, maybe gate all w/ both
if(NOT (@BUILD_SHARED_LIBS@ OR BUILD_SHARED_LIBS)), or remove dependencies completely and and let integrator choose?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.
Well, the thing is that pkg-config basically defines a protocol for a project to say "I want to use exiv2 as a shared library" or "I want to use exiv2 as a static library", in the form of --static, so you can just stick it into Libs.private and be done.
For cmake, the file
/usr/lib64/cmake/exiv2/exiv2Export-relwithdebinfo.cmakecontains the filename e.g.libexiv2.so.0.28.3. So it depends on:CMake doesn't enable the workflow of choosing one specific dependency via static linkage. Even if you build exiv2 twice, so that you can install both static and shared libraries, you can only install one version of the
*.cmakefiles, but the pkg-config files can and do support both.If the cmake Config files are configured to hardcode a shared library (exiv2 itself is built with BUILD_SHARED_LIBS) then the shared library target doesn't need to depend on zlib.
If the cmake Config files are configured to hardcode a static library (exiv2 itself is built without BUILD_SHARED_LIBS) then the static library target does need to depend on zlib.
If you write your own handwritten FindExiv2.cmake handler, then it becomes more like pkg-config, and there's an interesting use case for allowing to configure the dependency itself to select between shared/static.
Uh oh!
There was an error while loading. Please reload this page.
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.
Thanks again for your insights. Given CMake's inability to cover both types elegantly, I'm then inclined to simply go with the gating as suggested by @pinotree here (you either get the deps search if you build/install a static exiv2, or you don't), and let the consumer deal with any other more complex scenarios w/ a custom
FindExiv2.cmake...(FYI, in MSYS2 we always build/install shared artifacts last, so there will be no search OOTB in this case.)
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.
Also note that I haven't seen an example of this gating elsewhere yet, most other projects also just gate it by the presence of the feature at build time, regardless of build type:
https://github.com/webmproject/libwebp/blob/e4f7a9f0c7c9fbfae1568bc7fa5c94b989b50872/cmake/WebPConfig.cmake.in
https://github.com/AcademySoftwareFoundation/openexr/blob/89fd37e4d24b5bf176d93ca2c5649e9cdd488fbd/cmake/OpenEXRConfig.cmake.in
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.
Those examples aren't perfect. They both refer to threads, which:
In general, if you add an "unneeded" recursive dependency when linking against a shared library, it is harmless from the perspective of whether the resulting client build succeeds or fails, but it will force over-linking the client. There is an actively detrimental effect here, in that your client library gets ELF
DT_NEEDEDentries for libpng16.so.16 and libexpat.so.1, both of which are already dependencies of exiv2, but crucially...... if libpng or Expat ever bump their ABI version, you must rebuild:
Over-linking can be manually solved by the client, by linking with
-Wl,--as-needed, that detects that the client doesn't utilize libpng or Expat and drops them from the direct ELF dependency metadataWhen statically linking, this point is moot as the client includes the object code of exiv2 and thus directly depends on all of exiv2's dependencies (this may mean using libpng or Expat as statically linked dependencies too, but it is irrelevant since either way it's a direct dependency).
So in general it's very much "nice to have" to avoid requiring dependencies when using a shared exiv2. This is part of how pkg-config works, with Requires.private. CMake also does this on its own -- if a dependency is marked as PRIVATE it is not exported when generating a cmake export file for a shared target.
But for static targets (
BUILD_SHARED_LIBS=OFF), it produces the necessary private dependencies for static linking:The issue arises because ZLIB::ZLIB etc are not defined unless you do find_package(), because cmake's notion of both exports and, generally, finding dependencies, is very clumsy. In fact, underlying this is the fact that cmake's notion of a programming language syntax is very clumsy. It predates recent innovations such as the 1979 invention of the Bourne SHell, with its support for innovative features like a real first-class array (argv is an array in shell!). It has no other types either. So ZLIB::ZLIB is "just a string" and nothing guarantees that it is attached to a definition. In contrast, pkg-config exports simply list the package in the "Requires.private", and pkg-config itself considers the package and expands its target libraries / interfaces as and where required.
Hence, linking to Exiv2 statically via
find_package(exiv2)may fail unless the client searches for its LINK_ONLY dependencies, or unless exiv2Config.cmake.in contains:which is what it already does, of course. But this PR seeks to disable that find_dependency() lookup in the event that exiv2Export.cmake doesn't actually utilize ZLIB::ZLIB (that is to say, when
install(TARGETS exiv2lib EXPORT exiv2Export)refers to a sharedexiv2lib).The PR is "correct" to do this when BUILD_SHARED_LIBS, because the cmake config files are "last one wins" and don't support searching for both. But it's not correct to change the pkg-config files, which do support both.
Other than the cost of the lookup, it also kind of doesn't matter in the end. You must have zlib installed if exiv2 is built against zlib, whether you use
find_dependency(ZLIB)or not.Uh oh!
There was an error while loading. Please reload this page.
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.
How about we gate those then w/ something like
if(EXIV2_USE_STATIC_LIBS)(along the lines of curl and openssl), and require the client to set it eplicitly?But I guess that still doesn't really solve that you can't support both export definitions, it only saves the lookup in the shared/default case (same as the PR)...
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.
@kmilos @eli-schwartz
Apologies for dredging this up, but I don't quite understand the assertion that CMake can't handle exporting configuration for both static and shared libs. Yes, you have to build twice, but both configurations can be installed side-by-side in a manner that allows either to be selected, either explicitly by the consumer project or automatically based on the current build configuration.
It even works with dependencies. Lee Thomason's
tinyxml2(https://github.com/leethomason/tinyxml2) provides a good template for how this can work.I've extended that with an example project that builds a library which depends on
libtinyxml2, and can similarly be installed in both configurations so that it's selectable by the consumer. (Like tinyxml2's own configuration, it uses CMake packageCOMPONENTSfor explicit selection of the library type. In the case of my "xmltest" library definition (which is the tinyxml2 test code, reworked into a library), it ends up callingfind_dependency(tinyxml2 REQUIRED COMPONENTS {shared|static}), depending whether its own shared or static configuration has been selected for importing.Like I said, you do have to build and install each library twice, separately. But if you install them to the same place, the exports work in tandem to provide a dual-mode library configuration.
See https://github.com/ferdnyc/cmake_dual_config_example for the sample project, the README goes through everything step-by-step.
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.
@ferdnyc your test project is broken. I only have tinyxml2 shared libraries -- I cannot build the xmltest library (I tried building it statically). I was figuring that since I have a system tinyxml2 package I would use that, but since nothing else uses xmltest I would use it statically.
Sadly it doesn't work.