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

ignore Debian-specific error around urdfdom_headers #614

Merged

Conversation

v4hn
Copy link
Contributor

@v4hn v4hn commented Sep 23, 2024

Without the patch I see this building with the Debian ROS packages:

$ catkin_lint rviz_marker_tools/
rviz_marker_tools: package.xml: error: missing build_depend on 'urdfdom_headers'

Backstory for the error in catkin_lint: The python3-rosdep package defines this key although ROS does not do that from indigo to noetic anymore. However, ROS2 has a package - and thus a rosdep key - with that name again. So dropping the key entry from the Debian package is no real option assuming eventual support for ros2, but I also do not see another nice way for catkin_lint to support this check better. Thus, I discussed with @jspricke and we decided it might be best to nolint the error here.

Note that this prevents simple commits as catkin_lint without errors is a hard prerequisite for our pre-commit rules and I would like to keep it that way.

@roehling It seems catkin_lint does not support ignore_once missing_depend, which is why I had to disable the warning for the whole package. I seem to recall some issue about just this years ago, but I cannot find anything related. If relevant, I can also create an upstream issue of course.

@codecov-commenter
Copy link

codecov-commenter commented Sep 23, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 57.90%. Comparing base (6b0f2c8) to head (6c95679).
Report is 42 commits behind head on master.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #614      +/-   ##
==========================================
- Coverage   58.82%   57.90%   -0.92%     
==========================================
  Files          91      132      +41     
  Lines        8623    10665    +2042     
  Branches        0      956     +956     
==========================================
+ Hits         5072     6174    +1102     
- Misses       3551     4444     +893     
- Partials        0       47      +47     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@rhaschke
Copy link
Contributor

Why do you use python3-rosdep2 in first place? It is known that this is incompatible with ROS's rosdep.
https://answers.ros.org/question/353082/missing-packages-after-installing-rosdep-based-on-python3-rosdep2-in-noetic

@v4hn
Copy link
Contributor Author

v4hn commented Sep 23, 2024

It is known that this is incompatible with ROS's rosdep.

Of course it is incompatible with OpenRobotics packages, but it's a very valid basis to build ROS on Debian as well as Ubuntu 22.04 and newer and I'm very much for supporting the Debian Science Robotics initiative so more packages might eventually migrate to proper Debian packages.

@rhaschke
Copy link
Contributor

I am not convinced merging this.

Why isn't the Debian Science Robotics initiative simply using the rosdep package from OpenRobotics?
I'm doing exactly that on https://ros.packages.techfak.net and it works perfectly.
Using an outdated and incompatible version causes many problems, for example the one you want to workaround here.

Dropping the key entry from the Debian package is no real option assuming eventual support for ros2.

I cannot comprehend that argument. Obviously, the Debian packages provide ROS1 support currently. If so, rosdep should report packages compatible to ROS1. If the Debian packages switch to ROS2 in future, it will be the right time to reintroduce the urdfdom_headers package in rosdep.

Finally, disabling dependency checking here for all packages is a relaxation of the linting rules, which I don't like.

@jspricke
Copy link
Contributor

Just answering for the Debian part here.

Why isn't the Debian Science Robotics initiative simply using the rosdep package from OpenRobotics?

Debian only allows packages from Debian as a dependency so we need rosdep to build other ROS packages in Debian.

Using an outdated and incompatible version causes many problems

I don't agree here and I offered multiple times to help making the OSRF package compatible with Debian but in the end I don't see a problem here.

I cannot comprehend that argument. Obviously, the Debian packages provide ROS1 support currently.

The Debian package supports ROS1 and 2.

@rhaschke
Copy link
Contributor

I don't see a problem here.

There are numerous issues on the web complaining about the incompatibility of rosdep versions provided by OpenRobotics and Debian. Not seeing a problem with this, is weird.
It is fine to provide a rosdep package via Debian, but it should not interfere with the standard ROS ecosystem, requiring workarounds like in this PR.

The Debian package supports ROS1 and 2.

Surely rosdep itself supports both versions. But that's not the point. Your specific config exports a certain set of package keys, which is valid only for a specific subset of ROS distributions. To support varying key sets in different distros, OpenRobotics provides a separate key file for each distro, which you obviously don't do.
Anyways, I was talking about the ROS distro provided by your ROS packages, e.g. ros-desktop-full. This currently seems to be ROS1 noetic or obese. The set of keys provided by rosdep should match that decision.

@jspricke
Copy link
Contributor

There are numerous issues on the web complaining about the incompatibility of rosdep versions provided by OpenRobotics and Debian. Not seeing a problem with this, is weird. It is fine to provide a rosdep package via Debian, but it should not interfere with the standard ROS ecosystem, requiring workarounds like in this PR.

I agree that there is a problem but to my understanding this can only be solved on the OSRF side and not on the Debian side as explained here: https://discourse.ros.org/t/upstream-packages-increasingly-becoming-a-problem/10902/2

Surely rosdep itself supports both versions. But that's not the point. Your specific config exports a certain set of package keys, which is valid only for a specific subset of ROS distributions.

It provides a set of package keys for a specific Debian distribution.

Anyways, I was talking about the ROS distro provided by your ROS packages, e.g. ros-desktop-full. This currently seems to be ROS1 noetic or obese. The set of keys provided by rosdep should match that decision.

That seems unrelated, for example the keys are fine for building ROS 2 as I did for example here: https://github.com/jspricke/ros-two-packages just that not all packages are in Debian (but there are a number of ROS 2 packages in Debian already).

@rhaschke
Copy link
Contributor

Your specific config exports a certain set of package keys, which is valid only for a specific ROS distro.

It provides a set of package keys for a specific Debian distribution.

Ok. I agree. Probably, you need to release your own rosdep package with your own key list to have a consistent setup within the Debian distro.

There are a number of ROS 2 packages in Debian already.

I didn't know that. Isn't mixing ROS1 and ROS2 packages in the same distro (and both installing to /usr) becoming chaotic? I think, quite often, both packages will install exactly the same binaries? I guess you resolve that by declaring corresponding Conflicts and Replaces.

The problem faced by @v4hn is that the MTC package he wants to build in the Debian distro originates from the Noetic distro, which doesn't declare the urdfdom_headers key, which - for this reason - isn't used in its package.xml file.

According to @v4hn you refrained from removing the urdfdom_headers key because you are also building ROS2 packages. I think, there is the fundamental problem of this approach: While rosdep keys are ROS distro-dependent (they obviously change over time, which might not be desirable as well), in the Debian distro - when mixing packages from several ROS distros (1&2) - you need to merge all required keys.

I think, having separate apt repositories and separate install locations for individual ROS distros, is a much cleaner approach to separate them. For example, this allows to install multiple distros (e.g. 1&2) side-by-side.

I'm afraid we cannot solve that issue. I suggest ignoring the catkin_lint issue, @v4hn (but w/o the technical help suggested in this PR as this relaxes the linting too much).

@jspricke
Copy link
Contributor

I didn't know that. Isn't mixing ROS1 and ROS2 packages in the same distro (and both installing to /usr) becoming chaotic? I think, quite often, both packages will install exactly the same binaries? I guess you resolve that by declaring corresponding Conflicts and Replaces.

Till now we did not upload packages that conflict (and I would like to avoid using Conflicts in that case). But we have packages that work with ROS 1 and 2. I have some plans how to make conflicting packages coexists but I don't have a use case right now to do the work.

I'm afraid we cannot solve that issue. I suggest ignoring the catkin_lint issue, @v4hn (but w/o the technical help suggested in this PR as this relaxes the linting too much).

+1.

@jspricke
Copy link
Contributor

Btw. afair it is possible to override the keys provided by Debian by either modifying the files in /etc/ros/rosdep or using $ROSDEP_SOURCE_PATH.

@roehling
Copy link

The latest catkin_lint release (1.6.24) supports ignores with instance restrictions, i.e.,

#catkin_lint: ignore missing_depend[pkg=urdfdom_headers]

@v4hn v4hn force-pushed the pr-ignore-missing-depend-urdfdom-headers branch from 7cbb199 to acfe53c Compare September 24, 2024 17:53
@v4hn
Copy link
Contributor Author

v4hn commented Sep 24, 2024

Updated to include @roehling newly supported syntax. Thank you so much ❤️

Copy link
Contributor

@rhaschke rhaschke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with that solution, because it is restrive enough.

rviz_marker_tools/CMakeLists.txt Show resolved Hide resolved
Without the patch:

$ catkin_lint rviz_marker_tools/
rviz_marker_tools: package.xml: error: missing build_depend on 'urdfdom_headers'

requires catkin_lint 1.6.24 for successful error suppression.
@v4hn v4hn force-pushed the pr-ignore-missing-depend-urdfdom-headers branch from acfe53c to 6c95679 Compare September 24, 2024 18:05
@rhaschke rhaschke merged commit d50c846 into moveit:master Sep 24, 2024
7 checks passed
@v4hn v4hn deleted the pr-ignore-missing-depend-urdfdom-headers branch September 30, 2024 12:08
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.

5 participants