search

ros2 / sros2

tools to generate and distribute keys for SROS 2
Apache License 2.0
89 stars 44 forks source link

skip mypy test on centos 7 (alternative) #248

Closed mikaelarguedas closed 3 years ago

mikaelarguedas commented 3 years ago

Replaces https://github.com/ros2/sros2/pull/246 and uses rospkg to avoid duplicating os-release parsing logic.

mikaelarguedas commented 3 years ago

looks like CI doesn't install dependencies based on package.xml data. @clalancette is it possible to get rospkg installed on CI machines ?

clalancette commented 3 years ago

looks like CI doesn't install dependencies based on package.xml data. @clalancette is it possible to get rospkg installed on CI machines ?

Ah, I forgot about that. While I could certainly open a PR to https://github.com/ros2/ci to add rospkg to the list of pip packages to install, we would also have to update the documentation so that users would still be able to build and test from source. While we can certainly do this, I'm wondering if this is worth it. It seems we have 3 alternatives right now:

  1. Install the rospkg pip package in CI and in the documentation. Then we can merge this PR.
  2. Do nothing, and just let CentOS stay yellow on the buildfarm.
  3. Go back to my previous PR, which just uses pure Python. It is a bit of duplication for code that is already there, but it isn't really super complicated.

I'd go for 3) personally, but I'd like to hear other opinions.

mikaelarguedas commented 3 years ago

we would also have to update the documentation so that users would still be able to build and test from source

could the documentation use rosdep for that?

Does the fact that Red Hat announced discontinuation of CentOS impact the final decision? If it results in no plans for official ROS support for CentOS in the near future I'd vote for 2). Otherwise 1) or 3) with a preference for 1) as it seems that there is already more code than people can maintain and managing dependencies will benefit from being streamlined (but 3 is fine as well if this is the preferred option on OR's side)

kyrofa commented 3 years ago

Ditto @mikaelarguedas's thoughts. Preference for (2), not sure it makes sense to invest in a platform that is disappearing.

clalancette commented 3 years ago

Does the fact that Red Hat announced discontinuation of CentOS impact the final decision?

I don't think so. As far as I know, CentOS is not discontinued; it is just changed. Also, there are several new distributions popping up to fill the place that CentOS occupied. So while CentOS as it is today may go away, I think that there will be some RHEL-compatible clone (or RHEL itself) that we target.

mikaelarguedas commented 3 years ago

@clalancette thanks for clarifying. Is there a path forward for 1) (streamlining installing deps using rosdep) or do you believe we have to fall back to 3) (duplicate rospkg logic in this package) ?

cottsay commented 3 years ago

Since the problem this PR is trying to avoid is actually caused by the use if importlib_resources over importlib.resources, which could absolutely be a problem on any other platform that doesn't have Python 3.9, wouldn't it be better to base this test off from that? You could avoid the rospkg dependency and do the detection using the same import logic that manifests the bug.

Also, RHEL 8 is also using Python < 3.9 and is therefore using the importlib_resources package as well.

mikaelarguedas commented 3 years ago

Replaced by #258