search

google / benchmark

A microbenchmark support library
Apache License 2.0
8.91k stars 1.62k forks source link

Use of cmake 3.6 feature with cmake_minimum_required (VERSION 2.8.12) #603

Open jszuppe opened 6 years ago

jszuppe commented 6 years ago

https://github.com/google/benchmark/commit/e776aa0275e293707b6a0901e0e8d8a8a3679508 This commit adds list(FILTER SOURCE_FILES EXCLUDE REGEX "benchmark_main\\.cc") line to https://github.com/google/benchmark/blob/master/src/CMakeLists.txt. list(FILTER was introduced in CMake 3.6, however you script sets minimum required CMake version at 2.8.12.

fengggli commented 6 years ago

I got the same one!

kfitch commented 6 years ago

I can't get to a computer where I can fork/edit/test code at the moment, but I think the proper solution (that should work on cmake 2.8.12) is something like:

file(GLOB BENCHMARK_MAIN "benchmark_main.cc")
foreach(item ${BENCHMARK_MAIN})
    list(REMOVE_ITEM SOURCE_FILES ${item})
endforeach()
Maratyszcza commented 6 years ago

PR #612 for a fix

EricWF commented 6 years ago

I would much rather bump the required CMake version. CMake is trivial to install on all platforms. There is no reason for people to be using CMake versions that old.

Maratyszcza commented 6 years ago

@EricWF CMake 3.5.1 is the latest available in Ubuntu 14.04 and 16.04 (LTS releases).

EricWF commented 6 years ago

So we should at least bump it to that, but again, it's trivial for users to install newer versions.

LebedevRI commented 6 years ago

Documenting (and sticking with a) dependency versioning policy would be nice[r]. Personally i think trying to support 3-years old versions of the deps (+ debian stable + last two ubuntu lts) is the correct solution. There can be exceptions of course.

EricWF commented 6 years ago

LLVM is currently requiring 3.4.3, with the other information provided above it seems like we can agree to bump it to 3.5.1?

LebedevRI commented 6 years ago

cmake v3.5.1 was released Mar 24, 2016, which is 2 years 2 months. v3.2.3 is the newest that would fit into that sliding window. That sliding window seems to be somewhat reasonable. I don't mean to halt forward progress, i'm just trying to hint that cost-benefit analysis, and ideally effect on (not just one) users is a good thing.

EricWF commented 6 years ago

I would also settle for 3.4.3 if needed, because if LLVM is getting away with it, so can we.

I'm also a large fan of the potentially controversial policy of commit now, worry later. We have enough users that if bumping to 3.5.1 is problematic, we'll hear about it in short order, and we can walk it back.

In my experience maintaining large projects, I've found this methodology to be quite useful; Allowing forward momentum beyond what you might expect to be possible.

LebedevRI commented 6 years ago

Let me summarize my views, after brief irc chat:

  1. Forward momentum is good.
  2. Sticking with old-er versions just because is not good.
  3. Whatever the policy is, it's best to document, follow it.
  4. It'd be great if the 3-year sliding window would be the [new] policy. (more specifically, https://github.com/google/benchmark/issues/603#issuecomment-394858872)
  5. 2 years could work too.
  6. Any other more aggressive approach (no written rules, or not following them) is worse, but not my hill to fight on.
dmah42 commented 6 years ago

My undocumented and therefore completely made-up policy has been to support whatever comes "out of the box".

The latest LTS from Ubuntu is 18.04 with cmake 3.10.2, but the "current oldest" LTS is 14.04. That version is 2.8.12. In xenial (16.04), it's 3.5.1.

Debian stable is at 3.7.2-1.

If we go with "oldest_of(2 ubuntu LTS + debian stable)" then we're looking at 3.5.1.

I'm ok bumping to 3.5.1 and documenting the policy as the above. I think this is going to be unique to cmake though, as bazel wasn't around back then :P

Maratyszcza commented 6 years ago

@dominichamon Ubuntu 14.04 also has cmake3 package, which installs CMake 3.5.1

dmah42 commented 6 years ago

oh well then. 3.5.1 sgtm! :)

LebedevRI commented 6 years ago

Documentation first :)

LebedevRI commented 6 years ago

LLVM bundles/builds this library as of https://reviews.llvm.org/D50894, but LLVM has a required cmake version of 3.4.3: https://github.com/llvm-mirror/llvm/blob/0d21b888881a26b0b50065669163afbe0c9a5812/CMakeLists.txt#L3 So this won't be as simple going forward :/

LebedevRI commented 6 years ago

(To be noted, i'm still onboard with https://github.com/google/benchmark/pull/613 in it's current form)

dmah42 commented 3 years ago

looking at the options today, focal is on 3.16.3, bionic is on 3.10.2, and xenial is on 3.5.1. debian is on 3.13.4.

does this mean we can bump to 3.10.2 and close this issue? :)