ros-tooling / rosbag2_storage_mcap

rosbag2 storage implementation for MCAP file format
https://mcap.dev/
Apache License 2.0
32 stars 5 forks source link

rename test_fixture_interfaces package to testdata #64

Closed james-rms closed 1 year ago

james-rms commented 1 year ago

In attempt to fix windows CI. ROS2 packages appear to live in a global namespace (i think?) so this is by best attempt under that assumption - if there's a way to make a local-only package I can try that as well.

Fixes #60

Signed-off-by: James Smith james@foxglove.dev

emersonknapp commented 1 year ago

This is fine with me. I did rosbag2_storage_mcap_test_msgs in mine first attempt and I think that was fine, but it revealed some linkage issue

james-rms commented 1 year ago

How do I test this?

emersonknapp commented 1 year ago

Gist: https://gist.githubusercontent.com/emersonknapp/71cc6771a12ae04a44854e6ee89a6c7c/raw/c6766239aa52506ed71be2ace4abc718e338dfc1/ros2.repos BUILD args: --packages-above-and-dependencies rosbag2_storage_mcap TEST args: --packages-above rosbag2_storage_mcap ROS Distro: rolling Job: ci_launcher ci_launcher ran: https://ci.ros2.org/job/ci_launcher/11013

emersonknapp commented 1 year ago

To bikeshed on this just a little, maybe rosbag2_storage_mcap_test would be sufficient, testdata sounds a little weird to me

emersonknapp commented 1 year ago

When I was attempting to fix #54 build, I tried making a package-in-a-package that was just declared within the if(TESTING) block so as to not have to release a separate test package, but didn't have much luck in short order and was getting into CMake darkmagic territory messing with ament internals, possibly not worth the effort, there isn't any good example of that. There are some tests out there that codegen .msg files within the testing block only, but that didn't gel with certain expectations in the tests here. I think I have a branch somewhere with that attempt. But, writing tests around a nonstandard structure might not give us the assurance that it works properly with normal packages in the wild, so probably the current way is still the best way.

clalancette commented 1 year ago

To bikeshed on this just a little, maybe rosbag2_storage_mcap_test would be sufficient, testdata sounds a little weird to me

To bikeshed a little more; I was thinking of rosbag2_storage_mcap_test_interfaces. That seems to capture the spirit while still being 8 characters shorter. But whatever works for all of you :).

When I was attempting to fix #54 build, I tried making a package-in-a-package that was just declared within the if(TESTING) block so as to not have to release a separate test package

FYI, it is possible to not release this one package onto the buildfarm (and I'll suggest you don't release it if possible). In particular, with bloom it is possible to exclude certain packages from being released; see https://github.com/ros2-gbp/launch-release/blob/master/rolling.ignored for instance. Thus, I think the current approach is fine and you can just ignore the test package when doing the release.

james-rms commented 1 year ago

It's not obvious to me how to fix this issue - rosbag2_storage_mcap is looking for a .lib for mcap_vendor, but none exists: https://ci.ros2.org/job/ci_windows/18082/console#console-section-15

It looks like mcap_vendor does build but produces a DLL: https://ci.ros2.org/job/ci_windows/18082/consoleFull#console-section-202

@clalancette is there something we're missing when declaring the exported library in mcap_vendor? Is there an option to ament_export_targets that declares a shared library vs. a static lib? https://github.com/ros-tooling/rosbag2_storage_mcap/blob/fcd25645eade5e0bec9beae87b48019e16429830/mcap_vendor/CMakeLists.txt#L68

MichaelOrlov commented 1 year ago

@james-rms It could be missing ament_export_libraries(${PROJECT_NAME}) which I am trying to add in #62

MichaelOrlov commented 1 year ago

re-run Windows build: