[libcxx-commits] [PATCH] D112012: [benchmarks] Unify libcxx's fork of google/benchmark to llvm/utils

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added a comment.
This revision is now accepted and ready to land.

In D112012#3072064 <https://reviews.llvm.org/D112012#3072064>, @Meinersbur wrote:

> @ldionne What is your concrete proposal? Requiring Google Benchmark pre-installed on the host?

As I said in my comment, I was suggesting that we put it in the monorepo, but just not under `llvm/`. You current proposal for `third-party` sounds reasonable to me.

> I think <root>/llvm is different in that it is not a subproject but contains core and supporting infrastructure (such as `<root>/llvm/cmake/Modules`, llvm-lit etc).

This is historical, back when we would check out all the sub projects under `llvm/projects`. It makes no sense at all to store the common CMake infrastructure or `lit` under `llvm/` anymore, either. I'm not suggesting we change the whole world right now, I'm just saying that if we are going to move stuff around, we might as well do it correctly from the start.

> I don't think we should reject patches that just follow the status quo without having a plan what the future direction should be.

>From the libc++ perspective, the patch before moving to `third-party/` was adding a new hard dependency from `libcxx/` to `llvm/`, which is precisely what we're trying to move away from, hence my question and my request for changes.

IMO this patch looks great now, if everybody is happy with having a top-level `third-party`. I think it makes a lot of sense to have that, but I'm looking for other folks on this review to chime in. If we want to establish this as the new "canonical" place to store third-party dependencies, perhaps a post on the various lists would be a good idea to get people's attention? I'm not requesting it, just suggesting. Personally I'm happy with this change as-is.

However, we do need to fix the CI for libc++. Since you're moving the library to another place, we'll need to adjust the paths to it in `libcxx/`. I think this should be sufficient:

  diff --git a/libcxx/benchmarks/CMakeLists.txt b/libcxx/benchmarks/CMakeLists.txt
  index cad880858a78..91aadc864907 100644
  --- a/libcxx/benchmarks/CMakeLists.txt
  +++ b/libcxx/benchmarks/CMakeLists.txt
  @@ -35,7 +35,7 @@ ExternalProject_Add(google-benchmark-libcxx
           EXCLUDE_FROM_ALL ON
           DEPENDS cxx cxx-headers
           PREFIX benchmark-libcxx
  -        SOURCE_DIR ${LIBCXX_SOURCE_DIR}/utils/google-benchmark
  +        SOURCE_DIR ${LIBCXX_SOURCE_DIR}/../third-party/google-benchmark
           INSTALL_DIR ${CMAKE_CURRENT_BINARY_DIR}/benchmark-libcxx
           CMAKE_CACHE_ARGS
             -DCMAKE_C_COMPILER:STRING=${CMAKE_C_COMPILER}
  @@ -60,7 +60,7 @@ if (LIBCXX_BENCHMARK_NATIVE_STDLIB)
     ExternalProject_Add(google-benchmark-native
           EXCLUDE_FROM_ALL ON
           PREFIX benchmark-native
  -        SOURCE_DIR ${LIBCXX_SOURCE_DIR}/utils/google-benchmark
  +        SOURCE_DIR ${LIBCXX_SOURCE_DIR}/../third-party/google-benchmark
           INSTALL_DIR ${CMAKE_CURRENT_BINARY_DIR}/benchmark-native
           CMAKE_CACHE_ARGS
             -DCMAKE_C_COMPILER:STRING=${CMAKE_C_COMPILER}

I suspect you need similar changes elsewhere in LLVM.

Marking as approved in case I don't see this again for a while, but please don't submit until (at least) the libc++ CI is green.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112012