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

Revert "Update python-lxml key." #31570

Closed

Conversation

k-okada
Copy link
Contributor

@k-okada k-okada commented Dec 23, 2021

Reverts #31554

Canonical still supports xenial/kinetic https://ubuntu.com/engage/ros-kinetic-eol , I think we should revert this PR.
In addition to that, if we write like

  ubuntu:
    bionic: [python-lxml]
    focal: [python-lxml]

we need to update distribution key every 2 year, so I think

  ubuntu: [python-lxml]

is better

@k-okada k-okada requested a review from a team as a code owner December 23, 2021 10:03
k-okada added a commit to k-okada/jsk_3rdparty that referenced this pull request Dec 23, 2021
k-okada added a commit to k-okada/jsk_3rdparty that referenced this pull request Dec 23, 2021
@708yamaguchi
Copy link
Contributor

The same problem occurs in python-numpy.

#31552

k-okada added a commit to k-okada/jsk_3rdparty that referenced this pull request Dec 23, 2021
@nuclearsandwich
Copy link
Member

I was the contributor of the initial pull requests and have several more in flight which continue to remove Trusty or Xenial-specific definitions.

I think a better solution would be for the Kinetic packages provided by Canonical to provide and use separate rosdep sources which are maintained along with continued Xenial support. They could start, and possibly finish, with a snapshot of the rosdep database from April of last year. Since Canonical don't need to support other platforms beyond Ubuntu Xenial, they could generate additional rosdep defnitions for every package in Xenial and include that as a supplemental rosdep source.
This would allow us to continue maintaining the official rosdep database without requiring us to maintain keys for platforms we can no longer easily inspect via http://packages.ubuntu.com

we need to update distribution key every 2 year, so I think ubuntu: [python-lxml] is better

In general we prefer the most condensed valid expression of definitions for a rosdep key. However omitting the operating system version requires that the package be available by that name in all supported distributions and python-lxml is not available in Jammy which is a coming online as a target platform with #31563. Other pull requests introduced such as #31562 and #31561 introduce elisions where they're valid. When bionic and focal are both out of support we'll remove the python-lxml key entirely. It is not expected to grow or change at this point.

@cottsay
Copy link
Member

cottsay commented Dec 23, 2021

Just like we do with snapshot repositories, I recommend you target a snapshot of ros/rosdistro for rosdep keys. I recommend you remove your 20-default.list from /etc/ros/rosdep/sources.list.d and add something like this to target a snapshot: https://gist.github.com/cottsay/b27a46e53b8f7453bf9ff637d32ea283

You should be able to continue using rosdep as usual.

@k-okada
Copy link
Contributor Author

k-okada commented Dec 24, 2021

ok, if you'd like to go this direction, please support something like rosdep init --source-list-url https://gist.githubusercontent.com/cottsay/b27a46e53b8f7453bf9ff637d32ea283/raw/476b3714bb90cfbc6b8b9d068162fc6408fa7f76/30-xenial.list

https://github.com/ros-infrastructure/rosdep/blame/e52c3296ec59992960fbb0e89d1367ea46cd0dfa/src/rosdep2/main.py#L595-L599

@knorth55
Copy link
Contributor

knorth55 commented Dec 24, 2021

@nuclearsandwich @cottsay

I'm so sorry, but I don't understand your issues for supporting jammy clearly.
python-numpy key has bad effect in jammy?
rosdep with ROS_PYTHON_VERSION cannot avoid the issue?

I also want to know the concrete examples of the bad effects of old keys like python-numpy.

@knorth55
Copy link
Contributor

knorth55 commented Dec 24, 2021

@nuclearsandwich @cottsay

IMO, I want to keep the old rosdistro keys.
Below is my reason.

Our robots are running with ROS Indigo and Kinetic in private network.
The robots are protected in private network, so it is somehow safe.
of course, we want to upgrade to new distro, but we don't have enough time to do it (I think many university's labs are in the similar situation.)

Our softwares are open-source, maintained on github and running CI with rosdep and ros/rosdistro.
Because our softwares are used for our robots with ROS indigo and kinetic, it is really important to check the backward compatibility of our software in EOL distro.
So our CI system build and check our softwares in both new distro and EOL distro.

You recommended to use snapshot of rosdistro for old EOL distros,
but it will require changes in our CI system for EOL distros.
Also, for checking the software in new distro and EOL distro,
when we add new dependencies to our softwares,
we will want to have backport commit in ros/rosdistro for EOL distros.

In that case, some developers may start using their own fork rosdistro for backport checking in EOL distros, which causes more complexity.
If some software starts using forked rosdistro in their CI system, it may cause more troubles in releasing sync, too.

IMO, some part of ros releasing system relies on the policy that everyone should use the latest main branch of ros/rosdistro for the CI system of their sofwares.
I'm probably too nervous, but this ros/rosdistro policy change, allowing using their own rosdistro version or fork as they want, may cause a lot of troubles...

I don't understand the bad effect of old keys clearly,
but I guess there is some problems, which cannot be solved by ROS_PYTHON_VERSION.
So my idea is supporting new feature like condition="ROS_DISTRO>=noetic" in rosdep and package.xml.

In conclusion, for CI and releasing system, I think we should share and use the same and latest ros/rosdistro, and we can solve the issue of jammy by changing rosdep.

cc. @smits

@knorth55
Copy link
Contributor

I also want @smits to join this discussion, too.

@smits
Copy link
Contributor

smits commented Dec 24, 2021

As a company we are still using xenial/kinetic. We understand that both are EOL, but following the mantra don't fix what ain't broken, we have been pushing upgrading to noetic forward as long as we can. We've done an initial trial of porting our product to bionic/noetic in May 2021 but after 2 weeks of porting and testing, we were still being confronted with missing dependencies and changed behaviour, which is why we stopped as there was zero added value for our customers and only a lot more work and uncertainty ahead.

Our CI and CD stopped working since 2 days ago, we will be forced to find a solution fast which will probably be what @cottsay proposes if the community decides to move on and fully drop support for xenial/kinetic.

@nuclearsandwich
Copy link
Member

I'm so sorry, but I don't understand your issues for supporting jammy clearly.
python-numpy key has bad effect in jammy?
I also want to know the concrete examples of the bad effects of old keys like python-numpy.

The python-numpy package is not present in Ubuntu Jammy. You can see the package listed for focal and bionic but it is not present in jammy (link 404s) because the python2 version of the package has been dropped. If we left the key defined for all Ubuntu distributions then attempts to use rosdep for packages defining those platforms would result in package installation failures rather than correctly reporting that the key has no definition for that platform.

rosdep with ROS_PYTHON_VERSION cannot avoid the issue?
So my idea is supporting new feature like condition="ROS_DISTRO>=noetic" in rosdep and package.xml.

Conditional dependencies help ease the transition between ROS and ROS 2 and the ROS transition between Python 2 and Python 3 in non-distribution-specific manner. Part of the reason for these conditionals was to avoid using distribution names entirely as there are community distributions out there both based on official distributions and completely independent which are able to make use of these conditions without requiring any patches to the ROS infrastructure code.

Using conditions on the distribution name does not solve any problems that aren't already better addressed by ROS_PYTHON_VERSION.

Let's consider an example:

<depend condition="ROS_PYTHON_VERSION==2">python-numpy</depend>
<depend condition="ROS_PYTHON_VERSION==3">python3-numpy</depend>

Using rosdep to resolve a package with these dependencies will attempt to resolve python-numpy when ROS_PYTHON_VERSION==2 and python3-numpy when ROS_PYTHON_VERSION==3.
However, if you are trying to resolve dependencies for the ROS_PYTHON_VERSION=2 case and python-numpy (for Python 2) is not available on your target platform then there are many advantages to the rosdep definition accurately reporting that no definition for the key is found. For one, it prevents rosdep from trying to install an invalid package, it also prevents bloom from using rosdep to generate a package which will eventually fail on the build farm.

When dependencies evolve from platform to platform (and thus from ROS distribution to ROS distribution) rosdep is the conditional mechanism by which those dependencies are selected. The problem is not a lack of conditions to cover scenarios. It is that Debian, Ubuntu, and ROS are all evolving projects with maintenance burdens and the older distributions need to give way so maintainer effort can be focused on the newer ones. The rosdep database is hard to maintain, contains at least 400 known inaccuracies (based on the rosdep_repo_check tool) which I've been steadily working to fix. But part of that fix involves cleaning up keys for platforms that we no longer support in order to both retain the accuracy of the database for supported platforms and reduce the amount of information dedicated to unsupported platforms.

Official ROS distributions are not the only ROS distributions out there and I would be extremely hesitant to assign any kind of ordinality to named ROS distributions for a multitude of reasons. The current convention for ROS projects is to maintain different branches to support different distributions when needed and that should remain the case for dependency changes as with other source changes.

we will want to have backport commit in ros/rosdistro for EOL distros.

Changes to Kinetic and any inactive distribution in this repository are blocked. So backports into Kinetic itself using the official rosdistro database are not possible.#31570 (comment)
If you are making changes to the rosdep database which is also hosted here for platforms like Ubuntu Xenial, those changes are only reviewed and tested for supported platforms and we cannot commit to maintaining the integrity of the rosdep database for platforms we no longer support. I will echo @cottsay's comment that if you are using Kinetic and Xenial in a static fashion, updating your rosdep sources to point to a static rosdep db is the best way to guarantee that it will continue to work.

If you're still making changes and adding further dependencies to an unsupported distribution, my recommendation is again, use static upstream rosdep sources and then add an additional custom source for the additional dependencies you require.
The ROS team cannot sustainably manage a universal rosdep db for all unsupported platforms. Individual projects can much more readily maintain add-on sources, reduced in scope to just their target platform and any further packages they require and I expect many teams keeping Xenial/Kinetic projects running may not be adding dependencies will thus not need to make any additions beyond what was defined for Xenial in April 2021.


In order to reach consensus without half of the participants currently out on a limb, I've opened #31613 to tempporarily redefine the recently changed keys for Ubuntu Xenial which effectively closes this PR. However I want to keep the discussion going. I see that @smits opened #31569 and I think we should continue the discussion there. I'll add a link back to this pull request and continue responding there.

@cottsay
Copy link
Member

cottsay commented Dec 28, 2021

This was temporarily resolved by #31613, and discussion continues in #31569.

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

6 participants