[PATCH] D120788: [cmake] Add INTERFACE_INCLUDE_DIRECTORIES to LLVM and MLIR.

[Marius Brehler via Phabricator via llvm-commits]

marbre accepted this revision.
marbre added a comment.

Please excuse if my previous reply was confusing. I checked more carefully and think these changes are good to go.



================
Comment at: llvm/cmake/modules/AddLLVM.cmake:501
+      FOLDER "Object Libraries"
+    )
     if(ARG_DEPENDS)
----------------
With this, the interface include directories (usage-requirements) of the object libraries are mirrored. I would have assumed that mirroring the private include directories (build-requirements) might be sufficient, but seems I am wrong.


================
Comment at: llvm/cmake/modules/AddLLVM.cmake:821
+    $<BUILD_INTERFACE:${LLVM_INCLUDE_DIR}>
+  )
+
----------------
stellaraccident wrote:
> marbre wrote:
> > With your change to `llvm_add_library()` the generated objlib //can// propagate "public" includes, as `PUBLIC` in setting `INCLUDE_DIRECTORIES` and  `INTERFACE_INCLUDE_DIRECTORIES` (which looks good to me 👍).
> > 
> > Here, the public includes are added after the calling `llvm_add_library()`. A rapidly prototyped test script lets me assume that the order might important. Should the public includes be added before calling `llvm_add_library()` to mirror those?
> > Here, the public includes are added after the calling llvm_add_library(). A rapidly prototyped test script lets me assume that the order might important. Should the public includes be added before calling llvm_add_library() to mirror those?
> 
> Can you say more -- I'm not quite sure I'm following? Iiuc, the existing include mechanism depends on bootstrapping from the directory INCLUDE_DIRECTORIES property (which is a vestige of legacy interop). There is not such a mechanism for INTERFACE_INCLUDE_DIRECTORIES, so it does need to be after library creation. But I'm not seeing a more subtle order dependence (not saying there isn't one, just not seeing it).
I think the generator-expressions actually confused me a little. In `llvm_add_library()`, the target `${name}` is created **after** `${obj_name}`. Anyway, build-requirements and usage-requirements are propagated from `${name}` to `${obj_name}`. Since generator-expressions aren't resolved at runtime but at build time, this should work out.
Here, we have an generator-expression as well, but `target_include_directories()` of course needs an existing target. Unfortunately, I missed that the `add_library()` command for `${name}` is called inside `llvm_add_library()`. Only looking at the properties one could have moved this up, but not from the target perspective. So this is the way to do it:

Setting `target_include_directories` here should be perfectly fine. Even though includes are still set via `include_directories( ${LLVM_INCLUDE_DIR} ${LLVM_MAIN_INCLUDE_DIR})` in the `llvm/CMakeLists.txt`, setting the includes for the specific target is the preferred way and hopefully brings us closer to a state where we can drop the `include_directories` command. Thanks Stella!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120788