Skip to content

Add install rules and CMake package config for the Highway module (find_package(fdt_hw))#384

Open
pszemus wants to merge 3 commits into
ShiqiYu:masterfrom
pszemus:cmake-install
Open

Add install rules and CMake package config for the Highway module (find_package(fdt_hw))#384
pszemus wants to merge 3 commits into
ShiqiYu:masterfrom
pszemus:cmake-install

Conversation

@pszemus

@pszemus pszemus commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

Summary

The highway/ module builds the fdt_hw_kernels library and a public header
(include/facedetect_hw.h), but highway/CMakeLists.txt defines no install()
rules — so cmake --install is a no-op and downstream projects have to copy the
.a/.so and header by hand, and wire up the Highway (hwy) dependency
manually.

This PR makes the module installable and consumable via CMake:

find_package(fdt_hw REQUIRED CONFIG)
target_link_libraries(my_app PRIVATE fdt_hw::fdt_hw_kernels)

What it adds

  • Install rules for the fdt_hw_kernels target and the public header,
    using GNUInstallDirs (so lib vs lib64 follows the platform).
  • A CMake package config (fdt_hwConfig.cmake + version file) generated with
    configure_package_config_file / write_basic_package_version_file, plus an
    exported targets file under the fdt_hw:: namespace.
  • The generated config re-discovers Highway via find_dependency(hwy 1.3.0),
    because fdt_hw_kernels links hwy as a PUBLIC dependency. This makes the
    transitive hwy::hwy link/include usage requirements propagate to consumers
    automatically.
  • The public include directories are now expressed with
    $<BUILD_INTERFACE:> / $<INSTALL_INTERFACE:> generator expressions, which is
    required for install(EXPORT) to succeed (raw source-tree paths are rejected).
    Internal src/ headers stay build-tree only; only facedetect_hw.h is exported.

The header is installed under include/facedetection/, consistent with the main
library, 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:

find_package(fdt_hw REQUIRED CONFIG)
target_link_libraries(consumer PRIVATE fdt_hw::fdt_hw_kernels)

The consumer compiled against <facedetection/facedetect_hw.h> (include dir
propagated) and linked successfully, with hwy resolved transitively via the
generated config — no manual include/link wiring.

@Wwupup

Wwupup commented Jun 28, 2026

Copy link
Copy Markdown
Collaborator

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:

find_package(fdt_hw REQUIRED CONFIG)

It does not cover the generated version file. Since this PR installs fdt_hwConfigVersion.cmake, standard CMake consumers should also be able to do:

find_package(fdt_hw 0.0.3 REQUIRED CONFIG)

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.:

project(fdt_hw VERSION 0.0.3 LANGUAGES CXX)

and then:

write_basic_package_version_file(
    ${CMAKE_CURRENT_BINARY_DIR}/fdt_hwConfigVersion.cmake
    VERSION ${PROJECT_VERSION}
    COMPATIBILITY AnyNewerVersion)

On the include layout: installing the header as:

include/facedetection/facedetect_hw.h

and asking consumers to include:

#include <facedetection/facedetect_hw.h>

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:

#include "facedetect_hw.h"

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:

  1. install the package
  2. configure a standalone consumer with find_package(fdt_hw 0.0.3 REQUIRED CONFIG)
  3. compile/link using the documented public include path
  4. ensure hwy is resolved transitively

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.

pszemus added a commit to pszemus/libfacedetection that referenced this pull request Jun 29, 2026
- 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.
@pszemus

pszemus commented Jun 29, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the thorough review — both points were spot on. I've pushed fixes for both.

1. Package version (v0.0.30.0.3)

You're right that the v prefix broke find_package version matching. Fixed as you suggested:

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 set(BUILD_VERSION "v0.0.3") is gone; the version file now reports set(PACKAGE_VERSION "0.0.3").

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, and everything now uses:

#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 ${CMAKE_INSTALL_INCLUDEDIR}, so consumers get <facedetection/facedetect_hw.h>. I also added a short "Installing and consuming via find_package" section to COMPILE.md.

Validation (your checklist)

Built → installed → configured a standalone consumer:

  1. cmake --installinclude/facedetection/facedetect_hw.h + config/version/targets files.
  2. find_package(fdt_hw 0.0.3 REQUIRED CONFIG) now configures. As a negative check, find_package(fdt_hw 0.0.4 ...) is correctly rejected (version: 0.0.3), confirming version matching is active.
  3. ✅ Consumer compiles against #include <facedetection/facedetect_hw.h> and links.
  4. hwy resolves transitively via find_dependency(hwy) — no manual include/link wiring.

- 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.
@pszemus

pszemus commented Jun 30, 2026

Copy link
Copy Markdown
Contributor Author

@Wwupup I changed my mind about the include layout from my previous push.

Last time I moved the header into include/facedetection/ and rewrote every include — internal sources, tests, benchmarks, examples, and docs — to <facedetection/facedetect_hw.h>. That worked, but it was a bigger diff than necessary and churned a lot of internal code that consumers never see.

I've switched to the second, more conservative approach, which also lines up with how the main library already works:

  • Public/installed API + docs stay namespaced. Installed consumers, README.md, COMPILE.md, and the example/*-highway.cpp snippets use #include <facedetection/facedetect_hw.h>. The header still installs to ${CMAKE_INSTALL_INCLUDEDIR}/facedetection, so the consumer-facing include you asked about is unchanged.
  • Internal sources keep their existing include. highway/src, tests, and benchmark go back to #include "facedetect_hw.h", and the header stays at highway/include/facedetect_hw.h instead of being relocated.

This mirrors the main library exactly: internally it uses #include "facedetectcnn.h", installs the header into include/facedetection/, and exposes <facedetection/facedetectcnn.h> to installed consumers. So the hw module is now consistent with it, and the diff is considerably smaller.

Validation is unchanged and still green:

  1. cmake --installinclude/facedetection/facedetect_hw.h + config/version/targets files
  2. find_package(fdt_hw 0.0.3 REQUIRED CONFIG) configures; 0.0.4 is correctly rejected
  3. a standalone consumer compiles against <facedetection/facedetect_hw.h> and links
  4. hwy resolves transitively via find_dependency(hwy) — no manual wiring

@pszemus

pszemus commented Jul 7, 2026

Copy link
Copy Markdown
Contributor Author

@ShiqiYu @Wwupup can you guys look at this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants