changes to developer guide

[maryaB-osr]

This is related to REP 2004 -- don't merge until that's released

Fixes #531

Signed-off-by: maryaB-osr marya@openrobotics.org

[wjwwood]

It's a bit nuanced, so I'm not sure how this should be reworded, but technically semver doesn't speak about maintaining API stability. I would say it like this (awkward as it is):

Avoiding major API increases ensures API stability according to semver, but additionally we will maintain ABI stability in our minor and patch releases within a released ROS distribution.

[maryaB-osr]

Your second clause ("but additionally...") is essentially saying the same thing as line 733 below, right?

We will not allow patch (bug-fixes) and minor (non-breaking) version increments to affect API and ABI within a released distribution.

So does it make sense to just replace the first clause of my original with your explanation about API stability, and leave the second clause and line 733 as-is?

In addition to avoiding major API increases to ensure API stability according to semver, we will be ABI (and therefore API) stable within a ROS distribution.

[wjwwood]

I would link to instructions on how to use these linters and which linters are included and why. I think it makes sense for ament_lint_common to own this documentation.

[chapulina]

Every time I look at that wikipedia page I have a hard time connecting it to what we mean here. I also don't have a better link to suggest, so I'd suggest we just remove it to avoid confusion.

Another suggestion, I'd add a little example here to make the tick-tock process clearer, similar to what we have on the gazebo page. Something like:

Version	API
X-turtle	void foo();
Y-turtle	[[deprecated("use bar()")]] void foo(); void bar();
Z-turtle	void bar();

[maryaB-osr]

I also wanted to ask about the semver deprecation definition/suggestion: https://semver.org/#how-should-i-handle-deprecating-functionality

It says to put the deprecation in a minor release. Is that what we’ll do, together with separating by distro? I assumed it was by major release because I associated distros with major releases but that’s not necessarily the case

[wjwwood]

I feel like we discussed that point before...

In slack we discussed it, I had this to say about it:

2. w.r.t. when to tick and tock, if you deprecate a function (and that's all) during one release, then yes you could just do minor, but typically this comes with other changes at the same time (a first new release into a new distribution) and so usually new deprecations come with a major release. Also, we normally do not add deprecations after a distribution is done, as this would add new compiler warnings in an already released distribution.

[chapulina]

It says to put the deprecation in a minor release. Is that what we’ll do, together with separating by distro?

I believe we're going for putting the deprecation on the next major, so no warnings are introduced within a released distro. I'd just confirm with @wjwwood .

I associated distros with major releases but that’s not necessarily the case

Yeah I know it can be confusing, but it's basically: a major bump requires a new distro, but a new distro doesn't require a major bump.

[wjwwood]

I believe we're going for putting the deprecation on the next major, so no warnings are introduced within a released distro. I'd just confirm with @wjwwood.

Yes. Though we can use a minor if we want, I just wouldn't do it in a released distro. I would treat deprecation like breaking ABI, we want to avoid it in a release distro, but before the distro is released it can be done in a minor patch (like if we released 2.0.0 in preparation for foxy, but added a deprecation in 2.1.0 before foxy was actually released).

[dirk-thomas]

May I ask for clarification how the version number should be set in the following common scenario:

• Package foo is released into Foxy with version 1.2.3
• Foxy is officially released
• Package foo gets an ABI breaking change - so I assume it will need to bump to 2.0.0 and can only be released into the rolling distro which will become G-turtle at some point
• What happens when further API/ABI changes are merged into foo down the road before G-turtle is officially released? Does it have to bump the major each time? If not, why not?

--

• Also if deprecation can't be added into an already released distro that means they can only happen for the next ROS distro which also means they can only be removed in the overnext ROS distro. That sounds like an extremely long period to me. I would rather prefer to be able to add them with a minor version bump into an already released distro. Users can always opt to silence the deprecation warning if they don't care about them.

[chapulina]

(This is a reply to William before Dirk's comment showed up)

I don't think we need to go deeper into this if it doesn't make sense for ROS 2. My perspective is largely based on what we've been doing for Ignition, and it may not make sense in this context.

---

Just to put it another way: let's say I'm developing a feature for rclcpp on top of version 2.0.0. With my proposal, I'd have to ask myself:

• Does it break API or ABI, or introduces warnings? If so, then target version 3.0.0

With your proposal, I just need to ask myself extra questions:

• Does it break API? Then it has to target version 3.0.0
• Does it break ABI or introduces warnings?
  • If so, is 2.0.0 already part of a released ROS distro?
    • If so, then target version 3.0.0

That also affects consumers of the library. If I compiled against version 2.0.0, I may need to recompile against 2.1.0 if that happened to have been released before a ROS distro was released (and ABI was broken).

---

what can change between ~pre and ~pre1 and the final release

I think that should be defined by the project. On Ignition anything can change from 3.0.0~pre1 to 3.0.0, as long as the rules from 2.0.0 to 3.0.0 apply (i.e. tick-tock).

[chapulina]

it will need to bump to 2.0.0 and can only be released into the rolling distro which will become G-turtle at some point

That's a good point, the rolling distro adds some complexity. The concept of pre-releases could help here: release 2.0.0~preX into the rolling release and then 2.0.0 into G-turtle.

[wjwwood]

• Package foo is released into Foxy with version 1.2.3
• Foxy is officially released
• Package foo gets an ABI breaking change - so I assume it will need to bump to 2.0.0 and can only be released into the rolling distro which will become G-turtle at some point

The maintainer can choose what version to target, but our practice is to bump the major preemptively to allow for future minor and patch releases in foxy which are not ABI breaking. According to semver, however, it is not required to jump to 2.0.0, at least that's what I take away from it assuming the change is ABI breaking but not API breaking.

• What happens when further API/ABI changes are merged into foo down the road before G-turtle is officially released? Does it have to bump the major each time? If not, why not?

It does not, but again that's up to the maintainer. AFAIK, semver does not prevent you from bump the major when bumping the minor could be sufficient. Personally, I would bump just the minor or patch if it is going into the rolling release, pre g-turtle. But bumping the major is valid too I think. The reason for bumping to 2.0.0 in the previous step is, in my opinion, to leave room for non-ABI breaking changes in foxy, not just because it is ABI breaking.

As for "why not", how you change the version number is dictated by semver and semver only speaks about breaking API, adding to API, and fixing a bug. It never mentions ABI or considerations related to packaging for a platform (like our ROS distributions or something like Debian). It also, however, does not say that you may not raise the major version number for arbitrary reasons, so for me it's up to us as to how we adjust version numbers when ABI is broken.

To @chapulina's point we could simplify it and say "ABI breaks always bump major", but there's nothing saying we have to do that. I'd actually be fine with that, but this is what we're recommending for everyone, so I was trying to think beyond that.

---

Also if deprecation can't be added into an already released distro that means they can only happen for the next ROS distro which also means they can only be removed in the overnext ROS distro. That sounds like an extremely long period to me. I would rather prefer to be able to add them with a minor version bump into an already released distro. Users can always opt to silence the deprecation warning if they don't care about them.

I suppose we can do that too, but I was just going off the idea that I doubt Ubuntu or Debian would allow a patch adding a deprecation warning. I guess we don't have to aim for that level of stability, but I also think that people will complain if their foxy CI suddenly has compiler warnings from rclcpp a few months after we've officially released foxy.

---

Just to put it another way: let's say I'm developing a feature for rclcpp on top of version 2.0.0.

Are you thinking as the feature developer in this user story or the maintainer (assume for this case they're different people)?

If you're the feature developer, you only need to think about this:

• I target my feature or bug fix to the "default" or "master" branch
  • we discourage pr's to previous versions except in special cases

That's it, and if you as the feature developer wish it to be back ported to a previous major version for some reason you may ask.

If you do ask, the maintainer may need to consider then:

• does it break API
• does it break ABI
• how complex is it

in order to decide whether or not it can be back ported. When the maintainer is making a new release into the "rolling distro" or the N+1-turtle pre-release, they must also look at changes that have accumulated and decide what to change in the version number, but that doesn't need to happen on every pull request, in my opinion.

If both roles are the same person or the feature developer knows they want it back ported, then they may preemptively consider things like API and ABI stability, but they don't need to, in my opinion.

That's a good point, the rolling distro adds some complexity.

How so? You're either releasing into the rolling release or the staging ground for the next ROS release. I don't see a difference. In fact once we have the rolling release, we'll likely just peel off from the rolling release to make each new ROS release when the time is right, no?

The concept of pre-releases could help here: release 2.0.0~preX into the rolling release and then 2.0.0 into G-turtle.

Again, we don't have support for that right now, and I personally don't see why that's preferable to:

release 2.0.0~preX2.0.0 into the rolling release and then 2.0.02.0.1 or 2.1.0 or 3.0.0 (depending on the kind of change) into G-turtle.

[wjwwood]

Sorry, I know this conversation has run long, but can you guys give feedback on a few of the dangling points?

To @chapulina's point we could simplify it and say "ABI breaks always bump major",

Should we just recommend this? Of course there will be exceptions where we cannot bump the major (like we have an emergency ABI break with-in a release), but it would be simpler to explain.

I suppose we can do that too, but I was just going off the idea that I doubt Ubuntu or Debian would allow a patch adding a deprecation warning.

Should we or should we not recommend against deprecation warnings within a stable ROS release?

The concept of pre-releases could help here: release 2.0.0~preX into the rolling release and then 2.0.0 into G-turtle.

Given that we don't support extra version components in our pipeline, I think we won't recommend this right now. I think it should be tabled until we have enough need to justify spending time updating our release infrastructure to support it (my assumption is that right now we do not).

---

And anything else you'd like to continue discussing.

[chapulina]

This sentence feels out of place to me. Maybe move it to the C++ style guide with a recommendation to use the PIMPL pattern?

[maryaB-osr]

I'm not sure where this should go under the C++ section of the style doc. Under one of the existing subsections or its own subsection? I'm also not sure if it would make sense to have this sentence alone in a section or if some supporting context should be written with it

[chapulina]

There's already a section on the style guide which talks about access control. And that is actually relaxing the requirement for members to be private, which goes in the opposite direction of this line here. So actually I'm not sure what the ROS 2 team would prefer. @wjwwood ?

[wjwwood]

The thing you linked to says:

Drop requirement for all class members to be private and therefore require accessors

That means, to me, that you may have public members without accessors, whereas the GSG strictly says no public members, instead have private members with accessor methods. It only applys to attributes of a class that a user might want to read or write.

This line in the developer guide says that avoiding private members in the class (therefore moving them into a pimpl class or something) can help with ABI stability, but is not required. It could apply to any member of the class, even if the user never reads or writes to it directly.

I don't think they overlap? How does the thing you linked to go in the opposite direction of this line?

[chapulina]

It feels a bit strange to call out CONTRIBUTING but not other files like LICENSE and README. Should those be mentioned?

[maryaB-osr]

@chapulina these lines feel out of place here now. I was thinking of adding them to the Documentation section above, but that would imply that having a contributing doc is something we do (or anyone who wants to link to that section in their quality declaration) to maintain quality level 1

[chapulina]

Oh true that the README is already mentioned above. And I see now that this section mentions a LICENSE file per-package (as opposed to per-repository). It does look confusing to have 2 separate sections that mention these files... I don't think it hurts to put CONTRIBUTING above, but I think it's @wjwwood 's call.

[wjwwood]

I don't have a strong opinion, I think it's ok if they're spread out in the sections which they relate to, but they could also be collected in one section like "files to put in your repository" or something.

[maryaB-osr]

@chapulina merging this now to check off tasks. I put the REP content back in for now, added a new issue for elaborating on tick tock (#549), and will follow up on missing ament_common content separately