-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
base: main
Are you sure you want to change the base?
Changes from all commits
83d425d
c1bf7c7
cb18737
3aa2d57
eb3ad58
12109df
2234be8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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` |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Per the comment in make_nspkg_sdist, distname should be |
||
# 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 | ||
|
@@ -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, | ||
|
@@ -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' | ||
) | ||
|
||
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Still, the previous implementation would have accepted More importantly, however, I notice that I think this change is a mistake, and instead, this function should require that |
||
parts = namespace.split('.') | ||
nspackage = parts[0] | ||
|
||
packages = ['.'.join(parts[:idx]) for idx in range(1, len(parts) + 1)] | ||
|
There was a problem hiding this comment.
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.