cmake: Use --whole-archive to export ouster_client symbols#518
Closed
ovaag wants to merge 1 commit intoouster-lidar:masterfrom
Closed
cmake: Use --whole-archive to export ouster_client symbols#518ovaag wants to merge 1 commit intoouster-lidar:masterfrom
ovaag wants to merge 1 commit intoouster-lidar:masterfrom
Conversation
ouster_client was linked PRIVATE to ouster_ros, so only symbols directly used by ouster_ros sources were included in the shared library. Downstream packages linking against libouster_ros.so could not access other ouster_client symbols (e.g. make_xyz_lut). Use --whole-archive to bundle all ouster_client symbols into the shared library.
Samahu
reviewed
Mar 10, 2026
| target_link_libraries(ouster_ros | ||
| PUBLIC ${catkin_LIBRARIES} ouster_build OusterSDK::ouster_sensor pcl_common | ||
| PRIVATE ${OUSTER_TARGET_LINKS} | ||
| -Wl,--whole-archive ${OUSTER_TARGET_LINKS} -Wl,--no-whole-archive |
Contributor
There was a problem hiding this comment.
I think this needs to remain PRIVATE
Suggested change
| -Wl,--whole-archive ${OUSTER_TARGET_LINKS} -Wl,--no-whole-archive | |
| PRIVATE -Wl,--whole-archive ${OUSTER_TARGET_LINKS} -Wl,--no-whole-archive |
It should still be possible to link against your downstream package.
Contributor
|
Included in #521 with other fixes |
Contributor
other than keeping the PRIVATE I have ensured this code still compiles on MacOS as the |
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
2 participants
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.
671fc0a fixes the double-free by removing --whole-archive and linking ${OUSTER_TARGET_LINKS} directly to each internal target (nodelets, tests). This works within the ouster_ros package, but breaks external downstream catkin packages.
Since ouster_client remains PRIVATE to ouster_ros, only symbols referenced by os_ros.cpp end up in libouster_ros.so — everything else is stripped by the linker. Downstream packages that on ouster_ros get libouster_ros.so via catkin but have no way to access unreferenced ouster_client symbols (e.g. make_xyz_lut, sensor config APIs). The internal workaround of linking ${OUSTER_TARGET_LINKS} directly isn't available to external packages since ouster_client is a static library that isn't installed or exported.
this commit solves this by using --whole-archive as PUBLIC on ouster_ros only, so all ouster_client symbols are bundled into libouster_ros.so and visible to downstream consumers. The original double-free was caused by --whole-archive being applied to both ouster_ros and the nodelets, linking ouster_client twice into the same process. Applying it only to ouster_ros avoids this.
Related Issues & PRs
Summary of Changes
Validation
External packages using ouster_client libs now builds