[PATCH] D112012: [benchmarks] Move libcxx's fork of google/benchmark and llvm/utils' under third-party

[Louis Dionne via Phabricator via llvm-commits]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

To fix the CI, you'll want to rebase onto `main` once I've landed D113503 <https://reviews.llvm.org/D113503>. That patch removes support for the benchmarks under the libc++ standalone build, since this patch breaks that use case. I think it's fine though, we are trying to get rid of the standalone build anyway, it's just a matter of time.



================
Comment at: llvm/CMakeLists.txt:70
 set(LLVM_EXTRA_PROJECTS "flang")
+set(LLVM_THIRD_PARTY_PROJECTS "third-party")
 # List of all known projects in the mono repo
----------------
This is entirely new behavior, and I don't understand why we're doing this as part of this patch. IMO this patch should only be moving stuff around, period. If we then want to start considering the third-party projects as a "project" we can enable with `LLVM_ENABLE_PROJECTS`, we should tackle it as a separate effort (and frankly it's not clear to me that would fly).

Also, when you make changes like this as part of a much larger change, it is often a good idea to call them out to make sure reviewers see them.


================
Comment at: llvm/CMakeLists.txt:1190-1200
-  # Override benchmark defaults so that when the library itself is updated these
-  # modifications are not lost.
-  set(BENCHMARK_ENABLE_TESTING OFF CACHE BOOL "Disable benchmark testing" FORCE)
-  set(BENCHMARK_ENABLE_EXCEPTIONS OFF CACHE BOOL "Disable benchmark exceptions" FORCE)
-  set(BENCHMARK_ENABLE_INSTALL OFF CACHE BOOL "Don't install benchmark" FORCE)
-  set(BENCHMARK_DOWNLOAD_DEPENDENCIES OFF CACHE BOOL "Don't download dependencies" FORCE)
-  set(BENCHMARK_ENABLE_GTEST_TESTS OFF CACHE BOOL "Disable Google Test in benchmark" FORCE)
----------------
This is the minimal change. Just move stuff around and point to the new directory from `llvm/` and from `runtimes/` + `libcxx/benchmarks`. Everything else should be in a different patch since that's a meaningful change.


================
Comment at: runtimes/CMakeLists.txt:39
 
+if (NOT DEFINED LLVM_THIRD_PARTY_DIR)
+  set(LLVM_THIRD_PARTY_DIR "${CMAKE_CURRENT_SOURCE_DIR}/../third-party")
----------------
Why the `if (NOT DEFINED LLVM_THIRD_PARTY_DIR)`? If we can set it unconditionally, we should because it avoid giving the impression that folks can set it however they want.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112012