Skip to content

gh-113659: Skip hidden .pth files #113660

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.

Sign up for GitHub

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

Merged
merged 7 commits into from
Jan 16, 2024
Merged

gh-113659: Skip hidden .pth files #113660

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
499fb95
gh-113659: Skip hidden .pth files
serhiy-storchaka Jan 2, 2024
7da1ed4
Fix typo.
serhiy-storchaka Jan 2, 2024
348130d
Add missed import in tests.
serhiy-storchaka Jan 2, 2024
dfbc6ef
Update Lib/test/test_site.py
serhiy-storchaka Jan 2, 2024
59ee5f3
Test also the hidden flag for symlink target.
serhiy-storchaka Jan 5, 2024
0cb3519
Test hidden file attribute on Windows.
serhiy-storchaka Jan 5, 2024
1cef7cd
Revert "Test also the hidden flag for symlink target."
serhiy-storchaka Jan 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion Lib/site.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@
import builtins
import _sitebuiltins
import io
import stat

# Prefixes for site-packages; add additional prefixes like /usr/local here
PREFIXES = [sys.prefix, sys.exec_prefix]
Expand Down Expand Up @@ -168,6 +169,14 @@ def addpackage(sitedir, name, known_paths):
else:
reset = False
fullname = os.path.join(sitedir, name)
try:
st = os.lstat(fullname)
except OSError:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hum, maybe add a _trace() call to help debugging such issue. Something like:

_trace(f"Skipping .pth file, stat failed: {exc!r}")

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When open() fails below, it is simply silently returns.

If we want to add a _trace() for OSError, we should do this in both cases (and maybe in more cases). But this is a different issue. It can add a noise where it was silent.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I didn't notice. In this case, would you mind to add a comment (in the 2 places) to explain that ignoring silently the file on OSError is a deliberate choice?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most OSErrors are silently ignored in this module and do not have a comment. I do not know what to add in these two places and why they are special. I also not sure that it is a deliberate choice, it can just be a copied behavior.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As you want, I already approved the PR.

return
Comment on lines +172 to +175
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm slightly worried about the performance impact of calling stat on all .pth files. pip install -e will install a .pth file for every installed editable package.

On the other hand, the overhead should be neglectable as the inode needs to be read in memory anyway to actually read the contents.

if ((getattr(st, 'st_flags', 0) & stat.UF_HIDDEN) or
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which platforms support UF_HIDDEN?

On macOS the flag is a hint that's ignored by command-line tools.

E.g.:

ronald@Pelargir[0]% touch foo.txt                                                                   
[~/Projects/Forks/cpython/build/X]
ronald@Pelargir[0]% chflags hidden foo.txt                                                          
[~/Projects/Forks/cpython/build/X]
ronald@Pelargir[0]% ls -l                                                                           (gh-53502-long-times)cpython
total 0
-rw-r--r--  1 ronald  staff  0 Jan  2 21:10 foo.txt
ronald@Pelargir[0]% ls -lO                                                                          
total 0
-rw-r--r--  1 ronald  staff  hidden 0 Jan  2 21:10 foo.txt

This matches the documentation in sys/stat.h:

#define UF_HIDDEN       0x00008000      /* hint that this item should not be */
                                        /* displayed in a GUI */

(getattr(st, 'st_file_attributes', 0) & stat.FILE_ATTRIBUTE_HIDDEN)):
_trace(f"Skipping hidden .pth file: {fullname!r}")
return
_trace(f"Processing .pth file: {fullname!r}")
try:
# locale encoding is not ideal especially on Windows. But we have used
Expand Down Expand Up @@ -221,7 +230,8 @@ def addsitedir(sitedir, known_paths=None):
names = os.listdir(sitedir)
except OSError:
return
names = [name for name in names if name.endswith(".pth")]
names = [name for name in names
if name.endswith(".pth") and not name.startswith(".")]
for name in sorted(names):
addpackage(sitedir, name, known_paths)
if reset:
Expand Down
40 changes: 40 additions & 0 deletions Lib/test/test_site.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import os
import re
import shutil
import stat
import subprocess
import sys
import sysconfig
Expand Down Expand Up @@ -194,6 +195,45 @@ def test_addsitedir(self):
finally:
pth_file.cleanup()

def test_addsitedir_dotfile(self):
pth_file = PthFile('.dotfile')
pth_file.cleanup(prep=True)
try:
pth_file.create()
site.addsitedir(pth_file.base_dir, set())
self.assertNotIn(site.makepath(pth_file.good_dir_path)[0], sys.path)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe mock site.addpackage() and check that it's not called, rather than checking effects of site.addpackage() implementation. Same remark for the other added tests.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not called for dotfiles, but is called for files with hidden attribute. It is rather an implementation detail where we add a filter.

self.assertIn(pth_file.base_dir, sys.path)
finally:
pth_file.cleanup()

@unittest.skipUnless(hasattr(os, 'chflags'), 'test needs os.chflags()')
def test_addsitedir_hidden_flags(self):
pth_file = PthFile()
pth_file.cleanup(prep=True)
try:
pth_file.create()
st = os.stat(pth_file.file_path)
os.chflags(pth_file.file_path, st.st_flags | stat.UF_HIDDEN)
site.addsitedir(pth_file.base_dir, set())
self.assertNotIn(site.makepath(pth_file.good_dir_path)[0], sys.path)
self.assertIn(pth_file.base_dir, sys.path)
finally:
pth_file.cleanup()

@unittest.skipUnless(sys.platform == 'win32', 'test needs Windows')
@support.requires_subprocess()
def test_addsitedir_hidden_file_attribute(self):
pth_file = PthFile()
pth_file.cleanup(prep=True)
try:
pth_file.create()
subprocess.check_call(['attrib', '+H', pth_file.file_path])
site.addsitedir(pth_file.base_dir, set())
self.assertNotIn(site.makepath(pth_file.good_dir_path)[0], sys.path)
self.assertIn(pth_file.base_dir, sys.path)
finally:
pth_file.cleanup()

# This tests _getuserbase, hence the double underline
# to distinguish from a test for getuserbase
def test__getuserbase(self):
Expand Down
1 change: 1 addition & 0 deletions Misc/NEWS.d/next/Security/2024-01-02-19-52-23.gh-issue-113659.DkmnQc.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Skip ``.pth`` files with names starting with a dot or hidden file attribute.