-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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
[clang-doc][cmake] Copy assets to build directory #95187
Conversation
While we copy the asset files, like index.js, into the correct location in the install step, tests do not have access to those resources in the build directory. This patch copies the contents of the clang-doc/assets directory into the build folder, so that they can be used in testing. Pull Request: llvm#95185
@llvm/pr-subscribers-clang-tools-extra Author: Paul Kirth (ilovepi) ChangesWhile we copy the asset files, like index.js, into the correct location in the install step, tests do not have access to those resources in the build directory. This patch copies the contents of the clang-doc/assets directory into the build folder, so that they can be used in testing. Pull Request: #95185 Full diff: https://github.com/llvm/llvm-project/pull/95187.diff 1 Files Affected:
diff --git a/clang-tools-extra/clang-doc/tool/CMakeLists.txt b/clang-tools-extra/clang-doc/tool/CMakeLists.txt
index fb8317b272932..c2425747562b9 100644
--- a/clang-tools-extra/clang-doc/tool/CMakeLists.txt
+++ b/clang-tools-extra/clang-doc/tool/CMakeLists.txt
@@ -25,3 +25,11 @@ install(FILES ../assets/clang-doc-default-stylesheet.css
install(FILES ../assets/index.js
DESTINATION "${CMAKE_INSTALL_DATADIR}/clang"
COMPONENT clang-doc)
+
+add_custom_target(copy-clang-doc-assets
+ COMMAND ${CMAKE_COMMAND} -E copy_directory_if_different "${CMAKE_CURRENT_SOURCE_DIR}/../assets" "${CMAKE_BINARY_DIR}/share/clang"
+ DEPENDS "${CMAKE_CURRENT_SOURCE_DIR}/../assets"
+ COMMENT "Copying Clang-Doc Assets"
+ )
+set_target_properties(copy-clang-doc-assets PROPERTIES FOLDER "Clang-Doc/Assets")
+add_dependencies(clang-doc copy-clang-doc-assets)
|
@petrhosek I'm pretty sure there's a better way to spell this, but I think we need something similar to properly test clang-doc w/o an install step. cc: @PeterChou1 |
It looks like copy_directory_if_different is not available on windows |
@@ -25,3 +25,11 @@ install(FILES ../assets/clang-doc-default-stylesheet.css | |||
install(FILES ../assets/index.js | |||
DESTINATION "${CMAKE_INSTALL_DATADIR}/clang" | |||
COMPONENT clang-doc) | |||
|
|||
add_custom_target(copy-clang-doc-assets |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We prefer having a rule per file since most systems don't have a reliable way to tell if the directory content has changed. The approach we usually use to to have one add_custom_command
per file with a single add_custom_target
that depends on all output files, see for example
llvm-project/libcxx/include/CMakeLists.txt
Lines 1024 to 1043 in 092dbfa
foreach(f ${files}) | |
set(src "${CMAKE_CURRENT_SOURCE_DIR}/${f}") | |
set(dst "${LIBCXX_GENERATED_INCLUDE_DIR}/${f}") | |
add_custom_command(OUTPUT ${dst} | |
DEPENDS ${src} | |
COMMAND ${CMAKE_COMMAND} -E copy_if_different ${src} ${dst} | |
COMMENT "Copying CXX header ${f}") | |
list(APPEND _all_includes "${dst}") | |
endforeach() | |
# Generate the IWYU mapping. This depends on all header files but it's also considered as an | |
# "include" for dependency tracking. | |
add_custom_command(OUTPUT "${LIBCXX_GENERATED_INCLUDE_DIR}/libcxx.imp" | |
COMMAND "${Python3_EXECUTABLE}" "${LIBCXX_SOURCE_DIR}/utils/generate_iwyu_mapping.py" "-o" "${LIBCXX_GENERATED_INCLUDE_DIR}/libcxx.imp" | |
DEPENDS ${_all_includes} | |
COMMENT "Generate the mapping file for include-what-you-use" | |
) | |
list(APPEND _all_includes "${LIBCXX_GENERATED_INCLUDE_DIR}/libcxx.imp") | |
add_custom_target(generate-cxx-headers ALL DEPENDS ${_all_includes}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I’ll take a pass at adapting one of those for use here. If I think I can generalize it, I’ll make another PR to add it as a utility in the llvm cmake module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is addressed now. I more or less cribed directly from the clang header example. Initially I tried to have the function be a bit more generic and put it in llvm/cmake/modules/CopyLLVMFiles.cmake
, but my normal methods of including don't seem to work in clang-doc
. Likely I'm just holding that part wrong, but I don't see anything in clang-tools-extra
using include(AddLLVM)
or some other module.
Happy to refactor if you have some other suggestions.
Ugh. I forgot to check the minimum cmake version. This was introduced in 3.26, which IIRC is higher than LLVM’S minimum. https://cmake.org/cmake/help/latest/manual/cmake.1.html#cmdoption-cmake-E-arg-copy_directory_if_different |
While we copy the asset files, like index.js, into the correct location in the install step, tests do not have access to those resources in the build directory.
This patch copies the contents of the clang-doc/assets directory into the build folder, so that they can be used in testing.
Pull Request: #95185