Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revamp blt_convert_to_system_includes #671

Merged
merged 23 commits into from
Sep 4, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
da2a7c9
Revamp blt_convert_to_system_includes
adayton1 Jan 10, 2024
8071668
Replace INTRANSITIVE with CHILDREN
adayton1 Jan 10, 2024
5f50f73
Use blt_find_target_dependencies
adayton1 Jan 19, 2024
6c1e1e2
Merge branch 'develop' into feature/dayton8/system_includes
adayton1 Jan 19, 2024
deff1b5
Move blt_find_target_dependencies to installable macros file
adayton1 Jan 22, 2024
cd7afa8
Fix warning and error
adayton1 Jan 22, 2024
4214e3b
Add more checking
adayton1 Jan 22, 2024
e94a368
Pass empty string
adayton1 Jan 22, 2024
5586190
Handle BLT registered targets
adayton1 Jan 22, 2024
7c3a897
Merge branch 'develop' into feature/dayton8/system_includes
adayton1 Jan 22, 2024
ff41fba
Document blt_find_target_dependencies
adayton1 Jan 23, 2024
4e85473
Update cmake/BLTInstallableMacros.cmake
adayton1 Jan 23, 2024
483ccbf
Merge branch 'develop' into feature/dayton8/system_includes
adayton1 Jan 23, 2024
6c8c13e
Merge branch 'develop' into feature/dayton8/system_includes
adayton1 Jan 23, 2024
b944ad2
Add entry to release notes
adayton1 Jan 24, 2024
f6e95ee
Update cmake/BLTInstallableMacros.cmake
adayton1 Jan 24, 2024
2e34ac2
Merge branch 'develop' into feature/dayton8/system_includes
adayton1 Jan 30, 2024
2944bbb
Make default CHILDREN argument false
adayton1 Jan 30, 2024
39f17c5
Merge branch 'develop' into feature/dayton8/system_includes
white238 May 9, 2024
4f374c7
Merge branch 'develop' into feature/dayton8/system_includes
white238 Jun 26, 2024
2923a40
Merge branch 'develop' into feature/dayton8/system_includes
white238 Sep 4, 2024
cf9628b
move release note, remove deprecated warning, add extra error check, …
white238 Sep 4, 2024
b647cd8
add notes about future deprecation
white238 Sep 4, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
*.code-workspace
*.swp
3 changes: 3 additions & 0 deletions RELEASE-NOTES.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,9 @@ The project release numbers follow [Semantic Versioning](http://semver.org/spec/

## [Unreleased] - Release date yyyy-mm-dd

### Changed
- Modified `blt_convert_to_system_includes` to handle multiple targets and recursively update includes for dependencies

### Fixed
- Removed GoogleTest, GoogleMock, and GoogleBenchmarks calling CMake's `GNUInstallDirs`
which was causing non-deterministic install variables depending on your combination of
Expand Down
165 changes: 147 additions & 18 deletions cmake/BLTInstallableMacros.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -428,37 +428,166 @@ endmacro(blt_add_target_compile_flags)


##------------------------------------------------------------------------------
## blt_convert_to_system_includes(TARGET <target>)
## blt_find_target_dependencies(TARGET <target> TLIST <list name>)
##
## Converts existing interface includes to system interface includes.
## Store all target's dependencies (link libraries and interface link libraries)
## recursively in the variable name TLIST holds.
##------------------------------------------------------------------------------
Comment on lines 430 to 435
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did anything in blt_find_target_dependencies change? Or was it just moved to a new file?

macro(blt_convert_to_system_includes)
macro(blt_find_target_dependencies)
set(options)
set(singleValuedArgs TARGET)
set(singleValuedArgs TARGET TLIST)
set(multiValuedArgs)

## parse the arguments to the macro
# parse the arguments to the macro
cmake_parse_arguments(arg
"${options}" "${singleValuedArgs}" "${multiValuedArgs}" ${ARGN})

# check for required arguments
if(NOT DEFINED arg_TARGET)
message(FATAL_ERROR "TARGET is a required parameter for the blt_convert_to_system_includes macro.")
message(FATAL_ERROR "TARGET is a required parameter for the blt_find_target_dependencies macro")
endif()

# PGI does not support -isystem
if(NOT "${CMAKE_CXX_COMPILER_ID}" STREQUAL "PGI")
get_target_property(_include_dirs ${arg_TARGET} INTERFACE_INCLUDE_DIRECTORIES)
# Don't copy if the target had no include directories
if(_include_dirs)
# Clear previous value in INTERFACE_INCLUDE_DIRECTORIES so it is not doubled
# by target_include_directories
set_property(TARGET ${arg_TARGET} PROPERTY INTERFACE_INCLUDE_DIRECTORIES)
target_include_directories(${arg_TARGET} SYSTEM INTERFACE ${_include_dirs})
if(NOT DEFINED arg_TLIST OR NOT DEFINED ${arg_TLIST})
message(FATAL_ERROR "TLIST is a required parameter for the blt_find_target_dependencies macro")
endif()

set(_depends_on "")

# Get dependencies if BLT registered library
string(TOUPPER ${arg_TARGET} _target_upper)
if(_BLT_${_target_upper}_IS_REGISTERED_LIBRARY)
list(APPEND _depends_on "${_BLT_${_target_upper}_DEPENDS_ON}")
endif()
unset(_target_upper)

# Get dependencies if CMake target
if(TARGET ${arg_TARGET})
get_property(_target_type TARGET ${arg_TARGET} PROPERTY TYPE)
if(NOT "${_target_type}" STREQUAL "INTERFACE_LIBRARY")
get_target_property(_propval ${arg_TARGET} LINK_LIBRARIES)
if(_propval)
list(APPEND _depends_on ${_propval})
endif()
endif()

# get interface link libraries
get_target_property(_propval ${arg_TARGET} INTERFACE_LINK_LIBRARIES)
if (_propval)
list(APPEND _depends_on ${_propval})
endif()
endif()

blt_list_remove_duplicates(TO _depends_on)
foreach(t ${_depends_on})
if (NOT "${t}" IN_LIST ${arg_TLIST})
list(APPEND ${arg_TLIST} ${t})
blt_find_target_dependencies(TARGET ${t} TLIST ${arg_TLIST})
endif()
endforeach()

unset(_depends_on)
endmacro(blt_find_target_dependencies)


##------------------------------------------------------------------------------
## blt_convert_to_system_includes(TARGET <target>
## TARGETS [<target>...]
## CHILDREN [TRUE|FALSE]
## [QUIET])
##
## Converts existing interface includes to system interface includes.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that there was a discussion about not marking TARGET as deprecated for now.

Can we still include a comment that TARGET will be deprecated in the future, so users should prefer the TARGETS keyword? As is (and without a comment), the duplication of TARGET and TARGETS is confusing.

##
## Note: The argument ``TARGET`` will be deprecated in the near future. Until then,
## if both ``TARGET`` and ``TARGETS`` is given, all given target include
## directories will be converted.
##
##------------------------------------------------------------------------------
function(blt_convert_to_system_includes)
set(options QUIET)
set(singleValuedArgs CHILDREN TARGET)
set(multiValuedArgs TARGETS)

## parse the arguments to the macro
cmake_parse_arguments(arg
"${options}" "${singleValuedArgs}" "${multiValuedArgs}" ${ARGN})

if(NOT DEFINED arg_TARGET AND NOT DEFINED arg_TARGETS)
message(FATAL_ERROR "Neither required TARGET or TARGETS parameters for the blt_convert_to_system_includes macro were provided.")
endif()

if(DEFINED arg_TARGET)
list(APPEND arg_TARGETS ${arg_TARGET})
endif()

if(NOT DEFINED arg_TARGETS)
message(FATAL_ERROR "TARGETS is a required parameter for the blt_convert_to_system_includes macro.")
endif()

if(NOT DEFINED arg_CHILDREN)
set(arg_CHILDREN FALSE)
endif()

unset(_include_dirs)
endmacro()
# PGI does not support -isystem
if(NOT "${CMAKE_CXX_COMPILER_ID}" STREQUAL "PGI")
set(_target_list "")

foreach(_target ${arg_TARGETS})
adayton1 marked this conversation as resolved.
Show resolved Hide resolved
string(TOUPPER ${_target} _target_upper)

if(TARGET ${_target} OR _BLT_${_target_upper}_IS_REGISTERED_LIBRARY)
if(${arg_CHILDREN})
blt_find_target_dependencies(TARGET ${_target} TLIST _target_list)
adayton1 marked this conversation as resolved.
Show resolved Hide resolved
endif()

list(APPEND _target_list ${_target})
elseif(NOT ${arg_QUIET)
message(WARNING "${_target} does not exist!")
endif()

unset(_target_upper)
endforeach()

blt_list_remove_duplicates(TO _target_list)

foreach(_target ${_target_list})
if(TARGET ${_target})
adayton1 marked this conversation as resolved.
Show resolved Hide resolved
# Handle CMake target
get_target_property(_interface_include_dirs
${_target}
INTERFACE_INCLUDE_DIRECTORIES)

# Don't update properties if the target had no interface include directories
if(_interface_include_dirs)
# Clear previous value in INTERFACE_INCLUDE_DIRECTORIES
# so it is not doubled by target_include_directories
set_target_properties(${_target}
PROPERTIES
INTERFACE_INCLUDE_DIRECTORIES "")

target_include_directories(${_target}
SYSTEM
INTERFACE
${_interface_include_dirs})
endif()

unset(_interface_include_dirs)
else()
string(TOUPPER ${_target} _target_upper)

if(_BLT_${_target_upper}_IS_REGISTERED_LIBRARY)
# Handle BLT registered target
set(_BLT_${_target_upper}_TREAT_INCLUDES_AS_SYSTEM TRUE)
endif()

unset(_target_upper)
endif()
endforeach()

unset(_target_list)
elseif(NOT ${arg_QUIET})
message(WARNING "PGI compiler does not support system includes")
endif()
endfunction(blt_convert_to_system_includes)


##------------------------------------------------------------------------------
Expand Down Expand Up @@ -541,7 +670,7 @@ macro(blt_patch_target)
endif()

if(${arg_TREAT_INCLUDES_AS_SYSTEM})
blt_convert_to_system_includes(TARGET ${arg_NAME})
blt_convert_to_system_includes(TARGETS ${arg_NAME})
endif()

# FIXME: Is this all that's needed?
Expand Down
62 changes: 0 additions & 62 deletions cmake/BLTPrivateMacros.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -678,65 +678,3 @@ macro(blt_print_target_properties_private)
unset(_is_blt_registered_target)
unset(_is_cmake_target)
endmacro(blt_print_target_properties_private)

##------------------------------------------------------------------------------
## blt_find_target_dependencies(TARGET <target> TLIST <list name>)
##
## Store all target's dependencies (link libraries and interface link libraries)
## recursively in the variable name TLIST holds.
##------------------------------------------------------------------------------
macro(blt_find_target_dependencies)

set(options)
set(singleValuedArgs TARGET TLIST)
set(multiValuedArgs)

# parse the arguments to the macro
cmake_parse_arguments(arg
"${options}" "${singleValuedArgs}" "${multiValuedArgs}" ${ARGN})

# check for required arguments
if(NOT DEFINED arg_TARGET)
message(FATAL_ERROR "TARGET is a required parameter for the blt_find_target_dependencies macro")
endif()

if(NOT DEFINED arg_TLIST OR NOT DEFINED ${arg_TLIST})
message(FATAL_ERROR "TLIST is a required parameter for the blt_find_target_dependencies macro")
endif()

set(_depends_on "")

# Get dependencies if BLT registered library
string(TOUPPER ${arg_TARGET} _target_upper)
if(_BLT_${_target_upper}_IS_REGISTERED_LIBRARY)
list(APPEND _depends_on "${_BLT_${_target_upper}_DEPENDS_ON}")
endif()

# Get dependencies if CMake target
if(TARGET ${arg_TARGET})
get_property(_target_type TARGET ${arg_TARGET} PROPERTY TYPE)
if(NOT "${_target_type}" STREQUAL "INTERFACE_LIBRARY")
get_target_property(_propval ${arg_TARGET} LINK_LIBRARIES)
if(_propval)
list(APPEND _depends_on ${_propval})
endif()
endif()

# get interface link libraries
get_target_property(_propval ${arg_TARGET} INTERFACE_LINK_LIBRARIES)
if (_propval)
list(APPEND _depends_on ${_propval})
endif()
endif()

blt_list_remove_duplicates(TO _depends_on)
foreach(t ${_depends_on})
if (NOT "${t}" IN_LIST ${arg_TLIST})
list(APPEND ${arg_TLIST} ${t})
blt_find_target_dependencies(TARGET ${t} TLIST ${arg_TLIST})
endif()
endforeach()

unset(_depends_on)

endmacro(blt_find_target_dependencies)
29 changes: 26 additions & 3 deletions docs/api/utility.rst
Original file line number Diff line number Diff line change
Expand Up @@ -215,21 +215,44 @@ command but doesn't throw an error if the list is empty or not defined.
set(mylist A B A)
blt_list_remove_duplicates( TO mylist )

.. _blt_find_target_dependencies:

blt_find_target_dependencies
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

.. code-block:: cmake

set(_target_list "")
blt_find_target_dependencies(TARGET <target> TLIST _target_list)

Recursively adds all link libraries and interface link libraries of the given
target to the given list. Handles CMake targets and BLT registered libraries.

.. _blt_convert_to_system_includes:

blt_convert_to_system_includes
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

.. code-block:: cmake

blt_convert_to_system_includes(TARGET <target>)
blt_convert_to_system_includes(TARGET <target>
TARGETS [<target>...]
CHILDREN [TRUE | FALSE (default)]
[QUIET])

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docs should also have a comment/note that TARGET will be deprecated in the near future, so it would be better to use TARGETS

Converts existing interface includes to system interface includes.
Warns if a target does not exist unless ``QUIET`` is specified.
Recurses through link libraries and interface link libraries if
``CHILDREN TRUE`` is specified.

.. note::
The argument ``TARGET`` will be deprecated in the near future. Until then,
if both ``TARGET`` and ``TARGETS`` is given, all given target include
directories will be converted.

.. code-block:: cmake
:caption: **Example**
:linenos:

## convert to system includes for the foo target
blt_convert_to_system_includes(TARGET foo)
blt_convert_to_system_includes(TARGETS foo)