-
Notifications
You must be signed in to change notification settings - Fork 389
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
Uncap urllib3 version on Python < 3.10 #777
base: master
Are you sure you want to change the base?
Conversation
Hi 👋 A <2 pin of urllib3 is causing some troubles for us too and it would be great if this PR could solve it. I wanted to ask @kevin1024 if this PR is actually solves the issue? How can I contribute to testing this out? It would be great if the solution to unpin urllib3 would be found and a new version of vcrpy released? Cheers and thanks 🙏 |
@dgw can you rebase your branch with master to run CI tests please!? |
If after a rebase CI is all green, this can be merged. If it's not, it may be the same issue as back then, more details at #699 (comment) . |
urllib3 2.0 works with OpenSSL 1.1.1 and 3.0 on up-to-date builds of Python 3.8 & 3.9.
Left them on their own line for stylistic reasons, just because putting all of the py38-py312 permutations on one line was *very* long.
Rebased!
I hope this means the |
@dgw I'm not 100% sure what you mean. We can only merge things that are fully green, if there is unrelated breakage elsewhere, that breakage needs dedicated fixing outside of this pull request, first. At least that would be my take. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #777 +/- ##
=======================================
Coverage 90.10% 90.10%
=======================================
Files 27 27
Lines 1809 1809
Branches 335 335
=======================================
Hits 1630 1630
Misses 134 134
Partials 45 45 ☔ View full report in Codecov by Sentry. |
@hartwork I mean that every commit with a check status on But it appears I will indeed need to look at the test suite and reevaluate something; 3.8 and 3.9 failed on this patch after rebasing. (pypy-3.10 is failing on master, therefore likely unrelated to the patch.) Probably won't get to that today. Would you like this PR placed in draft state until it's ready for another look? |
@dgw yes, that is understood, I should have put myself more clearly, sorry. That red-CI-from-the-begging state is a killer for pull requests and something we need to get rid of with priority. I have created pull request #786 on this topic just now and I hope that @jairhenrique is okay with this approach also.
It's the same thing that breaks PyPy 3.10 apparently. I'd be good with status "draft", I support the idea. |
In reference to the errors here, the Also including the 3.9 failure log in its entirety here, rather than digging through GHA again
Great, I've marked the PR as a draft and will be sure to comment again when (if?) I have something worth looking at again. Meanwhile I will keep an eye on the status of #786. |
I started rebasing this to try again, just with Python 3.9 this time (rest in peace 3.8), and got stuck locally when I tried to prepare my venv for running tests with Keeping The good news is that if I comment out Presumably the skipped item at collection is My rebased branch is (for now) available at https://github.com/dgw/vcrpy/tree/rebase-uncap-urllib3 — pending some more thought (or hints) at how to finish setting up the GHA matrix. Alternatively we can just drop this whole idea. 😅 Python 3.9 has less than a year to live at this point, and it hasn't been as big of a deal as I thought it might be to use a custom fork of |
Between myself and another maintainer of our downstream project, we've tested Python 3.8 & 3.9 with both OpenSSL 1.1.1 and 3.0.2, using vcrpy 5.1.0 and a forcibly upgraded urllib3 2.0.6. Based on those results, I'm optimistically hoping that this incompatibility is resolved and #699 (comment) has become true.
However, since the test suite already appears to fail on the current HEAD revision prior to applying this change, I'm not sure how to use the test results to definitively verify this patch (except by noting that in manual checks of the relevant environments I didn't see any new failures as compared to running the same tox env on master).
Downstream, we plan to unblock ourselves on the cassette cross-compatibility problem we're having by adding a step to our CI that ensures urllib3 2.x prior to running tests—but if we're lucky, that will be only a temporary kludge.