Skip to content

mcap_vendor: only install public headers#1207

Merged
james-rms merged 1 commit intoros2:rollingfrom
james-rms:jrms/mcap-do-not-install-inl
Dec 13, 2022
Merged

mcap_vendor: only install public headers#1207
james-rms merged 1 commit intoros2:rollingfrom
james-rms:jrms/mcap-do-not-install-inl

Conversation

@james-rms
Copy link
Copy Markdown
Contributor

@james-rms james-rms commented Dec 12, 2022 •
edited
Loading

Right now:

parallels@ubuntu-linux-20-04-desktop:~$ dpkg-deb -c ros-rolling-mcap-vendor_0.5.0-1jammy.20221103.184148_arm64.deb
drwxr-xr-x root/root         0 2022-11-04 05:41 ./
drwxr-xr-x root/root         0 2022-11-04 05:41 ./opt/
drwxr-xr-x root/root         0 2022-11-04 05:41 ./opt/ros/
drwxr-xr-x root/root         0 2022-11-04 05:41 ./opt/ros/rolling/
drwxr-xr-x root/root         0 2022-11-04 05:41 ./opt/ros/rolling/include/
drwxr-xr-x root/root         0 2022-11-04 05:41 ./opt/ros/rolling/include/mcap_vendor/
drwxr-xr-x root/root         0 2022-11-04 05:41 ./opt/ros/rolling/include/mcap_vendor/mcap/
-rw-r--r-- root/root      3901 2022-11-04 05:41 ./opt/ros/rolling/include/mcap_vendor/mcap/crc32.hpp
-rw-r--r-- root/root      2808 2022-11-04 05:41 ./opt/ros/rolling/include/mcap_vendor/mcap/errors.hpp
-rw-r--r-- root/root      6565 2022-11-04 05:41 ./opt/ros/rolling/include/mcap_vendor/mcap/internal.hpp
-rw-r--r-- root/root      9110 2022-11-04 05:41 ./opt/ros/rolling/include/mcap_vendor/mcap/intervaltree.hpp
-rw-r--r-- root/root        58 2022-11-04 05:41 ./opt/ros/rolling/include/mcap_vendor/mcap/mcap.hpp
-rw-r--r-- root/root      3185 2022-11-04 05:41 ./opt/ros/rolling/include/mcap_vendor/mcap/read_job_queue.hpp
-rw-r--r-- root/root     25409 2022-11-04 05:41 ./opt/ros/rolling/include/mcap_vendor/mcap/reader.hpp
-rw-r--r-- root/root     66478 2022-11-04 05:41 ./opt/ros/rolling/include/mcap_vendor/mcap/reader.inl
-rw-r--r-- root/root     10356 2022-11-04 05:41 ./opt/ros/rolling/include/mcap_vendor/mcap/types.hpp
-rw-r--r-- root/root      1222 2022-11-04 05:41 ./opt/ros/rolling/include/mcap_vendor/mcap/types.inl
-rw-r--r-- root/root     15089 2022-11-04 05:41 ./opt/ros/rolling/include/mcap_vendor/mcap/writer.hpp
-rw-r--r-- root/root     33594 2022-11-04 05:41 ./opt/ros/rolling/include/mcap_vendor/mcap/writer.inl
drwxr-xr-x root/root         0 2022-11-04 05:41 ./opt/ros/rolling/lib/
-rw-r--r-- root/root    368504 2022-11-04 05:41 ./opt/ros/rolling/lib/libmcap.so
drwxr-xr-x root/root         0 2022-11-04 05:41 ./opt/ros/rolling/share/
drwxr-xr-x root/root         0 2022-11-04 05:41 ./opt/ros/rolling/share/ament_index/
drwxr-xr-x root/root         0 2022-11-04 05:41 ./opt/ros/rolling/share/ament_index/resource_index/
drwxr-xr-x root/root         0 2022-11-04 05:41 ./opt/ros/rolling/share/ament_index/resource_index/package_run_dependencies/
-rw-r--r-- root/root        11 2022-11-04 05:41 ./opt/ros/rolling/share/ament_index/resource_index/package_run_dependencies/mcap_vendor
drwxr-xr-x root/root         0 2022-11-04 05:41 ./opt/ros/rolling/share/ament_index/resource_index/packages/
-rw-r--r-- root/root         0 2022-11-04 05:41 ./opt/ros/rolling/share/ament_index/resource_index/packages/mcap_vendor
drwxr-xr-x root/root         0 2022-11-04 05:41 ./opt/ros/rolling/share/ament_index/resource_index/parent_prefix_path/
-rw-r--r-- root/root         8 2022-11-04 05:41 ./opt/ros/rolling/share/ament_index/resource_index/parent_prefix_path/mcap_vendor
drwxr-xr-x root/root         0 2022-11-04 05:41 ./opt/ros/rolling/share/mcap_vendor/
drwxr-xr-x root/root         0 2022-11-04 05:41 ./opt/ros/rolling/share/mcap_vendor/cmake/
-rw-r--r-- root/root      3938 2022-11-04 05:41 ./opt/ros/rolling/share/mcap_vendor/cmake/ament_cmake_export_dependencies-extras.cmake
-rw-r--r-- root/root       785 2022-11-04 05:41 ./opt/ros/rolling/share/mcap_vendor/cmake/ament_cmake_export_include_directories-extras.cmake
-rw-r--r-- root/root       964 2022-11-04 05:41 ./opt/ros/rolling/share/mcap_vendor/cmake/ament_cmake_export_targets-extras.cmake
-rw-r--r-- root/root       818 2022-11-04 05:41 ./opt/ros/rolling/share/mcap_vendor/cmake/mcapExport-none.cmake
-rw-r--r-- root/root      3452 2022-11-04 05:41 ./opt/ros/rolling/share/mcap_vendor/cmake/mcapExport.cmake
-rw-r--r-- root/root       432 2022-11-04 05:41 ./opt/ros/rolling/share/mcap_vendor/cmake/mcap_vendorConfig-version.cmake
-rw-r--r-- root/root      1404 2022-11-04 05:41 ./opt/ros/rolling/share/mcap_vendor/cmake/mcap_vendorConfig.cmake
drwxr-xr-x root/root         0 2022-11-04 05:41 ./opt/ros/rolling/share/mcap_vendor/environment/
-rw-r--r-- root/root        41 2022-11-04 05:41 ./opt/ros/rolling/share/mcap_vendor/environment/ament_prefix_path.dsv
-rw-r--r-- root/root       160 2022-11-03 04:10 ./opt/ros/rolling/share/mcap_vendor/environment/ament_prefix_path.sh
-rw-r--r-- root/root        42 2022-11-04 05:41 ./opt/ros/rolling/share/mcap_vendor/environment/library_path.dsv
-rw-r--r-- root/root       411 2022-09-14 08:06 ./opt/ros/rolling/share/mcap_vendor/environment/library_path.sh
-rw-r--r-- root/root        41 2022-11-04 05:41 ./opt/ros/rolling/share/mcap_vendor/environment/path.dsv
-rw-r--r-- root/root       185 2022-11-03 04:10 ./opt/ros/rolling/share/mcap_vendor/environment/path.sh
-rw-r--r-- root/root      1594 2022-11-04 05:41 ./opt/ros/rolling/share/mcap_vendor/local_setup.bash
-rw-r--r-- root/root       156 2022-11-04 05:41 ./opt/ros/rolling/share/mcap_vendor/local_setup.dsv
-rw-r--r-- root/root      5101 2022-11-04 05:41 ./opt/ros/rolling/share/mcap_vendor/local_setup.sh
-rw-r--r-- root/root      1957 2022-11-04 05:41 ./opt/ros/rolling/share/mcap_vendor/local_setup.zsh
-rw-r--r-- root/root       164 2022-11-04 05:41 ./opt/ros/rolling/share/mcap_vendor/package.dsv
-rw-r--r-- root/root       578 2022-11-03 12:21 ./opt/ros/rolling/share/mcap_vendor/package.xml
drwxr-xr-x root/root         0 2022-11-04 05:41 ./usr/
drwxr-xr-x root/root         0 2022-11-04 05:41 ./usr/share/
drwxr-xr-x root/root         0 2022-11-04 05:41 ./usr/share/doc/
drwxr-xr-x root/root         0 2022-11-04 05:41 ./usr/share/doc/ros-rolling-mcap-vendor/
-rw-r--r-- root/root      1116 2022-11-04 05:41 ./usr/share/doc/ros-rolling-mcap-vendor/changelog.Debian.gz
-rw-r--r-- root/root       295 2022-11-02 00:00 ./usr/share/doc/ros-rolling-mcap-vendor/copyright

The upshot is that we're installing implementation files (.inl) into the user's ROS workspace where they could find themselves including them by accident. An example of what could go wrong is:

#include <mcap/reader.hpp> // Correct way of using MCAP headers, and this will produce no problems for the user

#define MCAP_IMPLEMENTATION // technically a user error, but a user might see this in some example code and use it
#include <mcap/reader.hpp> // this will cause their compilation to fail with "#include <lz4frame.h>: header not found".

They might try to work around this by installing liblz4-dev, which would cause their program to compile but may cause linking issues down the road, since the installed libmcap.so is compiled with lz4 statically linked in, potentially a different version as well.

Testing

  • verified with bloom-generate rosdebian that only .hpp files are installed to the debian
  • build and test verifies that the required headers are still included
  • managed to run abi-compliance-checker locally, checking complicance of the new lib by itself:
parallels@ubuntu-linux-20-04-desktop:~/code/abi-compliance-checker$ cat v1.xml
<version>
    0.6.0
</version>

<headers>
../ros2_ws/rosbag2/install/mcap_vendor/include/mcap_vendor/
../ros2_ws/rosbag2/install/mcap_vendor/include/mcap_vendor/
</headers>

<libs>
../ros2_ws/rosbag2/install/mcap_vendor/lib/
</libs>
parallels@ubuntu-linux-20-04-desktop:~/code/abi-compliance-checker$ abi-compliance-checker -lib mcap_vendor -old v1.xml -new v1.xml
Preparing, please wait ...
Using GCC 11 (aarch64-linux-gnu, target: aarch64)
WARNING: May not work properly with GCC 4.8.[0-2], 6.* and higher due to bug #78040 in GCC. Please try other GCC versions with the help of --gcc-path=PATH option or create ABI dumps by ABI Dumper tool instead to avoid using GCC. Test selected GCC version first by -test and -gcc-path options.
Checking header(s) 0.6.0 ...
Checking header(s) 0.6.0 ...
Comparing ABIs ...
Comparing APIs ...
Creating compatibility report ...
Binary compatibility: 100%
Source compatibility: 100%
Total binary compatibility problems: 0, warnings: 0
Total source compatibility problems: 0, warnings: 0
Report: compat_reports/mcap_vendor/0.6.0_to_0.6.0/compat_report.html

@james-rms james-rms requested a review from a team as a code owner December 12, 2022 03:38
@james-rms james-rms requested review from emersonknapp and hidmic and removed request for a team December 12, 2022 03:38
@james-rms james-rms force-pushed the jrms/mcap-do-not-install-inl branch from a3e07a3 to 3d20e25 Compare December 12, 2022 03:39
@james-rms
mcap_vendor: only install public headers
31eea07 
Signed-off-by: James Smith <james@foxglove.dev>
@james-rms james-rms force-pushed the jrms/mcap-do-not-install-inl branch from 3d20e25 to 31eea07 Compare December 12, 2022 03:44
@james-rms james-rms requested a review from jhurliman December 12, 2022 04:25
@james-rms
Copy link
Copy Markdown
Contributor Author

james-rms commented Dec 13, 2022

Gist: https://gist.githubusercontent.com/james-rms/7429ad7cf59cee8627d00c71cb515b53/raw/527f75d0ec19b0a45c5b2935773a9dc818bd47b3/ros2.repos
BUILD args: --packages-above-and-dependencies mcap_vendor rosbag2_storage_mcap
TEST args: --packages-above mcap_vendor rosbag2_storage_mcap
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/11267

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@james-rms james-rms mentioned this pull request Dec 13, 2022
Merged
@james-rms james-rms merged commit 98ec717 into ros2:rolling Dec 13, 2022
@james-rms
Copy link
Copy Markdown
Contributor Author

james-rms commented Dec 14, 2022

@mergify backport humble

@mergify
Copy link
Copy Markdown

mergify bot commented Dec 14, 2022

backport humble

✅ Backports have been created

Details

mergify bot pushed a commit that referenced this pull request Dec 14, 2022
@james-rms @mergify
mcap_vendor: only install public headers (#1207)
708b0d2 
Signed-off-by: James Smith <james@foxglove.dev>
(cherry picked from commit 98ec717)
@mergify mergify bot mentioned this pull request Dec 14, 2022
Merged
@james-rms james-rms deleted the jrms/mcap-do-not-install-inl branch December 14, 2022 22:54
emersonknapp pushed a commit that referenced this pull request Dec 28, 2022
@mergify
mcap_vendor: only install public headers (backport #1207) (#1214)
edf9dbf 
Signed-off-by: James Smith <james@foxglove.dev>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@emersonknapp emersonknapp emersonknapp approved these changes

@hidmic hidmic Awaiting requested review from hidmic hidmic was automatically assigned from ros2/tooling-reviewers

@jhurliman jhurliman Awaiting requested review from jhurliman

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@james-rms @emersonknapp