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

Fix "Wheel naming is not following PEP 491 convention" #4766

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions newsfragments/4766.bugfix.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix wheel file naming to follow binary distribution specification -- by :user:`di`
8 changes: 7 additions & 1 deletion setuptools/_normalization.py
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,13 @@ def filename_component_broken(value: str) -> str:
def safer_name(value: str) -> str:
"""Like ``safe_name`` but can be used as filename component for wheel"""
# See bdist_wheel.safer_name
return filename_component(safe_name(value))
return (
# Per https://packaging.python.org/en/latest/specifications/name-normalization/#name-normalization
re.sub(r"[-_.]+", "-", safe_name(value))
.lower()
# Per https://packaging.python.org/en/latest/specifications/binary-distribution-format/#escaping-and-unicode
.replace("-", "_")
)


def safer_best_effort_version(value: str) -> str:
Expand Down
8 changes: 7 additions & 1 deletion setuptools/command/bdist_wheel.py
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,13 @@ def get_abi_tag() -> str | None:


def safer_name(name: str) -> str:
return safe_name(name).replace("-", "_")
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("-", "_")
)
Comment on lines +137 to +143
Copy link
Member

Choose a reason for hiding this comment

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

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.



def safer_version(version: str) -> str:
Expand Down
4 changes: 2 additions & 2 deletions setuptools/tests/test_bdist_wheel.py
Original file line number Diff line number Diff line change
Expand Up @@ -246,9 +246,9 @@ def test_no_scripts(wheel_paths):


def test_unicode_record(wheel_paths):
path = next(path for path in wheel_paths if "unicode.dist" in path)
path = next(path for path in wheel_paths if "unicode_dist" in path)
with ZipFile(path) as zf:
record = zf.read("unicode.dist-0.1.dist-info/RECORD")
record = zf.read("unicode_dist-0.1.dist-info/RECORD")

assert "åäö_日本語.py".encode() in record

Expand Down
2 changes: 1 addition & 1 deletion setuptools/tests/test_dist_info.py
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@ def test_dist_info_is_the_same_as_in_wheel(
dist_info = next(tmp_path.glob("dir_dist/*.dist-info"))

assert dist_info.name == wheel_dist_info.name
assert dist_info.name.startswith(f"{name.replace('-', '_')}-{version}{suffix}")
assert dist_info.name.startswith(f"my_proj-{version}{suffix}")
for file in "METADATA", "entry_points.txt":
assert read(dist_info / file) == read(wheel_dist_info / file)

Expand Down
20 changes: 11 additions & 9 deletions setuptools/tests/test_easy_install.py
Original file line number Diff line number Diff line change
Expand Up @@ -670,11 +670,11 @@ def test_setup_requires_override_nspkg(self, use_setup_cfg):

with contexts.save_pkg_resources_state():
with contexts.tempdir() as temp_dir:
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')
Copy link
Member

Choose a reason for hiding this comment

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

Per the comment in make_nspkg_sdist, distname should be foo.bar.

# Now actually go ahead an extract to the temp dir and add the
# extracted path to sys.path so foo.bar v0.1 is importable
foobar_1_dir = os.path.join(temp_dir, 'foo.bar-0.1')
foobar_1_dir = os.path.join(temp_dir, 'foo_bar-0.1')
os.mkdir(foobar_1_dir)
with tarfile.open(foobar_1_archive) as tf:
tf.extraction_filter = lambda member, path: member
Expand All @@ -697,14 +697,14 @@ def test_setup_requires_override_nspkg(self, use_setup_cfg):
len(foo.__path__) == 2):
print('FAIL')

if 'foo.bar-0.2' not in foo.__path__[0]:
if 'foo_bar-0.2' not in foo.__path__[0]:
print('FAIL')
"""
)

test_pkg = create_setup_requires_package(
temp_dir,
'foo.bar',
'foo_bar',
'0.2',
make_nspkg_sdist,
template,
Expand All @@ -718,8 +718,8 @@ def test_setup_requires_override_nspkg(self, use_setup_cfg):
# Don't even need to install the package, just
# running the setup.py at all is sufficient
run_setup(test_setup_py, ['--name'])
except pkg_resources.VersionConflict:
self.fail(
except pkg_resources.VersionConflict: # pragma: nocover
pytest.fail(
'Installing setup.py requirements caused a VersionConflict'
)

Expand Down Expand Up @@ -1120,8 +1120,10 @@ def make_nspkg_sdist(dist_path, distname, version):
package with the same name as distname. The top-level package is
designated a namespace package).
"""

parts = distname.split('.')
# The project name might not contain periods. Replace dashes and
# underscores with periods before constructing the namespace.
namespace = distname.replace('-', '.').replace('_', '.')
Comment on lines +1123 to +1125
Copy link
Member

Choose a reason for hiding this comment

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

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.

parts = namespace.split('.')
nspackage = parts[0]

packages = ['.'.join(parts[:idx]) for idx in range(1, len(parts) + 1)]
Expand Down
Loading