ros-infrastructure / bloom

A release automation tool which makes releasing catkin (http://ros.org/wiki/catkin) packages easier.
Other
58 stars 95 forks source link

Package version comparisons generated by the debian generator are broken #455

Open meyerj opened 6 years ago

meyerj commented 6 years ago

The debian generator of bloom translates version dependencies found in package.xml files to version restrictions in the Debian control file, without any interpretation of the version string: generators/debian/generator.py:197.

While this approach works fine in the common case of depending on a minimum version of a ROS package like in

<depend version_gte="1.12.12">roscpp</depend>

which is translated to

Depends: ros-kinetic-roscpp (>= 1.12.12)

for the control file, it is basically broken for version_lte, version_eq or version_gt because only very few Debian or Ubuntu packages have version numbers that exactly follow the major.minor.patch version scheme. Version specification and comparison on Debian packages is more complex, see https://www.debian.org/doc/debian-policy/#s-f-version for reference.

Examples that would fail or behave unexpectedly:

(The dummy package foo has been generated with equivs to emulate a control file generated by bloom.)

In the version_gte or version_lt case the generated comparison probably behaves as expected for most packages, but only in case the respective Debian version does have a zero or unspecified epoch.

Am I missing something here? Or is it allowed to specify exact Debian version strings in package.xml? The latter would break platform-independence and probably some other tools.

I am not sure what would be the best way to fix this, but maybe bloom should at least emit a warning if a package specifies version_eq or version_lte dependencies because they most likely cannot be satisfied and the generated debian package cannot be installed.

wjwwood commented 6 years ago

Am I missing something here?

No I think we just have never used anything other than version_gte in practice. It sounds practically broken, but it's not something we can easily fix in bloom I think, and is just a side-effect of how we do versions in our debs.

Or is it allowed to specify exact Debian version strings in package.xml?

No the idea is that you do not do that, but maybe @dirk-thomas or @tfoote disagree with me there.

I am not sure what would be the best way to fix this, but maybe bloom should at least emit a warning if a package specifies version_eq or version_lte dependencies because they most likely cannot be satisfied and the generated debian package cannot be installed.

That's probably a good idea. I don't have time to do it, but if you or someone else has time to make a pr for that I think we'd consider it, at the very least until we can come up with a fix.

tfoote commented 6 years ago

Yeah, gte is the only one I know of using in practice to enforce that a new required feature has been made available.

Or is it allowed to specify exact Debian version strings in package.xml?

No the idea is that you do not do that, but maybe @dirk-thomas or @tfoote disagree with me there.

The package.xml needs to be platform independent. We generate into rpm, arch, openembedded etc.

The goal of these statements is to do the package version comparisions on the release versions only ignoring the build number since the package cannot know about the build number. Is there a way we can tell debian to limit the comparison to just the package version?

nuclearsandwich commented 6 years ago

I forgot that I'd asked @j-rivero for suggestions here to see if there was a debian version comparator that would serve us but there unfortunately isn't one.

One possible way to handle version_eq would be to expand one package.xml version constraint to two debian version constraints. For example

<depend version_eq="X.Y.Z">roscpp</depend>

could be made to expand to

Depends: ros-kinetic-roscpp (>= X.Y.Z) ros-kinetic-roscpp (<< X.Y.Z+1)

I don't have a suggestion for the lte case but I agree that printing a warning of possible consequences when these conditions are used is a good idea.

tfoote commented 6 years ago

Couldn't the following mappings be made to work where +1 -1 is actually mutating the number in the debian generator. lte: << X.Y.Z+1 lt: << X.Y.Z gt: >=X.Y.Z+1 gte: >=X.Y.Z eq: >=X.Y.Z && << X.Y.Z+1

This will assume that the smallet increment is a patch release. But I think that's the basic philosophy of this platform agnostic approach.

meyerj commented 6 years ago

I assume that what @tfoote proposed would work and was thinking about the same.

Version comparisons for non-ROS packages that have a non-zero epoch are still broken (see zlib case above), but I guess there is no simple solution and tools like dpkg-shlibdeps will already add the correct dependencies including epoch if necessary.