Potential bug in "tests.lib.create_basic_wheel_for_package"

[deveshks]

Environment

• pip version: 20.1.dev0
• Python version: 3.8.2
• OS: OSX 10.15.4

Description

When the helper tests.lib.create_basic_wheel_for_package is utilized in a unit test to create a wheel with name containing a -, e.g. simple-package, and then the wheel is installed, the package name is taken as simple and the version is taken as package

Expected behavior

Seems like according to PEP-491 file name convention, the package name cannot contain a -, so such package names shouldn't be allowed while creating wheels for testing purposes. (My understanding of the PEP might also be wrong here)

How to Reproduce

Execute the following unit test

def test_create_wheel_bug(script):

    package = create_basic_wheel_for_package(script, 'simple-package', '1.0')
    script.pip("install", "--no-cache-dir", "--no-index", package)
    result = script.pip('list', '--format=json')
    assert 'simple-package' in json.loads(result.stdout)

Output

The test fails with an assertion error because the output of pip list --format=json lists the package name as simple and version as package

 assert 'simple-package' in [{'name': 'pip', 'version': '20.1.dev0'}, 
{'name': 'setuptools', 'version': '46.1.3'}, {'name': 'simple', 'version': 'package'}]

[uranusjr]

Ah, this makes sense. The culprit is this line:

archive_name = "{}-{}-py2.py3-none-any.whl".format(name, version)

which would give simple-package-1.0-py2.py3-none-any.whl, but the wheel spec expects simple_package-1.0-py2.py3-none-any.whl.

The fix should be straightforward as well:

archive_name = "{}-{}-py2.py3-none-any.whl".format(
    canonicalize_name(name).replace('-', '_'),
    version,
)

[deveshks]

I think that's the right approach, to canonicalize the name and replace - with _ before making the wheel file. Let me create a fix for it right away and use the same test which I wrote in the ticket to verify that it's been fixed :)

[uranusjr]

I just realised PEP 491 actually includes a regex to canonically transform a package name for a wheel’s file name. Maybe we should use that instead (with a link to the PEP); it would be much easier to understand for a future reader.

[deveshks]

I think you mean re.sub("[^\w\d.]+", "_", distribution, re.UNICODE) present at PEP-491 Escaping and Unicode ?

So we would create a correct wheel file name using that regex? Also do we want to do anything with the folder name of the package as well for this change?

pip/tests/lib/__init__.py

Line 1000 in 2f3a1be

package_init_py = "{name}/__init__.py".format(name=name)

Also will the unit test for this be similar to what we discussed in #8054 ? (Note that the below might not work due to the quirk we found with pkg_resources.safe_name until we start using canonicalize_name as #8054 (comment))

@pytest.mark.parametrize(
    'package_name',
    ['simple-package', 'simple_package', 'simple.package'],
)
def test_create_wheel_bug(script):

    package = create_basic_wheel_for_package(script, package_name, '1.0')
    script.pip("install", "--no-cache-dir", "--no-index", package)
    result = script.pip('list', '--format=freeze')
    assert package_name in result.stdout

[uranusjr]

Yes, that’s it. The same name can be used for the package as well (I don’t think the package is actually used by a lot of tests anyway).

@pradyunsg Does this need a test?

[pradyunsg]

Yea, I think adding a test in test_lib.py is a good idea.

[deveshks]

Yea, I think adding a test in test_lib.py is a good idea.

So what will that test do ? Is it enough to just try out variants of package names with dot, dash and underscore, and verify that the wheel file name is correct according to the spec for all 3 cases?