-
-
Notifications
You must be signed in to change notification settings - Fork 335
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
migrate trio.ssl from star import to explicit imports, to aid static analysis issue #542 #752
Conversation
One test is failing but I cannot see where the import gets overwritten again.
Any ideas are welcome. |
Codecov Report
@@ Coverage Diff @@
## master #752 +/- ##
==========================================
+ Coverage 99.01% 99.01% +<.01%
==========================================
Files 96 96
Lines 11673 11677 +4
Branches 832 830 -2
==========================================
+ Hits 11558 11562 +4
Misses 94 94
Partials 21 21
Continue to review full report at Codecov.
|
get_default_verify_paths, Purpose, enum_certificates, enum_crls, | ||
SSLSession, VerifyMode, VerifyFlags, Options, AlertDescription, | ||
SSLErrorNumber | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These don't need to be fake! It's a well-defined set of things that should always be available. So we can just do a regular old import
without any try
/except
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some actually don't but others do. I tried to import them directly but the test failed on some versions of python and with windows. I decided to revert back to being them all fake since doing a subset one way and the rest the other is more confusing and IMHO not worth the extra lines of code. Is this fine with you?
"VerifyFlags", "Options", "AlertDescription", "SSLErrorNumber" | ||
] | ||
} | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once we switch to a regular import above, we can delete this.
|
||
for _name in _stdlib_ssl.__dict__.keys(): | ||
if _name == _name.upper(): | ||
_reexport(_name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code is like trio.socket
: it re-exports all the uppercased symbols. So we need some similar handling: a fake try
/import
, plus moving this loop into trio/ssl.py
. With this PR right now it looks like we aren't re-exporting the uppercased symbols at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ups, I overlooked this totally, yes of course, this has to be addressed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are now imported, but see my comment below about external dependencies like ssl library and version.
This also needs a merge from master to pick up the changes in #756. Plus addressing the other comments above, of course :-) |
No worries 👍 |
That's strange. I expected the test to fail, as I am only importing the ssl Mac symbols, so the jedi and pylint test for windows and linux should show some missing symbols. I'll checkout this repo on windows and see if it passes locally as well check which symbols are imported. |
I can confirm the tests did not catch |
I think that the main thing that determines which symbols appear on a given Python is (a) which version of Python you're using, and (b) which version of OpenSSL this Python was built against. I don't think it varies across platforms, except that if you grab a random Windows build and a random MacOS build then they may well have been built against different versions of OpenSSL. |
Understood, but that does not make it especially easier. Python can not only be build against different versions of OpenSSL it can also be build against other SSL libraries, e.g. LibreSSL. |
NAMESPACES.remove("trio.ssl")
# Always available
from ssl import (
cert_time_to_seconds, CertificateError,
create_default_context, DER_cert_to_PEM_cert,
get_default_verify_paths, match_hostname,
PEM_cert_to_DER_cert, Purpose, SSLEOFError, SSLError,
SSLSyscallError, SSLZeroReturnError
)
# Added in python 3.6
try:
from ssl import AlertDescription, SSLErrorNumber, SSLSession, VerifyFlags, VerifyMode, Options
except ImportError:
pass
# Added in python 3.7
try:
from ssl import SSLCertVerificationError, TLSVersion
except ImportError:
pass
# Windows-only
try:
from ssl import enum_certificates, enum_crls
except ImportError:
pass (It looks like the constants available depend on openssl version, but these lowercase things are mostly dependent on Python version.) |
@njsmith Hi Nathaniel, thanks for the feedback and sorry for the delayed answer. I was really busy over the past days. I'll try to implement what you suggest and notify you once done. Thank you so much for letting me taking part of this awesome project. |
??? I thought the second last test failed for pypy but now it is actually telling me that this test succeeded? |
This is looking good, except for some dead code left behind... let me try deleting that and check if it still works. |
No semantic changes (I hope!)
Use less magic in constructing our public API exports, to make trio more intelligible to static analysis #542