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

skip mypy test on centos 7 (alternative) #248

Closed
wants to merge 1 commit into from
Closed

Conversation

mikaelarguedas
Copy link
Member

Replaces #246 and uses rospkg to avoid duplicating os-release parsing logic.

Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com>
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

Works for me!

@mikaelarguedas
Copy link
Member Author

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

@clalancette
Copy link
Contributor

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
Copy link
Member Author

mikaelarguedas commented Dec 17, 2020

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
Copy link
Member

kyrofa commented Dec 17, 2020

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

@clalancette
Copy link
Contributor

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
Copy link
Member Author

@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
Copy link
Member

cottsay commented Mar 29, 2021

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
Copy link
Member Author

Replaced by #258

@clalancette clalancette deleted the skip_centos branch April 13, 2021 20:36
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.

4 participants