Library Targets Include Directories Broken

[dbs4261]

Hi folks, when using the draco library as a subproject, or with CMake's FetchContent/ExternalProject machinery, the draco includes are broken. This stems from the definition of the draco_add_library macro. Specifically, on line 226, the library target has the INSTALL_INTERFACE set at the include directory of the install folder. This works as intended. However, the BUILD_INTERFACE is not set at any point. Instead on lines 319 and 323 target_include_directories is called again to provide private and public directories, those these are not restricted to build time.

Now, my CMake experience does not include using macro wrappers around the typical target oriented machinery. Instead I am used to the modern CMake style of generator expressions. I worry that producing a large pull request reworking the CMake would not be accepted as it would be difficult to review.

So here is how I would address the issue: All of the library targets use the same build and install include directories. I would replace the referenced lines by the singular block:

target_include_directories(${lib_NAME} PUBLIC
    $<BUILD_INTERFACE:${CMAKE_CURRENT_LIST_DIR}/src/>
    $<INSTALL_INTERFACE:${CMAKE_INSTALL_INCLUDEDIR}/>
)

Now this doesn't address the generated draco/draco_features.h header, at least at build time. This CMake generated header defines a set of macros describing which features are supported. Instead this should be handled using the target_compile_definitions machinery. By setting these values publicly on the targets, both consumers of the installed library and the build time library will have those macros defined. The header can still be generated for backwards compatibility purposes as well.

[dbs4261]

Actually the suggested resolution runs into problems when dealing with the third party includes. Now those should be addressed by linking to the interface library target created by each library's cmake. However, that would require a more significant update. Instead this code can be used to parse the existing library dir variables and remove incorrect values.

list(APPEND lib_COMBINED_INCLUDES "${lib_INCLUDES}" "${lib_PUBLIC_INCLUDES}")
  list(REMOVE_DUPLICATES lib_COMBINED_INCLUDES)
  list(REMOVE_ITEM lib_COMBINED_INCLUDES "${CMAKE_CURRENT_LIST_DIR}")
  list(REMOVE_ITEM lib_COMBINED_INCLUDES "")
  message(DEBUG "Relative include dirs: ${lib_COMBINED_INCLUDES}")

  target_include_directories(${lib_NAME} PUBLIC
    "$<INSTALL_INTERFACE:${CMAKE_INSTALL_INCLUDEDIR}>"
    "$<BUILD_INTERFACE:${lib_COMBINED_INCLUDES}>"
  )

Note the quotations around the BUILD_INTERFACE generator expression. Without the quotations, the current list directory gets included because of how the expression expands to the list.