Add install rules and CMake package config for the Highway module (find_package(fdt_hw))#384
Add install rules and CMake package config for the Highway module (find_package(fdt_hw))#384pszemus wants to merge 3 commits into
Conversation
|
Thanks, the motivation and most of the install/export structure make sense to me. I agree that highway/ currently needs install rules, and using install(TARGETS ... EXPORT ...), configure_package_config_file(), an exported fdt_hw:: namespace, and find_dependency(hwy 1.3.0) is the right direction. However, I think the current validation only covers the unversioned happy path: It does not cover the generated version file. Since this PR installs fdt_hwConfigVersion.cmake, standard CMake consumers should also be able to do: At the moment that fails because BUILD_VERSION is set to "v0.0.3". CMake package versions should be numeric version strings such as "0.0.3"; the "v" prefix is fine for Git tags, but not for write_basic_package_version_file()/find_package version matching. In practice, CMake reports the installed package version as "v0.0.3" and rejects a request for "0.0.3"; asking for "v0.0.3" is not valid find_package syntax either. So I think this should be fixed before merge, e.g.: and then: On the include layout: installing the header as: and asking consumers to include: is a reasonable API, especially if it is meant to match the main library. But this PR should then update the repo documentation/examples that currently show: Otherwise the installed package has a different public include style than the existing documented usage. Alternatively, if we want to preserve the current include style for installed consumers, the target should export ${CMAKE_INSTALL_INCLUDEDIR}/facedetection as its install include directory. So I am supportive of the PR direction, but I do not think the current validation is sufficient. I would like to see at least:
The current PR passes the unversioned consumer case, but the versioned package lookup is broken and the include style needs to be made explicit in docs or preserved through the exported include directory. |
- Use numeric project version (0.0.3) instead of "v0.0.3" so find_package(fdt_hw 0.0.3 CONFIG) matches; drive the version file from PROJECT_VERSION. - Move facedetect_hw.h under include/facedetection/ and switch all includes, examples, and docs to <facedetection/facedetect_hw.h> so build-tree and installed consumers use the same public include path. - Document installing/consuming the module via find_package in COMPILE.md.
|
Thanks for the thorough review — both points were spot on. I've pushed fixes for both. 1. Package version ( You're right that the project(fdt_hw VERSION 0.0.3 LANGUAGES CXX)
...
write_basic_package_version_file(
${CMAKE_CURRENT_BINARY_DIR}/fdt_hwConfigVersion.cmake
VERSION ${PROJECT_VERSION}
COMPATIBILITY AnyNewerVersion)The standalone 2. Include style consistency I went with making the public include path uniform across build-tree and installed consumers, rather than leaving two styles. The header moved to #include <facedetection/facedetect_hw.h>That includes the internal sources, tests, benchmarks, examples, and the README/COMPILE docs — so the documented usage and the installed package now match exactly. The install include interface stays Validation (your checklist) Built → installed → configured a standalone consumer:
|
- Use numeric project version (0.0.3) instead of "v0.0.3" so find_package(fdt_hw 0.0.3 CONFIG) matches; drive the version file from PROJECT_VERSION. - Move facedetect_hw.h under include/facedetection/ and switch all includes, examples, and docs to <facedetection/facedetect_hw.h> so build-tree and installed consumers use the same public include path. - Document installing/consuming the module via find_package in COMPILE.md.
|
@Wwupup I changed my mind about the include layout from my previous push. Last time I moved the header into I've switched to the second, more conservative approach, which also lines up with how the main library already works:
This mirrors the main library exactly: internally it uses Validation is unchanged and still green:
|
Summary
The
highway/module builds thefdt_hw_kernelslibrary and a public header(
include/facedetect_hw.h), buthighway/CMakeLists.txtdefines noinstall()rules — so
cmake --installis a no-op and downstream projects have to copy the.a/.soand header by hand, and wire up the Highway (hwy) dependencymanually.
This PR makes the module installable and consumable via CMake:
What it adds
fdt_hw_kernelstarget and the public header,using
GNUInstallDirs(solibvslib64follows the platform).fdt_hwConfig.cmake+ version file) generated withconfigure_package_config_file/write_basic_package_version_file, plus anexported targets file under the
fdt_hw::namespace.find_dependency(hwy 1.3.0),because
fdt_hw_kernelslinkshwyas a PUBLIC dependency. This makes thetransitive
hwy::hwylink/include usage requirements propagate to consumersautomatically.
$<BUILD_INTERFACE:> / $<INSTALL_INTERFACE:>generator expressions, which isrequired for
install(EXPORT)to succeed (raw source-tree paths are rejected).Internal
src/headers stay build-tree only; onlyfacedetect_hw.his exported.The header is installed under
include/facedetection/, consistent with the mainlibrary, so consumers use
#include <facedetection/facedetect_hw.h>.Installed artifacts
/lib(64)/libfdt_hw_kernels.a # or .so with -DBUILD_SHARED_LIBS=ON
/include/facedetection/facedetect_hw.h
/lib(64)/cmake/fdt_hw/fdt_hwConfig.cmake
/lib(64)/cmake/fdt_hw/fdt_hwConfigVersion.cmake
/lib(64)/cmake/fdt_hw/fdt_hwTargets.cmake
/lib(64)/cmake/fdt_hw/fdt_hwTargets-.cmake
Validation
Built and installed the module, then configured a standalone consumer project:
The consumer compiled against
<facedetection/facedetect_hw.h>(include dirpropagated) and linked successfully, with
hwyresolved transitively via thegenerated config — no manual include/link wiring.