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

[Chris Bieneman via Phabricator via llvm-commits]

beanz added a comment.

I think the two of you are speaking past each other with different goals.

@lebedev.ri wants to minimize changes to google benchmark and is trying to steer this patch so that parts of it can be upstreamed back to Google. While that isn't wholly unreasonable, it also isn't LLVM contributor burden to upstream contributions to Google. Personally, I think the goal should be less about making patches upstream-able, and more about making them less likely to conflict with merges from upstream.

@hintonda Is trying to minimize unnecessary changes to LLVM and encapsulating the necessary change to only where it needs to be, which is also reasonable.

I think it is probably reasonable to define `benchmark_add_library` in `AddLLVM.cmake` so that the benchmark sources don't need to directly reference LLVM. You can unconditionally define the function in `AddLLVM.cmake` then you only have one side of the if statement in the benchmark library (checking if `benchmark_add_library`) is already defined. I think that solution will satisfy @lebedev.ri's goals.

I can understand your desire to have the definition of `benchmark_add_library` in one place, but frankly CMake lacks any reasonable or reliable developer tools. If I see a call to `benchmark_add_library` I have to grep the entire CMake codebase to find possible definitions because there isn't reliable "jump to..." tooling. We also use a similar pattern to this in the runtime libraries with the `runtime_register_component` function, and it works well in practice.



================
Comment at: llvm/utils/benchmark/CMakeLists.txt:12
+
+  project (benchmark)
 endif()
----------------
hintonda wrote:
> beanz wrote:
> > The `project` call isn't what resets the policy state. You actually need to pull `cmake_minimum_required` into the `if` block to prevent resetting the policy state:
> > https://cmake.org/cmake/help/v3.0/command/cmake_minimum_required.html
> Thanks for pointing that out.  Should `clang/CMakeLists.txt`, et al, be updated as well?  They don't include `cmake_minimum_required()` within the project conditional either.
That isn't necessary because we actually duplicate the policy calls in our sub-projects. There has been talk in the past about trying to clean that up, I think we left it for after the mono-repo because it will be easier when we can rely on one source repo.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61660