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

remove rosbag2_transport header #742

Merged
merged 2 commits into from
Apr 21, 2021
Merged

Conversation

Karsten1987
Copy link
Collaborator

as per title, finally triggering the switch.

sits on top of #741

@Karsten1987 Karsten1987 requested a review from a team as a code owner April 20, 2021 03:02
@Karsten1987 Karsten1987 requested review from emersonknapp and prajakta-gokhale and removed request for a team April 20, 2021 03:02
@emersonknapp emersonknapp changed the title remove rosbag2_transport class remove rosbag2_transport header Apr 20, 2021
@Karsten1987
Copy link
Collaborator Author

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

recorder->record();
rclcpp::executors::SingleThreadedExecutor exec;
exec.add_node(recorder);
auto spin_thread = std::thread(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't you want to start spinning before calling record? IIRC record doesn't exit until it's cancelled.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

hmm. I honestly don't know. I had the same question at #741 within the tests - this code is actually really just copied from these.
I thought it would make sense to give record the change to setup all subscriptions and potential error handling and really only spin once that's completed.

Why do you advocate for spinning first?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh - Recorder::record returns right away after setup and runs in the background, until the Recorder object is destroyed. API confusion from the previous blocking API.

Not something we have to handle here, but maybe we ought to consider a slightly more obvious API - if recording is controlled by the object lifecycle, then should recording just start in the constructor? Or, alternatively should there be a way to stop recording without destroying the object?

virtual ~Recorder();

ROSBAG2_TRANSPORT_PUBLIC
void record();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Now that we are making these public I think we need a serious docstring pass. Not blocking this, but we should make an effort to document every public header in the "cleanup sprint"

@delete-merged-branch delete-merged-branch bot deleted the branch master April 21, 2021 19:50
Signed-off-by: Karsten Knese <Karsten1987@users.noreply.github.com>
Signed-off-by: Karsten Knese <Karsten1987@users.noreply.github.com>
@Karsten1987 Karsten1987 changed the base branch from kk/use_public_recorder_api to master April 21, 2021 20:07
@Karsten1987
Copy link
Collaborator Author

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

@Karsten1987
Copy link
Collaborator Author

@Emerson, could it be that the GH actions don't observe the xfail label?
There seems to be a discrepancy between the CI buildfarm results and the actions.

As a side note, I believe there's simply a bug in the record_end_to_end test. We expect a positive exit code, yet the command is clearly invoked incorrectly. So the system actually behaves correctly, the test is wrong in my opinion.

@emersonknapp
Copy link
Collaborator

emersonknapp commented Apr 21, 2021

could it be that the GH actions don't observe the xfail label?

That's correct - it is not configured out of the box to do anything specific about xfail - that's something we could configure for this specific repo, or we could make it a default behavior. I'm more inclined to allow it to be configured on a per-repo basis - given that it's not "built-in" to the Jenkins jobs, it's just prepopulated build args --ctest-args -LE xfail --pytest-args -m "not xfail"

@Karsten1987 Karsten1987 merged commit 237f0c0 into master Apr 21, 2021
@delete-merged-branch delete-merged-branch bot deleted the kk/remove_rosbag2_transport branch April 21, 2021 22:20
@emersonknapp
Copy link
Collaborator

#748

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.

None yet

2 participants