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 the mypy testing on CentOS 7. #246

Closed
wants to merge 2 commits into from

Conversation

clalancette
Copy link
Contributor

@clalancette clalancette commented Dec 2, 2020

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.

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

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@artivis
Copy link

artivis commented Dec 2, 2020

@Arnatious fyi

@mikaelarguedas
Copy link
Member

Is the problem also present on CentOS 8 ?

@kyrofa
Copy link
Member

kyrofa commented Dec 2, 2020

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

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

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.

🤔 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
Copy link
Contributor Author

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.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@clalancette
Copy link
Contributor Author

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

@mikaelarguedas
Copy link
Member

Thanks for the update. I opened #248 to avoid implementing a custom parsing logic for the os-release file, PTAL

@clalancette
Copy link
Contributor Author

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

@clalancette clalancette closed this Dec 3, 2020
@clalancette clalancette deleted the disable-mypy-on-centos7 branch December 3, 2020 13: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.

4 participants