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

Wheel contains no dependency metadata when built via sdist #530

Closed
gazpachoking opened this issue Nov 3, 2022 · 20 comments
Closed

Wheel contains no dependency metadata when built via sdist #530

gazpachoking opened this issue Nov 3, 2022 · 20 comments

Comments

@gazpachoking
Copy link

gazpachoking commented Nov 3, 2022

When I run python -m build on our project, the resulting wheel doesn't contain any dependency metadata.
When I run python -m build --sdist --wheel it does.

Not sure if this is to do with using dynamic dependencies from a requirements.txt file or not.

build version 0.9.0
The project in question, for reference.

@layday
Copy link
Member

layday commented Nov 3, 2022

Yes, you are probably not bundling requirements.txt in the sdist. build builds wheels from sdists by default to catch exactly this kind of consistency errors.

@henryiii
Copy link
Contributor

henryiii commented Nov 3, 2022

Are you missing the file in the SDist? My guess is it's reading dependencies from requirements.txt but that file isn't in your SDist, so your SDist is broken.

@henryiii
Copy link
Contributor

henryiii commented Nov 3, 2022

You don't have a MANIFEST.in. You should either add setuptools_scm or add a MANIFEST.in. requirements.txt is not included by setuptools by default.

@henryiii
Copy link
Contributor

henryiii commented Nov 3, 2022

[tool.setuptools.dynamic]
dependencies = {file = "requirements.txt"}

I had no idea setuptools now supported this... I thought for years the argument was this would support bad practices. Interesting!

@gazpachoking
Copy link
Author

Are you missing the file in the SDist? My guess is it's reading dependencies from requirements.txt but that file isn't in your SDist, so your SDist is broken.

Ahh. This makes sense. Good catch.

@gazpachoking
Copy link
Author

Although, the sdist does include the requires.txt in its egg-info, so I'm not sure the sdist is actually broken. It's just that the setuptools dynamic dependency loading doesn't work when built from the sdist.

@layday
Copy link
Member

layday commented Nov 3, 2022

I had no idea setuptools now supported this... I thought for years the argument was this would support bad practices. Interesting!

It has for a little while now, see pypa/setuptools#1951.

@layday
Copy link
Member

layday commented Nov 3, 2022

Although, the sdist does include the requires.txt in its egg-info, so I'm not sure the sdist is actually broken. It's just that the setuptools dynamic dependency loading doesn't work when built from the sdist.

setuptools does not look in egg-info's requires.txt when building a distribution anew. egg-info is a build artifact; if setuptools depended on it for builds we'd have a bit of a chicken and cough egg situation in our hands.

@gazpachoking
Copy link
Author

Gotcha. Thanks for all the help and info guys! I glad to remove the MANIFEST.in, but I guess it was a bit premature.

@henryiii
Copy link
Contributor

henryiii commented Nov 3, 2022

Ahh, in June, pypa/setuptools#3253. And that project has some weird setup - setuptools and poetry combined? Why not use PDM? PDM allows you to use any PEP 621 backend, including setuptools. Poetry still does not conform to standards, and won't until 2.0.

Maybe you should check with setuptools as to what they want to do if an SDist contains the requirements metadata but doesn't contain the file used to generate that metadata.

FYI, it is an extremely bad practice to pin all your requirements exactly, and it is meaningless to add and python_version < "4.0" after every one. See https://iscinumpy.dev/post/bound-version-constraints/ for proof that I care about this sort of thing. :)

@henryiii
Copy link
Contributor

henryiii commented Nov 3, 2022

Just adding setuptools_scm to your build requirements would fix this, it causes setuptools to include all files in the repo.

@layday
Copy link
Member

layday commented Nov 3, 2022

It looks like they use Poetry to lock their dependencies and export them to the req txt format using Poetry. I'm guessing it's Poetry that's adding all the and python_version < "4.0"'s because in its config they have:

python = "^3.7"

@gazpachoking
Copy link
Author

Ahh, in June, pypa/setuptools#3253. And that project has some weird setup - setuptools and poetry combined? Why not use PDM? PDM allows you to use any PEP 621 backend, including setuptools. Poetry still does not conform to standards, and won't until 2.0.

Yeah, it's certainly weird. I did it this way because poetry can flatten all the requirements, including transitive ones, retaining their environment markers in a way that pdm, or pip-compile can't. This means I can generate a requirements.txt that is cross platform (and builds with pinned deps that are cross platform.) I actually had a PR to switch to PDM, and was ready to pull the trigger until I realized that it can't do that. pdm-project/pdm#1476

FYI, it is an extremely bad practice to pin all your requirements exactly, and it is meaningless to add and python_version < "4.0" after every one. See https://iscinumpy.dev/post/bound-version-constraints/

Yeah, I know. We are doing this because it's an application where we are using (abusing?) pypi to do our distribution. We want installs to have deps pinned to reduce support issues. Our docs all say to install it to a virtualenv, so you don't mess up all your other packages with the pins.

Just adding setuptools_scm to your build requirements would fix this, it causes setuptools to include all files in the repo.

Thanks, will give that a try.

Maybe you should check with setuptools as to what they want to do if an SDist contains the requirements metadata but doesn't contain the file used to generate that metadata.

I think including the requirements.txt in the sdist should probably be required in our case, since it's needed for building.

It looks like they use Poetry to lock their dependencies and export them to the req txt format using Poetry. I'm guessing it's Poetry that's adding all the and python_version < "4.0"'s because in its config they have:

Precisely.

@layday
Copy link
Member

layday commented Nov 3, 2022

You probably want to change Poetry's python = "^3.7" to python = ">=3.7" to match your setuptools config :) ^3.7 in semver means >=3.7, <4 in PyPA terms.

@henryiii
Copy link
Contributor

henryiii commented Nov 3, 2022

an application where we are using (abusing?)

I'd say abusing, but okay, as an application in a fresh virtual environment, yes, it can be done (though you should release fast and often!). (Pipx might be very useful here, as that's always what it does).

You probably want to change Poetry's python = "^3.7" to python = ">=3.7"

I'd be pretty surprised if you can, actually - Poetry forces you to match the most strict of all your dependencies, and I bet one of them does also make this mistake - just one is enough to force the same mistake on you. It would have made the requirements.txt a lot nicer, though.

@henryiii
Copy link
Contributor

henryiii commented Nov 3, 2022

Poetry also makes a mistake in environment marker merging. They assume all versions of a wheel have the same requirements - which is not required. It's possible to have custom requirements per wheel. It's rare, but it could be done, for example, if you wanted to provide a manylinux1 wheel that required a package with a setup.py that printed an error message telling you that manylinux1 was not supposed and please upgrade your pip.

Even in those cases, though, it's probably a safe assumption, but it is an assumption.

@layday
Copy link
Member

layday commented Nov 3, 2022

Poetry forces you to match the most strict of all your dependencies, and I bet one of them does also make this mistake

Oh, I forgot about that. Anyhow, I think we established this isn't a build issue ;) Feel free to hop into the PyPA discord if you wanna talk packaging @gazpachoking.

@layday layday closed this as completed Nov 3, 2022
@henryiii
Copy link
Contributor

henryiii commented Nov 3, 2022

I think we established this isn't a build issue

I think we established this was a build success, it caught a broken SDist mistake. :)

@gazpachoking
Copy link
Author

an application where we are using (abusing?)

I'd say abusing, but okay, as an application in a fresh virtual environment, yes, it can be done (though you should release fast and often!). (Pipx might be very useful here, as that's always what it does).

Yep, we release any changes with an automatic daily build, and actually one of the reasons I set this poetry/setuptools system up was so that I could enable Renovate to handle our dependency upgrades more easily. I've thought for a while that we should have some other installer recommendation than pip but... that'd be more work 😄 Been meaning to add pipx as an install recommendation for a while.

I'd be pretty surprised if you can, actually - Poetry forces you to match the most strict of all your dependencies, and I bet one of them does also make this mistake - just one is enough to force the same mistake on you. It would have made the requirements.txt a lot nicer, though.

Quite right.

Poetry also makes a mistake in environment marker merging. They assume all versions of a wheel have the same requirements - which is not required. It's possible to have custom requirements per wheel. It's rare, but it could be done, for example, if you wanted to provide a manylinux1 wheel that required a package with a setup.py that printed an error message telling you that manylinux1 was not supposed and please upgrade your pip.

Even in those cases, though, it's probably a safe assumption, but it is an assumption.

Yep, I saw that while I was researching too, but it was closer than anything else to getting cross-platform, and I don't think any of our deps actually run into this issue.

Poetry forces you to match the most strict of all your dependencies, and I bet one of them does also make this mistake

Oh, I forgot about that. Anyhow, I think we established this isn't a build issue ;) Feel free to hop into the PyPA discord if you wanna talk packaging @gazpachoking.

I think we established this isn't a build issue

I think we established this was a build success, it caught a broken SDist mistake. :)

Thanks for all the advice and help! Amazing how fast you guys figured everything out given how wacky our setup is.

@henryiii
Copy link
Contributor

henryiii commented Nov 3, 2022

I don't think any of our deps actually run into this issue

I'm not sure a single package on PyPI does this (though I've seen a proposal for the manylinux one), certainly it's not a problem in practice. But I do think this is why pip-tools and PDM have a harder time handling this.

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

No branches or pull requests

3 participants