build_meta: produce informative error when a dist is not found #2608
Conversation
Previously, when `build_sdist` or `build_wheel` were unable
to build a distribution (and were therefore unable to find the
distribution file), they would throw a
ValueError: not enough values to unpack (expected 1, got 0)
which did not offer any clues as to where the issue might lie.
|
See pypa/packaging.python.org#838 (and subsequently pypa/packaging-problems#467 and pypa/packaging-problems#468) which motivated this change. |
setuptools/build_meta.py
Outdated
| file, = matching | ||
| return file | ||
| try: | ||
| return next(matching) |
There was a problem hiding this comment.
Previously, the code would check for exactly one match. Now it only checks for at least one match (and ignores all but the first). Could that silently swallow errors?
There was a problem hiding this comment.
I suppose, if multiple distributions would be built somehow or if the folder already had a distribution in it but then all bets are off anyway.
setuptools/build_meta.py
Outdated
| raise ValueError('No distribution was found. The distribution was ' | ||
| 'possibly not built. Ensure that your `setup.py` ' | ||
| 'is not empty and that it calls `setup()`.') |
There was a problem hiding this comment.
Avoid hanging indents and use of first/second person. Remove 'Distribution was possibly not built', as that's mainly redundant with no distribution was found.
| raise ValueError('No distribution was found. The distribution was ' | |
| 'possibly not built. Ensure that your `setup.py` ' | |
| 'is not empty and that it calls `setup()`.') | |
| raise ValueError( | |
| 'No distribution was found. Ensure that `setup.py` ' | |
| 'is not empty and that it calls `setup()`.') |
setuptools/tests/test_build_meta.py
Outdated
| match=re.escape( | ||
| 'No distribution was found. The distribution was ' | ||
| 'possibly not built. Ensure that your `setup.py` ' | ||
| 'is not empty and that it calls `setup()`.')): |
There was a problem hiding this comment.
Simplify match to avoid over-matching the expected text. Just assert on the gist of the message.
| match=re.escape( | |
| 'No distribution was found. The distribution was ' | |
| 'possibly not built. Ensure that your `setup.py` ' | |
| 'is not empty and that it calls `setup()`.')): | |
| match=re.escape('No distribution was found.')): |
| raise ValueError( | ||
| 'No distribution was found. Ensure that `setup.py` ' | ||
| 'is not empty and that it calls `setup()`.') |
There was a problem hiding this comment.
Should this be raise ValueError(...) from None, to simplify the output? The original ValueError from unpacking the tuple is not interesting to anyone and adding noise, since it's literally the only thing done here?
There was a problem hiding this comment.
Then again, somehing else might raise a ValueError since the generator isn't consumed before it's unpacked and ValueError is so ubiquitous that I'm a little wary of erasing it.
There was a problem hiding this comment.
If you'd like to convert the generator to a list comprehension and then raise from None, I think that'd be fine.
There was a problem hiding this comment.
Since the generator is created on the line above, there currently should be no way to for it to be exhausted before it hits this point?
There was a problem hiding this comment.
Yes, hence it might raise an unrelated value error in the try block.
Followup to pypa#2608. Makes sure an unrelated ValueError is not caught by the try/except, then removes the extra exception printout. Also moves the msg to the line above to avoid a double message printout in the traceback.
Summary of changes
Previously, when
build_sdistorbuild_wheelwere unableto build a distribution (and were therefore unable to find the
distribution file), they would throw a
which did not offer any clues as to where the issue might lie.
Pull Request Checklist
changelog.d/.(See documentation for details)