Revamp blt_convert_to_system_includes #671
Conversation
adayton1
commented
Jan 10, 2024
•
adayton1
commented
- Allow passing multiple targets (TARGETS [...])
- Follow transitive dependencies (unless CHILDREN is set to FALSE)
- Warn if target does not exist or if using the PGI compiler (unless QUIET is specified)
- Warn about deprecated TARGET option
|
We frequently have multiple targets to handle (e.g. all the axom targets), so it's a definite upgrade to handle them all in one invocation. Perhaps I'm thinking about the transitive dependencies incorrectly, but it seems like if we change a library's interface include directories to system include directories, but don't do that for one of its dependencies, a warning from its dependency could propagate up. If I'm wrong about that, please let me know. The other case I'm thinking about is where some targets may or may not exist depending on the build configuration (e.g. if some of axom's components are turned off). It would be easier just to specify them all in the same invocation and have the function ignore them. For safety, it would warn by default if the target does not exist (only for the targets specified by the user). To turn off the warnings, you could specify QUIET. Another way to handle this would be to have both TARGETS and OPTIONAL_TARGETS as options (or REQUIRED_TARGETS and OPTIONAL_TARGETS). Or handling of possibly existing targets could be left to the user. A further complication is how to deal with transitive dependencies that may not exist (or may not be targets - is that possible?). |
Co-authored-by: Chris White <white238@llnl.gov>
|
I think the tests just timed out rather than failed. |
cmake/BLTInstallableMacros.cmake
Outdated
| endif() | ||
|
|
||
| if(NOT DEFINED arg_CHILDREN) | ||
| set(arg_CHILDREN TRUE) |
There was a problem hiding this comment.
@kennyweiss should this default to true? I'm torn...
There was a problem hiding this comment.
Is this the last blocker?
There was a problem hiding this comment.
Would you like me to change this?
There was a problem hiding this comment.
The default is now FALSE.
|
This needs a line added to RELEASE_NOTES |
Done. |
