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.

Sign up for GitHub

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

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fde3e38
pkg-config: do not output static linking deps when building shared lib
pinotree Dec 27, 2023
a1ff128
cmake: do not require static deps for a shared library
pinotree Dec 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 7 additions & 5 deletions cmake/exiv2Config.cmake.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,14 @@
cmake_minimum_required(VERSION 3.5)
include(CMakeFindDependencyMacro)

if(@EXIV2_ENABLE_PNG@) # if(EXIV2_ENABLE_PNG)
find_dependency(ZLIB REQUIRED)
endif()
if(NOT @BUILD_SHARED_LIBS@) # if(NOT BUILD_SHARED_LIBS)
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

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:

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

Copy link
Collaborator

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?

Copy link
Contributor

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

Copy link
Collaborator

@kmilos kmilos Dec 18, 2024 •
edited
Loading

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

Copy link
Collaborator

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

Copy link
Contributor

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:

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

Copy link
Collaborator

@kmilos kmilos Dec 19, 2024 •
edited
Loading

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

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

if(@EXIV2_ENABLE_XMP@) # if(EXIV2_ENABLE_XMP)
find_dependency(EXPAT REQUIRED)
if(@EXIV2_ENABLE_XMP@) # if(EXIV2_ENABLE_XMP)
find_dependency(EXPAT REQUIRED)
endif()
endif()

include("${CMAKE_CURRENT_LIST_DIR}/exiv2Export.cmake")
Expand Down
34 changes: 18 additions & 16 deletions src/CMakeLists.txt
View file
Open in desktop
Copy link
Collaborator

@kmilos kmilos Dec 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Original file line number Diff line number Diff line change
Expand Up @@ -288,23 +288,25 @@ if(EXIV2_ENABLE_INIH)
list(APPEND requires_private_list "INIReader")
endif()

# Convert private lists to delimited strings
list(SORT libs_private_list)
string(REPLACE ";" " -l" libs_private_string "${libs_private_list}")
if(libs_private_string)
string(PREPEND libs_private_string "-l")
if(NOT BUILD_SHARED_LIBS)
# Convert private lists to delimited strings
list(SORT libs_private_list)
string(REPLACE ";" " -l" libs_private_string "${libs_private_list}")
if(libs_private_string)
string(PREPEND libs_private_string "-l")
endif()
list(SORT requires_private_list)
string(REPLACE ";" ", " requires_private_string "${requires_private_list}")

set(libs_private_for_pc_file
"${libs_private_string}"
PARENT_SCOPE
)
set(requires_private_for_pc_file
"${requires_private_string}"
PARENT_SCOPE
)
endif()
list(SORT requires_private_list)
string(REPLACE ";" ", " requires_private_string "${requires_private_list}")

set(libs_private_for_pc_file
"${libs_private_string}"
PARENT_SCOPE
)
set(requires_private_for_pc_file
"${requires_private_string}"
PARENT_SCOPE
)

write_basic_package_version_file(exiv2ConfigVersion.cmake COMPATIBILITY ExactVersion)

Expand Down