[PATCH] D61660: [cmake] Make google benchmark project handle libraries properly when built in-tree

[Chris Bieneman via Phabricator via llvm-commits]

beanz added inline comments.


================
Comment at: llvm/utils/benchmark/CMakeLists.txt:1-3
+# If we are not building as a part of another project, e.g., LLVM,
+# build Benchmark as an standalone project.
+if( CMAKE_SOURCE_DIR STREQUAL CMAKE_CURRENT_SOURCE_DIR )
----------------
lebedev.ri wrote:
> @beanz Is this problem (ensuring that 'submodules' don't call `cmake_minimum_required()`)
> a common CMake issue? Or is it a side-effect of llvm's CMake files? I'm genuinely curious.
> 
> That being said, is this *only* about the `cmake_minimum_required()`?
> If so, do you need to also wrap the call to `project (benchmark)` into this `if`?
> If you don't, then the `configure_file()` change should also not be needed?
@lebedev.ri In general, it is a good practice for a project to structure their code exactly how Benchmark did. If we were not modifying benchmark to call into LLVM's CMake modules, this wouldn't be an issue. The problem is that LLVM's minimum CMake version is much higher than Benchmark's and we make assumptions based on that in our CMake modules. As such you cannot safely use LLVM's modules inside Benchmark.

In terms of the `project` call, that isn't really as destructive. The biggest thing it messes with are the `PROJECT_*` variables, which can be safe or not depending on how those variables are used. We don't use them widely in LLVM, so I'd expect them to mostly be safe, and the `configure_file` change may not be needed.

That said, the `configure_file` change is how benchmark should have done it. That is a much more robust solution to the problem.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61660/new/

https://reviews.llvm.org/D61660