build: Remove tox constraint. #354
Conversation
feanil
force-pushed
the
feanil/remove_tox_constraint
branch
2 times, most recently
from
9281bb7 to
19e3738
Compare
| @@ -21,7 +21,3 @@ elasticsearch<7.14.0 | |||
|
|
|||
| # django-simple-history>3.0.0 adds indexing and causes a lot of migrations to be affected | |||
| django-simple-history==3.0.0 | |||
|
|
|||
| # tox>4.0.0 isn't yet compatible with many tox plugins, causing CI failures in almost all repos. | |||
There was a problem hiding this comment.
... causing CI failures in almost all repos.
This scares me, and seem like a good reason to have a common constraint. Did you confirm that a repo that was failing on 4.0.0 now works with the latest upgrade, and that we can reasonably expect that it is now passing for most repos?
There was a problem hiding this comment.
Unfortunately, it's nearly impossible to test this because of how the common constraints are setup in a repo. You can't actually remove the constraint at the repo level without make upgrade automatically adding it back before it rebuilds requirements.
That said, this could be added to the constraints at the repo level and my plan was to do a big announce so that maintainers are prepared to do that if they see issues. As I said in the description, most of our repos have tox-battery as a dependency and that already constrains tox<4.0.0 so I'm less concerned about this breaking everything like it did when tox 4.0.0 was first released. (At that time tox-battery did not have its constraint on tox<4.0.0.
There was a problem hiding this comment.
We could use this hacky solution on a branch just for testing: openedx/edx-platform@56d12bf. I think it would be useful. If we discover a problem that is easily fixable, but that all or many repos might run into, it might be nice to provide the solution rather than having everyone add constraints, and then having the same problem we have now (a constraint where we don't want one), only a much more global version of it. Thoughts?
There was a problem hiding this comment.
Also see #355 which I just moved from Jira. This ticket is for fixing this common constraints issue.
There was a problem hiding this comment.
@robrap good call on digging further into this. I had missed the fact that 0.6.2 of tox-battery(the version that constrains tox) has been merged but has not been released. I think the real path forward is gonna have to be dropping the dependency on tox-battery everywhere first and then dropping this common constraint.
There was a problem hiding this comment.
Cool. Thanks for digging further. Good luck. Should this ticket be closed for now, or will you block on some new issue?
There was a problem hiding this comment.
I think the order of the work changes, this is now blocked on fixing tox-battery to publishing a new version or for us to drop our dependency on tox-battery in all repos(This might be the fastest way forward.)
There was a problem hiding this comment.
Sounds good. Are you planning on ticketing and pushing forward the tox-battery work? Was this all just inform for me at this point? Thanks.
The `tox` package is already 6 minor revisions ahead at 4.6.4. If there are still plugins that don't support 4.x.x, then they are likely stagnant and need to be removed or updated ourselves. However, as long as we keep this constraint here, we can't easily find and fix those issues. In many of the cases, this constraint was added due to the incompatibility of tox-battery with tox 4.x.x. However, tox-battery has updated its `install_requires` to be explicit of this dependency. https://github.com/signalpillar/tox-battery/blob/master/setup.py#L20 Another issue we're running into is that some of the dependencies of tox are starting to publish security vulnerabilities. It's lower risk since this is in dev and CI but leaving this as is will increase security noise making it harder to respnod to real signals. Specifically, tox<4.0.0 depends on a version of `py` which has a security vulnerability. Dependabot is picking this up and making some noise in a lot of our repos.
feanil
force-pushed
the
feanil/remove_tox_constraint
branch
from
19e3738 to
635e31c
Compare
|
We have an issue on our board for this and was pending due to the team focusing on the Django upgrade, but now we have prioritized this and will work on first upgrading tox in edx-platform and then remove this constraint. Also this constraint removal work is blocked this issue edx/edx-arch-experiments#130 |
|
This got fixed in a different PR. |
The
toxpackage is already 6 minor revisions ahead at 4.6.4. If thereare still plugins that don't support 4.x.x, then they are likely
stagnant and need to be removed or updated ourselves.
However, as long as we keep this constraint here, we can't easily find
and fix those issues. In many of the cases, this constraint was added
due to the incompatibility of tox-battery with tox 4.x.x. However,
tox-battery has updated its
install_requiresto be explicit of thisdependency.
https://github.com/signalpillar/tox-battery/blob/master/setup.py#L20
Another issue we're running into is that some of the dependencies of tox
are starting to publish security vulnerabilities. It's lower risk since
this is in dev and CI but leaving this as is will increase security
noise making it harder to respnod to real signals.
Specifically, tox<4.0.0 depends on a version of
pywhich has asecurity vulnerability. Dependabot is picking this up and making some
noise in a lot of our repos.
Closes openedx/public-engineering#203
Process Note
Before merging this, I'll be posting a detailed message on Discourse(cross posted to slack) to let people know it's coming, what to look for and how to fix it locally in their repos. That should allow people to fix forward locally without the need for the global constraint. This will also allow us to incrementally move forward and fix one or two repos at a time.