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

[hintonda]

Summary

Handle benchmark like other subprojects by not calling
cmake_minimum_required(), project(), and cmake_policy() when
building in-tree.

Also add a new function, benchmark_add_library() that calls add_library()
by default, and llvm_add_library() when available.

This gets rid of an rpath warning on MacOS when compiled with
BUILD_SHARED_LIBS=ON.

Thanks to @lebedev.ri for suggesting how to conditionally wrap add_library().

[lebedev.ri]

Let's make this a bit more upstreamable:
Add this to llvm/CMakeLists.txt

  function(benchmark_add_library target)
    llvm_add_library(${target} ${ARGN})
  endfunction()

  add_subdirectory(utils/benchmark)
  add_subdirectory(benchmarks)
endif()

Change add_library/llvm_add_library to benchmark_add_library in llvm/utils/benchmark/src/CMakeLists.txt

Add

# Creates a new library build target. Use this instead of `add_library`.
if(NOT COMMAND benchmark_add_library)
  function(benchmark_add_library target)
    add_library(${target} ${ARGN})
  endfunction()
endif() # NOT COMMAND benchmark_add_library

maybe to llvm/utils/benchmark/CMakeLists.txt ?

Submit a PR upstream.
Add README.llvm entry

[hintonda]

In D61660#1494517, @lebedev.ri wrote:

Let's make this a bit more upstreamable:

Thanks for taking a look. I like the changes you suggest and will implement them in my next diff, then submit a PR, plus documentation, once it lands.

thanks again...

Add this to llvm/CMakeLists.txt

  function(benchmark_add_library target)
    llvm_add_library(${target} ${ARGN})
  endfunction()

  add_subdirectory(utils/benchmark)
  add_subdirectory(benchmarks)
endif()

Change add_library/llvm_add_library to benchmark_add_library in llvm/utils/benchmark/src/CMakeLists.txt

Add

# Creates a new library build target. Use this instead of `add_library`.
if(NOT COMMAND benchmark_add_library)
  function(benchmark_add_library target)
    add_library(${target} ${ARGN})
  endfunction()
endif() # NOT COMMAND benchmark_add_library

maybe to llvm/utils/benchmark/CMakeLists.txt ?

Submit a PR upstream.
Add README.llvm entry

[hintonda]

• Add wrapper function for add_library, and set it to llvm_add_library if building as a part of LLVM.

[lebedev.ri]

.. this doesn't look like what i proposed, and has some unrelated diff?

[hintonda]

In D61660#1495331, @lebedev.ri wrote:

.. this doesn't look like what i proposed, and has some unrelated diff?

It uses your wrapper suggestion, but keeps everything local to benchmark. Generally, if projects are build within other projects, e.g., see clang/CMakeLists.txt, they don't redeclare project(). Doing so wipes out some state, most notably policies. By checking, you preserve whatever the parent project set.

The only other change is just to fix the path to be relative to the cmake file instead of the project root, which will be different when contained in another project.

[lebedev.ri]

In D61660#1495354, @hintonda wrote:

In D61660#1495331, @lebedev.ri wrote:

.. this doesn't look like what i proposed, and has some unrelated diff?

It uses your wrapper suggestion, but keeps everything local to benchmark. Generally, if projects are build within other projects, e.g., see clang/CMakeLists.txt, they don't redeclare project(). Doing so wipes out some state, most notably policies. By checking, you preserve whatever the parent project set.

I'm sorry, i don't understand.
Why?
The diff i proposed i will happily merge upstream into google benchmark.
This - no.

The only other change is just to fix the path to be relative to the cmake file instead of the project root, which will be different when contained in another project.

[beanz]

Could you change the title and commit message to reference that this isn't just about using llvm_add_library it is more broadly about making the benchmark library behave like a proper LLVM library target when built in-tree.

@lebedev.ri, the extra changes here that you're objecting to are required. If Benchmark isn't built with the same CMake policies as the rest of LLVM you can't use llvm_add_library because it makes assumptions about the policy settings.

llvm/utils/benchmark/CMakeLists.txt

6

Setting this to Clang seems wrong...

[hintonda]

In D61660#1495366, @lebedev.ri wrote:

In D61660#1495354, @hintonda wrote:

In D61660#1495331, @lebedev.ri wrote:

.. this doesn't look like what i proposed, and has some unrelated diff?

It uses your wrapper suggestion, but keeps everything local to benchmark. Generally, if projects are build within other projects, e.g., see clang/CMakeLists.txt, they don't redeclare project(). Doing so wipes out some state, most notably policies. By checking, you preserve whatever the parent project set.

I'm sorry, i don't understand.
Why?
The diff i proposed i will happily merge upstream into google benchmark.
This - no.

Sure, but so will this one, and it doesn't require any changes to LLVM.

Alternatively, I could do one of the following:

• leave the project(benchmark) command as is and move the conditional before it.
• fix benchmark to build libraries correctly on OSX -- essentially duplicating logic in llvm_setup_rpath().

But I really don't think fixing broken sub-projects should require changes to LLVM. It's the subproject that's broken, not LLVM.

The only other change is just to fix the path to be relative to the cmake file instead of the project root, which will be different when contained in another project.

[hintonda]

llvm/utils/benchmark/CMakeLists.txt

6

Opps, typo. Thanks for catching that.

[hintonda]

• Fix cut-n-paste typo.

[hintonda]

In D61660#1495378, @beanz wrote:

Could you change the title and commit message to reference that this isn't just about using llvm_add_library it is more broadly about making the benchmark library behave like a proper LLVM library target when built in-tree.

@lebedev.ri, the extra changes here that you're objecting to are required. If Benchmark isn't built with the same CMake policies as the rest of LLVM you can't use llvm_add_library because it makes assumptions about the policy settings.

@beanz, should I also include the policies in the conditional? I left them out, but wasn't sure that was the correct thing to do. Based on you comment, they should only be set if not building within LLVM, right?

[beanz]

llvm/utils/benchmark/CMakeLists.txt

12

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

16

@lebedev.ri's suggestion to make this change more upstream-able to google benchmark is to define benchmark_add_library in LLVM (maybe in AddLLVM.cmake), then change to checking if benchmark_add_library is defined and define it if it isn't.

[lebedev.ri]

llvm/utils/benchmark/CMakeLists.txt

5

As discussed in IRC with @beanz, is this really about project(), or CMake policies cmake_minimum_required() ?

15–23

I'm serious, there is no need to have this *here*.
Here, only add:

# Creates a new library build target. Use this instead of `add_library`.
if(NOT COMMAND benchmark_add_library)
  function(benchmark_add_library target)
    add_library(${target} ${ARGN})
  endfunction()
endif() # NOT COMMAND benchmark_add_library

And add this in to llvm/CMakeLists.txt, right before add_subdirectory(utils/benchmark)

function(benchmark_add_library target)
  llvm_add_library(${target} ${ARGN})
endfunction()

This results in smallest diff from upstream.

[hintonda]

• Move cmake_miniimum_required and policies into project conditional, and change test to check for llvm_add_library instead of project name.

[lebedev.ri]

I think i'm failing to convey the thoughts here.
Can you explain why you insist on having

if(COMMAND llvm_add_library)
  function(benchmark_add_library target)
    llvm_add_library(${target} ${ARGN})
  endfunction()
else()
  function(benchmark_add_library target)
    add_library(${target} ${ARGN})
  endfunction()
endif()

in llvm/utils/benchmark/CMakeLists.txt?

[hintonda]

In D61660#1496266, @lebedev.ri wrote:

I think i'm failing to convey the thoughts here.
Can you explain why you insist on having

if(COMMAND llvm_add_library)
  function(benchmark_add_library target)
    llvm_add_library(${target} ${ARGN})
  endfunction()
else()
  function(benchmark_add_library target)
    add_library(${target} ${ARGN})
  endfunction()
endif()

in llvm/utils/benchmark/CMakeLists.txt?

Sorry, I seem to have touched a nerve, and certainly didn't mean to offend
anyone. As a volunteer, my only goal is to do what I can to improve the
project.

So, here's my logic for structuring the patch in this way:

If the new function were used anywhere else in llvm, et al, other than
benchmark, I'd agree that it belongs AddLLVM.cmake. However, as far as
I can see, it's only used in benchmark, so it made sense to isolate the
change and put the new function definitions where they are used.

Additionally, breaking up the if/else logic and putting part of it in llvm and the
other part in benchmark seems overly complicated. It also unnecessarily
increases the technical debt in the llvm cmake files, and forces maintainers to
look in two files instead of one in order to grok what's going on.

llvm/utils/benchmark/CMakeLists.txt

12

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.

[beanz]

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.

llvm/utils/benchmark/CMakeLists.txt

12

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.

[hintonda]

In D61660#1496888, @beanz wrote:

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.

Okay, that sounds reasonable.

Btw, I looked at the implementation of llvm_add_library and the wrapper will need to be a macro instead of a function so as not to break anything on Window -- there's at least one set() command that includes PARENT_SCOPE, which will break if we create another scope.

[beanz]

Using a macro seems reasonable if a bit icky.

[hintonda]

In D61660#1496944, @beanz wrote:

Using a macro seems reasonable if a bit icky.

Just thought I'd mention it. Probably not necessary in this context since it shouldn't get called anywhere other than benchmark. As a wrapper, scope if the only difference.

[hintonda]

• Address comments.

[lebedev.ri]

In D61660#1496731, @hintonda wrote:

In D61660#1496266, @lebedev.ri wrote:

I think i'm failing to convey the thoughts here.
Can you explain why you insist on having

if(COMMAND llvm_add_library)
  function(benchmark_add_library target)
    llvm_add_library(${target} ${ARGN})
  endfunction()
else()
  function(benchmark_add_library target)
    add_library(${target} ${ARGN})
  endfunction()
endif()

in llvm/utils/benchmark/CMakeLists.txt?

Sorry, I seem to have touched a nerve, and certainly didn't mean to offend
anyone. As a volunteer, my only goal is to do what I can to improve the
project.

I'm sorry, the problem is most quite likely on my side indeed,
I'n *not* offended, i was just not observing the logic about *this specific layout* for the change.
I agree that the change is needed.

In D61660#1496888, @beanz wrote:

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

Yes, i believe so.

@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.

That is faithful to my position.

While that isn't wholly unreasonable, it also isn't LLVM contributor burden to upstream contributions to Google.

I don't disagree. (PSA: i don't work for google and/or googlebenchmark, i only contribute to benchmark)
I wouldn't insist on changing the patch to the state that would be most
upstreamable *if* the author said that he will not even try upstreaming it.
But i did not observe any such statements, thus i'm trying to guide the patch into the most upstreamable state.

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.

I don't believe it is possible to just take some change that was made to LLVM's google benchmark copy,
and upstream it, unless the original author does the upstreaming, or at least he states that he is ok with that.
(separate upstreaming may also add extra burden of rewriting the change into upstreamable state,
and then backpropagating it, after upstreaming, into llvm, without breaking the original llvm patch)

@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.

As long as llvm's override for benchmark_add_library is outside of the bundled benchmark sources, it is fine!
I.e. the current diff is perfect, modulo the comment.

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.

Yeah, cmake is a 'bit' messy at times :(
Again, i'm sorry if i appeared stubborn, it wasn't the intent.

Last portion of comments.

llvm/utils/benchmark/CMakeLists.txt

14

I.e. i'm wondering if this endif() should be here

15–17

@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?

44–45

I think the comment reads weird, inside-out.
Maybe

# Define this wrapper unless it is already defined,
# e.g. because benchmark is included as a subproject,
# and the parent project has provided it's own wrapper.

44–50

Perfect, thank you!
If you wish to submit this upstream, i will gladly merge this part.

[hintonda]

Thanks, and no worries...

llvm/utils/benchmark/CMakeLists.txt

14

It matches line 8: if(POLICY CMP0048), so it currently is needed.

However, the entire if(POLICY CMP0048) may no longer be needed if all of this remains couched in the larger if(...). I'll let @beanz comment on that, since I haven't really investigated it.

That's more a benchmark issue, so as long as this works within LLVM, I'm happy.

44–45

You're right, I'll fix it.

[beanz]

llvm/utils/benchmark/CMakeLists.txt

15–17

@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.

[hintonda]

• Fix comment.

[hintonda]

Since this is a problem with a 3rd party project I'm not working on, I'm going to let them decide how to deal with this and just disable it in my builds by setting LLVM_INCLUDE_BENCHMARK=OFF.

llvm/utils/benchmark/CMakeLists.txt

 cmake_minimum_required (VERSION 2.8.12)
 
+# 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 )
   # Tell cmake 3.0+ that it's safe to clear the PROJECT_VERSION variable in the
   # call to project() below.
   if(POLICY CMP0048)
     cmake_policy(SET CMP0048 NEW)
   endif()
 
   project (benchmark)
+endif()
+
+if(CMAKE_PROJECT_NAME STREQUAL "LLVM")
+  function(benchmark_add_library target)
+    llvm_add_library(${target} ${ARGN})
+  endfunction()
+else()
+  function(benchmark_add_library target)
+    add_library(${target} ${ARGN})
+  endfunction()
+endif()
 
 foreach(p
     CMP0054 # CMake 3.1
     CMP0056 # export EXE_LINKER_FLAGS to try_run
     CMP0057 # Support no if() IN_LIST operator
     )
   if(POLICY ${p})
     cmake_policy(SET ${p} NEW)
   endif()
 endforeach()
 
 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)
 option(BENCHMARK_ENABLE_LTO "Enable link time optimisation of the benchmark library." OFF)
 option(BENCHMARK_USE_LIBCXX "Build and test using libc++ as the standard library." OFF)
 option(BENCHMARK_BUILD_32_BITS "Build a 32 bit version of the library." OFF)
 option(BENCHMARK_ENABLE_INSTALL "Enable installation of benchmark. (Projects embedding benchmark may want to turn this OFF.)" ON)
 
 # Allow unmet dependencies to be met using CMake's ExternalProject mechanics, which
 # may require downloading the source code.
 option(BENCHMARK_DOWNLOAD_DEPENDENCIES "Allow the downloading and in-tree building of unmet dependencies" OFF)
 
 # This option can be used to disable building and running unit tests which depend on gtest
 # in cases where it is not possible to build or find a valid version of gtest.
 option(BENCHMARK_ENABLE_GTEST_TESTS "Enable building the unit tests which depend on gtest" OFF)
 
 set(ENABLE_ASSEMBLY_TESTS_DEFAULT OFF)
 function(should_enable_assembly_tests)
   if(CMAKE_BUILD_TYPE)
     string(TOLOWER ${CMAKE_BUILD_TYPE} CMAKE_BUILD_TYPE_LOWER)
     if (${CMAKE_BUILD_TYPE_LOWER} MATCHES "coverage")
       # FIXME: The --coverage flag needs to be removed when building assembly
       # tests for this to work.
       return()
     endif()

llvm/utils/benchmark/src/CMakeLists.txt

SOURCE_FILES
     *.cc
     ${PROJECT_SOURCE_DIR}/include/benchmark/*.h
     ${CMAKE_CURRENT_SOURCE_DIR}/*.h)
 file(GLOB BENCHMARK_MAIN "benchmark_main.cc")
 foreach(item ${BENCHMARK_MAIN})
   list(REMOVE_ITEM SOURCE_FILES "${item}")
 endforeach()
 
-add_library(benchmark ${SOURCE_FILES})
+benchmark_add_library(benchmark ${SOURCE_FILES})
 set_target_properties(benchmark PROPERTIES
   OUTPUT_NAME "benchmark"
   VERSION ${GENERIC_LIB_VERSION}
   SOVERSION ${GENERIC_LIB_SOVERSION}
   FOLDER "Utils"
 )
 target_include_directories(benchmark PUBLIC
     $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/../include>

 endif()
 
 # We need extra libraries on Solaris
 if(${CMAKE_SYSTEM_NAME} MATCHES "SunOS")
   target_link_libraries(benchmark kstat)
 endif()
 
 # Benchmark main library
-add_library(benchmark_main "benchmark_main.cc")
+benchmark_add_library(benchmark_main "benchmark_main.cc")
 set_target_properties(benchmark_main PROPERTIES
   OUTPUT_NAME "benchmark_main"
   VERSION ${GENERIC_LIB_VERSION}
   SOVERSION ${GENERIC_LIB_SOVERSION}
   FOLDER "Utils"
 )
 target_include_directories(benchmark PUBLIC
     $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/../include>

 
 set(namespace "${PROJECT_NAME}::")
 
 include(CMakePackageConfigHelpers)
 write_basic_package_version_file(
     "${version_config}" VERSION ${GIT_VERSION} COMPATIBILITY SameMajorVersion
 )
 
-configure_file("${PROJECT_SOURCE_DIR}/cmake/Config.cmake.in" "${project_config}" @ONLY)
-configure_file("${PROJECT_SOURCE_DIR}/cmake/benchmark.pc.in" "${pkg_config}" @ONLY)
+configure_file("${CMAKE_CURRENT_LIST_DIR}/../cmake/Config.cmake.in" "${project_config}" @ONLY)
+configure_file("${CMAKE_CURRENT_LIST_DIR}/../cmake/benchmark.pc.in" "${pkg_config}" @ONLY)
 
 if (BENCHMARK_ENABLE_INSTALL)
   # Install target (will install the library to specified CMAKE_INSTALL_PREFIX variable)
   install(
     TARGETS benchmark benchmark_main
     EXPORT ${targets_export_name}
     ARCHIVE DESTINATION ${lib_install_dir}
     LIBRARY DESTINATION ${lib_install_dir}