Skip to content
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

Optional dependency issues #963

Closed
kitterma opened this issue Jul 20, 2023 · 4 comments
Closed

Optional dependency issues #963

kitterma opened this issue Jul 20, 2023 · 4 comments

Comments

@kitterma
Copy link
Contributor

kitterma commented Jul 20, 2023

I'm submitting this as an issue instead of a PR, because I'm not 100% certain on the resolution. The current pyproject.toml includes:

[tool.poetry.dependencies]
python = "^3.8"
httpx = {version=">=0.24.1", optional=true, python=">=3.8"}
httpcore = {version=">=0.17.3", optional=true, python=">=3.8"}
h2 = {version=">=4.1.0", optional=true, python=">=3.8"}

Is it really necessary to specify a python interpreter version with these that's the same as the minimum required python version? It seems to me that the , python=">=3.8" could be removed. I noticed because Debian's Python dependency generation doesn't handle this well (it's a bug, which I've filed in Debian), but it seems to me it could be cleaned up.

Also, it seems like the optional dependencies ought to be related to one of the extras:

[tool.poetry.extras]
doh = ['httpx', 'h2']
idna = ['idna']
dnssec = ['cryptography']
trio = ['trio']
wmi = ['wmi']
doq = ['aioquic']

This is currently true with the exception of httpcore and sniffio. I think httpcore should be added to doh and sniffio should be in a new extra, maybe called async?

Scott K

@rthalley
Copy link
Owner

sniffio is a non-optional dependency of trio, and httpcore is a non-optional dependency of httpx, so we don't need to list them as extras. We list h2 as part of the 'doh' extra because h2 is an optional dependency of httpx, but we always want it. Httpcore is in tool.poetry.dependencies as we need at least that version to work due to API changes. I don't think we actually need sniffio in there as I doubt we care about the version at this point.

I think it is save to remove the python=">=3.8" part of the specifications.

@kitterma
Copy link
Contributor Author

Thanks. The problem is the way the Poetry generates the dist-info with this input (I've manually removed the obsolete Python version requirements for clarity):

Requires-Dist: aioquic (>=0.9.20) ; extra == "doq"
Requires-Dist: cryptography (>=2.6,<42.0) ; extra == "dnssec"
Requires-Dist: h2 (>=4.1.0) ; (extra == "doh")
Requires-Dist: httpcore (>=0.17.3) ;
Requires-Dist: httpx (>=0.24.1) ; (extra == "doh")
Requires-Dist: idna (>=2.1,<4.0) ; extra == "idna"
Requires-Dist: sniffio (>=1.1,<2.0)
Requires-Dist: trio (>=0.14,<0.23) ; extra == "trio"
Requires-Dist: wmi (>=1.5.1,<2.0.0) ; extra == "wmi"

The generated dist-info metadata shows them as required, there's no indication that they are optional. Sniffio 1.1 has been out for a long time now, so perhaps it could be deleted. Would you be up for adding httpcore to extras doh as a workaround for Poetry not correctly dealing with optional dependencies not connected to an extra?

rthalley added a commit that referenced this issue Jul 20, 2023
rthalley added a commit that referenced this issue Jul 20, 2023
(cherry picked from commit 063bd7f)
@rthalley
Copy link
Owner

Ah, I didn't realize that! I agree with your suggestion, let's add httpcore to doh, and remove sniffio. I figured it would be easier if I just did it rather than doing a PR, so I just did it. Thanks for the clear description!

@kitterma
Copy link
Contributor Author

Excellent. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants