Skip to content

Conversation

@ivanpauno
Copy link
Member

This is a question and a PR at the same time.

Currently, foxy branch is being used in rmw_cyclonedds to target Foxy.

In the past, it was desired to maintain several versions from the same branch (dashing-eloquent was split to a separate branch due to several breaking changes in Foxy).

Considering that there are not much changes between foxy and master, should both be unified?
In that case, this fix is required, as I have broken compatibility in #192.

Before unifying branches, it should be double checked if #187 is completely backwards compatible.

PS: We usually create branches in other repos during the release process, thus one was "automatically" created here in the release process.

@ivanpauno ivanpauno requested a review from eboasson June 22, 2020 17:15
@ivanpauno ivanpauno self-assigned this Jun 22, 2020
@ivanpauno ivanpauno changed the title Add preprocessor logic to preserve compatibility with Foxy Add preprocessor logic to preserve compatibility with Foxy in master Jun 22, 2020
Copy link
Collaborator

@eboasson eboasson left a comment

Choose a reason for hiding this comment

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

@ivanpauno, now that all features are supported, there is not much point in maintaining compatibility with all versions in the RMW layer. As long as it is easy to do so (like here), there is still some value in it, but when the differences become larger (like with the node-to-participant mapping change) it is not worth it.

Once there is a Cyclone 0.7.x branch (which will include security), there will also no longer be a need to track Cyclone master. So from that point on everything will be "normal".

@ivanpauno
Copy link
Member Author

@ivanpauno, now that all features are supported, there is not much point in maintaining compatibility with all versions in the RMW layer. As long as it is easy to do so (like here), there is still some value in it, but when the differences become larger (like with the node-to-participant mapping change) it is not worth it.

Sure.
I opened this because I wasn't sure if the foxy branch was created by mistake or if it is intentional to maintain different versions from different branches now.
In the former case, I wanted to make sure I didn't break compatibility in master unintentionally 😁 .

@eboasson feel free to merge or close, depending if foxy/master branches are going to be unified or not.
I'm not an usual maintainer of this package, so I don't have a preference.

@jacobperron
Copy link
Member

I opened this because I wasn't sure if the foxy branch was created by mistake or if it is intentional to maintain different versions from different branches now.

I think I was the one who created the foxy branch in a batch process. It wasn't necessarily a mistake, as I expected ABI to diverge at some point and wanted to have an ABI-stable version for Foxy. Considering that master is several releases ahead of foxy, I'm not sure that we should switch to using master for Foxy (I haven't looked closely at possible ABI changes).

@eboasson
Copy link
Collaborator

eboasson commented Jul 8, 2020

Let's stick to the "standard" model, so keep using the foxy branch that @jacobperron made during Foxy release and master for the current development. In the past, keeping a single branch meant that Dashing and Eloquent ended up with full coverage of features. Foxy is already fully supported by the foxy branch, and in my view, that means the extra work (and risk) of maintaining compatibility no outweighs the benefits.

Closing as suggested by @ivanpauno.

@eboasson eboasson closed this Jul 8, 2020
@rotu
Copy link
Collaborator

rotu commented Jul 8, 2020

in my view, that means the extra work (and risk) of maintaining compatibility no outweighs the benefits.

@eboasson, that should probably be documented. Adding a backwards incompatibility should be accompanied with a bump of the major version number. Using the branch as the de-facto major version number is a terrible habit.

@eboasson
Copy link
Collaborator

eboasson commented Jul 8, 2020

@rotu while I agree in general, as I understand it, that's the common practice in ROS 2 packages. The rmw_cyclonedds_cpp package is purely an adaptation thing for ROS 2, so conforming to the development/branching/versioning policy of ROS 2 only makes sense.

Cyclone DDS itself is a different story, that one has an independent life, has a stable interface and maintains backwards compatibility.

@ivanpauno ivanpauno deleted the ivanpauno/foxy-galactic-same-branch branch July 8, 2020 16:32
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.

5 participants