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

[Marius Brehler via Phabricator via llvm-commits]

marbre added inline comments.


================
Comment at: llvm/cmake/modules/AddLLVM.cmake:501
+      FOLDER "Object Libraries"
+    )
     if(ARG_DEPENDS)
----------------
stellaraccident wrote:
> marbre wrote:
> > 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.
> Ok, I think you may be right because of "The INTERFACE, PUBLIC and PRIVATE keywords are required to specify the scope of the following arguments. PRIVATE and PUBLIC items will populate the INCLUDE_DIRECTORIES property of <target>. PUBLIC and INTERFACE items will populate the INTERFACE_INCLUDE_DIRECTORIES property of <target>. The following arguments specify include directories."
> 
> So a `target_include_directories(Foo PUBLIC dir/)` will populate both the INCLUDE_DIRECTORIES and INTERFACE_INCLUDE_DIRECTORIES. By that logic, this should not be necessary.
Yes, `PRIVATE` sets `INCLUDE_DIRECTORIES`, `INTERFACE` sets `INTERFACE_INCLUDE_DIRECTORIES` and with `PUBLIC` both are set. This can be easily checked with the CMake code attached.

Anyway, do we really need to set `INTERFACE_INCLUDE_DIRECTORIES` for object libraries?


```
cmake_minimum_required(VERSION 3.13.4)

project(cmake_test LANGUAGES C)
set(TEST_INCLUDE_DIR ${PROJECT_SOURCE_DIR}/include)

file(WRITE dummy.c)
add_library(test_private "dummy.c")
add_library(test_interface "dummy.c")
add_library(test_public "dummy.c")

macro(print_includes name)
  message(STATUS "Target \"${name}\"")
  get_target_property(_INCLUDES ${name} INCLUDE_DIRECTORIES)
  message(STATUS "  INCLUDE_DIRECTORIES:")
  foreach(dir ${_INCLUDES})
    message(STATUS "    ${dir}")
  endforeach()

  get_target_property(_INCLUDES ${name} INTERFACE_INCLUDE_DIRECTORIES)
  message(STATUS "  INTERFACE_INCLUDE_DIRECTORIES:")
  foreach(dir ${_INCLUDES})
    message(STATUS "    ${dir}")
  endforeach()
endmacro()

target_include_directories(test_private PRIVATE ${TEST_INCLUDE_DIR})
print_includes(test_private)
target_include_directories(test_interface INTERFACE ${TEST_INCLUDE_DIR})
print_includes(test_interface)
target_include_directories(test_public PUBLIC ${TEST_INCLUDE_DIR})
print_includes(test_public)
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120788