This patch ensures all resources are installed into CLANG_RESOURCE_DIR.
Fixes: https://github.com/llvm/llvm-project/issues/59726
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Time | Test | |
---|---|---|
1,500,040 ms | libcxx CI Apple back-deployment macosx11.0 arm64 > apple-libc++-backdeployment-cfg-in.std/thread/thread_condition::notify_all_at_thread_exit_lwg3343.pass.cpp Script:
--
: 'COMPILED WITH'; /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/c++ /Users/libcxx-buildkite-agent/libcxx.buildkite-agent/builds/w4-4macminivaultcom-local/llvm-project/libcxx-ci/libcxx/test/std/thread/thread.condition/notify_all_at_thread_exit_lwg3343.pass.cpp -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX12.3.sdk --target=arm64-apple-macosx11.0 -nostdinc++ -I /Users/libcxx-buildkite-agent/libcxx.buildkite-agent/builds/w4-4macminivaultcom-local/llvm-project/libcxx-ci/build/apple-system-backdeployment-11.0/include/c++/v1 -I /Users/libcxx-buildkite-agent/libcxx.buildkite-agent/builds/w4-4macminivaultcom-local/llvm-project/libcxx-ci/libcxx/test/support -std=c++2b -Werror -Wall -Wctad-maybe-unsupported -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-noexcept-type -Wno-atomic-alignment -Wno-user-defined-literals -Wno-tautological-compare -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -Werror=thread-safety -Wuser-defined-warnings -nostdlib++ -L /Users/libcxx-buildkite-agent/libcxx.buildkite-agent/builds/w4-4macminivaultcom-local/llvm-project/libcxx-ci/build/apple-system-backdeployment-11.0/lib -lc++ -o /Users/libcxx-buildkite-agent/libcxx.buildkite-agent/builds/w4-4macminivaultcom-local/llvm-project/libcxx-ci/build/apple-system-backdeployment-11.0/test/std/thread/thread.condition/Output/notify_all_at_thread_exit_lwg3343.pass.cpp.dir/t.tmp.exe
| |
1,500,100 ms | libcxx CI Apple back-deployment with assertions enabled > apple-libc++-backdeployment-cfg-in.std/thread/thread_condition::notify_all_at_thread_exit_lwg3343.pass.cpp Script:
--
: 'COMPILED WITH'; /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/c++ /Users/libcxx-buildkite-agent/libcxx.buildkite-agent/builds/f1-1-macminivault-com/llvm-project/libcxx-ci/libcxx/test/std/thread/thread.condition/notify_all_at_thread_exit_lwg3343.pass.cpp -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX12.3.sdk --target=arm64-apple-macosx11.0 -nostdinc++ -I /Users/libcxx-buildkite-agent/libcxx.buildkite-agent/builds/f1-1-macminivault-com/llvm-project/libcxx-ci/build/apple-system-backdeployment-assertions-11.0/include/c++/v1 -I /Users/libcxx-buildkite-agent/libcxx.buildkite-agent/builds/f1-1-macminivault-com/llvm-project/libcxx-ci/libcxx/test/support -std=c++2b -Werror -Wall -Wctad-maybe-unsupported -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-noexcept-type -Wno-atomic-alignment -Wno-user-defined-literals -Wno-tautological-compare -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D_LIBCPP_ENABLE_ASSERTIONS=1 -Werror=thread-safety -Wuser-defined-warnings -nostdlib++ -L /Users/libcxx-buildkite-agent/libcxx.buildkite-agent/builds/f1-1-macminivault-com/llvm-project/libcxx-ci/build/apple-system-backdeployment-assertions-11.0/lib -lc++ -o /Users/libcxx-buildkite-agent/libcxx.buildkite-agent/builds/f1-1-macminivault-com/llvm-project/libcxx-ci/build/apple-system-backdeployment-assertions-11.0/test/std/thread/thread.condition/Output/notify_all_at_thread_exit_lwg3343.pass.cpp.dir/t.tmp.exe
|
Event Timeline
While the general direction seems sound, I suspect this may be a real PITA given that for us:
-DCLANG_RESOURCE_DIR="../../../../lib/clang/${LLVM_MAJOR}"
I'm going to try and see later throughout the week if it's possible to keep that working with this patch.
Ok, this turned out to be surprisingly painless, at least within our packaging.
However, I'd prefer if someone more experienced looked at the CMake changes.
The CMake part of this patch improves the matter. Manually constructed resource dir (many duplicates) string is replace with a library function call.
However, the introduced conditions in C++ code like
std::string path_to_clang_dir = std::string(CLANG_RESOURCE_DIR).empty() ? "/foo/bar/" LLDB_INSTALL_LIBDIR_BASENAME "/clang/" CLANG_VERSION_MAJOR_STRING : "/foo/bar/bin/" CLANG_RESOURCE_DIR;
makes me uncomfortable.
I wish that we can just replace all manually constructed CLANG_INSTALL_LIBDIR_BASENAME "/clang/" CLANG_VERSION_MAJOR_STRING with a simple CLANG_RESOURCE_DIR which is guaranteed to be non-empty.
edeaf16f2c2f02d6e43312d48d26d354d87913f3 (2011) added the CMake variable CLANG_RESOURCE_DIR but did not explain why.
In the long term, the CMake variable CLANG_RESOURCE_DIR probably should be removed.
cmake/Modules/GetClangResourceDir.cmake | ||
---|---|---|
18 | This variable is only used once on the following line, I'd just inline it which is more idiomatic. | |
19 | It's more idiomatic to parse arguments at the start of the function. | |
19 | This is just a nit, but we usually use a prefix like ARGS to make it clear that these are input arguments. |
Ping.
lldb/source/Plugins/ExpressionParser/Clang/ClangHost.cpp | ||
---|---|---|
56 | Should be "${CMAKE_INSTALL_BINDIR}/lldb" here, but need another variable in config.h. |
cmake/Modules/GetClangResourceDir.cmake | ||
---|---|---|
13 | Why is the bin prefix here? |
cmake/Modules/GetClangResourceDir.cmake | ||
---|---|---|
13 | See https://clang.llvm.org/doxygen/classclang_1_1driver_1_1Driver.html#acda8dfdf4f80efa84df98157e1779152 |
cmake/Modules/GetClangResourceDir.cmake | ||
---|---|---|
13 | GetResourcesPath calls sys::path::parent_path twice on the path to the clang executable which is going to give you /usr on most Linux systems, so it's returning <prefix>/CLANG_RESOURCE_DIR not <prefix>/bin/CLANG_RESOURCE_DIR. |
cmake/Modules/GetClangResourceDir.cmake | ||
---|---|---|
13 | Sorry, you are correct. I was looking at the wrong branch. It's really strange that it does that. |
cmake/Modules/GetClangResourceDir.cmake | ||
---|---|---|
13 | @paperchalice Can you update the description of the CLANG_RESOURCE_DIR cache variable in clang/CMakeLists.txt to mention that the path should be relative to the directory with the clang executable. |
Thanks for the patch. I've tested this and it works for my build configuration. LGTM.
Sorry for the late, the information in phabricator is OK, and GetClangResourceDir.cmake is not landed correctly. Ping @tstellar
The review should not be closed, since the patch was reverted and should be recommitted (this time possibly with a reference to this review?)
This is recommitted as 0beffb854209a41f31beb18f9631258349a99299, but break some lldb builds, so I created https://reviews.llvm.org/D152225 .