Skip zipp.Path when it is not a file - [merged]

[jaraco]

In GitLab by @asottile on Jun 4, 2019, 21:59

Merges skip_zipp_not_a_file -> master

pypy3 has a file-descriptor leak with open('directory') which
zipp.Path(...) triggers. This preempts passing a directory and tickling
that.

More information:

• https://bitbucket.org/pypy/pypy/issues/3021/ioopen-directory-leaks-a-file-descriptor
• pypy3 + coverage: unclosed file <_io.FileIO fd=3 mode='rb' closefd=True> nedbat/coveragepy#807
• ResourceWarning during collection pytest-dev/pytest#5342

[jaraco]

In GitLab by @asottile on Jun 4, 2019, 22:04

added 1 commit

• 2ac791de - Skip zipp.Path when it is not a file

Compare with previous version

[jaraco]

In GitLab by @codecov on Jun 4, 2019, 22:11

Codecov Report

Merging #75 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master    #75   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           2      2           
  Lines         191    194    +3     
  Branches       18     19    +1     
=====================================
+ Hits          191    194    +3

Impacted Files	Coverage Δ
importlib_metadata/_compat.py	100% <100%> (ø)	⬆️
importlib_metadata/__init__.py	100% <100%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ff02b7f...898b200. Read the comment docs.

[jaraco]

In GitLab by @asottile on Jun 6, 2019, 22:16

@jaraco @warsaw thoughts on this? currently this is causing lots of warning spew :S

[jaraco]

In GitLab by @warsaw on Jun 7, 2019, 20:45

Commented on importlib_metadata/init.py line 9

I generally prefer not to import os.path, just os.

[jaraco]

In GitLab by @warsaw on Jun 7, 2019, 20:47

Commented on importlib_metadata/init.py line 370

Since this is a little icky, and it's not obvious from the code why it's being done, and because it's motivated by a bug in PyPy, could you please as a comment with a reference URL?

[jaraco]

In GitLab by @asottile on Jun 7, 2019, 20:47

Commented on importlib_metadata/init.py line 9

afaik it's an implementation detail that os triggers os.path as a side-effect, since this is using os.path.X below shouldn't this be import os.path?

[jaraco]

In GitLab by @asottile on Jun 7, 2019, 20:48

Commented on importlib_metadata/init.py line 370

sure! I was hoping the contents in the PR description would be enough to link it to this change (and not clutter the code) but I can put that inline as well

[jaraco]

In GitLab by @asottile on Jun 7, 2019, 20:50

added 1 commit

• 1187a121 - Skip zipp.Path when it is not a file

Compare with previous version

[jaraco]

In GitLab by @warsaw on Jun 7, 2019, 20:55

Commented on importlib_metadata/init.py line 9

path has a been and will always be automatically included in os. Yeah, it's a wart, but one we've lived with since the beginning of time. ;). Changing that would break the world. I know others prefer to import explicitly from os.path, but I like the simpler import for consistency.

[jaraco]

In GitLab by @jaraco on Jun 7, 2019, 20:57

One reason I used the suppress model was because it avoided potential race conditions.

I feel like the workaround is not well-placed here. Does this change imply that every attempt to try-open a file path should first check for is_file? I don't think so, but that's what it does.

If this change is to be accepted, I'd prefer for it to be more selective - to only run on PyPy versions that have the defect. What do you think about having a function in _compat that is a no-op except on affected versions... and then the Python 3.8 port could omit the call for that function, as it's not needed in the stdlib?

[jaraco]

In GitLab by @jaraco on Jun 7, 2019, 20:57

Commented on importlib_metadata/init.py line 9

I would agree with @asottile on this point for any module except os.path, in which import os is the sole exception. I can't recall where I made this shift, but I seem to recall it was due to some semi-official endorsement. Perhaps it was Barry ;).

[jaraco]

In GitLab by @warsaw on Jun 7, 2019, 20:59

@jaraco Ah, sorry, I merged it already. Should we revert and/or merge a better fix?

Also, does this need to be cross-ported to CPython's stdlib?

[jaraco]

In GitLab by @asottile on Jun 7, 2019, 21:00

Commented on importlib_metadata/init.py line 9

"Special cases aren't special enough to break the rules" "There should be one-- and preferably only one --obvious way to do it."

(except for os)

I'm fine changing it 😆

[jaraco]

In GitLab by @asottile on Jun 7, 2019, 21:00

this one hasn't been merged yet, that's the other one ;)

[jaraco]

In GitLab by @asottile on Jun 7, 2019, 21:03

One reason I used the suppress model was because it avoided potential race conditions.

I feel like the workaround is not well-placed here. Does this change imply that every attempt to try-open a file path should first check for is_file? I don't think so, but that's what it does.

If this change is to be accepted, I'd prefer for it to be more selective - to only run on PyPy versions that have the defect. What do you think about having a function in _compat that is a no-op except on affected versions... and then the Python 3.8 port could omit the call for that function, as it's not needed in the stdlib?

I don't think this should be ported to stdlib since pypy won't be using that.

I also don't think this creates any race conditions, it simply pre-checks that it's a file first to prevent the warning explosion

I don't think the change implies that every open call should be guarded by that, however in the case of importlib-metadata most of the calls to zipp.Path are going to fail because they are sys.path entries.

[jaraco]

In GitLab by @warsaw on Jun 7, 2019, 21:03

D'oh! ;)

[jaraco]

In GitLab by @asottile on Jun 7, 2019, 21:05

Commented on importlib_metadata/init.py line 9

changed this line in version 4 of the diff

[jaraco]

In GitLab by @asottile on Jun 7, 2019, 21:05

added 1 commit

• 1374a2a5 - Skip zipp.Path when it is not a file

Compare with previous version

[jaraco]

In GitLab by @warsaw on Jun 7, 2019, 21:06

Commented on importlib_metadata/init.py line 9

Yeah, that's the 20th ZoP! :)

[jaraco]

In GitLab by @asottile on Jun 7, 2019, 22:14

Commented on importlib_metadata/init.py line 370

changed this line in version 5 of the diff

[jaraco]

In GitLab by @asottile on Jun 7, 2019, 22:14

added 1 commit

• d16a9974 - Skip zipp.Path when it is not a file

Compare with previous version

[jaraco]

In GitLab by @asottile on Jun 7, 2019, 22:14

I've gone ahead and gated the check to only occur in pypy on affected versions

[jaraco]

In GitLab by @asottile on Jun 7, 2019, 22:27

added 1 commit

• 898b200 - Skip zipp.Path when it is not a file

Compare with previous version

[jaraco]

In GitLab by @jaraco on Jun 8, 2019, 24:32

What do you think of https://gitlab.com/python-devs/importlib_metadata/commit/fc4ea66351a21d9fdc443053254ff912ad928897 ? I mean 7d9b156?

[jaraco]

In GitLab by @asottile on Jun 8, 2019, 04:42

idk, kinda like my version more since there's less indirection and the constant better explains what is going on -- fine with that though if that's what you'd like -- another plus, my version avoids the extra function call and is 5 fewer lines 🤷

[jaraco]

In GitLab by @asottile on Jun 8, 2019, 04:43

the pragma is no branch not nobranch btw: https://coverage.readthedocs.io/en/coverage-4.3.4/branch.html#structurally-partial-branches

[jaraco]

In GitLab by @jaraco on Jun 9, 2019, 17:38

approved this merge request

[jaraco]

In GitLab by @jaraco on Jun 9, 2019, 17:38

merged

[jaraco]

In GitLab by @jaraco on Jun 9, 2019, 17:38

mentioned in commit 35c25b4

[jaraco]

In GitLab by @asottile on Jun 11, 2019, 16:11

Thanks! Looking forward to the 0.18 release :)

[jaraco]

In GitLab by @warsaw on Jun 11, 2019, 17:32

https://pypi.org/project/importlib-metadata/

[jaraco]

In GitLab by @jaraco on Jun 11, 2019, 18:26

mentioned in commit 72d70c9