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

Always modify UserObjectQosPolicy regardless of override policy #53

Merged
merged 1 commit into from
Apr 28, 2021

Conversation

asorbini
Copy link
Collaborator

This PR updates rmw_connextdds to always modify DomainParticipantQos::user_object (as introduced by #25) regardless of the participant QoS override policy requested by the user (introduced by #41).

This is required to let the participant be shared with applications using Connext's C++11 API, even when they use policies basic or never. In these cases, applications would not be able to properly use the participant because UserObjectQosPolicy cannot be modified from XML:

RTIXMLParser_validateOnStartTag:Parse error at line 263: Unexpected tag 'user_object'

The change is trivial, since it only consists of moving a block to the beginning of rmw_connextdds_initialize_participant_qos_impl().

@asorbini asorbini added dashing-backports PR should be backported to Dashing eloquent-backports PR should be backported to Eloquent foxy-backports PR should be backported to Foxy galactic-backports PR should be backported to Galactic. humble PR scheduled for the H-turtle labels Apr 28, 2021
@asorbini asorbini requested a review from ivanpauno April 28, 2021 02:24
@asorbini
Copy link
Collaborator Author

Since this PR has no functional impact wrt/unit testing when compared to #25 and #41 (because the differences only occur when RMW_CONNEXT_PARTICIPANT_QOS_OVERRIDE_POLICY is set to a value other than all), I'd like to merge it without running CI to save some time, but if we want the "checkbox marked", then I'll start the plans.

@ivanpauno is this allowed by the process?

@ivanpauno
Copy link
Member

Since this PR has no functional impact wrt/unit testing when compared to #25 and #41 (because the differences only occur when RMW_CONNEXT_PARTICIPANT_QOS_OVERRIDE_POLICY is set to a value other than all), I'd like to merge it without running CI to save some time, but if we want the "checkbox marked", then I'll start the plans.

@ivanpauno is this allowed by the process?

I agree that there's no functional impact in this PR.
I typically run at least CI in one platform with colcon build ... --packages-up-to rmw_connextdds_common colcon test ... --packages-select rmw_connextdds_common, to check that everything still builds/linters still pass (I used rmw_dds_common in the commands as an example, I write there the list of packages affected by the PR).

@asorbini
Copy link
Collaborator Author

Quick CI validation (Linux, build --packages-up-to rmw_connextdds, test --packages-above rti_connext_dds_cmake_module): Build Status

@ivanpauno
Copy link
Member

ivanpauno commented Apr 28, 2021

--packages-up-to rmw_connextdds, test --packages-above rti_connext_dds_cmake_module

test --packages-above rti_connext_dds_cmake_module failed here because it's trying to run tests in packages that weren't built.
You have to make sure that the packages selected in the test command are a subset of the ones selected in the build command (which can be a bit confusing sometimes).

Linux Build Status

edit: Passing to colcon graph/list locally the same "package selection" flags might help to understand which packages are being selected.

@asorbini
Copy link
Collaborator Author

Oops, thank you for running with the right arguments, I see the issue now. I tried to be "slick" but I was definitely too quick :)

I think I could have used --packages-above rti_connext_dds_cmake_module --packages-up-to rmw_connextdds to just test the packages from ros2/rmw_connextdds.

Anyway, since things are green I'll go ahead and merge this PR.

@asorbini asorbini merged commit 9d0dec6 into master Apr 28, 2021
@delete-merged-branch delete-merged-branch bot deleted the asorbini/always-share branch April 28, 2021 20:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dashing-backports PR should be backported to Dashing eloquent-backports PR should be backported to Eloquent foxy-backports PR should be backported to Foxy galactic-backports PR should be backported to Galactic. humble PR scheduled for the H-turtle
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants