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

Breaking changes within rosdistro #191

Closed
reinzor opened this issue May 1, 2020 · 17 comments
Closed

Breaking changes within rosdistro #191

reinzor opened this issue May 1, 2020 · 17 comments
Assignees

Comments

@reinzor
Copy link
Contributor

reinzor commented May 1, 2020

AFAIK no breaking changes should be introduced with releases to the same rosdistro. I noticed that this happens quite often in the mbf_msgs for example. When do you plan to create a stable 1.0.0 that adhere non-breaking changes within rosdistro's? Follow up of #130

@jspricke
Copy link

jspricke commented May 1, 2020

This will be solved by #190, right?

@spuetz
Copy link
Member

spuetz commented May 1, 2020

The version should be stable. It is just not called 1.0.0. Where are the breaking changes? What exactly is breaking the API?

@reinzor
Copy link
Contributor Author

reinzor commented May 1, 2020

This will be solved by #190, right?

Depends on how this is released ;)

Where are the breaking changes? What exactly is breaking the API?

0.3.1 release

[src/move_base_flex/mbf_msgs][12:41] ((0.3.1))
rein@rein $ rosmsg md5 mbf_msgs/ExePathResult
b22f308686bb4e3a7364ea944ef68fd0

0.2.5 release

[src/move_base_flex/mbf_msgs][12:46] ((0.2.5))
rein@rein $ rosmsg md5 mbf_msgs/ExePathResult
b7651e70ee0ff6aa4897e884a7e10403

Both released for melodic:

[/tmp/rosdistro/melodic][12:51] (master=)
rein@rein $ git log distribution.yaml | grep move_base_flex
    move_base_flex: 0.3.1-1 in 'melodic/distribution.yaml' [bloom] (#24409)
    move_base_flex: 0.3.0-1 in 'melodic/distribution.yaml' [bloom] (#24297)
    move_base_flex: 0.2.5-1 in 'melodic/distribution.yaml' [bloom] (#22629)
    move_base_flex: 0.2.4-1 in 'melodic/distribution.yaml' [bloom] (#21567)
    move_base_flex: 0.2.3-0 in 'melodic/distribution.yaml' [bloom] (#19471)
    move_base_flex: 0.2.2-0 in 'melodic/distribution.yaml' [bloom] (#19269)
    move_base_flex: 0.2.1-0 in 'melodic/distribution.yaml' [bloom] (#19221)

@spuetz
Copy link
Member

spuetz commented May 1, 2020

Yes, sure, the md5 sum, rebuilding the software should solve that issue. But it isn't really a version thing. Its just how the messages are generated and the md5 sum is generated. We have to add things to the messages / actions, otherwise we get stuck while developing MBF further. But, the compiler should not fail, so the API is stable. The ABI in this case maybe not.

@spuetz spuetz self-assigned this May 1, 2020
@reinzor
Copy link
Contributor Author

reinzor commented Jun 4, 2020

We just today encountered again a breaking change from 0.3.1 to 0.3.2 (mbf_msgs different md5 checksum). When our robots are running 0.3.1 and we upgrade our desktop computers, we cannot communicate anymore without upgrading the robot software (which is often not desired).

What are your plan regarding these breaking changes within ROS distro's? If this is not going to be changed, we might have to work with forks of move_base_flex or abandon the move_base_flex packages.

@spuetz
Copy link
Member

spuetz commented Jun 4, 2020

Don't worry. Since the master's code (and the changed messages) already existed when we found out that the message changes were the problem for maintaining binary compatibility, we couldn't take it out, since other software already depends on it.

Message changes will be made only if we change the minor version.
So for the next versions 0.3.* there will be no message, service or action changes.
Are you fine with that?

@spuetz
Copy link
Member

spuetz commented Jun 4, 2020

And we will announce any changes in the messages if we bump to a new minor version. Are you fine with that?

@reinzor
Copy link
Contributor Author

reinzor commented Jun 5, 2020

In my opinion, message changes break the interface since the md5 sum changes so these changes should bump the major version and the major version should stay constant within one rosdistro according to https://www.ros.org/reps/rep-0009.html . AFAIK, there is no way to pin the minor release version using rosdep.

@jspricke
Copy link

jspricke commented Jun 5, 2020

I agree with @reinzor here, messages should not change within a ROS release. If you consider MBF work in progress you should state so in the package description so people would not use it for production systems. But given that not only Magazino is in fact using it in production I would vote against doing that. The other option is to maintain two branches/releases and do breaking changes only in the unstable/rolling release one.

@Timple
Copy link
Contributor

Timple commented Jun 5, 2020

This package has been released for noetic and noetic is the last ROS1 release. This means there will never be any ROS1 updates possible for mbf_msgs. Just something to consider.

@spuetz
Copy link
Member

spuetz commented Jun 5, 2020

Yes, I don't think this is good. Because, maybe you guys need to enhance the action interface at some point, too. Also the API compatibility has been defined to be stable between two minor versions. #130 (comment)
We should find a compromise here, which works for all, and which is not too strict. Otherwise we harm great future developments and improvements.

@reinzor
Copy link
Contributor Author

reinzor commented Jun 5, 2020

This package has been released for noetic and noetic is the last ROS1 release. This means there will never be any ROS1 updates possible for mbf_msgs. Just something to consider.

That is the downside of open-source releases. You always lag behind if you want the stable version. But I think this is the only way to keep your users because they don't want to be annoyed with breaking changes. If you want to be up to date with the latest version, you can choose to run from source.

We should find a compromise here, which works for all, and which is not too strict. Otherwise we harm great future developments and improvements.

We can still develop to not block new great future developments and improvements. But, if these developments are breaking the ABI, we have to wait for a new ROS release. So people that want these features can just pin the repo at some point. This is how it is done for gazebo, nav stack, rviz, common messages etc.

@spuetz
Copy link
Member

spuetz commented Jun 5, 2020

There is now new ROS1 release. This would result in no message changes for five years in ROS1.
We are and also would follow the rules here https://www.ros.org/reps/rep-0009.html "ABI compatibility is required only for even-cycle patch releases."
However, for now there are no message, service or action changes planned. But we should find a compromise here. A good example that changes were requested and wanted by the community is this change here: #182 This is now used by more than four institutions / companies. This solved their problems related to dynamic tolerance.

@reinzor
Copy link
Contributor Author

reinzor commented Jun 10, 2020

I understand your motives but this will force people that want a stable version to run from source instead of release. IMO it should be the other way around. Anyway, we will set-up a source build then :).

@spuetz
Copy link
Member

spuetz commented Jun 10, 2020

Okay, but I think I'm outvoted and we are not going to change the messages, services, and actions anymore. As discussed with @corot, we hack in additional action servers and new messages /actions e.g. GetPath2, ExePath2, and Recovery2 etc, if necessary.

@MichaelGrupp
Copy link
Contributor

My 2 cents:

Hacking new message files into the repo to avoid clashes with stable release packages is, indeed, a hack. This adds a new layer for versioning new features on the file level. And it's a hurdle for developing/refactoring the bleeding edge version IMO. I agree with @jspricke that there should be a distinction between release (stable API) and development code (new features etc.) via branching in this case.

If we want to have stability in a ROS distro release, ideally only important bug patches should be released for that distro and people who need newest features immediately should use a source build from master.

This wouldn't help with the "5 years without new package" problem though, but I guess that's a general problem of ROS 1 now... 😕

@spuetz
Copy link
Member

spuetz commented Aug 4, 2020

See #216

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

No branches or pull requests

5 participants