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

NFC: Make clang resource headers an interface library #88317

Merged

Conversation

etcwilde
Copy link
Contributor

Making the clang resource headers into an interface library instead of a custom target means that we can attach the header search paths to the library. Targets that "link" against this library will automatically have the appropriate paths added to their header search paths to find them. Then downstream projects that embed a copy of clang can query the generated ClangTargets.cmake file for the header location instead of attempting to compute them.

@llvmbot llvmbot added clang Clang issues not falling into any other category backend:X86 clang:headers Headers provided by Clang, e.g. for intrinsics labels Apr 10, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Apr 10, 2024

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-backend-x86

Author: Evan Wilde (etcwilde)

Changes

Making the clang resource headers into an interface library instead of a custom target means that we can attach the header search paths to the library. Targets that "link" against this library will automatically have the appropriate paths added to their header search paths to find them. Then downstream projects that embed a copy of clang can query the generated ClangTargets.cmake file for the header location instead of attempting to compute them.


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

1 Files Affected:

  • (modified) clang/lib/Headers/CMakeLists.txt (+6-2)
diff --git a/clang/lib/Headers/CMakeLists.txt b/clang/lib/Headers/CMakeLists.txt
index 97104ccd8db59c..0367f611526f41 100644
--- a/clang/lib/Headers/CMakeLists.txt
+++ b/clang/lib/Headers/CMakeLists.txt
@@ -437,14 +437,14 @@ foreach( f ${generated_files} )
 endforeach( f )
 
 function(add_header_target target_name file_list)
-  add_custom_target(${target_name} DEPENDS ${file_list})
+  add_library(${target_name} INTERFACE ${file_list})
   set_target_properties(${target_name} PROPERTIES
     FOLDER "Misc"
     RUNTIME_OUTPUT_DIRECTORY "${output_dir}")
 endfunction()
 
 # The catch-all clang-resource-headers target
-add_custom_target("clang-resource-headers" ALL DEPENDS ${out_files})
+add_library(clang-resource-headers INTERFACE ${out_files})
 set_target_properties("clang-resource-headers" PROPERTIES
   FOLDER "Misc"
   RUNTIME_OUTPUT_DIRECTORY "${output_dir}")
@@ -501,6 +501,10 @@ add_header_target("windows-resource-headers" ${windows_only_files})
 add_header_target("utility-resource-headers" ${utility_files})
 
 get_clang_resource_dir(header_install_dir SUBDIR include)
+target_include_directories(clang-resource-headers INTERFACE
+  $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}>
+  $<INSTALL_INTERFACE:${header_install_dir}>)
+set_property(GLOBAL APPEND PROPERTY CLANG_EXPORTS clang-resource-headers)
 
 #############################################################
 # Install rules for the catch-all clang-resource-headers target

Copy link
Member

@compnerd compnerd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a step in the right direction - avoiding custom targets is generally better.

clang/lib/Headers/CMakeLists.txt Outdated Show resolved Hide resolved
Making the clang resource headers into an interface library means that
we can attach the header search paths to the library. Targets that
"link" against this library will automatically have the appropriate
paths added to their header search paths to find them.
@etcwilde etcwilde force-pushed the ewilde/clang-resource-headers-as-a-library branch from de73a40 to 753df93 Compare April 10, 2024 21:36
Copy link
Member

@compnerd compnerd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this seems good!

@etcwilde etcwilde merged commit 8d468c1 into llvm:main Apr 12, 2024
4 checks passed
@etcwilde etcwilde deleted the ewilde/clang-resource-headers-as-a-library branch April 12, 2024 16:06
bazuzi pushed a commit to bazuzi/llvm-project that referenced this pull request Apr 15, 2024
Making the clang resource headers into an interface library instead of a
custom target means that we can attach the header search paths to the
library. Targets that "link" against this library will automatically
have the appropriate paths added to their header search paths to find
them. Then downstream projects that embed a copy of clang can query the
generated `ClangTargets.cmake` file for the header location instead of
attempting to compute them.
@JDevlieghere
Copy link
Member

Hey @etcwilde, this breaks the Xcode standalone build for lldb: https://green.lab.llvm.org/job/llvm.org/view/LLDB/job/lldb-cmake-standalone/121/

The failing command in the generated script phase is:

cmake -E copy_directory  /Users/jonas/llvm/xcode-build/Debug/bin/LLDB.framework/Versions/A/Resources/Clang/include

It looks like it's missing the source directly. Mind taking a look?

JDevlieghere added a commit that referenced this pull request Apr 15, 2024
In #88317, the clang resource headers was converted to an interface
library. Update LLDB and fix the Xcode standalone build. Thanks Evan for
the help!
aniplcc pushed a commit to aniplcc/llvm-project that referenced this pull request Apr 15, 2024
In llvm#88317, the clang resource headers was converted to an interface
library. Update LLDB and fix the Xcode standalone build. Thanks Evan for
the help!
@ayermolo
Copy link
Contributor

I am seeing a cmake error in one of the build steps in our enviroment:

CMake Error at CMakeLists.txt:86 (get_property):
  get_property could not find TARGET clang-resource-headers.  Perhaps it has
  not yet been created.

Reverting this diff internally made build pass.
I'll try to create small repro, but in meantime can you revert?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend:X86 clang:headers Headers provided by Clang, e.g. for intrinsics clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants