Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose the QoS object wrapper #910

Merged
merged 7 commits into from
Nov 23, 2021

Conversation

gbiggs
Copy link
Member

@gbiggs gbiggs commented Nov 11, 2021

This PR exposes the QoS wrapper in rosbag_transport for use by external users. This enables users of rosbag2 to create their own recorder objects that mimic the behaviour of the provided Recorder object with regards to QoS.

Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>
Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>
@gbiggs gbiggs requested a review from a team as a code owner November 11, 2021 04:02
@gbiggs gbiggs requested review from emersonknapp and MichaelOrlov and removed request for a team November 11, 2021 04:02
Copy link
Contributor

@MichaelOrlov MichaelOrlov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good for me with green CI.

@MichaelOrlov
Copy link
Contributor

Running CI
Gist: https://gist.githubusercontent.com/MichaelOrlov/83f9321f21d3e3f335e590b56aadbfdf/raw/f9d6343b8c9671d97c868ffc3c5689e8990fce87/ros2.repos
BUILD args: --packages-up-to rosbag2_transport rosbag2_tests
TEST args: --packages_select rosbag2_transport rosbag2_tests
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/9337/

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

@MichaelOrlov
Copy link
Contributor

Ups. I made mistake in gist.
Rerunning CI one more time
Gist: https://gist.githubusercontent.com/MichaelOrlov/83f9321f21d3e3f335e590b56aadbfdf/raw/b4a57608e47b583465cc965a2f53fdbad86e4d49/ros2.repos
BUILD args: --packages-up-to rosbag2_transport rosbag2_tests
TEST args: --packages_select rosbag2_transport rosbag2_tests
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/9338/

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

@MichaelOrlov
Copy link
Contributor

Something weird happening on CI. It looks like some problems with ccache.
CI reports some errors in test urdfdom_headers

Starting >>> urdfdom_headers
23:10:42 Traceback (most recent call last):
23:10:42   File "/home/jenkins-agent/workspace/ci_linux/venv/lib/python3.8/site-packages/colcon_core/executor/__init__.py", line 91, in __call__
23:10:42     rc = await self.task(*args, **kwargs)
23:10:42   File "/home/jenkins-agent/workspace/ci_linux/venv/lib/python3.8/site-packages/colcon_core/task/__init__.py", line 93, in __call__
23:10:42     return await task_method(*args, **kwargs)
23:10:42   File "/home/jenkins-agent/workspace/ci_linux/venv/lib/python3.8/site-packages/colcon_cmake/task/cmake/test.py", line 45, in test
23:10:42     assert os.path.exists(args.build_base), \
23:10:42 AssertionError: Has this package been built before?

This package present in Gist ros2.repos and it looks like that CI expecting to take it from ccache but it seems it's not there.

@MichaelOrlov
Copy link
Contributor

Trying to rerun CI
Gist: https://gist.githubusercontent.com/MichaelOrlov/83f9321f21d3e3f335e590b56aadbfdf/raw/b4a57608e47b583465cc965a2f53fdbad86e4d49/ros2.repos
BUILD args: --packages-up-to rosbag2_cpp rosbag2_transport rosbag2_tests rosbag2
TEST args: --packages-select rosbag2_cpp rosbag2_transport rosbag2_tests rosbag2
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/9339/

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

Copy link
Contributor

@MichaelOrlov MichaelOrlov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gbiggs I see some linker errors on Windows platform https://ci.ros2.org/job/ci_windows/15821/console#console-section-13.
Could you please take a look on it?

Copy link
Collaborator

@emersonknapp emersonknapp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good though - take tidiness nitpick if you like

Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>
@gbiggs
Copy link
Member Author

gbiggs commented Nov 15, 2021

I changed the export to be on the class rather than the individual methods. @MichaelOrlov maybe this will fix those linker errors? I don't have any experience with Windows linking more recent than 20 years ago.

@MichaelOrlov
Copy link
Contributor

MichaelOrlov commented Nov 16, 2021

@gbiggs I am also not an expert in Windows platform and don't have local install of the ROS build environment on Windows.
However I spawned rerun for Windows build on CI to see if it will pass with your latest changes.

  • Windows Build Status

@gbiggs
Copy link
Member Author

gbiggs commented Nov 17, 2021

The linker errors are still happening, but I can't figure out why. @emersonknapp do you have any ideas?

…re it was exposed

Signed-off-by: Emerson Knapp <eknapp@amazon.com>
@emersonknapp
Copy link
Collaborator

emersonknapp commented Nov 22, 2021

I think it was from some outdated CMake workarounds, from before qos.hpp was exposed for use. The new visibility macros made it so that test_qos declared as dllimport, while actually defining the functions within the same shared library, causing a mismatch. By removing that source file from the test library, I believe that gets rid of the problem.

@emersonknapp emersonknapp dismissed MichaelOrlov’s stale review November 22, 2021 22:14

all PRs require green CI - don't need to use "request changes" for this

@emersonknapp
Copy link
Collaborator

Gist: https://gist.githubusercontent.com/emersonknapp/ac55fdc23ec56ceadaeef4b401da616b/raw/a4b2a884c7a896a710454ad8a5d6a4df94fd0822/ros2.repos
BUILD args: --packages-up-to rosbag2_transport rosbag2_tests
TEST args: --packages-select rosbag2_transport rosbag2_tests
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/9369

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

Signed-off-by: Emerson Knapp <eknapp@amazon.com>
@emersonknapp
Copy link
Collaborator

At least that exposed a new issue:

Windows Build Status

Signed-off-by: Emerson Knapp <eknapp@amazon.com>
@emersonknapp
Copy link
Collaborator

Had to remove the source file from another test Build Status

Signed-off-by: Emerson Knapp <eknapp@amazon.com>
@emersonknapp
Copy link
Collaborator

I wish I had a repeatable Windows environment but haven't been able to figure that out - Docker on Windows is so hard to set up. This is a bit of a hassle: Build Status

@emersonknapp
Copy link
Collaborator

emersonknapp commented Nov 23, 2021

Gist: https://gist.githubusercontent.com/emersonknapp/b7c9052c91d5e019c0bb348d3cbb28a8/raw/a4b2a884c7a896a710454ad8a5d6a4df94fd0822/ros2.repos
BUILD args: --packages-up-to rosbag2_transport rosbag2_tests
TEST args: --packages-select rosbag2_transport rosbag2_tests
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/9375

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

@emersonknapp emersonknapp merged commit e9686a3 into ros2:master Nov 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants