[Kilted] Namespace installed headers and runtime libs - #1948
Open
bjsowa wants to merge 3 commits into
Open
Conversation
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
This is a port of #1946 to
ros-develbranch (Kilted, Lyrical)Specification
CMAKE_INSTALL_INCLUDEDIRis now set once, globally, toinclude/${PROJECT_NAME}near the top ofCMakeLists.txt, before dependencies are processed. This namespaces bothdepthai's own installed headers and the vendoredFetchContentdependencies (xtensor, xtl, libnop, XLink), which all install headers via the plainCMAKE_INSTALL_INCLUDEDIRvariable.includepath inside the$<INSTALL_INTERFACE:...>generator expression of their ownINTERFACE_INCLUDE_DIRECTORIEStarget property (instead of referencing${CMAKE_INSTALL_INCLUDEDIR}), so their exported CMake targets would otherwise still advertise the old, unnamespaced path to consumers. A new_depthai_namespace_install_interface_includedir()macro incmake/depthaiDependencies.cmakepatches this property in-place forxtl,xtensor,libnop, andXLinkPublicright after they're made available viaFetchContent.target_include_directories(${TARGET_CORE_NAME} ...)now references${CMAKE_INSTALL_INCLUDEDIR}directly instead of a hardcodedincludepath, staying in sync automatically.libdynamic_calibration.so(used whenDEPTHAI_DYNAMIC_CALIBRATION_SUPPORTis enabled) is now installed tolib/${PROJECT_NAME}/instead of directly underlib/.INSTALL_RPATHfor the core target is extended with$ORIGIN/${PROJECT_NAME}(and@loader_path/${PROJECT_NAME}on macOS) so it's still found at runtime. Windows DLL install location is left unchanged (must stay next to the loading binary).libdynamic_calibration.sounderlib/${PROJECT_NAME}/reintroduces a name collision it was meant to avoid, but at a different resolution layer (ld/ld.sosearch order) instead of the filesystem: on Linux, some distros (e.g. aros-*-depthaisystem package) may already ship an unrelated, incompatiblelibdynamic_calibration.soonLD_LIBRARY_PATH. Two additional fixes close this gap:${TARGET_CORE_NAME}is now linked with-Wl,--disable-new-dtags, forcing the classicDT_RPATHtag (searched beforeLD_LIBRARY_PATHat runtime) instead of the defaultDT_RUNPATH(searched after it), so the correct, namespaced copy is always preferred at runtime regardless of environment.${TARGET_CORE_NAME}also gets anINTERFACE-Wl,-rpath-link,<dir>link option (pointing at theFetchContentsource dir at build time, and atlib/${PROJECT_NAME}at install time via$<BUILD_INTERFACE:>/$<INSTALL_INTERFACE:>). Sincedynamic_calibration_importedis aPRIVATEdependency, downstream consumers (e.g.depthai_bridge) otherwise have no path hint for it, andld's link-time search order for transitiveNEEDEDsymbols checksLD_LIBRARY_PATHbefore the dependency's ownRPATH/RUNPATH(the reverse of the runtime order) - without this, consumers could fail to link with "undefined reference" errors against a wrong/olderlibdynamic_calibration.sofound viaLD_LIBRARY_PATH.lib/cmake/libnop,lib/cmake/XLink,share/cmake/xtensor,share/cmake/xtl, andshare/pkgconfig/{xtensor,xtl}.pc- are still installed unnamespaced by their owninstall(EXPORT)/install(FILES)rules. Only their header install locations and exportedINTERFACE_INCLUDE_DIRECTORIESare namespaced here (see Dependencies & Potential Impact below).Old install layout:
New install layout:
Dependencies & Potential Impact
#includestatement changes for consumers -#include <depthai/...>keeps working unchanged,lib/cmake/libnop,lib/cmake/XLink,share/cmake/xtensor,share/cmake/xtl, andshare/pkgconfig/{xtensor,xtl}.pcremain unnamespaced (see Specification) since we can't change vendored third-party install rules without patching sources fetched fresh each configure.dynamic_calibration.soruntime location; Windows DLL handling is unchanged.--disable-new-dtags/-rpath-linkfixes only apply onUNIX(Linux and macOS); Windows resolves DLLs differently (viaPATHand the runtime-dependency-copying macro already in place) and is unaffected.depthaipackage (e.g.depthai_bridge) automatically pick up the-rpath-linkfix throughdepthai::core'sINTERFACE_LINK_OPTIONS- no changes needed on their end.Deployment Plan
None / not applicable - this is a build/install-layout change only, no runtime service impact. A clean rebuild of
depthai(and any package consuming it, e.g.depthai_bridge,depthai_ros_driver) is recommended after pulling this change to avoid stale install artifacts from the previous unnamespaced layout.Testing & Validation
depthaiand confirmedinstall/depthai/include/only contains the namespaceddepthai/subfolder (previously also had unnamespacednop/,XLink/,xtensor/,xtensor.hpp,xtl/).depthaiTargets.cmake(andlibnop's/XLink's own independently-exported configs) correctly referenceinclude/depthaiinINTERFACE_INCLUDE_DIRECTORIES.depthai_bridgeanddepthai_ros_driver(downstream consumers viafind_package(depthai CONFIG)) from scratch against the new layout - build and link successfully with no changes needed on the consumer side.libdynamic_calibration.soinstalls tolib/depthai/andlddresolves it correctly via the extendedRPATH(no "not found" entries).AI Usage
Assisted-by: Claude Sonnet 5.0
Submitted code was reviewed by a human: YES
The author is taking the responsibility for the contribution: YES