Skip to content
This repository has been archived by the owner on Nov 30, 2022. It is now read-only.

rename test_fixture_interfaces package to testdata #64

Merged
merged 1 commit into from
Oct 25, 2022

Conversation

james-rms
Copy link
Contributor

@james-rms james-rms commented Oct 23, 2022

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 [email protected]

@james-rms james-rms force-pushed the jrms/reduce-testdata-file-length branch from 6275e32 to a015e39 Compare October 23, 2022 23:53
@emersonknapp
Copy link
Contributor

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
Copy link
Contributor Author

How do I test this?

@emersonknapp
Copy link
Contributor

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

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

@emersonknapp
Copy link
Contributor

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

@emersonknapp
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor Author

james-rms commented Oct 24, 2022

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?

ament_export_targets(export_mcap HAS_LIBRARY_TARGET)

@MichaelOrlov
Copy link
Member

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

@james-rms james-rms merged commit 9d67fa1 into main Oct 25, 2022
@james-rms james-rms deleted the jrms/reduce-testdata-file-length branch October 25, 2022 21:47
@MichaelOrlov
Copy link
Member

re-run Windows build:

  • Windows Build Status

james-rms added a commit to james-rms/rosbag2 that referenced this pull request Nov 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Windows CI not succeeding
4 participants