-
Notifications
You must be signed in to change notification settings - Fork 11.9k
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
[libc++] Run the Lit test suite against an installed version of the library #96910
[libc++] Run the Lit test suite against an installed version of the library #96910
Conversation
@llvm/pr-subscribers-libcxx Author: Louis Dionne (ldionne) ChangesWe always strive to test libc++ as close as possible to the way we are actually shipping it. This was approximated reasonably well by setting up the minimal driver flags when running the test suite, however we were running the test suite against the library located in the build directory. This patch improves the situation by installing the library (the headers, the built library, modules, etc) into a fake location and then running the test suite against that fake "installation root". This should open the door to getting rid of the temporary copy of the headers we make during the build process, however this is left for a future improvement. Note that this adds quite a bit of verbosity whenever running the test suite because we install the headers beforehand every time. We should be able to override this to silence it, however CMake doesn't currently give us a way to do that, see https://gitlab.kitware.com/cmake/cmake/-/issues/26085. Full diff: https://github.com/llvm/llvm-project/pull/96910.diff 4 Files Affected:
diff --git a/libcxx/modules/CMakeLists.txt b/libcxx/modules/CMakeLists.txt
index 82cd7b66beb7a..d47d19a475531 100644
--- a/libcxx/modules/CMakeLists.txt
+++ b/libcxx/modules/CMakeLists.txt
@@ -202,7 +202,6 @@ add_custom_target(generate-cxx-modules
ALL DEPENDS
${_all_modules}
)
-add_dependencies(cxx-test-depends generate-cxx-modules)
# Configure the modules manifest.
# Use the relative path between the installation and the module in the json
diff --git a/libcxx/src/CMakeLists.txt b/libcxx/src/CMakeLists.txt
index 9e6c70335a794..caaaeff309f53 100644
--- a/libcxx/src/CMakeLists.txt
+++ b/libcxx/src/CMakeLists.txt
@@ -322,7 +322,6 @@ endif()
# Add a meta-target for both libraries.
add_custom_target(cxx DEPENDS ${LIBCXX_BUILD_TARGETS})
-add_dependencies(cxx-test-depends cxx)
set(LIBCXX_EXPERIMENTAL_SOURCES
experimental/keep.cpp
@@ -370,7 +369,6 @@ set_target_properties(cxx_experimental
)
cxx_add_common_build_flags(cxx_experimental)
target_compile_options(cxx_experimental PUBLIC -D_LIBCPP_ENABLE_EXPERIMENTAL)
-add_dependencies(cxx-test-depends cxx_experimental)
if (LIBCXX_INSTALL_SHARED_LIBRARY)
install(TARGETS cxx_shared
diff --git a/libcxx/test/CMakeLists.txt b/libcxx/test/CMakeLists.txt
index cdd1c2d90fbcf..33451223e63bf 100644
--- a/libcxx/test/CMakeLists.txt
+++ b/libcxx/test/CMakeLists.txt
@@ -7,6 +7,62 @@ if (NOT LIBCXX_CXX_ABI_LIBRARY_PATH)
"The path to libc++abi library.")
endif()
+# Install the library at a fake location so we can run the test suite against it.
+# This ensures that we run the test suite against a setup that matches what we ship
+# in production as closely as possible (in terms of file paths, rpaths, etc).
+set(LIBCXX_TESTING_INSTALL_PREFIX "${LIBCXX_BINARY_DIR}/test-suite-install")
+if (LIBCXX_CXX_ABI STREQUAL "cxxabi")
+ add_custom_target(install-cxxabi-test-suite-prefix
+ DEPENDS cxxabi-headers
+ cxxabi
+ COMMAND ${CMAKE_COMMAND} -E make_directory "${LIBCXX_TESTING_INSTALL_PREFIX}"
+ COMMAND "${CMAKE_COMMAND}"
+ -DCMAKE_INSTALL_COMPONENT=cxxabi-headers
+ -DCMAKE_INSTALL_PREFIX="${LIBCXX_TESTING_INSTALL_PREFIX}"
+ -P "${CMAKE_BINARY_DIR}/cmake_install.cmake"
+ COMMAND "${CMAKE_COMMAND}"
+ -DCMAKE_INSTALL_COMPONENT=cxxabi
+ -DCMAKE_INSTALL_PREFIX="${LIBCXX_TESTING_INSTALL_PREFIX}"
+ -P "${CMAKE_BINARY_DIR}/cmake_install.cmake")
+ add_dependencies(cxx-test-depends install-cxxabi-test-suite-prefix)
+endif()
+
+if (LIBCXXABI_USE_LLVM_UNWINDER)
+ add_custom_target(install-unwind-test-suite-prefix
+ DEPENDS unwind-headers
+ unwind
+ COMMAND ${CMAKE_COMMAND} -E make_directory "${LIBCXX_TESTING_INSTALL_PREFIX}"
+ COMMAND "${CMAKE_COMMAND}"
+ -DCMAKE_INSTALL_COMPONENT=unwind-headers
+ -DCMAKE_INSTALL_PREFIX="${LIBCXX_TESTING_INSTALL_PREFIX}"
+ -P "${CMAKE_BINARY_DIR}/cmake_install.cmake"
+ COMMAND "${CMAKE_COMMAND}"
+ -DCMAKE_INSTALL_COMPONENT=unwind
+ -DCMAKE_INSTALL_PREFIX="${LIBCXX_TESTING_INSTALL_PREFIX}"
+ -P "${CMAKE_BINARY_DIR}/cmake_install.cmake")
+ add_dependencies(cxx-test-depends install-unwind-test-suite-prefix)
+endif()
+
+add_custom_target(install-cxx-test-suite-prefix
+ DEPENDS cxx-headers
+ cxx
+ cxx_experimental
+ cxx-modules
+ COMMAND ${CMAKE_COMMAND} -E make_directory "${LIBCXX_TESTING_INSTALL_PREFIX}"
+ COMMAND "${CMAKE_COMMAND}"
+ -DCMAKE_INSTALL_COMPONENT=cxx-headers
+ -DCMAKE_INSTALL_PREFIX="${LIBCXX_TESTING_INSTALL_PREFIX}"
+ -P "${CMAKE_BINARY_DIR}/cmake_install.cmake"
+ COMMAND "${CMAKE_COMMAND}"
+ -DCMAKE_INSTALL_COMPONENT=cxx-modules
+ -DCMAKE_INSTALL_PREFIX="${LIBCXX_TESTING_INSTALL_PREFIX}"
+ -P "${CMAKE_BINARY_DIR}/cmake_install.cmake"
+ COMMAND "${CMAKE_COMMAND}"
+ -DCMAKE_INSTALL_COMPONENT=cxx
+ -DCMAKE_INSTALL_PREFIX="${LIBCXX_TESTING_INSTALL_PREFIX}"
+ -P "${LIBCXX_BINARY_DIR}/cmake_install.cmake")
+add_dependencies(cxx-test-depends install-cxx-test-suite-prefix)
+
set(AUTO_GEN_COMMENT "## Autogenerated by libcxx configuration.\n# Do not edit!")
set(SERIALIZED_LIT_PARAMS "# Lit parameters serialized here for llvm-lit to pick them up\n")
diff --git a/libcxx/test/configs/cmake-bridge.cfg.in b/libcxx/test/configs/cmake-bridge.cfg.in
index 78d0cb5a25748..fe93e0bfa25e3 100644
--- a/libcxx/test/configs/cmake-bridge.cfg.in
+++ b/libcxx/test/configs/cmake-bridge.cfg.in
@@ -24,8 +24,8 @@ config.test_exec_root = os.path.join('@CMAKE_BINARY_DIR@', 'test')
# Add substitutions for bootstrapping the test suite configuration
config.substitutions.append(('%{libcxx-dir}', '@LIBCXX_SOURCE_DIR@'))
-config.substitutions.append(('%{include-dir}', '@LIBCXX_GENERATED_INCLUDE_DIR@'))
-config.substitutions.append(('%{target-include-dir}', '@LIBCXX_GENERATED_INCLUDE_TARGET_DIR@'))
-config.substitutions.append(('%{lib-dir}', '@LIBCXX_LIBRARY_DIR@'))
-config.substitutions.append(('%{module-dir}', '@LIBCXX_GENERATED_MODULE_DIR@'))
+config.substitutions.append(('%{include-dir}', '@LIBCXX_TESTING_INSTALL_PREFIX@/@LIBCXX_INSTALL_INCLUDE_DIR@'))
+config.substitutions.append(('%{target-include-dir}', '@LIBCXX_TESTING_INSTALL_PREFIX@/@LIBCXX_INSTALL_INCLUDE_TARGET_DIR@'))
+config.substitutions.append(('%{lib-dir}', '@LIBCXX_TESTING_INSTALL_PREFIX@/@LIBCXX_INSTALL_LIBRARY_DIR@'))
+config.substitutions.append(('%{module-dir}', '@LIBCXX_TESTING_INSTALL_PREFIX@/@LIBCXX_INSTALL_MODULES_DIR@'))
config.substitutions.append(('%{test-tools-dir}', '@LIBCXX_TEST_TOOLS_PATH@'))
|
@mstorsjo The following Windows job is failing: https://github.com/llvm/llvm-project/actions/runs/9703339701/job/26781775745?pr=96910
It is failing because we try installing the |
Side note, we do build libunwind on Windows. We don't build it in clang-cl configs, but we do build it in mingw configs.
In https://github.com/llvm/llvm-project/blob/main/libcxx/CMakeLists.txt#L279 you have |
I feel a bit dumb. Thanks for pointing out the obvious to me 🙂 |
3978ae8
to
132ef17
Compare
@mstorsjo We are still seeing strange failures on Windows and MinGW after fixing the obvious. Windows and MinGW both fail with different modes. The MinGW failures look like (from https://github.com/llvm/llvm-project/actions/runs/9779406851/job/26999519602?pr=96910):
On MinGW we see only 33 failures:
Can you think about an obvious reason why these tests would fail when we start running against the installed library? |
Nothing obvious that comes to mind right now… I can try to have a look at this by the end of the week, or next week - I’m on vacation at the moment, with limited time/access to things, but I’ll try to tend to it when possible. |
No worries. Please don't work on this while you're on vacation. |
68ea814
to
a45785a
Compare
The reason for this, is that the built libc++ DLL actually isn't found at runtime. In the clang-cl configurations, that means that essentially all tests fail. In the MinGW cases, there's an older libc++ DLL from the toolchain that gets found and used instead, which explains why there's only a handful of failures. Also, in static linking mode, all tests pass. The reason why the DLL isn't found, is because in an installed tree, the DLL gets installed into In |
Thanks a lot for the clear explanation! |
23fc88b
to
54b6392
Compare
@philnik777 @mordante @EricWF I'd like someone to take a look at this. IMO this is an important improvement to the way we conceptually run the test suite: we run it on the result of installing the library, which means we test exactly what we ship. However, this adds some verbosity to the build due to some CMake limitations and I'd like someone else to try it out and let me know what they think. Specifically, when running |
54b6392
to
5235278
Compare
I had a quick look at the code, before looking closer and doing some testing I've question. I see no documentation changes, does this mean there is no difference in the work-flow with this patch? Specifically the information on this page https://libcxx.llvm.org/TestingLibcxx.html. |
That is correct, this doesn't affect the workflow at all. The only difference is that you'll notice that we install the library to a fake location and the paths that we use for testing (e.g. to refer to |
…ibrary We always strive to test libc++ as close as possible to the way we are actually shipping it. This was approximated reasonably well by setting up the minimal driver flags when running the test suite, however we were running the test suite against the library located in the build directory. This patch improves the situation by installing the library (the headers, the built library, modules, etc) into a fake location and then running the test suite against that fake "installation root". This should open the door to getting rid of the temporary copy of the headers we make during the build process, however this is left for a future improvement. Note that this adds quite a bit of verbosity whenever running the test suite because we install the headers beforehand every time. We should be able to override this to silence it, however CMake doesn't currently give us a way to do that, see https://gitlab.kitware.com/cmake/cmake/-/issues/26085.
5235278
to
ebac193
Compare
I'd like to merge this soon to unblock other work. @mordante did you have any blocking feedback on this PR? |
I merged this -- I will address any feedback post-commit. |
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/193/builds/2254 Here is the relevant piece of the build log for the reference
|
@vvereschaka It looks like that bot might not be building the |
It is possible. The bot uses the following command to run the test suites
instead of using I don't remember the reason of it already, but initially it was changed to use the direct execution of the tests by |
I see the failed ibc++ test of the aarch64 cross toolchain builder. |
Ack. Please let me know. This definitely looks like the bot doing something slightly wrong, I don't think that's a problem with the patch. But let's discuss in case the fix isn't obvious. |
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/38/builds/339 Here is the relevant piece of the build log for the reference
|
…ibrary (llvm#96910) We always strive to test libc++ as close as possible to the way we are actually shipping it. This was approximated reasonably well by setting up the minimal driver flags when running the test suite, however we were running the test suite against the library located in the build directory. This patch improves the situation by installing the library (the headers, the built library, modules, etc) into a fake location and then running the test suite against that fake "installation root". This should open the door to getting rid of the temporary copy of the headers we make during the build process, however this is left for a future improvement. Note that this adds quite a bit of verbosity whenever running the test suite because we install the headers beforehand every time. We should be able to override this to silence it, however CMake doesn't currently give us a way to do that, see https://gitlab.kitware.com/cmake/cmake/-/issues/26085.
…515f18a36 Local branch amd-gfx 4ce515f Merged main:69c43468d3f21df6232fda0530f03f18b0f40345 into amd-gfx:45ac84d24a03 Remote branch main 0e8208e [libc++] Run the Lit test suite against an installed version of the library (llvm#96910)
We always strive to test libc++ as close as possible to the way we are actually shipping it. This was approximated reasonably well by setting up the minimal driver flags when running the test suite, however we were running the test suite against the library located in the build directory.
This patch improves the situation by installing the library (the headers, the built library, modules, etc) into a fake location and then running the test suite against that fake "installation root".
This should open the door to getting rid of the temporary copy of the headers we make during the build process, however this is left for a future improvement.
Note that this adds quite a bit of verbosity whenever running the test suite because we install the headers beforehand every time. We should be able to override this to silence it, however CMake doesn't currently give us a way to do that, see https://gitlab.kitware.com/cmake/cmake/-/issues/26085.