add full VERSIONs / SONAMEs to our libraries #273

v4hn · 2016-10-09T11:54:26Z

This has been on my mind for quite some time and I consider the discussion (and hopefully merge) of this request as a blocker for the upcoming indigo and kinetic releases.

This request is still incomplete and only adds versions to moveit_core's libraries. I would like to include moveit_ros in this change too. However, I want to start the discussion, so please give your opinion.

As a result of this request the libraries do not install as libmoveit_xyz.so anymore, but as libmoveit_xyz.so.${MOVEIT_VERSION} and provide libmoveit_xyz.so as a symlink pointing to the versioned file.

Because this sets each library's SONAME to the full version, this enforces that every binary links with the versioned library file from now on and has to be relinked with each new release of MoveIt!.

An alternative would be to set the SONAME to $MAJOR.$MINOR and ignore the patch version. But because we currently stay with one $MAJOR.$MINOR number within each ROS distribution, we had (and likely will have) ABI changes in the $PATCH version releases too.

The reason for this commit is that it is practically impossible to maintain full ABI compatibility within each ROS distribution and still add the the features/patches the community asks for. This has resulted in more than one ABI-incompatible MoveIt! release in the recent past within a ROS distribution. Because the libraries have not been versioned up to now, there was no way to indicate the incompatible changes and users who did not rebuild their whole workspace with the new release encountered weird and hard-to-track segfaults or broken behavior.

The main alternative I see for us at the moment is to set hard-coded versions for each library. Then, whenever we do a release and changed the ABI of individual libraries within MoveIt, we could increase the version of each of these libraries independent of each other. As a result, only binaries that link against libraries that actually changed their ABI would have to be rebuild with a new release.
This adds quite a bit of overhead to the release process and also introduces a variety of library versions within a single MoveIt version over time that will probably confuse new users. I believe the alternative I propose in this request is superior.

@jspricke I discussed the need for SONAMEs with you quite a bit, so it would be great if you could have a look at it too.

de-vri-es · 2016-10-10T07:49:26Z

+1. If we have to break things, the louder and earlier it happens during the build, the better.

As for using using the moveit version or manually bumping on ABI breakage: it sounds like the manual method is too easy to forget. I'd prefer to err on the safe side even if that means unnecessary rebuilds. So unless someone thinks they can put in the effort for reliably bumping sonames on releases with ABI change, my preference is for using the moveit version.

As for including the patch number or not: it would be nice if it can be left out. That should be fine as long as we bump minor version when ABI compatibility breaks. The only problem with that is the indigo/jade/kinetic releases conflicting with each other, but I think that is a different discussion. But as long as we break ABI in patch-level releases we should include it.

v4hn · 2016-10-10T11:12:55Z

On Mon, Oct 10, 2016 at 12:49:26AM -0700, Maarten de Vries wrote:

As for including the patch number or not: it would be nice if it can be left out.
That should be fine as long as we bump minor version when ABI compatibility breaks.

Yes and yes.

The problem is that we do (and want to) backport new features and ABI changes to older branches, but will not backport API changes.*
Thus we would need to bump the minor version of stable branches separately from the most current branch and that conflicts with our current "different minor versions for different branches" policy.
I do not consider it an option to use different major versions for each branch instead.

*This, by the way, somewhat relates to the proposal to have a "master" branch that should work across the supported ROS distributions/Linux distributions.

davetcoleman · 2016-10-11T18:47:52Z

moveit_core/background_processing/CMakeLists.txt

-add_library(${MOVEIT_LIB_NAME}
-  src/background_processing.cpp
-  )
+add_library(${MOVEIT_LIB_NAME} src/background_processing.cpp)


while its not worth changing, I prefer the previous format because it makes adding more files to the library list cleaner in the future

davetcoleman · 2016-10-11T18:49:16Z

moveit_core/background_processing/CMakeLists.txt

-  src/background_processing.cpp
-  )
+add_library(${MOVEIT_LIB_NAME} src/background_processing.cpp)
+set_target_properties(${MOVEIT_LIB_NAME} PROPERTIES VERSION ${${PROJECT_NAME}_VERSION})


Out of curiosity, where is the value of ${${PROJECT_NAME}_VERSION} set?

davetcoleman · 2016-10-11T18:55:33Z

Thanks @v4hn!

+1 to including the patch version in the sonames, we release so rarely I don't think the rebuild time is that important and I do forsee us still breaking ABI between patch versions due to the way our versioning is done between I/J/K. I also like having the sonames the same as the versions, to ease maintenance. I'd like to see this PR expanded to all of MoveIt!, as @v4hn intends.

@wjwwood perhaps you have any wisdom on this change for moveit?

jspricke · 2016-10-12T14:48:02Z

+1 from me, would be really great to have it :).

v4hn · 2016-10-12T16:23:36Z

On Tue, Oct 11, 2016 at 11:49:31AM -0700, Dave Coleman wrote:

Out of curiosity, where is the value of ${${PROJECT_NAME}_VERSION} set?

In the guts of catkin_package() where it is extracted from the package.xml.

davetcoleman · 2016-10-16T22:17:27Z

@v4hn can you finish this PR so we can get Kinetic out of the door?

v4hn · 2016-10-17T11:18:57Z

I just came back to my keyboard yesterday.
I'll try to get through my pending todo list fast.

rhaschke · 2016-10-17T17:49:40Z

I like the idea of introducing sonames in general. However, I think we missed some important points:

Usually, supporting sonames should include support for several installed lib versions in parallel. However, as far as I know, this is not possible with the ROS release process: bloom only allows for one unique version at a time and it doesn't distinguish between bin and dev packages as well.
Looks like Michael considered this already, because he wrote that "every binary has to be relinked with each new release of MoveIt!" This will break running setups on every release. While we research guys might not care about this, I'm curious what industry thinks about this? @mikeferguson? @mikepurvis?
I think we should continue to try to avoid ABI breakage within ROS versions. Having this PR in place shouldn't be an excuse to relax this policy. Breaking ABI and API on a new ROS release is fine, I think - people are expecting changes in this case. However, I think this means that we cannot simply backport all fixes and features to older branches as proposed by @v4hn.
To allow for releases without ABI changes (within a ROS version), the patch version should be not included / different from the soname patch version. I know that this will produce more effort on our side to take care of these changes. However, as said in 3. we should take care!

mikepurvis · 2016-10-17T17:54:43Z

Our deployment is via a bundled workspace now, so everything rebuilds each time— we don't worry about ABI at all. See: https://github.com/mikepurvis/ros-bundling

davetcoleman · 2016-10-17T22:35:06Z

@rhaschke its not clear what you are proposing - just that we shouldn't include the patch version in the sonames?

After your points its also not clear to me what the advantage of this PR is - currently people who use MoveIt! debians sometimes have their catkin workspace broken after a ROS update because of ABI changes. With this new PR, it will be the same except that will always happen - correct?

gavanderhoorn · 2016-10-18T06:28:27Z

The point is that with sonames, the breakage will be explicit (ie: things will not start because they cannot find the libraries they were linked against), while right now, things will appear to be fine (because all the libraries are there), but due to ABI incompatibilities, users may run into all sorts of hard to understand / debug segfaults.

As @rhaschke writes, with the current infrastructure (Bloom, etc) , the other advantage of sonames (ie: linking against a particular version, with several different versions all present in parallel) will not work.

v4hn · 2016-10-18T12:50:22Z

On Mon, Oct 17, 2016 at 10:49:47AM -0700, Robert Haschke wrote:

Usually, supporting sonames should include support for several installed lib versions in parallel. However, as far as I know, this is not possible with the ROS release process: bloom only allows for one unique version at a time and it doesn't distinguish between bin and dev packages as well.

Yes. Having sonames in the libraries supports having multiple versions of the same library installed. lib<libraryname>.so will point to one (e.g. the most current) version of lib<libraryname>.so.x.y.z but other (older) versions can still exist in lib<libraryname>.so.a.b.c. This ensures that newly compiled modules will link to the current version, while old modules are still linked to the old library version.

And also: Yes, bloom and OSRF's packaging schemes do not account for such version "transitions" (that's the term for it in Debian) at all. As far as I've been told this was discussed at length some years ago and (I've been told) the final argument was that OSRF does not want to "guarantee the stability this would imply". The argument does not make any sense to me, but this thread is clearly not the place to discuss this.
Anyway, anyone building the packages on their own (e.g. for Debian, Arch, or for their own personal use) is free to keep the old library versions around for transitions.

Looks like Michael considered this already, because he wrote that "every binary has to be relinked with each new release of MoveIt!" This will break running setups on every release.

Yes, this is intentional.

While we research guys might not care about this, I'm curious what industry thinks about this? @mikeferguson? @mikepurvis?

All "industry" setups I know (including OSRF's build system) rebuilds the whole dependent software stack for each update.
This is precisely because most ROS components do not use sonames and break ABI whenever they think it makes sense.

I think we should continue to try to avoid ABI breakage within ROS versions.
Having this PR in place shouldn't be an excuse to relax this policy.

We should definitely minimize ABI changes, I agree. Also to avoid having to recompile your whole moveit development workspace all the time.
But the whole point of this request is to allow ABI breakage between releases within a single ROS distribution.
We receive requests such as an fcl Object cache and I don't think it is an option at the moment to merge these only to the most current branch.
Especially when this most current branch targets a different set of versions for the dependencies (e.g. kinetic-devel when the request targets indigo-devel).
Imho experience tells us, that full ABI stability within a ROS distribution for a research oriented library as MoveIt is a pretty illusive goal..

To allow for releases without ABI changes (within a ROS version), the patch version should be not included / different from the soname patch version. I know that this will produce more effort on our side to take care of these changes. However, as said in 3. we should take care!

In theory I agree. This does not work with our current versioning scheme though.
If we would either change the library names to include the ROS distribution or increase the major version with each branch we could get around that, but I don't consider either of these a good idea.

Breaking ABI and API on a new ROS release is fine, I think - people are expecting changes in this case.
However, I think this means that we cannot simply backport all fixes and features to older branches as proposed by @v4hn.

Of course ABI and API are two entirely different things. We should definitely not backport any API change and did not do that in the recent past.
But having the full version as the soname allows to have new features that break ABI in "older" ROS distributions.
Because users provide patches for the branches they actually use, we often get patches for these branches that also break ABI for legitimate reasons (see for example the fcl request above).
It sounds just wrong to me to tell them their change will not be merged in the branch they want to merge it in.

To allow for releases without ABI changes (within a ROS version), the patch version should be not included / different from the soname patch version.

I considered this and don't think this makes sense for the moment. It would leave us with the same problems we have right now.

rhaschke · 2016-10-18T14:24:26Z

I think, we agree on most points. My post was just to bring some pros and cons of the approach to our attention. Sorry, if I failed @davetcoleman.

To allow for releases without ABI changes (within a ROS version), the patch version should be not included / different from the soname patch version. I know that this will produce more effort on our side to take care of these changes. However, as said in 3. we should take care!

In theory I agree. This does not work with our current versioning scheme though.
If we would either change the library names to include the ROS distribution or increase the major version with each branch we could get around that, but I don't consider either of these a good idea.

I agree. However, I see a third option: increasing the patch level of soname independent of the patch level of the release version (slower or leaving out / not increasing when ABI stability is maintained).

version  soname
0.8.3    0.8.3
0.8.4    0.8.3 (ABI stability was maintained)
0.8.5    0.8.5 (ABI broken)

Because users provide patches for the branches they actually use, we often get patches for these branches that also break ABI for legitimate reasons (see for example the fcl request above).
It sounds just wrong to me to tell them their change will not be merged in the branch they want to merge it in.

I didn't argued against backporting at all, but to carefully think about what to backport and what not. Furthermore, I would like to reduce the effort to rebuild downstream packages as much as possible - which is possible if we manually increase the patch version of SONAMES.

To allow for releases without ABI changes (within a ROS version), the patch version should be not included / different from the soname patch version.

I considered this and don't think this makes sense for the moment. It would leave us with the same problems we have right now.

I think, if we increase the patch version manually, it could work. Maybe, you didn't yet considered this option.

rhaschke · 2016-10-18T16:14:40Z

On telephone, @v4hn and I agreed to use soname = patch version for this next release. However, in future we should try to bump the patch version only on ABI changes.

davetcoleman · 2016-10-18T17:34:48Z

This PR still only covers moveit_core and is not ready to be merged in. This change should also be held off until the next Kinetic release - a line must be drawn in the sand for releasing software or it will never happen. There is still too much debate going on here.

mikeferguson · 2016-10-18T19:02:53Z

How about a middle of the road that promotes getting a kinetic release out, but also managing the upgrade path:

We do an initial release of Kinetic based on the current state. In the announce email we make it very clear that SONAMEs are coming, and that our next release will include them (Effectively, the library name with no version is the first "version" of SONAMES). We also make it clear that we are making no ABI promises with the current Kinetic release.
SONAME support gets merged into Kinetic/Jade as soon as the PR is ready and fully reviewed.
We merge SONAME support for Indigo as soon as we identify the next ABI breakage (it's bound to happen). That way, we don't double-break our users (adding SONAMEs in a release that includes no other ABI breakage).

v4hn · 2016-10-19T09:00:41Z

This PR still only covers moveit_core and is not ready to be merged in. This change should also be held off until the next Kinetic release

I just added the remaining libraries. I was too tired to do this yesterday evening.
I will test this further today and don't expect any problems.
In my opinion this can be merged today, because there is not much to review.
I simply added a PROPERTIES VERSION line for each add_library in the repo.
Also @rhaschke agreed that his objections can be discussed after we have this change in for the release.

We do an initial release of Kinetic based on the current state. In the announce email we make it very clear that SONAMEs are coming, and that our next release will include them (Effectively, the library name with no version is the first "version" of SONAMES). We also make it clear that we are making no ABI promises with the current Kinetic release.
SONAME support gets merged into Kinetic/Jade as soon as the PR is ready and fully reviewed.
We merge SONAME support for Indigo as soon as we identify the next ABI breakage (it's bound to happen). That way, we don't double-break our users (adding SONAMEs in a release that includes no other ABI breakage).

@mikeferguson Your whole proposal presupposes that binaries built against libxyz.so break when you add an soname to the next release of that library. This is not the case! The build still provides (and has to provide) a symlink libxyz.so pointing to the named library file.
This is exactly why I want to have sonames in this release! Because we can only mark ABI breakage in releases after we have sonames in already.

v4hn · 2016-10-19T09:05:31Z

moveit_setup_assistant/CMakeLists.txt

+    moveit_ros_planning
+    moveit_ros_visualization
+)
+


This is the only non-trivial change I did: I had to move the catkin_package command above the library declarations.
This is required because the resp. add_library and set_target_properties commands should stay near each other, but ${${PROJECT_NAME}_VERSION} is defined by catkin_package.

As a result the libraries do not install as `libmoveit_xyz.so` anymore, but as `libmoveit_xyz.so.${MOVEIT_VERSION}` and only provide `libmoveit_xyz.so` as a symlink pointing to the versioned file. Because this sets each library's SONAME to the *full version*, this enforces that *every* binary links with the versioned library file from now on and has to be relinked with *each* new release of MoveIt!. The alternative would be to set the SONAME to `$MAJOR.$MINOR` and ignore the patch version, but because we currently stay with one `$MAJOR.$MINOR` number within each ROS distribution, we had (and likely will have) ABI changes in the `$PATCH` version releases too. The reason for this commit is that it is practically impossible to maintain full ABI compatibility within each ROS distribution and still add the the features/patches the community asks for. This has resulted in more than one ABI-incompatible MoveIt! release in the recent past within a ROS distribution. Because the libraries have not been versioned up to now, there was no way to indicate the incompatible changes and users who did not rebuild their whole workspace with the new release encountered weird and hard-to-track segfaults or broken behavior.

jspricke · 2016-10-19T11:48:51Z

+1 for the new commits as well. Looking really good :).

rhaschke

+1. Did you ensured, that you don't miss a library?

v4hn · 2016-10-19T16:05:36Z

Yes. I compiled the whole repository in an otherwise empty namespace and checked that all install libraries have an soname-version and a .so-symlink .

davetcoleman · 2016-10-19T17:35:41Z

I also just tested this locally in a fresh workspace and verified all libraries are consistently setup with sonames.

As an aside, would we want to apply this same change to geometric_shapes?

* add full VERSIONs / SONAMEs to all core libraries As a result the libraries do not install as `libmoveit_xyz.so` anymore, but as `libmoveit_xyz.so.${MOVEIT_VERSION}` and only provide `libmoveit_xyz.so` as a symlink pointing to the versioned file. Because this sets each library's SONAME to the *full version*, this enforces that *every* binary links with the versioned library file from now on and has to be relinked with *each* new release of MoveIt!. The alternative would be to set the SONAME to `$MAJOR.$MINOR` and ignore the patch version, but because we currently stay with one `$MAJOR.$MINOR` number within each ROS distribution, we had (and likely will have) ABI changes in the `$PATCH` version releases too. The reason for this commit is that it is practically impossible to maintain full ABI compatibility within each ROS distribution and still add the the features/patches the community asks for. This has resulted in more than one ABI-incompatible MoveIt! release in the recent past within a ROS distribution. Because the libraries have not been versioned up to now, there was no way to indicate the incompatible changes and users who did not rebuild their whole workspace with the new release encountered weird and hard-to-track segfaults or broken behavior. * add SONAMES to all non-core libraries too

v4hn · 2016-11-11T16:52:29Z

On Wed, Oct 19, 2016 at 10:35:41AM -0700, Dave Coleman wrote:

would we want to apply this same change to geometric_shapes?

I don't think so. At least not the way we use sonames in MoveIt now.
MoveIt is changing rather rapidly, so full sonames within a distro make sense to allow breaking the ABI in releases within the distro.

For a package like geometric_shapes, I believe it shouldn't be a problem to keep the ABI stable within each ROS distribution,
so it's not that important to have sonames there. If we want to add them there, they should be bumped by hand.

This is similar to moveit#273 / 0a7a895, but hard-codes the version for each library instead of using the project's version. Thus, we have to bump the version of a library *manually* if we break ABI in a release. === Below is the original commit message of the patch targeting the kinetic branch. * add full VERSIONs / SONAMEs to all core libraries As a result the libraries do not install as `libmoveit_xyz.so` anymore, but as `libmoveit_xyz.so.${MOVEIT_VERSION}` and only provide `libmoveit_xyz.so` as a symlink pointing to the versioned file. Because this sets each library's SONAME to the *full version*, this enforces that *every* binary links with the versioned library file from now on and has to be relinked with *each* new release of MoveIt!. The alternative would be to set the SONAME to `$MAJOR.$MINOR` and ignore the patch version, but because we currently stay with one `$MAJOR.$MINOR` number within each ROS distribution, we had (and likely will have) ABI changes in the `$PATCH` version releases too. The reason for this commit is that it is practically impossible to maintain full ABI compatibility within each ROS distribution and still add the the features/patches the community asks for. This has resulted in more than one ABI-incompatible MoveIt! release in the recent past within a ROS distribution. Because the libraries have not been versioned up to now, there was no way to indicate the incompatible changes and users who did not rebuild their whole workspace with the new release encountered weird and hard-to-track segfaults or broken behavior. * add SONAMES to all non-core libraries too

* Port moveit_ros_warehouse package.xml, CMakeLists.txt * Apply ROS 2 migration steps * Enable warehouse in motion_planning_frame * Run mongo db in run_move_group.launch

davetcoleman mentioned this pull request Oct 9, 2016

Kinetic Release #18

Closed

19 tasks

v4hn mentioned this pull request Oct 9, 2016

Initial Indigo release from ros-planning/moveit repo #100

Closed

15 tasks

davetcoleman approved these changes Oct 11, 2016

View reviewed changes

rhaschke mentioned this pull request Oct 17, 2016

Invalid Trajectory: start point deviates #283

Closed

v4hn commented Oct 19, 2016

View reviewed changes

v4hn force-pushed the pr-kinetic-sonamesl branch from fc32d76 to 8f3d19a Compare October 19, 2016 09:08

v4hn added 2 commits October 19, 2016 11:08

add SONAMES to all non-core libraries too

8f3d19a

v4hn changed the title ~~add full VERSIONs / SONAMEs to all core libraries~~ add full VERSIONs / SONAMEs to our libraries Oct 19, 2016

rhaschke approved these changes Oct 19, 2016

View reviewed changes

davetcoleman merged commit 0a7a895 into moveit:kinetic-devel Oct 19, 2016

v4hn mentioned this pull request Dec 15, 2016

add full VERSIONs / SONAMEs to all libraries #383

Merged

knorth55 mentioned this pull request Dec 28, 2016

[indigo] add correct version to all libraries so files #390

Closed

mlautman mentioned this pull request Mar 11, 2019

Port planning_scene to ROS2 moveit/moveit2#43

Merged

v4hn mentioned this pull request Aug 19, 2020

Any plan for the noetic release moveit/moveit_msgs#86

Closed

v4hn mentioned this pull request Jan 30, 2024

Version of ROS package dependencies RoboStack/ros-noetic#440

Open

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add full VERSIONs / SONAMEs to our libraries #273

add full VERSIONs / SONAMEs to our libraries #273

v4hn commented Oct 9, 2016

de-vri-es commented Oct 10, 2016

v4hn commented Oct 10, 2016

davetcoleman Oct 11, 2016

davetcoleman Oct 11, 2016

davetcoleman commented Oct 11, 2016

jspricke commented Oct 12, 2016

v4hn commented Oct 12, 2016

davetcoleman commented Oct 16, 2016

v4hn commented Oct 17, 2016

rhaschke commented Oct 17, 2016

mikepurvis commented Oct 17, 2016

davetcoleman commented Oct 17, 2016

gavanderhoorn commented Oct 18, 2016

v4hn commented Oct 18, 2016

rhaschke commented Oct 18, 2016

rhaschke commented Oct 18, 2016

davetcoleman commented Oct 18, 2016

mikeferguson commented Oct 18, 2016

v4hn commented Oct 19, 2016

v4hn Oct 19, 2016

jspricke commented Oct 19, 2016

rhaschke left a comment

v4hn commented Oct 19, 2016

davetcoleman commented Oct 19, 2016

v4hn commented Nov 11, 2016

add full VERSIONs / SONAMEs to our libraries #273

add full VERSIONs / SONAMEs to our libraries #273

Conversation

v4hn commented Oct 9, 2016

de-vri-es commented Oct 10, 2016

v4hn commented Oct 10, 2016

davetcoleman Oct 11, 2016

Choose a reason for hiding this comment

davetcoleman Oct 11, 2016

Choose a reason for hiding this comment

davetcoleman commented Oct 11, 2016

jspricke commented Oct 12, 2016

v4hn commented Oct 12, 2016

davetcoleman commented Oct 16, 2016

v4hn commented Oct 17, 2016

rhaschke commented Oct 17, 2016

mikepurvis commented Oct 17, 2016

davetcoleman commented Oct 17, 2016

gavanderhoorn commented Oct 18, 2016

v4hn commented Oct 18, 2016

rhaschke commented Oct 18, 2016

rhaschke commented Oct 18, 2016

davetcoleman commented Oct 18, 2016

mikeferguson commented Oct 18, 2016

v4hn commented Oct 19, 2016

v4hn Oct 19, 2016

Choose a reason for hiding this comment

jspricke commented Oct 19, 2016

rhaschke left a comment

Choose a reason for hiding this comment

v4hn commented Oct 19, 2016

davetcoleman commented Oct 19, 2016

v4hn commented Nov 11, 2016