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

[RFC] Add strict versions to shlibs #426

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jspricke
Copy link
Contributor

@jspricke jspricke commented May 8, 2017

bloom doesn't create separate library packages according to the Debian
policy naming scheme. As we allow ABI breakage in the release (see
Moveit), this results in too lax dependencies for example:

Depends: ros-kinetic-moveit-core

instead of (for example)

Depends: ros-kinetic-moveit-core (= 0.9.6)

Resulting in a library not found when the library name changes.

This adds strict dependencies for all packages containing shlibs.

More discussion: ros-planning/moveit#497

bloom doesn't create separate library packages according to the Debian
policy naming scheme. As we allow ABI breakage in the release (see
Moveit), this results in too lax dependencies for example:

Depends: ros-kinetic-moveit-core

instead of (for example)

Depends: ros-kinetic-moveit-core (= 0.9.6)

Resulting in a library not found when the library name changes.

This adds strict dependencies for all packages containing shlibs.

More discussion: ros-planning/moveit#497
@jspricke jspricke changed the title Add strict versions to shlibs [RFC] Add strict versions to shlibs May 8, 2017
@jspricke
Copy link
Contributor Author

jspricke commented May 8, 2017

I've only done basic testing of this, but based on the moveit discussion and the limitations of bloom, I think it's the best we can do. Comments welcome.

@simonschmeisser
Copy link

I haven't used bloom so far, but I assume we do not have any information about the libraries and their so-name we are packaging? If that's the case I think this is as good as it will get (unless we want to rewrite/extend dh_makeshlibs)

+1

(note: I haven't tested it myself)

@wjwwood
Copy link
Contributor

wjwwood commented May 8, 2017

I'd have to defer to @tfoote, but my understanding is that we don't automatically add the version dependency so that non-ABI breaking changes don't require downstream packages to reinstall.

There were several discussions on how to specify the ABI version of a package in addition to the normal version number, but there was no consensus due to a hang up on how to separate SO versioning with package "ABI". So bloom can't do anything about this until that is specified (outside of what you've proposed here).

bloom doesn't create separate library packages according to the Debian
policy naming scheme.

Seems to me you'd have the same problem with ABI breaks and lax versioning whether or not there is a separate library package or not. Maybe I'm missing some as to how having the separate library would address this issue implicitly.

It looks like this is a symptom of them (the moveit developers) using SO versioning, which changes the library name for each release. If that's the case then I think they can add ${shlibs:Depends} to their Depends as a patch during the bloom release. Anything you can do in a rules file you can do and still use bloom.

I don't mind adding this to the default bloom rules file template as long as there is no regression for existing packages or other drawbacks. I'd also like to hear comments from others.

@jspricke
Copy link
Contributor Author

jspricke commented May 8, 2017 via email

@wjwwood
Copy link
Contributor

wjwwood commented May 8, 2017

But non-ABI breaking changes shouldn't change the Soname as well ;).

I didn't say that they did. I don't see why pointing that out should change how we build the packages which don't use so versioning.

@wjwwood
Copy link
Contributor

wjwwood commented May 8, 2017

Also, putting the library into its own package only helps if your also using so versioning, which was what I didn't understand from your original post.

You can modify the control file to produce multiple packages after bloom creates the basic control file. That should work ok. Or they could turn off so versioning when building on our build farm since the advantages are mostly lost due to not having multiple debs, one for each so version, which can be installed simultaneously. That way they don't have the problem of the rolling library name. Or they could figure out why the downstream packages are not linking against the symlinked so without a version in the name.

I'm still ok to add this patch if it can be shown to be low risk for regressions and others think it's a good idea.

@jspricke
Copy link
Contributor Author

jspricke commented May 8, 2017 via email

@wjwwood
Copy link
Contributor

wjwwood commented May 8, 2017

Given the numerous comments about the Soname change, I think it's quite
the opposite and working quite nicely :).

This is the point (from the Debian guide quoted above) that I meant was not working:

This allows several versions of the shared library to be installed at the same time, allowing installation of the new version of the shared library without immediately breaking binaries that depend on the old version.

If there's still benefit to having it as-is then that's fine too.

(If breaking the ABI in a
release is a totally different question.)

I don't understand what you mean here.

That's on purpose and the correct behaviour. Otherwise the Soname
linking wouldn't work.

You're right, sorry I was thinking about something else.

@wjwwood
Copy link
Contributor

wjwwood commented May 8, 2017

@simonschmeisser

I haven't used bloom so far, but I assume we do not have any information about the libraries and their so-name we are packaging? If that's the case I think this is as good as it will get (unless we want to rewrite/extend dh_makeshlibs)

We do not have information on the libraries or their so-name that are being packaged, so bloom cannot automate this process. However, you can modify the control and rules files generated by bloom and add any information you like there.

@jspricke
Copy link
Contributor Author

jspricke commented May 8, 2017 via email

@wjwwood
Copy link
Contributor

wjwwood commented May 8, 2017

I don't think changing the ABI in a release is a good idea, as it will
break all downstream projects (doing so explicitly by changing the
Soname is good, though). But that's a question for Moveit/OSRF.

I agree, and we try to avoid it on the core packages, but we do not wish to enforce that for all ROS packages. So it is up to each individual maintainer to work with their users to meet their needs with respect to ABI compatibility. I know that's not ideal, but it's a compromise to make it easier to release packages for novice maintainers until we have better tools for specify this information in a cross-platform way and a way to enforce ABI compatibility on some packages but not others.

@ros-discourse
Copy link

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/no-version-constraints-in-package-dependencies/28677/1

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.

None yet

4 participants