Fix "Wheel naming is not following PEP 491 convention" #4766
Conversation
|
Thank you! The failing test seems to be happening now regardless of this change: https://github.com/pypa/setuptools/actions/runs/12298183072/job/34321035714#step:3:49. I don't know if anything changed in the latest versions of Python being distributed to macOS, so that |
|
Aha, that's a different failure actually, |
abravalheri
force-pushed
the
fix/3777
branch
from
92d42d1 to
c1bf7c7
Compare
This looks like it is checking deprecated functionality? Not sure we have to account such an edge case since we are moving away from |
|
I'm not sure! I think the test is just broken in a way I haven't been able to figure out (it mixes the distribution name and the namespace name as one thing in the helper functions) but I think the underlying functionality probably still works? |
|
All tests are passing, this is now ready for review. |
setuptools/command/bdist_wheel.py
Outdated
| return ( | ||
| # Per https://packaging.python.org/en/latest/specifications/name-normalization/#name-normalization | ||
| re.sub(r"[-_.]+", "-", safe_name(name)) | ||
| .lower() | ||
| # Per https://packaging.python.org/en/latest/specifications/binary-distribution-format/#escaping-and-unicode | ||
| .replace("-", "_") | ||
| ) |
There was a problem hiding this comment.
I'm not a fan of this duplicated code being further expanded. I'd like to see this logic consolidated (in setuptools if not in something like packaging), especially since now it's more than one line that's being duplicated.
| # The project name might not contain periods. Replace dashes and | ||
| # underscores with periods before constructing the namespace. | ||
| namespace = distname.replace('-', '.').replace('_', '.') |
There was a problem hiding this comment.
This seems slightly terrible. Is this helper called with a normalized distname, in which case it's not possible to know what the separators are? Is it always called with a normalized distname, or is it called with the specified distribution name? This function, if called with more-itertools will produce a namespace package for more. I guess I can see from the previous implementation that it always assumes there's a single namespace component at the front.
Still, the previous implementation would have accepted j_o.foo and produced a namespace package j_o, but this new implementation will get j-o-foo and produce a namespace packgae j.
More importantly, however, I notice that setuptools.setup is being called with name={distname}. In that case, {distname} should not be normalized and should contain the periods.
I think this change is a mistake, and instead, this function should require that distname contain at least one period.
| foobar_1_archive = os.path.join(temp_dir, 'foo.bar-0.1.tar.gz') | ||
| make_nspkg_sdist(foobar_1_archive, 'foo.bar', '0.1') | ||
| foobar_1_archive = os.path.join(temp_dir, 'foo_bar-0.1.tar.gz') | ||
| make_nspkg_sdist(foobar_1_archive, 'foo_bar', '0.1') |
There was a problem hiding this comment.
Per the comment in make_nspkg_sdist, distname should be foo.bar.
|
This is ready for re-review. |
Summary of changes
Fixes #3777.
Marking this as draft as there is one outstanding failing test that I have been unable to resolve (setuptools/tests/test_easy_install.py::TestSetupRequires::test_setup_requires_override_nspkg).Pull Request Checklist
newsfragments/.(See documentation for details)