Do not require dependencies for static linking when exiv2 was built as shared

[pinotree]

exiv2 can be built either as shared library, or static, never both at the same time. Hence, requiring the dependencies needed for static linking to exiv2 when exiv2 was built shared does not make much sense, and it only adds unneeded bits to the exiv2 users.

Hence, fix both the pkg-config file, and the cmake config file, to require the dependencies needed for static linking only in case exiv2 was built as static library. This avoids e.g. having the expat and zlib development bits when using exiv2 in cmake with find_package(exiv2).

See the specific commit messages for longer descriptions.

[ghost]

👇 Click on the image for a new way to code review

Legend

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 63.89%. Comparing base (b4f145e) to head (a1ff128).
Report is 139 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2872   +/-   ##
=======================================
  Coverage   63.89%   63.89%           
=======================================
  Files         103      103           
  Lines       22381    22381           
  Branches    10872    10872           
=======================================
  Hits        14301    14301           
  Misses       5857     5857           
  Partials     2223     2223           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[kmilos]

Some distros ship both static and shared libs, how would you handle that? We still need a single .pc file that handles both. If you don't pass --static to pkg-config there is no harm done, no?

[pinotree]

Some distros ship both static and shared libs,

Do you have examples of this? I checked Arch, Debian (which I maintain) and thus Ubuntu, Fedora, FreeBSD, Gentoo, Mageia, OpenBSD, openSUSE, Void, and other distros are derivatives of the ones mentioned (thus sharing the build configuration). All of the ones I inspected build using the default built type, which is shared, and no static bits are shipped (with the exception of libexiv2-xmp.a, gone in 0.28).

From what I can see (I also mentioned that in my commit messages), you can build only one variant at a time with cmake, and most likely the same with meson. Hence, if the situation you mention must be supported somehow, then I need to see exactly how and what for.

[eli-schwartz]

From what I can see (I also mentioned that in my commit messages), you can build only one variant at a time with cmake, and most likely the same with meson.

No, meson has always supported building both at the same time, and it does this quite well.

This is vital for cross-platform support since unix platforms frequently need this, and it works quite well there. It doesn't work quite so well on Windows (you either need to use .def files or compile every object twice, and then import libraries and static libraries both frequently use .lib) which is usually "this is fine" because on Windows you have to build private copies of your entire dep tree without benefit of a package manager.

And since CMake was primarily designed to work well on Windows, it assumes the Windows model and doesn't bother to support meson -Ddefault_library=both / autotools "--enable-static --enable-shared".

Distros such as Alpine and Debian often package both, when the build system supports both, but won't necessarily go out of their way to run cmake twice in order to build and install both.

[kmilos]

won't necessarily go out of their way to run cmake twice in order to build and install both

OTOH, we frequently do run CMake twice on MSYS2 and install both artifacts.

[eli-schwartz]

Very true, MSYS2 has a somewhat stronger motivation to provide both.

[kmilos]

Suggested change

	if(NOT @BUILD_SHARED_LIBS@) # if(NOT BUILD_SHARED_LIBS)
	if(NOT BUILD_SHARED_LIBS)

This is actually to be determined by the client linking to exiv2, not at exiv2 build time, no?

[eli-schwartz]

Defining this based on whether the client is building shared libs is also wrong though, as it really depends on whether:

• exiv2 itself has built shared libs
• the client selects to receive dependencies as shared libs

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.

[kmilos]

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?

[eli-schwartz]

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.cmake contains the filename e.g. libexiv2.so.0.28.3. So it depends on:

• whether exiv2 itself has built shared libs: YES
• the client selects to receive dependencies as shared libs: NOT A VALID DECISION

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 *.cmake files, 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.

[kmilos]

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.)

[kmilos]

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

[eli-schwartz]

Those examples aren't perfect. They both refer to threads, which:

• isn't actually a real "dependency", usually, but a kind of compiler option, so passing it is effectively harmless
• likely necessary if the library exposes threading in the API

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_NEEDED entries 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:

• exiv2 (it needs to use the new ABI)
• and also the client program (which doesn't use libpng or Expat in any way, but the library loader will error out if it cannot find the older library name)

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 metadata

When 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.

DESTDIR-shared/usr/lib64/cmake/exiv2/exiv2Export.cmake

# Create imported target Exiv2::exiv2lib
add_library(Exiv2::exiv2lib SHARED IMPORTED)

set_target_properties(Exiv2::exiv2lib PROPERTIES
  INTERFACE_COMPILE_DEFINITIONS "EXV_LOCALEDIR=\"../share/locale\""
  INTERFACE_INCLUDE_DIRECTORIES "${_IMPORT_PREFIX}/include"
)

But for static targets (BUILD_SHARED_LIBS=OFF), it produces the necessary private dependencies for static linking:

DESTDIR-static/usr/lib64/cmake/exiv2/exiv2Export.cmake

# Create imported target Exiv2::exiv2lib
add_library(Exiv2::exiv2lib STATIC IMPORTED)

set_target_properties(Exiv2::exiv2lib PROPERTIES
  INTERFACE_COMPILE_DEFINITIONS "EXV_LOCALEDIR=\"../share/locale\""
  INTERFACE_INCLUDE_DIRECTORIES "${_IMPORT_PREFIX}/include"
  INTERFACE_LINK_LIBRARIES "\$<LINK_ONLY:EXPAT::EXPAT>;\$<LINK_ONLY:ZLIB::ZLIB>;\$<LINK_ONLY:inih::libinih>;\$<LINK_ONLY:inih::inireader>"
)

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:

if(@EXIV2_ENABLE_PNG@) # if(EXIV2_ENABLE_PNG)
  find_dependency(ZLIB REQUIRED)
endif()

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 shared exiv2lib).

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.

[kmilos]

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)...

[kmilos]

I still think we should fill out Libs.private and Requires.private unconditionally, no matter what the build was - this covers clients linking either static or shared and requires no special handling/patching of the .pc file if both artifacts are shipped (like e.g. Windows mentioned). There are no side-effects if those are present in a shared only build, as both are only used when running pkg-config --static. E.g. in my MSYS2 UCRT64 environment w/ the same exiv2.pc I get:

$ pkg-config --libs exiv2
-lexiv2

$ pkg-config --static --libs exiv2
-lexiv2 -liconv -lintl -lINIReader -linih -lexpat -lcurl -lws2_32 -lbcrypt -lcrypto -lz -lwldap32 -ladvapi32 -lcrypt32 -lssl -lbrotlidec -lbrotlicommon -lzstd -lnghttp2 -lnghttp3 -lpsl -lunistring -lws2_32 -liconv -lidn2 -L/ucrt64/lib -liconv -L/ucrt64/lib -lunistring -lssh2 -lws2_32 -L/ucrt64/lib -lssl -L/ucrt64/lib -L/ucrt64/lib -lcrypto -lws2_32 -lgdi32 -lcrypt32 -lz

So I suggest to just drop the changes to this file?