ros2 / sros2

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

Skip the mypy testing on CentOS 7. #246

Closed clalancette closed 3 years ago

clalancette commented 3 years ago

The inline comment has a lot more details on why we need to do this.

Signed-off-by: Chris Lalancette clalancette@openrobotics.org

This should fix up the last of the lingering errors on the CentOS job: https://ci.ros2.org/view/nightly/job/nightly_linux-centos_debug/611/

I'll run CI on this once I get agreement that this is the right way to go.

artivis commented 3 years ago

@arnatious fyi

mikaelarguedas commented 3 years ago

Is the problem also present on CentOS 8 ?

kyrofa commented 3 years ago

Yeah +1 on the change in general, but I second @mikaelarguedas's question. This is an unversioned check, are we sure this is broken across all centos releases?

clalancette commented 3 years ago

Yeah +1 on the change in general, but I second @mikaelarguedas's question. This is an unversioned check, are we sure this is broken across all centos releases?

No, I'm not sure of that. But my thinking here is that we are already doing static analysis of the mypy types on Ubuntu, macOS, and Windows. There isn't really benefit to also running it on CentOS (regardless of the version), in my opinion.

mikaelarguedas commented 3 years ago

I'll run CI on this once I get agreement that this is the right way to go.

I'm no up-to-date regarding the CentOS story. AFAICT from the REPs no current or upcoming ROS distro targets that platform. And one could assume that if a future ROS distro starts targeting CentOS it may target the one from 2019 and not from 2014, won't it ?

Is the goal of this PR that CentOS users compiling from source can run tests locally ? or that the nightly job becomes green ?

If the former I'd argue that this is a reason for making the patch as narrow as necessary so that users can run all tests compatible with their setup.

If the latter and following your point:

But my thinking here is that we are already doing static analysis of the mypy types on Ubuntu, macOS, and Windows. There isn't really benefit to also running it on CentOS (regardless of the version), in my opinion.

:thinking: Would it be fair to say that all linter tests can be skipped on CentOS as they are ran on all the other platforms ? If that is the case, an alternative is for the nightly jobs to pass de relevant flag for skipping those without having to add workaround code in various repos (that will then need to be maintained in multiple places).

clalancette commented 3 years ago

AFAICT from the REPs no current or upcoming ROS distro targets that platform. And one could assume that if a future ROS distro starts targeting CentOS it may target the one from 2019 and not from 2014, won't it ?

I would think that we would target CentOS 8 instead of 7 going forward, yes. But as of today, the buildfarm is testing CentOS 7 nightly.

Is the goal of this PR that CentOS users compiling from source can run tests locally ? or that the nightly job becomes green ?

A little bit of both, but mostly to make the nightly job green.

Would it be fair to say that all linter tests can be skipped on CentOS as they are ran on all the other platforms ? If that is the case, an alternative is for the nightly jobs to pass de relevant flag for skipping those without having to add workaround code in various repos (that will then need to be maintained in multiple places).

That is a good point; we could do that. If the problems were widespread, I'd probably consider doing that instead. But in this case, this is the only test that is causing problems on CentOS, so a more targeted fix seems appropriate.

Overall, I'm hearing that we should narrow this to CentOS 7. So I'll go ahead and do that for now.

clalancette commented 3 years ago

239f197 makes it only skip the test on CentOS 7.

mikaelarguedas commented 3 years ago

Thanks for the update. I opened https://github.com/ros2/sros2/pull/248 to avoid implementing a custom parsing logic for the os-release file, PTAL

clalancette commented 3 years ago

248 looks good to me, so closing this one out.