ENH: Modernize CMake packaging to match vtkAddon (supersedes #27)#28
ENH: Modernize CMake packaging to match vtkAddon (supersedes #27)#28RafaelPalomar wants to merge 2 commits into
Conversation
- Removed old configuration and use CMake files in favor of`qRestAPIConfig.cmake.in` - Enable install configuration through implementation of qRestAPIInstallConfig.cmake.in - Enabled installation of development files.
…tall) Export an imported qRestAPI target for both build and install trees via install(EXPORT), and generate the install-tree package config with configure_package_config_file() so the install is relocatable (no absolute CMAKE_INSTALL_PREFIX baked in). Declare include directories as PUBLIC usage requirements with BUILD_INTERFACE/INSTALL_INTERFACE and link Qt with explicit PUBLIC visibility. Legacy path variables (qRestAPI_INCLUDE_DIRS, qRestAPI_LIBRARY_DIRS, qRestAPI_LIBRARIES) are kept for backward compatibility. Config templates remain at the project root, matching the vtkAddon convention.
|
Thanks a lot for this, looks promising
Sounds good. Could apply a similar Readme update PR on Slicer/vtkAddon#70 so that it's documented there too, for consistency? |
lassoan
left a comment
There was a problem hiding this comment.
Thank you, this looks good to me.
Would it be possible to add an option for ${PROJECT_NAME}_INSTALL_NO_DEVELOPMENT?
- It is the standard way of telling that it is a public configuration variable.
- It would allow setting a default value, so we could avoid the awkward double-negative: instead of
qRestAPI_INSTALL_NO_DEVELOPMENT=OFFwe could simply haveqRestAPI_INSTALL_DEVELOPMENT=ON. - Its documentation would show up both in the CMakeLists.txt file and in the CMake GUI (you would not need to go anywhere else for documentation and the risk of documentation and implementation getting out of sync would be lower).
Consistency is useful, so we could fix this negative naming and lack of option defintion in vtkAddon, before it spreads to too many places.
lassoan
left a comment
There was a problem hiding this comment.
I've reviewed the code with Claude and found these comments to be considered:
1. CMake/ directory references are now dead.
All three files under CMake/ are deleted and nothing new is added there, so the directory ceases to exist in a fresh clone (git does not materialize empty directories). Yet the PR keeps:
set(${PROJECT_NAME}_CMAKE_DIR ${CMAKE_CURRENT_SOURCE_DIR}/CMake)andset(CMAKE_MODULE_PATH ${qRestAPI_CMAKE_DIR} ...)— prepends a nonexistent path to the module path (harmless but misleading);set(qRestAPI_CMAKE_DIR "@qRestAPI_CMAKE_DIR@")in the build-tree config — exports a pointer to a directory that doesn't exist.
Either drop these lines, or move the two *Config.cmake.in templates under CMake/ — which would also resolve the open placement question from the weekly meeting and give the directory a reason to exist. As is, the code and the PR-description rationale ("CMake/ is reserved for build-helper modules") don't match reality, since the directory is empty.
2. find_dependency() ordering. Both config templates include(...Targets.cmake) first and call find_dependency(Qt...) after. It happens to work (link-interface targets are resolved lazily at the consumer's generate step), but the canonical and safer order is dependencies first, then the targets file. One-line swap in each template.
3. Private header gets installed. file(GLOB headers "*.h") picks up qRestAPI_p.h. Consider filtering *_p.h out (list(FILTER headers EXCLUDE REGEX "_p\.h$")). Minor, and vtkAddon has the same pattern, but it's a cheap fix while touching this code.
4. Qt4 branch inconsistency in the configs. The configs re-resolve only QtGui QtNetwork for Qt4, while the target publicly links the full ${QT_LIBRARIES} (including QtScript/QtXml) as absolute paths — so the Qt4 install tree is not actually relocatable and the find_dependency component list doesn't match the link interface. => Maybe we could remove Qt4 support
Minor
-
The Qt
Testcomponent is still required even withBUILD_TESTING=OFF=> not a regression, but would be nice to fix by adding a separate find_package(Qt${qRestAPI_QT_VERSION} COMPONENTS Test REQUIRED) inside the if(BUILD_TESTING) block (or in Testing/CMakeLists.txt), so the dependency exists only when the tests are built. -
Nitpicking: the same block mixes
${PROJECT_NAME}_...and literalqRestAPI_...spellings (e.g.${PROJECT_NAME}_CMAKE_DIRset, then read asqRestAPI_CMAKE_DIR); pick one.
In addition to all these, my comment on switching to qRestAPI_INSTALL_DEVELOPMENT still holds. Some information to this from Claude:
The main historical reason Slicer-family projects used the if(NOT DEFINED ...) pattern instead of option() is that before CMake 3.13, option() would clobber a normal variable set by a parent project before add_subdirectory(). With CMP0077 set to NEW, option() honors a pre-set normal variable and becomes strictly better. qRestAPI's cmake_minimum_required(VERSION 3.5.0) leaves that policy unset, so it would be worth pairing the change with a minimum-version bump to ≥3.13 (3.5 is deprecated in current CMake anyway and CMake 4 warns loudly about it) or an explicit cmake_policy(SET CMP0077 NEW). For Slicer's actual usage (external project passing -D cache args) it works either way, but the add_subdirectory case is what the pattern was defending against.
This supersedes #27 and builds on it: it keeps that PR's install/config rework and brings qRestAPI's CMake packaging in line with vtkAddon, the reference project for Slicer-family libraries. The goal is a library that installs cleanly and can be consumed standalone (e.g. building Slicer with
SUPERBUILD=OFF, or via distro/homebrew packaging).What changes
qRestAPItarget for both the build tree and the install tree throughinstall(EXPORT). Downstream projects link the target and inherit include directories transitively, soUseqRestAPI.cmake/qRestAPI_USE_FILEare no longer needed and are removed.configure_package_config_file(), making installed trees relocatable (no absoluteCMAKE_INSTALL_PREFIXis baked in).PUBLICusage requirements (BUILD_INTERFACE/INSTALL_INTERFACE) and link Qt with explicit visibility.qRestAPI_INCLUDE_DIRS,qRestAPI_LIBRARY_DIRSandqRestAPI_LIBRARIESvariables for backward compatibility.Consumers that relied on
qRestAPI_USE_FILEmust drop it and link theqRestAPItarget instead (a matching one-line change is needed in Slicer'sBase/QTCore).Location of the config templates
The predecessor PR was discussed at the Slicer developer weekly meeting on 2026-07-07, where @lassoan suggested moving the CMake config files under a
CMake/directory rather than leaving them at the project root.This PR keeps the
*Config.cmake.intemplates at the root. The rationale is consistency with vtkAddon, where the root holds the public package-config templates whileCMake/is reserved for build-helper modules (macros, wrap scripts). The placement is functionally neutral — the template path is given explicitly toconfigure_file()and is not affected byCMAKE_MODULE_PATH— so matching the reference project is the deciding factor. Happy to move them underCMake/if the preference stands.qRestAPI_INSTALL_NO_DEVELOPMENTkept as a plain variable@ferdymercury suggested (in #27) promoting this to a cache option. This PR keeps it as an
if(NOT DEFINED ...)default, primarily for consistency with vtkAddon and other Slicer-family libraries, which use the same pattern for their install-layout variables. The functional difference is small; the main trade-off of not using a cache option is reduced discoverability incmake-gui, which is addressed by documenting these variables in the README. This is a convention call rather than a technical constraint — happy to discuss a change on this with the devs.This PR was prepared with the help of Claude Code (Opus 4.8).