Skip to content

Conversation

ktpa
Copy link
Contributor

@ktpa ktpa commented Jun 10, 2024

Tested according to contribution guidelines.

Closes #3290.

@ktpa
Copy link
Contributor Author

ktpa commented Jun 18, 2024

@sethmlarson friendly nudge

@ktpa
Copy link
Contributor Author

ktpa commented Jun 25, 2024

@sethmlarson @illia-v @shazow nudge^3

Comment on lines 48 to 61
# 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"
)
Copy link
Member

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.

@@ -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"):
Copy link
Member

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?

sethmlarson
sethmlarson previously approved these changes Aug 12, 2024
Copy link
Member

@sethmlarson sethmlarson left a 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!

sethmlarson
sethmlarson previously approved these changes Aug 12, 2024
Copy link
Member

@sethmlarson sethmlarson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@sethmlarson sethmlarson merged commit 9af8d60 into urllib3:main Aug 13, 2024
36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add check for correct h2 version range
4 participants