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

RFC: [cmake] Export CLANG_RESOURCE_DIR in ClangConfig #97197

Closed
wants to merge 1 commit into from

Conversation

kimgr
Copy link
Contributor

@kimgr kimgr commented Jun 30, 2024

I recently opened #95747 to see if it would be advisable to expose CLANG_RESOURCE_DIR from ClangConfig.cmake.

Here's a patch to do the most naive thing I could think of, hopefully enough to trigger discussion.

Open questions/concerns:

  • ClangConfig.cmake now defines CLANG_RESOURCE_DIR -- will that interfere with the CMake system's CLANG_RESOURCE_DIR (which is e.g. baked into config.h)
  • The builddir vs. installdir distinction when generate ClangConfig.cmake isn't 100% clear to me, I guessed a little as to reasonable prefix paths

@llvmbot llvmbot added the clang Clang issues not falling into any other category label Jun 30, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Jun 30, 2024

@llvm/pr-subscribers-clang

Author: Kim Gräsman (kimgr)

Changes

I recently opened #95747 to see if it would be advisable to expose CLANG_RESOURCE_DIR from ClangConfig.cmake.

Here's a patch to do the most naive thing I could think of, hopefully enough to trigger discussion.

Open questions/concerns:

  • ClangConfig.cmake now defines CLANG_RESOURCE_DIR -- will that interfere with the CMake system's CLANG_RESOURCE_DIR (which is e.g. baked into config.h)
  • The builddir vs. installdir distinction when generate ClangConfig.cmake isn't 100% clear to me, I guessed a little as to reasonable prefix paths

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

2 Files Affected:

  • (modified) clang/cmake/modules/CMakeLists.txt (+9)
  • (modified) clang/cmake/modules/ClangConfig.cmake.in (+1)
diff --git a/clang/cmake/modules/CMakeLists.txt b/clang/cmake/modules/CMakeLists.txt
index d2d68121371bf..4b0a38bd0fc1e 100644
--- a/clang/cmake/modules/CMakeLists.txt
+++ b/clang/cmake/modules/CMakeLists.txt
@@ -2,6 +2,7 @@ include(GNUInstallPackageDir)
 include(ExtendPath)
 include(LLVMDistributionSupport)
 include(FindPrefixFromConfig)
+include(GetClangResourceDir)
 
 # Generate a list of CMake library targets so that other CMake projects can
 # link against them. LLVM calls its version of this file LLVMExports.cmake, but
@@ -29,6 +30,10 @@ set(CLANG_CONFIG_INCLUDE_DIRS
   "${CLANG_SOURCE_DIR}/include"
   "${CLANG_BINARY_DIR}/include"
   )
+
+get_clang_resource_dir(resource_builddir PREFIX ${CLANG_BINARY_DIR})
+set(CLANG_CONFIG_RESOURCE_DIR ${resource_builddir})
+
 configure_file(
   ${CMAKE_CURRENT_SOURCE_DIR}/ClangConfig.cmake.in
   ${clang_cmake_builddir}/ClangConfig.cmake
@@ -60,6 +65,10 @@ extend_path(base_includedir "\${CLANG_INSTALL_PREFIX}" "${CMAKE_INSTALL_INCLUDED
 set(CLANG_CONFIG_INCLUDE_DIRS
   "${base_includedir}"
   )
+
+get_clang_resource_dir(resource_installdir PREFIX ${CLANG_INSTALL_PREFIX})
+set(CLANG_CONFIG_RESOURCE_DIR "${resource_installdir}")
+
 configure_file(
   ${CMAKE_CURRENT_SOURCE_DIR}/ClangConfig.cmake.in
   ${CMAKE_CURRENT_BINARY_DIR}/CMakeFiles/ClangConfig.cmake
diff --git a/clang/cmake/modules/ClangConfig.cmake.in b/clang/cmake/modules/ClangConfig.cmake.in
index 5f67681649c66..1b97e103c93ca 100644
--- a/clang/cmake/modules/ClangConfig.cmake.in
+++ b/clang/cmake/modules/ClangConfig.cmake.in
@@ -10,6 +10,7 @@ set(CLANG_EXPORTED_TARGETS "@CLANG_EXPORTS@")
 set(CLANG_CMAKE_DIR "@CLANG_CONFIG_CMAKE_DIR@")
 set(CLANG_INCLUDE_DIRS "@CLANG_CONFIG_INCLUDE_DIRS@")
 set(CLANG_LINK_CLANG_DYLIB "@CLANG_LINK_CLANG_DYLIB@")
+set(CLANG_RESOURCE_DIR "@CLANG_CONFIG_RESOURCE_DIR@")
 
 # Provide all our library targets to users.
 @CLANG_CONFIG_INCLUDE_EXPORTS@

@kimgr
Copy link
Contributor Author

kimgr commented Jun 30, 2024

cc @llvm-beanz -- you seem to have done a fair bit with the LLVM CMake system.

@tstellar tstellar self-requested a review July 8, 2024 16:40
@llvm-beanz
Copy link
Collaborator

Is there a reason you need to collect those files for your build tree instead of using them from where Clang built/installed them?

@kimgr
Copy link
Contributor Author

kimgr commented Jul 12, 2024

I think at the time we started doing this, Clang's builtin includes lookup basically did $dirname($0)/../lib/llvm-$version/clang/$version/include. So when building include-what-you-use in a separate tree, the resulting binary didn't have the builtin headers in the right place, and we wouldn't be able to run tests.

We support three build modes:

  • as part of LLVM+Clang with LLVM_EXTERNAL_PROJECTS=iwyu and LLVM_EXTERNAL_IWYU_SOURCE_DIR
  • with CMAKE_PREFIX_PATH pointing to a package-installed LLVM+Clang
  • with CMAKE_PREFIX_PATH pointing to an LLVM+Clang build tree

In the first case, depending on the Headers target makes sure all headers are copied into the build tree.

For the latter two we synthesize a Headers target (because it's not exported) based on the Clang resource dir: https://github.com/include-what-you-use/include-what-you-use/blob/master/CMakeLists.txt#L54

@llvm-beanz
Copy link
Collaborator

I think at the time we started doing this, Clang's builtin includes lookup basically did $dirname($0)/../lib/llvm-$version/clang/$version/include. So when building include-what-you-use in a separate tree, the resulting binary didn't have the builtin headers in the right place, and we wouldn't be able to run tests.

clang::Driver has this method which allows you to specify the starting path for the clang installation to use for the resource lookup.

/// Takes the path to a binary that's either in bin/ or lib/ and returns
/// the path to clang's resource directory.
static std::string GetResourcesPath(StringRef BinaryPath,
                                    StringRef CustomResourceDir = "");

We support three build modes:

  • as part of LLVM+Clang with LLVM_EXTERNAL_PROJECTS=iwyu and LLVM_EXTERNAL_IWYU_SOURCE_DIR
  • with CMAKE_PREFIX_PATH pointing to a package-installed LLVM+Clang
  • with CMAKE_PREFIX_PATH pointing to an LLVM+Clang build tree

With the function above the first two use cases should work fine if you just point at the clang installation.

For the third case... Posting at where you grab clang from will work, but you can't install from that configuration. That's probably fine for development workflows but won't work if you're using it in your package distribution workflows. I'm not sure we should support that build configuration generally anyways. We have supported it because the Swift build system does unholy and awful things, but it's all a pile of hacks and it has lots of edge cases where it breaks.

@llvm-beanz
Copy link
Collaborator

(cc @compnerd, @gottesmm, & @etcwilde who have played roles in building and maintaining all the build-tree stuff for Swift).

@etcwilde
Copy link
Contributor

So, Swift actually has/had a similar issue. We embed a copy of clang in Swift, so we needed to find the corresponding clang resource headers and install them with our compiler which is why I wrote #88317 to expose them. It adds the clang-resource-headers as an interface library to the ClangConfig.cmake that gets loaded when you do find_package(Clang CONFIG). "Linking" against that interface library adds the clang-resource-header location to your include search paths.

# Find 'ClangConfig.cmake' and import its targets
find_package(Clang CONFIG REQUIRED)

# Pass the location as a header-search path to "SomeLibrary"
target_link_libraries(SomeLibrary PRIVATE clang-resource-headers)

# Alternatively, if you just want the location, it's available as a target-property:
get_target_property(CLANG_IMPORT_HEADERS clang-resource-headers INTERFACE_INCLUDE_DIRECTORIES)

To install the ClangConfig.cmake and the clang cmake exports files in the LLVM build/install, I believe the install component is clang-cmake-exports.

@kimgr
Copy link
Contributor Author

kimgr commented Jul 13, 2024

@llvm-beanz Thanks! Yes, copying the headers does seem like a hack, in retrospect. I think I started down that path because I don't know how packaging works -- I had assumed I might have to bundle the headers with IWYU, but I guess the right procedure is to build against an installed Clang, wire the paths for that installation, and express a dependency in the resulting package?

So is the recommendation to:

  • feed path to Clang binary in from CMake via preprocessor symbol or something (where do I find that on the CMake side, btw?)
  • resPath = GetResourcesPath(clangBinaryPath)
  • Add a -resource-dir $resPath switch as part of driver construction

?

@etcwilde Cool, that might be helpful! I don't need it on the include path, I need to somehow forward it into my tool so it will be able to find <stddef.h> and friends at runtime. Your last example appears to get the path in a form where that's possible, thanks!

@llvm-beanz
Copy link
Collaborator

@kimgr you should be able to have CMake query the path of libclang and go from there. Something like:

get_target_property(SHARED_LIB_DIR libclang RUNTIME_OUTPUT_DIRECTORY)

That should give you the binary location of libclang, which should work for resolving the final path.

@kimgr
Copy link
Contributor Author

kimgr commented Jul 15, 2024

@llvm-beanz Thanks for the pointers!

Here's what I ended up with as a first step:
include-what-you-use/include-what-you-use@b03f60a

I couldn't get the get_target_property spelling you suggested to work (it doesn't resolve when building against the Debian packages), so I ended up with something else.

The LLVM_EXTERNAL_PROJECT mode solution using $<TARGET_FILE:clang> won't work so well, as it pins the resource directory to the build tree. So it won't resolve correctly after install, I suppose.

@kimgr
Copy link
Contributor Author

kimgr commented Jul 15, 2024

The LLVM_EXTERNAL_PROJECT mode solution using $<TARGET_FILE:clang> won't work so well,
as it pins the resource directory to the build tree. So it won't resolve correctly after install, I suppose.

Ah, in that mode I guess we could use the default behavior, and not override the resource dir.

@tuliom
Copy link
Contributor

tuliom commented Jul 25, 2024

clang::Driver has this method which allows you to specify the starting path for the clang installation to use for the resource lookup.

/// Takes the path to a binary that's either in bin/ or lib/ and returns
/// the path to clang's resource directory.
static std::string GetResourcesPath(StringRef BinaryPath,
                                    StringRef CustomResourceDir = "");

I don't think an implementation based on GetResourcesPath() will work outside of Clang.
In order for this function to return the correct PATH for all scenarios, we need that CustomResourceDir=CLANG_RESOURCE_DIR.
That forces us back to the original discussion about exporting CLANG_RESOURCE_DIR.

Here is an example on how this function is used in Clang:
https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/Driver.cpp#L242

@tstellar
Copy link
Collaborator

I think that the GetResourcesPath function is incorrect. There's no reason it should return a wrong value, since the CLANG_RESOURCE_DIR value is always known at compile time.

@etcwilde
Copy link
Contributor

That forces us back to the original discussion about exporting CLANG_RESOURCE_DIR

The path is already available through the exported clang-resource-header imported interface target.

@kimgr
Copy link
Contributor Author

kimgr commented Jul 26, 2024

@etcwilde

The path is already available through the exported clang-resource-header imported interface target

I think that one has the full path to the compiler-supplied built-in headers, not the resource dir (which is basically its parent). We could do some path mangling, but that's what burned us in the first place.

@kimgr
Copy link
Contributor Author

kimgr commented Jul 29, 2024

@tstellar @tuliom It seems to me almost all callers pass CLANG_RESOURCE_DIR for the CustomResourceDir optional argument:
https://github.com/search?q=repo%3Allvm%2Fllvm-project%20Driver%3A%3AGetResourcesPath&type=code

the one exception being:

ResourcesPath = driver::Driver::GetResourcesPath(LibClangPath);

I wonder if that last one is a bug...? If so, it seems it would be better to just fold CLANG_RESOURCE_DIR into Driver::GetResourcesPath and remove the optional argument.

@tuliom
Copy link
Contributor

tuliom commented Jul 29, 2024

I wonder if that last one is a bug...?

Agreed.

If so, it seems it would be better to just fold CLANG_RESOURCE_DIR into Driver::GetResourcesPath and remove the optional argument.

Or making CustomResourceDir = CLANG_RESOURCE_DIR by default instead of "".
This would allow users to continue setting a different resource directory than CLANG_RESOURCE_DIR, although I still don't understand in which case that would be used.

@kimgr
Copy link
Contributor Author

kimgr commented Jul 29, 2024

Or making CustomResourceDir = CLANG_RESOURCE_DIR by default instead of "".

Binding that default value in Driver.h would leave it to external projects (using Clang) to resolve CLANG_RESOURCE_DIR, which is not available outside Clang. So it somehow needs to move into the body of GetResourcesPath.

Also, looking at the use of CustomResourceDir, it looks like it expects path relative to dirname(clang), i.e. it would have to be something like BinaryPath="/usr/lib/llvm-20/bin/clang" and {CLANG_RESOURCE_DIR,CustomResourceDir}="../share/my-tool/". Overall CLANG_RESOURCE_DIR and that custom resource dir seems pretty hard to get right, I wonder if it's ever used.

It was originally added to the LLVM CMake in ffae4a6 and used in the subsequent 06067c5. I'm curious about these 'strange packaging environments" :)

@tuliom
Copy link
Contributor

tuliom commented Jul 29, 2024

@kimgr Good points. I agree with you.

@llvm-beanz
Copy link
Collaborator

@etcwilde's point above is that the CLANG_RESOURCE_DIR is already available in CMake for projects that import the Clang package or are built in-tree with Clang. You can get the directory via CMake with:

get_target_property(CLANG_IMPORT_HEADERS clang-resource-headers INTERFACE_INCLUDE_DIRECTORIES)

That should make this change unnecessary even in the case where you need the resource directory in CMake.

@kimgr
Copy link
Contributor Author

kimgr commented Aug 1, 2024

@llvm-beanz

@etcwilde's point above is that the CLANG_RESOURCE_DIR is already available in CMake for projects
that import the Clang package or are built in-tree with Clang.

I don't think so. As far as I can tell:

  • clang-resource-headers interface header dir would be the full path to the builtin headers dir, e.g. /usr/lib/llvm-19/lib/clang/19/include
  • Unfortunately the Debian packages from apt.llvm.org don't define clang-resource-headers, though the target name is listed in CLANG_EXPORTED_TARGETS
  • clang-19 -print-resource-dir prints the effective resource dir: /usr/lib/llvm-19/lib/clang/19
  • CLANG_RESOURCE_DIR appears to be designed to be a relative path appended to /usr/lib/llvm-19/{bin,lib}

So I'm thinking even if the clang-resource-headers target was available, it would not give the path to the resource dir, but rather the path to the subdir containing the builtin headers. That might be fine, i.e. I could take that path and add it with -isystem ..., but if the resource dir is already implicitly on the header search path, that seems preferable.

@llvm-beanz
Copy link
Collaborator

So I'm thinking even if the clang-resource-headers target was available, it would not give the path to the resource dir, but rather the path to the subdir containing the builtin headers. That might be fine, i.e. I could take that path and add it with -isystem ..., but if the resource dir is already implicitly on the header search path, that seems preferable.

clang-resource-headers gives you the path to the header folder inside the resource dir, but that's always just one level in. So you can just drop the last directory then in your CMake you have the resource dir. You can do anything you want with that in your CMake the same way you would with the variable you're proposing to add in this change.

I don't see why you need a separate variable.

We can't really help you with whatever is wrong with Debian's packages unless it is a bug caused by our build configurations. The change you're proposing in this PR could just as easily be impacted in some distro's packages.

@kimgr
Copy link
Contributor Author

kimgr commented Aug 2, 2024

@llvm-beanz Thanks for clarifying! Yeah, I no longer think this PR has anything to offer. If you think it's future-proof to assume the interface include dir of clang-resource-headers is one level down from the resource dir, that works to polish and seed it into a hardwired -resource-dir switch.

@etcwilde
Copy link
Contributor

etcwilde commented Aug 2, 2024

We can't really help you with whatever is wrong with Debian's packages unless it is a bug caused by our build configurations. The change you're proposing in this PR could just as easily be impacted in some distro's packages.

The exported target is new and hasn't been in a release yet. It should be in LLVM-19 once it's released and Debian gets updated. Of course, this is also true of any other new thing that gets added now anyway.

I am a little confused about what your goal is with the path manipulations though. The part of IWYU that is linked above is looking for the clang-resource-headers target if one exists, or trying to find the include directory. I'm not familiar with how IWYU works either though. It kind of looks like it's embedding a copy of clang (sort of like how Swift does), in which case, it should embed the resource headers that match that clang and you should know where you're installing them. If that's not the case and it's invoking clang somewhere, then passing -print-resource-dir to that clang will give you the resource directory, and then the headers are under the include directory there. But in both cases, from what I can tell, you're asking about where the headers are, which is what the clang-resource-headers target contains. What other bits are you looking for in the resource directory?

@kimgr
Copy link
Contributor Author

kimgr commented Aug 2, 2024

The exported target is new and hasn't been in a release yet. It should be in LLVM-19 once it's released and Debian gets updated. Of course, this is also true of any other new thing that gets added now anyway.

👍

I am a little confused about what your goal is with the path manipulations though

Aren't we all 🙂? I think what I'm trying to achieve is a portable way to refer to or install the built-in headers to avoid spurious "<stddef.h> not found" errors. My current take is we can defer to clang packaging to put the headers on disk, and make them available to IWYU with a package dependency + some kind of header search path. I don't think we need anything else from the resource dir.

@kimgr
Copy link
Contributor Author

kimgr commented Aug 13, 2024

@tstellar

I think that the GetResourcesPath function is incorrect. There's no reason it should return a wrong value, since the CLANG_RESOURCE_DIR value is always known at compile time.

Please see #103388 for an attempt to make this internally consistent.

@kimgr
Copy link
Contributor Author

kimgr commented Aug 27, 2024

#103388 was merged, I think I know how to work this now. Thanks everyone for helping!

@kimgr kimgr closed this Aug 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants