Conversation
|
Any chance someone could look at this? Having green CI would be very useful. |
jeckersb
left a comment
jeckersb
left a comment
There was a problem hiding this comment.
This looks like good progress to me! Besides the style nit, I also had to bump pyopenssl to 22.1.0 in order to get the py38-openssl test to pass. With the existing 20.0.0 requirement it was failing with:
File "/home/runner/work/eventlet/eventlet/.tox/py38-openssl/lib/python3.8/site-packages/OpenSSL/crypto.py", line 1577, in X509StoreFlags
CB_ISSUER_CHECK = _lib.X509_V_FLAG_CB_ISSUER_CHECK
AttributeError: module 'lib' has no attribute 'X509_V_FLAG_CB_ISSUER_CHECK'
With pyopenssl 21.0.0 and 22.0.0 it instead fails with:
File "/home/runner/work/eventlet/eventlet/.tox/py38-openssl/lib/python3.8/site-packages/OpenSSL/crypto.py", line 3268, in <module>
_lib.OpenSSL_add_all_algorithms()
AttributeError: module 'lib' has no attribute 'OpenSSL_add_all_algorithms'
Version 22.0.1 fixes this error via pyca/pyopenssl#1108
|
|
||
| """ | ||
| # This breaks locks created by _fix_py3_rlock, so we comment it out: | ||
| #if self._owner != get_ident(): |
There was a problem hiding this comment.
This needs to be indented, otherwise the style workflow fails with:
eventlet/patcher.py:522:9: E265 block comment should start with '# '
#if self._owner != get_ident():
^
1
1 E265 block comment should start with '# '
There was a problem hiding this comment.
From a functional perspective commenting this test seems a bit dangerous, isn't?
Without that we would release a un-acquired lock, I'm not an expert but this seems a non-sense to release something not aquired by the current process, and I don't think we want to do that.
However, as this test break py_rlock and as py_rlock is a forked buggy version of rlock, I'm not sure commenting this line will be worst than with this line un-commented.
Thoughts?
There was a problem hiding this comment.
Actually I agree with @4383 it seems a bit dangerous and in the case we want to have this line comment with need to add the space.
There was a problem hiding this comment.
is all python version impacted by this break, or simply recent version like 3.11?
May changes from https://github.com/eventlet/eventlet/pull/754/files could help to solve this problem and allow us to uncomment this condition. Especially the changes that modifying eventlet/patcher.py and the fix_py3_rlock.
However, to avoid pulling the entire ball of wool, we could leave this line commented for now (with the pep8 warning fixed), and, then, once merged, rewind on #754 to see if we are now able to merge it too, and, then, try to uncomment this line once all the things are aligned.
There was a problem hiding this comment.
Versions previous to 3.11 have a slightly different _PyRLock implementation that doesn't have this line. So previous versions could just patch threading.RLock with threading._PyRLock and it "worked".
3.7 is past its end-of-life[1] so I'd totally drop support for anything older than 3.8, maybe in a self-contained PR though, without mixing fixes and dropping older Python version support. |
I agree with that comment. |
|
So how about as a next step I open PR to drop 2.7-3.7, and then merge this forward. |
|
OK I opened #827 |
|
I am going to split this PR into two, with one only covering Python 3.8 to 3.10, and do the 3.11 changes in a separate PR, to make it easier to review and easier to get to green CI ASAP. |
6 participants
Locally I now have passing tests on 3.7, 3.8, 3.9, 3.10, 3.11. I don't think there's any reason to support older versions (or 3.7, really), so I didn't bother with those.