google / google/draco

Library Targets Include Directories Broken

Open
#1,016 1 comment 0 reactions 0 assignees View on GitHub
bug cmake
Dominant language
C++
Stars
7.5k
Forks
1.1k
Avg merge
47m
Merged PRs (30d)
1

Description

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`](https://github.com/google/draco/blob/9f856abaafb4b39f1f013763ff061522e0261c6f/cmake/draco_targets.cmake) macro. Specifically, on [line 226](https://github.com/google/draco/blob/9f856abaafb4b39f1f013763ff061522e0261c6f/cmake/draco_targets.cmake#L296), 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](https://github.com/google/draco/blob/9f856abaafb4b39f1f013763ff061522e0261c6f/cmake/draco_targets.cmake#L319) and [323](https://github.com/google/draco/blob/9f856abaafb4b39f1f013763ff061522e0261c6f/cmake/draco_targets.cmake#L323) `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
$
$
)
```
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.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.