-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Add check for correct h2 version range #3402
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
Conversation
@sethmlarson friendly nudge |
@sethmlarson @illia-v @shazow nudge^3 |
src/urllib3/__init__.py
Outdated
# Ensure that Python is compiled with h2>=4.0.0 and <5.0.0 | ||
# If the 'h2' module isn't installed at all that's | ||
# fine, we only care if the module is installed. | ||
try: | ||
import h2 # type: ignore[import-untyped] | ||
except ImportError: | ||
pass | ||
else: | ||
if not (h2.__version__ >= "4.0.0" and h2.__version__ < "5.0.0"): | ||
raise ImportError( | ||
"urllib3 v2 supports h2 version 4.x.x, currently " | ||
f"the 'h2' module is compiled with {h2.__version__!r}. " | ||
"See: https://github.com/urllib3/urllib3/issues/3290" | ||
) |
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.
Since the HTTP/2 support is rudimentary and it has to be enabled explicitly by calling urllib3.http2.inject_into_urllib3
, I believe we shouldn't raise the error at the top level. This can be breaking needlessly for environments that have both urllib3 and h2 installed but never call inject_into_urllib3
.
src/urllib3/http2/__init__.py
Outdated
@@ -11,6 +13,15 @@ | |||
|
|||
|
|||
def inject_into_urllib3() -> None: | |||
# First check if h2 version is valid | |||
h2_version = version("h2") | |||
if not h2_version.startswith("4"): |
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.
super nit: Shall we do 4.
instead of 4
?
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.
Thanks Quentin for getting this one in good shape!
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.
LGTM
Tested according to contribution guidelines.
Closes #3290.