Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[libcxx] Passthrough the necessary CMake variables to benchmarks #116644

Merged
merged 1 commit into from
Nov 19, 2024

Conversation

petrhosek
Copy link
Member

This addresses the issue uncovered by #115361. Previously, we weren't building benchmarks in many cases due to the following block:

if (WIN32 OR MINGW OR ANDROID OR ${CMAKE_SYSTEM_NAME} MATCHES "AIX"
OR NOT LIBCXX_ENABLE_LOCALIZATION
OR NOT LIBCXX_ENABLE_THREADS
OR NOT LIBCXX_ENABLE_FILESYSTEM
OR NOT LIBCXX_ENABLE_RANDOM_DEVICE
OR NOT LIBCXX_ENABLE_EXCEPTIONS
OR NOT LIBCXX_ENABLE_RTTI)
set(_include_benchmarks OFF)
else()
set(_include_benchmarks ON)
endif()

We need to passthrough the necessary variables into the benchmarks subbuild and use correct syntax.

This addresses the issue uncovered by llvm#115361. Previously, we weren't
building benchmarks in many cases due to the following block:

https://github.com/llvm/llvm-project/blob/e58949632e91477af58d983f3b66369e6a2c8233/libcxx/CMakeLists.txt#L162-L172

We need to passthrough the necessary variables into the benchmarks
subbuild and use correct syntax.
@petrhosek petrhosek requested a review from ldionne November 18, 2024 15:57
@petrhosek petrhosek requested a review from a team as a code owner November 18, 2024 15:57
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Nov 18, 2024
@llvmbot
Copy link
Member

llvmbot commented Nov 18, 2024

@llvm/pr-subscribers-libcxx

Author: Petr Hosek (petrhosek)

Changes

This addresses the issue uncovered by #115361. Previously, we weren't building benchmarks in many cases due to the following block:

if (WIN32 OR MINGW OR ANDROID OR ${CMAKE_SYSTEM_NAME} MATCHES "AIX"
OR NOT LIBCXX_ENABLE_LOCALIZATION
OR NOT LIBCXX_ENABLE_THREADS
OR NOT LIBCXX_ENABLE_FILESYSTEM
OR NOT LIBCXX_ENABLE_RANDOM_DEVICE
OR NOT LIBCXX_ENABLE_EXCEPTIONS
OR NOT LIBCXX_ENABLE_RTTI)
set(_include_benchmarks OFF)
else()
set(_include_benchmarks ON)
endif()

We need to passthrough the necessary variables into the benchmarks subbuild and use correct syntax.


Full diff: https://github.com/llvm/llvm-project/pull/116644.diff

1 Files Affected:

  • (modified) libcxx/test/benchmarks/CMakeLists.txt (+4-3)
diff --git a/libcxx/test/benchmarks/CMakeLists.txt b/libcxx/test/benchmarks/CMakeLists.txt
index b5a4aae82c06ab..b0fe600623d964 100644
--- a/libcxx/test/benchmarks/CMakeLists.txt
+++ b/libcxx/test/benchmarks/CMakeLists.txt
@@ -35,13 +35,14 @@ ExternalProject_Add(google-benchmark
         SOURCE_DIR ${LLVM_THIRD_PARTY_DIR}/benchmark
         INSTALL_DIR ${CMAKE_CURRENT_BINARY_DIR}/google-benchmark
         CMAKE_CACHE_ARGS
-          -DCMAKE_C_COMPILER:STRING=${CMAKE_C_COMPILER}
-          -DCMAKE_CXX_COMPILER:STRING=${CMAKE_CXX_COMPILER}
+          -DCMAKE_C_COMPILER:FILEPATH=${CMAKE_C_COMPILER}
+          -DCMAKE_CXX_COMPILER:FILEPATH=${CMAKE_CXX_COMPILER}
+          -DCMAKE_MAKE_PROGRAM:FILEPATH=${CMAKE_MAKE_PROGRAM}
           -DCMAKE_BUILD_TYPE:STRING=RELEASE
           -DCMAKE_INSTALL_PREFIX:PATH=<INSTALL_DIR>
           -DCMAKE_CXX_FLAGS:STRING=${BENCHMARK_COMPILE_FLAGS}
           -DBENCHMARK_USE_LIBCXX:BOOL=ON
           -DBENCHMARK_ENABLE_TESTING:BOOL=OFF
-          -DBENCHMARK_CXX_LIBRARIES:STRING="${BENCHMARK_CXX_LIBRARIES}")
+          -DBENCHMARK_CXX_LIBRARIES:STRING=${BENCHMARK_CXX_LIBRARIES})
 
 add_dependencies(cxx-test-depends google-benchmark)

@PiJoules
Copy link
Contributor

ping is this ready to land?

@petrhosek
Copy link
Member Author

ping is this ready to land?

I need an approval from one of the libc++ reviewers.

@petrhosek
Copy link
Member Author

I'm going to land this change because our builders have been broken since #115361 landed and I'm not getting any response from @ldionne.

@petrhosek petrhosek merged commit 012dd8b into llvm:main Nov 19, 2024
63 of 64 checks passed
@ldionne
Copy link
Member

ldionne commented Nov 19, 2024

This LGTM. Sorry for the delay, I am at a C++ Committee Meeting this week and things are much more hectic than usual over here (stuff rushing for the C++26 deadline), so I haven't had time to do any code review.

swatheesh-mcw pushed a commit to swatheesh-mcw/llvm-project that referenced this pull request Nov 20, 2024
…m#116644)

This addresses the issue uncovered by llvm#115361. Previously, we weren't
building benchmarks in many cases due to the following block:

https://github.com/llvm/llvm-project/blob/e58949632e91477af58d983f3b66369e6a2c8233/libcxx/CMakeLists.txt#L162-L172

We need to passthrough the necessary variables into the benchmarks
subbuild and use correct syntax.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants