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

Pin version, or drop 'coverage' re Python 2-3 move #2325

Closed
phillxnet opened this issue Nov 18, 2021 · 4 comments · Fixed by #2326
Closed

Pin version, or drop 'coverage' re Python 2-3 move #2325

phillxnet opened this issue Nov 18, 2021 · 4 comments · Fixed by #2326

Comments

@phillxnet
Copy link
Member

Thanks to @FroggyFlox for highlighting this issue. We currently depend upon coverage and do not specify a version. However the more recent versions of coverage have dropped Python 2.7 compatibility.

See our own issue regarding doing the same here: "move to python 3" #1877

References:
Coverage changelog: https://coverage.readthedocs.io/en/latest/changes.html#version-6-0b1-2021-07-18

It is proposed that we, for the time being, either:

  1. Pin our dependency to our last know working version of 5.5 (2021-02-28). Which equates to the last 'stable' version released given that from the above 5.6b1 release notes we have: "These changes are part of 6.0."
  2. Drop this dependency until we are the other side of our own Python 2 to 3 transition move to python 3 #1877.
@FroggyFlox
Copy link
Member

Thanks, @phillxnet !

Personally, I would vote for option 1 as dropping it might have repercussions that we might not see right away... It seems safer to me and we can always revisit our "pinnings" once #1877 is behind us (it is my understanding that such revisiting will be required for #1877 anyway).

@phillxnet
Copy link
Member Author

@FroggyFlox Thank. And initially I was favouring this option also. But my current incomplete understanding of our prior coverage use was for the raw data of for example the now defunct sub domain of coverage. I.e. see the following forum post:
https://forum.rockstor.com/t/code-test-coverage-report/99

And given we have quite a few changes in-the-works I was hoping that was it's only reason to have been included.

Looking into this now and will report back here when I have a little more background. It it is indeed unused currently then I think we would be best to go for option 2.

@phillxnet
Copy link
Member Author

@FroggyFlox It's addition dates back to May 4, 2015 via the following commit:

f95b797#diff-60f61ab7a8d1910d86d9fda2261620314edcae5894d5aaa236b821c7256badd7

@phillxnet
Copy link
Member Author

I can also find no other mention of it's use via:

grep -R 'coverage' * | less

I'm currently working on the assumption this is an optional test coverage metrics generation tool and given we are not currently using it to generate any results (i.e. in the way we run our tests for example) we can safely remove it until we are ready to re-add it post our python 2 to 3 migration. Cutting down on the moving parts. At that point we can re-assess if coverage works for us. And it does look like a nice tool. But less moving parts, especially ones we aren't using, has to be the way to go during a major transition point. And given we have a build breakage here this is a fairly significant issue.

I'll create a pr and use it to test build scenarios.

phillxnet added a commit to phillxnet/rockstor-core that referenced this issue Nov 18, 2021
…2325

Although version 5.5 still has Python 2.7 compatibility we are not
currently making use of coverage's facilities as we once did a few years
ago. So remove for the time being and consider re-adding post our own
Python 2 to 3 migration.
phillxnet added a commit that referenced this issue Nov 21, 2021
…verage'_re_Python_2-3_move

Remove coverage library as unused and now requires Python 3 #2325
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 a pull request may close this issue.

2 participants