Add depended on libraries and C++ runtime to link to in pkg-config file #119
Conversation
|
This is needed when the host app is coded in C and has no idea that chromaprint uses C++ internally. |
…e C++ runtime Submitted upstream acoustid/chromaprint#119
|
@robUx4 Could you post the diff of the pkg-config config on all platforms? The change are kind of hard to review and the diffs would help a lot. |
|
As the C++ library is only needed for static linking, this should be added only to |
|
Indeed and static linking is tricky for multiple reasons, it's much better to just include the libraries needed for the particular build in the project's build system. |
|
On a Linux build, Before: After: I agree it my be better in The C++ runtime to use is needed because the library can be used with a C interface. The host app cannot guess what it really needs to link to. When used from C++ code it will pick its own runtime (and should match the one from the library). |
…e C++ runtime Submitted upstream acoustid/chromaprint#119 (cherry picked from commit 7a77242)
WalkthroughAdds CMake logic (under NOT BUILD_FRAMEWORK) that normalizes entries from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
CMakeLists.txt
Outdated
| if(PKG_LIBS) | ||
| # Blacklist for MinGW-w64 | ||
| list(REMOVE_ITEM PKG_LIBS | ||
| "-lmingw32" "-lgcc_s" "-lgcc" "-lmoldname" "-lmingwex" "-lmingwthrd" | ||
| "-lmsvcrt" "-lpthread" "-ladvapi32" "-lshell32" "-luser32" "-lkernel32") | ||
| string(REPLACE ";" " " CHROMAPRINT_ADDITIONAL_LIBS "${PKG_LIBS}") | ||
| endif() |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider initializing CHROMAPRINT_ADDITIONAL_LIBS explicitly and clarify Libs vs Libs.private semantics.
When if(PKG_LIBS) is false (line 198), CHROMAPRINT_ADDITIONAL_LIBS is never assigned, leaving it uninitialized. CMake will substitute it as an empty string in configure_file, which works, but it's clearer to initialize it explicitly:
+ set(CHROMAPRINT_ADDITIONAL_LIBS "")
foreach(LIB ${CMAKE_CXX_IMPLICIT_LINK_LIBRARIES})
...Separately, per PR review comments, these dependencies should likely go into Libs.private not Libs in the pkg-config file, because they are only needed when statically linking libchromaprint. Dynamic linking doesn't require them. This is a pkg-config semantic convention: Libs lists direct runtime dependencies for dynamic linking, while Libs.private lists additional flags needed only for static linking. Revisit this design choice when reviewing the pkg-config template.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In CMakeLists.txt around lines 198-204, CHROMAPRINT_ADDITIONAL_LIBS is only set
inside the if(PKG_LIBS) block so it can remain uninitialized when PKG_LIBS is
false; initialize CHROMAPRINT_ADDITIONAL_LIBS="" before the if to ensure a
defined value, and after you generate the pkg-config data, move the conditional
dependency flags into the pkg-config Libs.private field rather than Libs (or add
logic to populate Libs.private when static linking is being prepared) so that
runtime dynamic-link consumers don’t get unnecessary static-only flags; update
the pkg-config template/logic accordingly and add a short comment explaining
Libs vs Libs.private semantics.
|
The new version only deals with the IMPLICIT libraries reported by CMake. For avcodec and such, a Requires would be better in the .pc file. That will be a different Pull Request. |
robUx4
force-pushed
the
c++-runtime
branch
2 times, most recently
from
528b07d to
e1496ae
Compare
gcc needs libstdc++ and clang needs libc++.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
CMakeLists.txt(1 hunks)libchromaprint.pc.cmake(1 hunks)
🔇 Additional comments (2)
libchromaprint.pc.cmake (1)
11-11: Libs.private correctly isolates static-only dependencies.This placement is correct. Dynamic linking consumers get only
-lchromaprintfromLibs, while static linking consumers (pkg-config --libs --static) get the C++ runtime and implicit libs fromLibs.private. Matches pkg-config conventions and addresses the PR's goal.CMakeLists.txt (1)
198-204: Deduplication and platform-specific blacklist are both correct.Line 200 removes duplicate flags (addressing the earlier critical review). The MinGW-w64 blacklist (lines 201-203) filters out compiler runtime libs and Windows system libs that shouldn't appear in pkg-config—these are either implicitly linked or would break cross-compilation. The list is hardcoded but pragmatic; extending it to other platforms would require platform detection, which isn't needed unless other toolchains show similar issues.
| if(NOT BUILD_FRAMEWORK) | ||
| foreach(LIB ${CMAKE_CXX_IMPLICIT_LINK_LIBRARIES}) | ||
| if(IS_ABSOLUTE ${LIB} AND EXISTS ${LIB}) | ||
| list(APPEND PKG_LIBS "${LIB}") | ||
| elseif(LIB MATCHES "-l:lib.*.a") | ||
| string(LENGTH ${LIB} LIBLEN) | ||
| math(EXPR LIBLEN "${LIBLEN}-8") | ||
| string(SUBSTRING ${LIB} 6 ${LIBLEN} DIRECT_LIB) | ||
| list(APPEND PKG_LIBS "-l${DIRECT_LIB}") | ||
| elseif(LIB MATCHES "-l.*") | ||
| list(APPEND PKG_LIBS "${LIB}") | ||
| else() | ||
| list(APPEND PKG_LIBS "-l${LIB}") | ||
| endif() | ||
| endforeach() | ||
| if(PKG_LIBS) | ||
| # Blacklist for MinGW-w64 | ||
| list(REMOVE_DUPLICATES PKG_LIBS) | ||
| list(REMOVE_ITEM PKG_LIBS | ||
| "-lmingw32" "-lgcc_s" "-lgcc" "-lmoldname" "-lmingwex" "-lmingwthrd" | ||
| "-lmsvcrt" "-lpthread" "-ladvapi32" "-lshell32" "-luser32" "-lkernel32") | ||
| endif() | ||
| string(REPLACE ";" " " CHROMAPRINT_ADDITIONAL_LIBS "${PKG_LIBS}") |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider initializing CHROMAPRINT_ADDITIONAL_LIBS explicitly.
If PKG_LIBS is empty (line 198 evaluates false), CHROMAPRINT_ADDITIONAL_LIBS is never set. CMake substitutes undefined variables as empty strings in configure_file, so the .pc file gets an empty Libs.private—functionally correct. However, explicit initialization makes the intent clearer:
if(NOT BUILD_FRAMEWORK)
set(CHROMAPRINT_ADDITIONAL_LIBS "")
foreach(LIB ${CMAKE_CXX_IMPLICIT_LINK_LIBRARIES})
# ... existing logic ...This documents that an empty value is expected when no implicit libs are found.
🤖 Prompt for AI Agents
In CMakeLists.txt around lines 183 to 205, CHROMAPRINT_ADDITIONAL_LIBS is only
set when PKG_LIBS is non-empty, so it may remain undefined; explicitly
initialize CHROMAPRINT_ADDITIONAL_LIBS to an empty string before the foreach
loop (e.g., set(CHROMAPRINT_ADDITIONAL_LIBS "")) so the variable is always
defined and the intent of an empty value is clear when no implicit libs are
found.
| elseif(LIB MATCHES "-l:lib.*.a") | ||
| string(LENGTH ${LIB} LIBLEN) | ||
| math(EXPR LIBLEN "${LIBLEN}-8") | ||
| string(SUBSTRING ${LIB} 6 ${LIBLEN} DIRECT_LIB) | ||
| list(APPEND PKG_LIBS "-l${DIRECT_LIB}") |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
-l:lib.a pattern handling is correct but lacks defensive check.*
The regex ensures at least one character exists between lib and .a, so DIRECT_LIB won't be empty in practice. However, adding an explicit guard would make the code more robust against unexpected input formats:
elseif(LIB MATCHES "-l:lib.*.a")
string(LENGTH ${LIB} LIBLEN)
math(EXPR LIBLEN "${LIBLEN}-8")
string(SUBSTRING ${LIB} 6 ${LIBLEN} DIRECT_LIB)
if(DIRECT_LIB)
list(APPEND PKG_LIBS "-l${DIRECT_LIB}")
else()
message(WARNING "Unexpected library format: ${LIB}")
endif()This prevents silent failures if the input format changes in future CMake versions.
🤖 Prompt for AI Agents
In CMakeLists.txt around lines 187-191, the handling for LIB matching
"-l:lib.*.a" extracts DIRECT_LIB but lacks a defensive check; add an explicit
conditional after computing DIRECT_LIB that verifies it is non-empty before
appending to PKG_LIBS and otherwise emit a warning message about the unexpected
library format so malformed inputs do not silently produce incorrect list
entries.
There was a problem hiding this comment.
If CMAKE_CXX_IMPLICIT_LINK_LIBRARIES is poisoned we would be screwed no matter what.
There was a problem hiding this comment.
@robUx4 Fair point—if CMAKE_CXX_IMPLICIT_LINK_LIBRARIES contains garbage, the issue would surface elsewhere in the build. The regex already filters for the expected pattern, so an extra check here would be redundant. I'll drop the suggestion.
3 participants
When linking statically with chromaprint the app needs to know which libraries to link with. It also needs the C++ runtime if the calling code doesn't have any C++.