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.

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

[BUG] installed benchmark thinks it's version 0.0.0 #1046

Closed
germasch opened this issue Sep 24, 2020 · 6 comments · Fixed by #1047
Closed

[BUG] installed benchmark thinks it's version 0.0.0 #1046

germasch opened this issue Sep 24, 2020 · 6 comments · Fixed by #1047

Comments

@germasch
Copy link
Contributor

When downloading a release (e.g., 1.5.2) and building/installing it via cmake, it ends up recording its version as 0.0.0 in its installed cmake config (lib/cmake/benchmark/benchmarkConfigVersion.cmake):

set(PACKAGE_VERSION "0.0.0")

if(PACKAGE_VERSION VERSION_LESS PACKAGE_FIND_VERSION)
  set(PACKAGE_VERSION_COMPATIBLE FALSE)

The consequence of this is that unfortunately something like find_package(benchmark 1.5.2) will fail, since 0.0.0 is considered not compatible.

This is caused by benchmark usually automatically getting its version from git describe, but that doesn't work when building a tarball.

@dmah42
Copy link
Member

dmah42 commented Sep 24, 2020

either something here where we fail to identify a git version, or here checking if we fail to find a git version would need to be changed.

one option might be to write the git version out to a file as part of the release process and read it from there if the current git version isn't found.

@germasch
Copy link
Contributor Author

Well, the question is how you'd prefer to handle it.

Here's one suggestion (I can make a PR): If the VERSION is set in the project command, it'll be used. If it is not set, it'll get the version from git instead. So with this option, in the git tree you'd continue to not provide a VERSION to project() (you wouldn't merge that hunk). In the release process, you'd add VERSION x.y.z to the project() command and that'll get picked up when using the tarball.

--- benchmark/CMakeLists.txt	2020-09-22 10:36:09.000000000 -0400
+++ benchmark-1.5.2/CMakeLists.txt	2020-09-24 08:26:10.000000000 -0400
@@ -13,7 +13,7 @@
   endif()
 endforeach()

-project (benchmark CXX)
+project (benchmark VERSION 1.5.2 LANGUAGES CXX)

 option(BENCHMARK_ENABLE_TESTING "Enable testing of the benchmark library." ON)
 option(BENCHMARK_ENABLE_EXCEPTIONS "Enable the use of exceptions in the benchmark library." ON)
@@ -77,12 +77,16 @@
 list(APPEND CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/cmake")


-# Read the git tags to determine the project version
-include(GetGitVersion)
-get_git_version(GIT_VERSION)
-
+# If the version hasn't been set in the project() command, read the
+# git tags to determine the project version
+if (NOT benchmark_VERSION)
+  include(GetGitVersion)
+  get_git_version(GIT_VERSION)
+  string(REGEX MATCH "[0-9]+\\.[0-9]+\\.[0-9]+" VERSION ${GIT_VERSION})
+else()
+  set(VERSION ${benchmark_VERSION})
+endif()
 # Tell the user what versions we are using
-string(REGEX MATCH "[0-9]+\\.[0-9]+\\.[0-9]+" VERSION ${GIT_VERSION})
 message(STATUS "Version: ${VERSION}")

 # The version of the libraries

@dmah42
Copy link
Member

dmah42 commented Sep 24, 2020

This means having the release in a separate branch or doing the release locally each time (to avoid accidentally setting the project). i wonder if the other way round is better: always set the project version to the latest release, and use it iff the git version returns nothing?

@germasch
Copy link
Contributor Author

I can do that, too. Though in the end, that will do just the same as not using the version info from git at all (which would be fine with me):

At the release, the version will be set to, say, 1.5.3 and will be tagged correspondingly. So the two versions give the same answer. During follow-up development, the cmake release will remain at 1.5.3, and git will return 1.5.3-dirty, which gets simplified into 1.5.3 anyway. So easiest would be to just remove getting the git version in the first place.

@dmah42
Copy link
Member

dmah42 commented Sep 25, 2020 via email

@marehr
Copy link

marehr commented Oct 27, 2020

Downstream packager specifically use the source release and not the git version.

For example debian explicitly patches get_git_version (since 2017!):

--- benchmark.orig/CMakeLists.txt
+++ benchmark/CMakeLists.txt
@@ -79,7 +79,7 @@ list(APPEND CMAKE_MODULE_PATH "${CMAKE_C
 
 # Read the git tags to determine the project version
 include(GetGitVersion)
-get_git_version(GIT_VERSION)
+#get_git_version(GIT_VERSION)
 
 # Tell the user what versions we are using
 string(REGEX MATCH "[0-9]+\\.[0-9]+\\.[0-9]+" VERSION ${GIT_VERSION})

https://salsa.debian.org/science-team/benchmark/-/blob/master/debian/patches/0001-Create-shared-lib.patch

so that they can pass in the version explicitly

#!/usr/bin/make -f

include /usr/share/dpkg/pkg-info.mk
export DEB_BUILD_MAINT_OPTIONS = hardening=+all

%:
	dh $@ --buildsystem=cmake

override_dh_auto_configure:
	dh_auto_configure -- -DGIT_VERSION="$(DEB_VERSION_UPSTREAM)" \
		-DGOOGLETEST_PATH=/usr/src/googletest \
		-DCMAKE_BUILD_TYPE=Release -DBENCHMARK_ENABLE_LTO=true

https://salsa.debian.org/science-team/benchmark/-/blob/master/debian/rules


Another example is arch linux and the reason why I commented here:

cmake \
  -DCMAKE_BUILD_TYPE=None \
  -DCMAKE_CXX_FLAGS="${CXXFLAGS} -DNDEBUG" \
  -DCMAKE_INSTALL_PREFIX=/usr \
  -DCMAKE_INSTALL_LIBDIR=lib \
  -DBUILD_SHARED_LIBS=ON \
  -DBENCHMARK_ENABLE_LTO=ON \
  -DBENCHMARK_ENABLE_GTEST_TESTS=OFF \
  ..

make DESTDIR="$pkgdir/" install

https://github.com/archlinux/svntogit-community/blob/packages/benchmark/trunk/PKGBUILD

They did not patch it and thus created a /usr/lib/cmake/benchmark/benchmarkConfigVersion.cmake that contains an invalid /usr/lib/cmake/benchmark/benchmarkConfigVersion.cmake

CMake Warning at /seqan3/test/cmake/seqan3_require_benchmark.cmake:47 (find_package):
  Could not find a configuration file for package "benchmark" that is
  compatible with requested version "1.5.0".

  The following configuration files were considered but not accepted:

    /usr/lib64/cmake/benchmark/benchmarkConfig.cmake, version: 0.0.0
    /usr/lib/cmake/benchmark/benchmarkConfig.cmake, version: 0.0.0
    /lib64/cmake/benchmark/benchmarkConfig.cmake, version: 0.0.0
    /lib/cmake/benchmark/benchmarkConfig.cmake, version: 0.0.0

Call Stack (most recent call first):
  CMakeLists.txt:30 (seqan3_require_benchmark)

If you use the source package (e.g. https://github.com/google/benchmark/archive/v1.5.2.tar.gz) to install benchmark, you aren't able to determine the version of benchmark.

I guess for those packagers, they would like to be able to at least pass in a version. The optimum would be if the upstream package would provide the version. On a side note: a include/benchmark/version.hpp would be awesome to have. A recent addition of #include <version> (C++20) shows that it is desirable to even be able to determine the version at compile time.

kou added a commit to kou/arrow that referenced this issue Jul 22, 2021
pitrou added a commit to apache/arrow that referenced this issue Jul 22, 2021
google/benchmark#1046 has been resolved.

Closes #10771 from kou/cpp-conda-remove-workaround-for-benchmark

Lead-authored-by: Sutou Kouhei <kou@clear-code.com>
Co-authored-by: Antoine Pitrou <antoine@python.org>
Signed-off-by: Antoine Pitrou <antoine@python.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants