Fix CMake compile flags definition.#398
Open
jdumas wants to merge 1 commit into
Open
Conversation
Author
|
@svenwoop can we get any feedback on this PR please? |
There was a problem hiding this comment.
Pull request overview
This PR modernizes Embree’s CMake compile-flag propagation to fix downstream link/compile issues when Embree is built as a subproject (e.g., via add_subdirectory()) and as a static library on Windows. It introduces an INTERFACE config target intended to carry compile definitions transitively to downstream targets.
Changes:
- Introduces a new
INTERFACEtargetembree_config(aliased asembree::config) to carry Embree build configuration compile definitions. - Replaces directory-scope
add_definitions()usage with target-scoped compile definitions onembree_config. - Links internal component libraries and ISA-specific libraries against
embree::configto pick up configuration defines.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| CMakeLists.txt | Adds embree_config interface target and moves various global -D... defines to target properties. |
| kernels/CMakeLists.txt | Moves EMBREE_CONFIG define to embree_config, links kernel/ISA libs to embree::config, and installs embree_config. |
| common/tasking/CMakeLists.txt | Links tasking to embree::config (TBB path) to inherit configuration defines. |
| common/sys/CMakeLists.txt | Links sys to embree::config to inherit configuration defines. |
| common/simd/CMakeLists.txt | Links simd to embree::config to inherit configuration defines. |
| common/math/CMakeLists.txt | Links math to embree::config to inherit configuration defines. |
| common/lexers/CMakeLists.txt | Links lexers to embree::config to inherit configuration defines. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
111
to
114
| IF (EMBREE_STATIC_LIB) | ||
| SET(EMBREE_LIB_TYPE STATIC) | ||
| ADD_DEFINITIONS(-DEMBREE_STATIC_LIB) | ||
| TARGET_COMPILE_DEFINITIONS(embree_config INTERFACE -DEMBREE_STATIC_LIB) | ||
| ELSE() |
Comment on lines
182
to
187
| IF (EMBREE_TASKING_SYSTEM STREQUAL "TBB") | ||
| SET(TASKING_TBB ON ) | ||
| SET(TASKING_INTERNAL OFF) | ||
| SET(TASKING_PPL OFF ) | ||
| ADD_DEFINITIONS(-DTASKING_TBB) | ||
| TARGET_COMPILE_DEFINITIONS(embree_config INTERFACE -DTASKING_TBB) | ||
| LIST(APPEND ISPC_DEFINITIONS -DTASKING_TBB) |
Comment on lines
188
to
193
| ELSEIF (EMBREE_TASKING_SYSTEM STREQUAL "PPL") | ||
| SET(TASKING_PPL ON ) | ||
| SET(TASKING_TBB OFF ) | ||
| SET(TASKING_INTERNAL OFF) | ||
| ADD_DEFINITIONS(-DTASKING_PPL) | ||
| TARGET_COMPILE_DEFINITIONS(embree_config INTERFACE -DTASKING_PPL) | ||
| LIST(APPEND ISPC_DEFINITIONS -DTASKING_PPL) |
Comment on lines
194
to
199
| ELSE() | ||
| SET(TASKING_INTERNAL ON ) | ||
| SET(TASKING_TBB OFF) | ||
| SET(TASKING_PPL OFF ) | ||
| ADD_DEFINITIONS(-DTASKING_INTERNAL) | ||
| TARGET_COMPILE_DEFINITIONS(embree_config INTERFACE -DTASKING_INTERNAL) | ||
| LIST(APPEND ISPC_DEFINITIONS -DTASKING_INTERNAL) |
Comment on lines
485
to
487
| IF (EMBREE_ISA_SSE2) | ||
| ADD_DEFINITIONS(-DEMBREE_TARGET_SSE2) | ||
| TARGET_COMPILE_DEFINITIONS(embree_config INTERFACE -DEMBREE_TARGET_SSE2) | ||
| IF (NOT EMBREE_ARM) |
Comment on lines
519
to
521
| IF (EMBREE_ISA_AVX2) | ||
| ADD_DEFINITIONS(-DEMBREE_TARGET_AVX2) | ||
| TARGET_COMPILE_DEFINITIONS(embree_config INTERFACE -DEMBREE_TARGET_AVX2) | ||
| IF (NOT EMBREE_ARM) |
Comment on lines
531
to
533
| IF (EMBREE_ISA_AVX512) | ||
| ADD_DEFINITIONS(-DEMBREE_TARGET_AVX512) | ||
| TARGET_COMPILE_DEFINITIONS(embree_config INTERFACE -DEMBREE_TARGET_AVX512) | ||
| IF (NOT EMBREE_ARM) |
Comment on lines
4
to
6
| IF (EMBREE_CONFIG) | ||
| ADD_DEFINITIONS(-DEMBREE_CONFIG="${EMBREE_CONFIG}") | ||
| TARGET_COMPILE_DEFINITIONS(embree_config INTERFACE -DEMBREE_CONFIG="${EMBREE_CONFIG}") | ||
| ENDIF() |
Comment on lines
+17
to
21
| TARGET_LINK_LIBRARIES(tasking PUBLIC embree::config) | ||
|
|
||
| if (TARGET TBB::${EMBREE_TBB_COMPONENT}) | ||
| message("-- TBB: reuse existing TBB::${TBB_COMPONENT} target") | ||
| TARGET_LINK_LIBRARIES(tasking PUBLIC TBB::${EMBREE_TBB_COMPONENT}) |
Comment on lines
21
to
24
| SET_PROPERTY(TARGET sys APPEND PROPERTY COMPILE_FLAGS " ${FLAGS_LOWEST}") | ||
|
|
||
| TARGET_LINK_LIBRARIES(sys ${CMAKE_THREAD_LIBS_INIT} ${CMAKE_DL_LIBS}) | ||
| TARGET_LINK_LIBRARIES(sys embree::config ${CMAKE_THREAD_LIBS_INIT} ${CMAKE_DL_LIBS}) | ||
|
|
Author
|
@stefanatwork I'm happy to take another look at this 4-years old PR, but I'd like some human confirmation that there is interest in getting it merged before I spend more time on this? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When building embree as a subproject (via
add_subdirectory()) and as a static library on Windows, I do get a bunch of errors of the sort:This is because the compile flag
EMBREE_STATIC_LIBis only defined for embree files (such asrtcore.cpp), but is NOT propagated transitively to downstream targets. The reason is that Embree's CMake uses the deprecated constructadd_definitions(), which adds flags to a folder property. The modern CMake way to do this is to work with target properties instead, attaching compile flags to targets and properly express which ones should be transitively propagated (PUBLICorINTERFACEproperties).This PR remedies this shortcoming by attaching all compilation flags to a new
INTERFACEtargetembree_config, which will properly propagate the information to downstream targets.