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

Fri May 10 10:30:34 PDT 2019

lebedev.ri added a comment.

In D61660#1496731 <https://reviews.llvm.org/D61660#1496731>, @hintonda wrote:

> In D61660#1496266 <https://reviews.llvm.org/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 <https://reviews.llvm.org/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.

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

================
Comment at: llvm/utils/benchmark/CMakeLists.txt:11
+  endif()

+  project (benchmark)
----------------
I.e. i'm wondering if this `endif()` should be here

================
Comment at: llvm/utils/benchmark/CMakeLists.txt:25-26
+
+# This wrapper is needed when including this project as a subproject,
+# and the parent project has defined benchmark_add_library, e.g., LLVM.
+if(NOT COMMAND benchmark_add_library)
----------------
lebedev.ri wrote:
> Perfect, thank you!
> If you wish to submit this upstream, i **will** //gladly// merge this part.
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.
```

================
Comment at: llvm/utils/benchmark/CMakeLists.txt:25-31
+# This wrapper is needed when including this project as a subproject,
+# and the parent project has defined benchmark_add_library, e.g., LLVM.
+if(NOT COMMAND benchmark_add_library)
+  macro(benchmark_add_library)
+    add_library(${ARGN})
+  endfunction()
+endif()
----------------
Perfect, thank you!
If you wish to submit this upstream, i **will** //gladly// merge this part.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61660