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

Make safe_name compliant to PEP 503 and behaviour of pip > 8.1.2 #1324

Merged
merged 2 commits into from
Sep 16, 2018

Conversation

torsava
Copy link
Contributor

@torsava torsava commented Apr 12, 2018

According to PEP 503, a "normalized" project name has all runs of the
characters ., - and _ replaced with a single - character. [0]

Similarly, since version 8.1.2, that is the behaviour of pip as well. [1]

However, Setuptools still allows a . in the normalized name, which
is causing trouble down the line.

[0] https://www.python.org/dev/peps/pep-0503/#normalized-names
[1] pypa/pip#3666

According to PEP 503, a "normalized" project name has all runs of the
characters ., - and _ replaced with a single - character. [0]

Similarly, since version 8.1.2, that is the behaviour of pip as well. [1]

However, Setuptools still allows a . in the normalized name, which
is causing trouble down the line.

[0] https://www.python.org/dev/peps/pep-0503/#normalized-names
[1] pypa/pip#3666
@torsava
Copy link
Contributor Author

torsava commented Apr 12, 2018

The tests failed as expected, because they're testing the wrong specification (with dots allowed in "normalized" projects name). If the change is approved I can fix the tests as well.

@jaraco
Copy link
Member

jaraco commented Apr 12, 2018

This change seems reasonable. What is the impact for the users (package maintainers and consumers)? Are there compatibility concerns for an environment that transitions from the old behavior to the new?

@torsava
Copy link
Contributor Author

torsava commented Apr 12, 2018

This change seems reasonable. What is the impact for the users (package maintainers and consumers)?

In Fedora, all packages provide a tag with their canonical name (pythonX.Ydist(__CANONICAL_NAME__)) and then other packages can use this tag for dependencies without knowing the actual package name where the module resides. The tags are generated using setuptools and thus leave dots in the name, whereas the dependencies are canonized using a macro that follows PEP 503 and thus correctly mangles dots into dashes.

See: https://bugzilla.redhat.com/show_bug.cgi?id=1564095

Are there compatibility concerns for an environment that transitions from the old behavior to the new?

We just discovered the incompatibility in Fedora thus no issues are expected. And given that pip handles this correctly for a long time now (8.1.2 released on 2016-05-10) and does not accept dots in the name, IMHO it's more dangerous to leave setuptools incompatible than to fix it.

@jmbowman
Copy link
Contributor

There don't seem to be any objections to this; I think it's ok to merge once the tests are updated and a changelog.d entry is added. The documentation already matches the intended behavior.

@silkentrance
Copy link
Contributor

silkentrance commented Jul 18, 2018

I think that in tests

>               assert 'FAIL' not in stdout.getvalue()

it should read

>               assert 'FAIL' not in stdout.getvalue().split('\n')

instead.

This, in turn will still fail as there is FAIL in the stdout, i.e.

E               AssertionError: assert 'FAIL' not in '\nInstalled /tmp/tmpjs_...7.egg\ntest_pkg\nFAIL\n'

@jaraco jaraco merged commit 9e23b6d into pypa:master Sep 16, 2018
@jaraco
Copy link
Member

jaraco commented Sep 16, 2018

@silkentrance In order to avoid more delays, I've decided to make this merge without addressing your concern, but I would welcome a PR according to your suggestion.

@jaraco
Copy link
Member

jaraco commented Sep 16, 2018

Well heck. I thought I saw tests were passing, but now I see that tests are failing (as Jeremy pointed out). I'm going to revert the changes.

@silkentrance
Copy link
Contributor

And I am not going to make a PR for this trivial.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants