[Openmp-commits] [PATCH] D122667: llvm14 patch: hwloc include directory for libompd

Wed Mar 30 10:09:25 PDT 2022

AndreyChurbanov added a comment.

In D122667#3417095 <https://reviews.llvm.org/D122667#3417095>, @Ericson2314 wrote:

> @tianshilei is right. Basically the older CMake stuff like `include_directories` only worked by side effects, and therefore is impossible to use correctly and modulary and is just generally terrible. the new `target` stuff is much better, and actually allows tracking who depends on what.
>
> `target_link_libraries` is definitely a good idea, but I not trying to force @s-sajid-ali to fix the world just to fix the bug :). If we can just get away with `target_include_directories` that is a good initial step.
>
> @andreychurbanov Can you explain
>
>> No, it is used for library internals only. And libompd wants to include libomp's internal header, so I am not sure how target_include_directories can apply here.
>
> in a bit more detail? I know CMake well, but not the LLVM openmp stuff.
>
> Is `kmp.h` that includes `hwloc.h` installed or not? If it is installed, then what @s-sajid-ali is correct even if the header is not really designed for external use. If it is not installed, then we will have to be more careful about what "target" to use.

The kmp.h is not installed, it is used only for library build. And hwloc library is used by libomp dynamically only if it is accessible at runtime, it is not "linked" with libomp.

I actually was able to use `target_include_directories`, but this needs more changes - at least it needs to specify target which is not yet set at the current place of CMakeLists.txt.  And then the problem of libompd compilation is not fixed by this, the `target_include_directories` will need to also be added to `openmp/libompd/src/CMakeLists.txt` anyway.
So the "simple" solution as I said - addition of `include_directories` into `openmp/libompd/src/CMakeLists.txt`.  The solution with `target_include_directories` needs more files restructuring.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122667/new/

https://reviews.llvm.org/D122667