update boost install to include natvis files #81
Conversation
Probe the interfaces sources of a library target and manually patch the target's properties such that the natvis files are installed and probably linked against for downstream targets.
|
This dumps the Otherwise libraries can easily install their files on top of each other if they have the same name. |
|
|
||
| # Installs a single target | ||
| # boost_install_target(TARGET target VERSION version [HEADER_DIRECTORY directory]) | ||
| # boost_install_target(TARGET target VERSION version [HEADER_DIRECTORY directory] [EXTRA_DIRECTORY directory]) |
There was a problem hiding this comment.
The cmake_parse_arguments call below is missing EXTRA_DIRECTORY
| if(__EXTRA_DIRECTORY AND NOT BOOST_SKIP_INSTALL_RULES AND NOT CMAKE_SKIP_INSTALL_RULES) | ||
|
|
||
| get_filename_component(__EXTRA_DIRECTORY "${__EXTRA_DIRECTORY}" ABSOLUTE) | ||
| install(DIRECTORY "${__EXTRA_DIRECTORY}/" DESTINATION "${CMAKE_INSTALL_DATADIR}") |
There was a problem hiding this comment.
I'm also concerned with dumping the "extra" directory directly into CMAKE_INSTALL_DATADIR. I'd suggest changing this current if statement to only normalize __EXTRA_DIRECTORY
if(__EXTRA_DIRECTORY)
get_filename_component(__EXTRA_DIRECTORY "${__EXTRA_DIRECTORY}" ABSOLUTE)
end()Then inside the foreach below, adding the install call right before the call to boost_install_target
if(__EXTRA_DIRECTORY AND NOT BOOST_SKIP_INSTALL_RULES AND NOT CMAKE_SKIP_INSTALL_RULES)
install(DIRECTORY "${__EXTRA_DIRECTORY}/" DESTINATION "${CMAKE_INSTALL_DATADIR}/${target}-${__VERSION}")
endif()
boost_install_target(...)|
|
||
| foreach(src IN LISTS sources) | ||
|
|
||
| set(natvis_file "${extradir}/${lib}.natvis") |
There was a problem hiding this comment.
Given that we're only looking for natvis files that match this exact name, could we emit a warning if we encounter a natvis file that doesn't match? It would be nice to mitigate against the contributor/maintainer making this mistake
Or maybe some other Boost tool should lint the library repos to maintain consistency. Then this suggestion would be worthwhile in that place
|
|
||
| if("${src}" STREQUAL "${natvis_file}" OR "${src}" STREQUAL "$<BUILD_INTERFACE:${natvis_file}>") | ||
|
|
||
| message("successfully patching natvis for: ${lib}, with: ${natvis_file}") |
There was a problem hiding this comment.
Is this print just meant for development? It doesn't match the style of the other message calls in this file, and I'm not sure it's worth keeping
| if("${src}" STREQUAL "${natvis_file}" OR "${src}" STREQUAL "$<BUILD_INTERFACE:${natvis_file}>") | ||
|
|
||
| message("successfully patching natvis for: ${lib}, with: ${natvis_file}") | ||
| set_target_properties(${lib} PROPERTIES INTERFACE_SOURCES "$<BUILD_INTERFACE:${natvis_file}>;$<INSTALL_INTERFACE:${CMAKE_INSTALL_DATADIR}/${lib}.natvis>") |
There was a problem hiding this comment.
If you go with the change to the install directory, then you should probably forward along the version argument to this function, and use ${CMAKE_INSTALL_DATADIR}/${lib}-${version}/${lib}.natvis
3 participants
Probe the interfaces sources of a library target and manually patch the target's properties such that the natvis files are installed and probably linked against for downstream targets.
Partially closes boostorg/unordered#307
For pure CMake builds, it seems like natvis files aren't loaded into the solution unless the end-target has the natvis file as a source file, which is only possible when they're carried transitively as an interface source.
This code enforces the largely existing idiom of using
extra/boost_<lib>.natvisbut we can maybe massage the regex to be more accommodating of other libraries like JSON and URL.