Page MenuHomePhabricator

[CMake] Ensure `CLANG_RESOURCE_DIR` is respected
Needs ReviewPublic

Authored by paperchalice on Jan 17 2023, 2:29 AM.

Details

Summary

This patch ensures all resources are installed into CLANG_RESOURCE_DIR.
Fixes: https://github.com/llvm/llvm-project/issues/59726

Diff Detail

Unit TestsFailed

TimeTest
1,500,040 mslibcxx 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 mslibcxx 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

paperchalice created this revision.Jan 17 2023, 2:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 17 2023, 2:29 AM
Herald added a subscriber: Enna1. · View Herald Transcript
paperchalice requested review of this revision.Jan 17 2023, 2:29 AM
Herald added projects: Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project. · View Herald TranscriptJan 17 2023, 2:29 AM

Use LLVM_LIBRARY_OUTPUT_INTDIR.

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.

MaskRay added a comment.EditedJan 31 2023, 1:41 PM

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.

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.

clang::driver::Driver::GetResourcesPath would be a better choice.

phosek added inline comments.Feb 2 2023, 1:07 AM
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.

  • Use clang::driver::Driver::GetResourcesPath
  • Use ARG as prefix

Ping.

lldb/source/Plugins/ExpressionParser/Clang/ClangHost.cpp
56

Should be "${CMAKE_INSTALL_BINDIR}/lldb" here, but need another variable in config.h.