Skip to content

Conversation

@ambitslix
Copy link
Contributor

Proposed fix for:
#928

@googlebot
Copy link

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.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

# 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)
Copy link
Collaborator

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?

Copy link
Contributor Author

@ambitslix ambitslix Jan 25, 2020

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)

Copy link
Collaborator

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.

Copy link
Contributor Author

@ambitslix ambitslix Jan 25, 2020

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

@coveralls
Copy link

coveralls commented Feb 7, 2020

Coverage Status

Coverage remained the same at 92.041% when pulling 5402672 on ambitslix:master into e5ea03c on google:master.

@LebedevRI
Copy link
Collaborator

So.
I've digged, and it is possible in principle:

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, TARGET_FILE_BASE_NAME only appeared in CMake-3.15.

I think we should revert #923 instead.

@ambitslix
Copy link
Contributor Author

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:

  1. Cmake has it's own facility to handle what pkg-config does, and after all this is cmake
  2. There are only awkward solutions to this issue using pkg-config via cmake
  3. Projects using cmake neglect pkg-config support, because of the above

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.

@LebedevRI
Copy link
Collaborator

The caveat being, TARGET_FILE_BASE_NAME only appeared in CMake-3.15.

And even thought TARGET_FILE_NAME is already present in cmake-3.5,
TARGET_FILE_SUFFIX/TARGET_FILE_PREFIX were also only added in 3.15,
so i'm not sure how we could workaround this..

@ambitslix
Copy link
Contributor Author

ambitslix commented Feb 25, 2020

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.

Copy link
Collaborator

@LebedevRI LebedevRI left a 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@
Copy link
Collaborator

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.

LebedevRI added a commit that referenced this pull request Mar 16, 2020
This reverts commit 5ce2429.

Reverts #923
Reopens #922
Fixes #928
Closes #930

See discussion in #928
this broke pkg-config support, since there we don't account
for the suffix, nor is it trivial to do so.
LebedevRI added a commit that referenced this pull request Mar 16, 2020
This reverts commit 5ce2429.

Reverts #923
Reopens #922
Fixes #928
Closes #930

See discussion in #928
this broke pkg-config support, since there we don't account
for the suffix, nor is it trivial to do so.
JBakamovic pushed a commit to JBakamovic/benchmark that referenced this pull request Sep 11, 2020
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.
JBakamovic pushed a commit to JBakamovic/benchmark that referenced this pull request Sep 11, 2020
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.
openvela-robot pushed a commit to open-vela/external_googlebenchmark that referenced this pull request Jul 23, 2025
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.
openvela-robot pushed a commit to open-vela/external_googlebenchmark that referenced this pull request Jul 23, 2025
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.
liujinye-sys pushed a commit to open-vela/external_googlebenchmark that referenced this pull request Dec 16, 2025
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.
liujinye-sys pushed a commit to open-vela/external_googlebenchmark that referenced this pull request Dec 16, 2025
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants