-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Fix library name for pkg-config. #930
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
Conversation
|
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. ℹ️ Googlers: Go here for more info. |
src/CMakeLists.txt
Outdated
| # For configuring pkg-config benchmark.pc library name | ||
| string(TOLOWER ${PROJECT_NAME} PROJECT_NAME_LOWER) | ||
| if(CMAKE_BUILD_TYPE STREQUAL "Debug") | ||
| get_target_property(_BENCHMARK_LIB_POSTFIX ${PROJECT_NAME} DEBUG_POSTFIX) |
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.
That is awfully indirect..
Can't we ask cmake what filename it will use, instead of guessing at it?
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.
It is, but is this better?
get_target_property(OUT benchmark OUTPUT_NAME)
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.
I mean, this only accounts for the POSTFIX for debug build,
so if we add postfix for release build, this will need to be updated too.
What i'm specifically not happy about here is that we are still making a guess
about the library file name, instead of asking cmake about it.
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.
No I totally agree, I haven't found a simple way without generator expression? Also I think it's more proper to set output names for each build type separately anyway:
get_target_property(BENCHMARK_LIB_NAME benchmark OUTPUT_NAME)
if(CMAKE_BUILD_TYPE STREQUAL "Debug")
get_target_property(_BENCHMARK_LIB_POSTFIX benchmark DEBUG_POSTFIX)
set(BENCHMARK_LIB_NAME ${BENCHMARK_LIB_NAME}${_BENCHMARK_LIB_POSTFIX})
endif()
|
So. diff --git a/cmake/benchmark.pc.in b/cmake/benchmark.pc.in
index 43ca8f9..f465931 100644
--- a/cmake/benchmark.pc.in
+++ b/cmake/benchmark.pc.in
@@ -7,6 +7,6 @@ Name: @PROJECT_NAME@
Description: Google microbenchmark framework
Version: @VERSION@
-Libs: -L${libdir} -lbenchmark
+Libs: -L${libdir} -l$<TARGET_FILE_BASE_NAME:benchmark>
Libs.private: -lpthread
Cflags: -I${includedir}
diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
index 81c902c..008e566 100644
--- a/src/CMakeLists.txt
+++ b/src/CMakeLists.txt
@@ -84,7 +84,7 @@ write_basic_package_version_file(
)
configure_file("${PROJECT_SOURCE_DIR}/cmake/Config.cmake.in" "${project_config}" @ONLY)
-configure_file("${PROJECT_SOURCE_DIR}/cmake/benchmark.pc.in" "${pkg_config}" @ONLY)
+file(GENERATE OUTPUT "${pkg_config}" INPUT "${PROJECT_SOURCE_DIR}/cmake/benchmark.pc.in")
if (BENCHMARK_ENABLE_INSTALL)
# Install target (will install the library to specified CMAKE_INSTALL_PREFIX variable)The caveat being, I think we should revert #923 instead. |
|
Ok I've digged a bit more too, and I realized that I've been spending too much time on something that should be rather simple, and it usually means I'm doing something the wrong way...so I've come to the conclusion that I will avoid using pkg-config if possible for a few reasons:
Perhaps it's best to not have pkg-config support at all rather then broken support. I switched all my 3rd party projects to cmake only, If I find I need a package that only has pkg-config support that it shall have proper pkg-config support hopefully. And if not than I won't try fixing that via cmake, I will just do what I've always done; rename files manually. |
And even thought |
|
I would remove pkg-config (pc) support as it is broken, there are no good fixes. Or at least note in the documentation, to not use it in the future. It's a tool on top of another tool that does the same thing with different syntax at this point. |
LebedevRI
left a comment
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.
@ambitslix if we end up proceeding with this solution, CLA issue would need to be resolved
| Version: @VERSION@ | ||
|
|
||
| Libs: -L${libdir} -lbenchmark | ||
| Libs: -L${libdir} -l@BENCHMARK_LIB_NAME@ -l@BENCHMARK_MAIN_LIB_NAME@ |
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.
-l@BENCHMARK_MAIN_LIB_NAME@ should not be here.
This reverts commit 5ce2429. Reverts google#923 Reopens google#922 Fixes google#928 Closes google#930 See discussion in google#928 this broke pkg-config support, since there we don't account for the suffix, nor is it trivial to do so.
This reverts commit 5ce2429. Reverts google#923 Reopens google#922 Fixes google#928 Closes google#930 See discussion in google#928 this broke pkg-config support, since there we don't account for the suffix, nor is it trivial to do so.
This reverts commit 5ce2429. Reverts google/benchmark#923 Reopens google/benchmark#922 Fixes google/benchmark#928 Closes google/benchmark#930 See discussion in google/benchmark#928 this broke pkg-config support, since there we don't account for the suffix, nor is it trivial to do so.
This reverts commit 5ce2429. Reverts google/benchmark#923 Reopens google/benchmark#922 Fixes google/benchmark#928 Closes google/benchmark#930 See discussion in google/benchmark#928 this broke pkg-config support, since there we don't account for the suffix, nor is it trivial to do so.
This reverts commit 5ce2429. Reverts google/benchmark#923 Reopens google/benchmark#922 Fixes google/benchmark#928 Closes google/benchmark#930 See discussion in google/benchmark#928 this broke pkg-config support, since there we don't account for the suffix, nor is it trivial to do so.
This reverts commit 5ce2429. Reverts google/benchmark#923 Reopens google/benchmark#922 Fixes google/benchmark#928 Closes google/benchmark#930 See discussion in google/benchmark#928 this broke pkg-config support, since there we don't account for the suffix, nor is it trivial to do so.
Proposed fix for:
#928