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

Stop recognizing files ending with .dist-info as dist #2075

Merged
merged 7 commits into from May 10, 2020
Merged

Stop recognizing files ending with .dist-info as dist #2075

merged 7 commits into from May 10, 2020

Conversation

McSinyx
Copy link
Contributor

@McSinyx McSinyx commented Apr 24, 2020

Summary of changes

As proposed in PEP 376, dist-info distributions must be directories.

Closes pypa/pip#8122.

Pull Request Checklist

  • Changes have tests
  • News fragment added in changelog.d

Edit: apparently files with dist-info extension seems to be an supported behavior for setuptools. I'll try to investigate why.

As proposed in PEP 376, dist-info distributions must be directories.
@McSinyx McSinyx marked this pull request as draft Apr 25, 2020
@McSinyx
Copy link
Contributor Author

@McSinyx McSinyx commented Apr 25, 2020

tox cannot build setuptools locally for me either, even on master 😞 I'm not sure what to do please help! Seemingly we have some dependencies undeclared.

@McSinyx McSinyx marked this pull request as ready for review Apr 25, 2020
Copy link
Member

@jaraco jaraco left a comment

I'm uneasy just dropping support for this usage. Presumably someone created it with a purpose. Even the pep makes a nod to backward compatibility, basically not addressing the issue and hoping to create a new tool that's not limited by the behavior.

If Setuptools is to drop support for these files, we should know why those were created and how a user whose environment has those files needs to react.

@jaraco
Copy link
Member

@jaraco jaraco commented May 3, 2020

tox cannot build setuptools locally for me either, even on master

It's building for me locally and on CI, so probably the issue is in your environment or due to your patch. It may be that when you switched to master, you still had a corrupted environment, so you may want to tox -r or git clean -fdx (destructive of ignored files).

@McSinyx
Copy link
Contributor Author

@McSinyx McSinyx commented May 3, 2020

Could you please give me some me some context about the mentioned backward compatibility? I am too young to be familiar with none of the name listed in that section in the PEP.

we should know why [files ending with .dist-info] were created and how a user whose environment has those files needs to react.

This is to solve the linked pip issue, which IIRC occurs when Docker replace the original directory with a white-out file with the same name extension. I am unaware (again, my experience is very limited) of any actual use of dist-info as a file, since

  • dist-info AFAIK is an invention by PEP 376, which specifies that it is a directory
  • The issue popped up on pip for the first time, and I imagine that if there is there's some project already creating it as a file, statistically there should be more instances of the crash

Since this was to fix pip's problem, I'd be happy to simply do some try-except over there instead. While the current behavior of pkg_resources is not 100% PEP-376-compliance, I don't want any of us to be responsible of breaking someone else's setup. That being said, I'd be grateful if you can take another look at it though.

Thanks for the tips about the tests BTW, I'm feeling so dumb now 😄 should have figured that out earlier. Edit: converted to draft because I have yet to be able to debug this.

@McSinyx McSinyx marked this pull request as draft May 9, 2020
@jaraco
Copy link
Member

@jaraco jaraco commented May 10, 2020

If the only reason these dist-info files exists is because of Dockerfile whiteout files, then yeah, setuptools/pkg_resources should just ignore them.

@jaraco
Copy link
Member

@jaraco jaraco commented May 10, 2020

In this latest push, I've found the issue - the issue was that a filename was being passed ("setuptools-4XXX.dist-info") that did not exist. Therefore, isdir was false, and that led to another code path that would continue. I'm not exactly sure, but this latest change does pass the tests.

As you can see, this code is really hairy. I'm still not confident this approach is the right one.

I think more investigation is warranted into why os.path.isdir didn't produce the desired effect.

@jaraco
Copy link
Member

@jaraco jaraco commented May 10, 2020

In this latest commit, I investigated further and now understand why 'isdir' wasn't working as a test - it wasn't including the full path to the entry, and now including that detail, the 'isdir' test works as you had intended.

changelog.d/2075.breaking.rst Outdated Show resolved Hide resolved
@jaraco jaraco marked this pull request as ready for review May 10, 2020
@jaraco jaraco merged commit dfc4778 into pypa:master May 10, 2020
19 of 22 checks passed
@McSinyx
Copy link
Contributor Author

@McSinyx McSinyx commented May 11, 2020

In this latest commit, I investigated further and now understand why 'isdir' wasn't working as a test - it wasn't including the full path to the entry, and now including that detail, the 'isdir' test works as you had intended.

Thanks for providing the insight!

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