ros-visualization / rviz

ROS 3D Robot Visualizer
BSD 3-Clause "New" or "Revised" License
831 stars 463 forks source link

[ROS-O] rviz exports patch version number in rviz_QT_VERSION #1773

Closed v4hn closed 1 year ago

v4hn commented 1 year ago

RViz exports the full version of qt with which it is compiled. That is usually not harmful, but @jspricke maintains the Debian package for rviz and does not rebuild all packages with each patch release for Qt (API/ABI must not be changed in patch releases, so this is the correct thing to do). However, as soon as the current patch version of qt is updated in Debian all downstream packages of rviz (for me notably moveit_ros_visualization and rviz_visual_tools) break because they use the full exported version to find the rviz-compatible rviz version.

The three options are

  1. export only major.minor version in rviz_QT_VERSION in rviz
  2. extract major.minor from the variable in downstream packages
  3. force a package rebuild of rviz whenever upstream qt components receive patch versions.

I do not really care which solution we implement, but let's fix this please @rhaschke @jspricke

jspricke commented 1 year ago

I think exporting only major.minor is better (if that really reflects the ABI version), this also seems to be the intended behavior back then: https://github.com/ros-visualization/rviz/pull/980#issuecomment-203815636

roehling commented 1 year ago

Can you be a bit more specific why the build fails? Because find_package(Qt5 5.15.6) (for example) will happily accept any Qt version which has version 5.15.6 or higher, so I don't see how limiting the asked version to major.minor would make any difference at all.

rhaschke commented 1 year ago

I was fine to limit the exported version to major.minor. However, given Timo's comment, this shouldn't be needed.

v4hn commented 1 year ago

Sadly I updated my debian testing system state in between and the inconsistency is gone. I suspect the qt5 version of the librviz-dev build was actually newer than the system installed version, as I can't see any other way to trigger the error, but according to @jspricke that's impossible. :shrug:

There are also some tutorial repositories in noetic that use EXACT with the find_package call, but that's an error on their side and cannot be fixed here.

So while I would still propose to move to major.minor, the actual error w.r.t. the debian packages is gone for now.

roehling commented 1 year ago

I'd advise against the move. The find_package() documentation says that the decision if a requested version is compatible to the installed one is the responsibility of the package, not its user. Adding implicit assumptions about the version number semantics only creates technical debt and additional work when that assumption is violated.

Regarding your bug, we have had earlier instances of build failures with ROS packages where catkin leaked fully qualified, hard-coded library file names such as /usr/lib/x86_64-linux-gnu/libfoo.so.1.2.3 into the _LIBRARIES variable, which had been resolved at build time and would obviously stop working after an upgrade. This sounds suspiciously familiar.

rhaschke commented 1 year ago

Given the last comments, I will close this issue for now.

jspricke commented 1 year ago

I suspect the qt5 version of the librviz-dev build was actually newer than the system installed version, as I can't see any other way to trigger the error, but according to @jspricke that's impossible.

Actually that was the case, sorry for mixing that up. rviz was recompiled against the new QT and migrated to testing, due to ABI compatibility, but QT only migrated some weeks later.

I wonder if we should change it to some generic string (5?) in Debian cause ABI compatibility is already enforced by apt. @roehling what do you think?

roehling commented 1 year ago

In an ideal word, librviz-dev should probably depend on qtbase5-dev (>= ${full_qt_version}), because even though rviz itself may not require the Qt headers directly, most useful plugins do.

rhaschke commented 1 year ago

Of course, rviz exposes Qt headers. And sure, rviz depends on qtbase5-dev: https://github.com/ros-visualization/rviz/blob/6a234b4404c5ee71051f0b338ab8d5d2d931c27b/package.xml#L24

But, I don't see a possibility to add the current Qt version there.

jspricke commented 1 year ago

Of course, rviz exposes Qt headers.

True, thanks. I was just blind.

But, I don't see a possibility to add the current Qt version there.

We have this in Debian now:

https://salsa.debian.org/science-team/ros-rviz/-/compare/95311dc...8ca36ae?from_project_id=4114

I don't think there is support for something like that in bloom.

rhaschke commented 1 year ago

https://salsa.debian.org/science-team/ros-rviz/-/compare/95311dc...8ca36ae?from_project_id=4114

Interesting link! @jspricke, can you provide (or link to) more information about this effort releasing ROS1 into future Debian/Ubuntu distros (beyond Focal)? How is driving this? Which distros are targeted? Is there an automatic process to build .debs from released package sources?

jspricke commented 1 year ago

can you provide (or link to) more information about this effort releasing ROS1 into future Debian/Ubuntu distros (beyond Focal)?

There is a long running focus on getting ROS into Debian (and with this into Ubuntu), see my talk at ROSCon 2015:

https://vimeo.com/142151399#t=1754s

And the Debian wiki:

https://wiki.debian.org/DebianScience/Robotics/ROS

For example rviz is installable out of the box on every Debian/Ubuntu since 2016:

https://tracker.debian.org/pkg/ros-rviz

And it is also referenced in the ROS wiki:

http://wiki.ros.org/UpstreamPackages

How is driving this?

Nowadays mostly Timo and me, we are welcome more contributors, see:

https://wiki.debian.org/Teams/RoboticsTeam

It is also closely coupled to the ROS-O effort Michael gave a talk at ROSCon 2022.

Which distros are targeted?

Not sure I understand that question. Being part of Debian means it is part of any Debian release (as long there is no release critical bug at release time) and also any downstream distribution.

Is there an automatic process to build .debs from released package sources?

No, all packages in Debian are maintained by a Debian contributor and manually uploaded. The process is highly automated, though. Like we have tooling to get informed of new upstream version, integrate it into the Debian git, test and upload.