doc: recursive glob ** follows symlinks to directories #12918

marc-h38 · 2019-04-23T07:16:58Z

The existing note does already warn that "Using the ** pattern in large
directory trees may consume an inordinate amount of time", however
following symbolic links to directories brings that to entirely new
levels; learned the hard way.

I initially assumed this was a bug, tentatively fixed with a simple:

-   if not dironly or entry.is_dir():
+   if not dironly or entry.is_dir(follow_symlinks=False):

... in glob.py#_iterdir(dirname, dironly):

Later I found https://bugs.python.org/issue13968 in the git log and saw
this is working as intended with even a symlink loop test.

the-knights-who-say-ni · 2019-04-23T07:17:01Z

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

You can check yourself to see if the CLA has been received.

Thanks again for your contribution, we look forward to reviewing it!

marc-h38 · 2019-04-23T07:57:24Z

cc @serhiy-storchaka

The initial purpose was to replace glob.glob(**, recursive=True) with pathlib.Path.glob(**) because the former follows symbolic links to directories in recursive mode (including "infinite" loops), whereas the latter (and more modern) doesn't. While trying to stick to this change, testing found a significant number of other issues and addressing all of them unfortunately turned into this significant rewrite. The entire Codeowners check is too small to isolate every change in separate commit, sorry for the resulting "bulk" review work load (~100 lines) Here's a simplified and extreme reproduction: - run a fairly extensive sanitycheck run. This creates 100,000s of files in sanity-out (thank you cmake) - observe how check_compliance.py -m Codeowners now takes minutes - create the following symbolic link: ln -s ../zephyr myself - observe how check_compliance.py -m Codeowners now takes hours or more. Only "hours" and not infinity because operating systems have limits to break infinite loops of symbolic links. Now considering Zephyr supports Windows and none of the multiple flavors of symbolic links offered by Windows is usable, it's unlikely Zephyr will ever _require_ symbolic links. However this shouldn't _forbid_ other users to use symbolic links privately. Funny enough neither glob nor pathlib documents whether they follow symbolic links to directories in recursive mode. "glob" does and "pathlib" doesn't. So I thought this was bug in glob and even had a tentative fix for it. Then I scanned the glob's git log and found it's actually a "feature": python/cpython#12918. glob's own documentation mentions pathlib as a higher-level and newer alternative: let's switch to pathlib. Other changes: - Fixed failing "path not found" detection of bogus lines in CODEOWNERS. glob seems to have a bug where glob.glob('doesntexist/**', recursive=True) returns 'doesntexit/' instead of '' (easter egg: there is one bogus line right now) - Successfully tested on Windows. Requires hack replacing sh with pbs and disabling a couple unrelated imports though. Previous glob version failed to match, new files were never owned. - Remove (accidental?) limitation to run from the git top level. Moved git top-level command to a global and remove the same limitation on -m Checkpatch too. - Tighten the regular expression parsing CODEOWNERS lines to actually report invalid lines: make sure pattern starts in first character; make sure the owner is not mister whitespace. Performance: - Down from 1 sec to less than 0.5s in the most common case: not adding any new file. Fix broken optimization that still scanned the whole tree even when there are no new files (hint: [''] is not false) - About 20% faster on large sets without symbolic links (up to "infinitely" faster *with* symlinks) One possible reason: the list of owned files is now split per CODEOWNERS line for better logging which also means scanning smaller lists. Other minor fixes: - Remove unused git ls-files invocation, replaced with an optimization TODO. - Use more portable splitlines() instead of split('\n') - renamed parse_codeowners() to ls_owned_files() because but it does far more than parsing CODEOWNERS - Use "early returns" and extract new function to reduce cyclomatic complexity (up to 9 levels of indentation before in parse_codeowners(). Other minor simplifications. - Add some pydocs and logging Signed-off-by: Marc Herbert <marc.herbert@intel.com>

serhiy-storchaka · 2019-05-07T08:40:45Z

It is technically correct, but since my English is bad I'm not sure about the best phrasing.

The initial purpose was to replace glob.glob(**, recursive=True) with pathlib.Path.glob(**) because the former follows symbolic links to directories in recursive mode (including "infinite" loops), whereas the latter (and more modern) doesn't. While trying to stick to this change, testing found a significant number of other issues and addressing all of them unfortunately turned into this significant rewrite. The entire Codeowners check is too small to isolate every change in separate commit, sorry for the resulting "bulk" review work load (~100 lines) Here's a simplified and extreme reproduction: - run a fairly extensive sanitycheck run. This creates 100,000s of files in sanity-out (thank you cmake) - observe how check_compliance.py -m Codeowners now takes minutes - create the following symbolic link: ln -s ../zephyr myself - observe how check_compliance.py -m Codeowners now takes hours or more. Only "hours" and not infinity because operating systems have limits to break infinite loops of symbolic links. Now considering Zephyr supports Windows and none of the multiple flavors of symbolic links offered by Windows is usable, it's unlikely Zephyr will ever _require_ symbolic links. However this shouldn't _forbid_ other users to use symbolic links privately. Funny enough neither glob nor pathlib documents whether they follow symbolic links to directories in recursive mode. "glob" does and "pathlib" doesn't. So I thought this was bug in glob and even had a tentative fix for it. Then I scanned the glob's git log and found it's actually a "feature": python/cpython#12918. glob's own documentation mentions pathlib as a higher-level and newer alternative: let's switch to pathlib. Other changes: - Fixed failing "path not found" detection of bogus lines in CODEOWNERS. glob seems to have a bug where glob.glob('doesntexist/**', recursive=True) returns 'doesntexit/' instead of '' (easter egg: there is one bogus line right now) - Successfully tested on Windows. Requires hack replacing sh with pbs and disabling a couple unrelated imports though. Previous glob version failed to match, new files were never owned. - Remove (accidental?) limitation to run from the git top level. Moved git top-level command to a global and remove the same limitation on -m Checkpatch too. - Tighten the regular expression parsing CODEOWNERS lines to actually report invalid lines: make sure pattern starts in first character; make sure the owner is not mister whitespace. Performance: - Down from 1 sec to less than 0.5s in the most common case: not adding any new file. Fix broken optimization that still scanned the whole tree even when there are no new files (hint: [''] is not false) - About 20% faster on large sets without symbolic links (up to "infinitely" faster *with* symlinks) One possible reason: the list of owned files is now split per CODEOWNERS line for better logging which also means scanning smaller lists. Other minor fixes: - Remove unused git ls-files invocation, replaced with an optimization TODO. - Use more portable splitlines() instead of split('\n') - renamed parse_codeowners() to ls_owned_files() because but it does far more than parsing CODEOWNERS - Use "early returns" and extract new function to reduce cyclomatic complexity (up to 9 levels of indentation before in parse_codeowners(). Other minor simplifications. - Add some pydocs and logging Signed-off-by: Marc Herbert <marc.herbert@intel.com>

marc-h38 · 2019-06-07T17:15:39Z

@vstinner , @ncoghlan , @bitdancer, could you review this one-line doc change or help find someone who can? Just picking up semi-random names from the git log and from https://bugs.python.org/issue13968, sorry.

ncoghlan · 2019-06-09T06:45:48Z

My main question would be with the second half of the sentence: I think we follow symlinks regardless, but the revised wording makes it sound like appending / turns off the symlink following as well.

(I suspect there's another error in that wording as well: when os.altsep is defined, appending it should have the same effect as appending os.sep)

marc-h38 · 2019-06-19T08:07:40Z

but the revised wording makes it sound like appending / turns off the symlink following as well.

Good catch, thanks for the review. How about something like this then:

If *recursive* is true, the pattern ** will match any files and zero or
more directories, subdirectories and symbolic links to directories. If the
pattern is followed by an ``os.sep`` [or os.altsep] then files won't match.

I will test os.altsep. UPDATE: a trailing os.altsep has the same effect as a trailing os.sep. Will mention both.

ncoghlan · 2019-06-21T12:11:06Z

@marc-h38 Aye, that revised wording looks good to me.

The existing note does already warn that "Using the ** pattern in large directory trees may consume an inordinate amount of time", however following symbolic links to directories brings that to entirely new levels; learned the hard way. I initially assumed this was a bug, tentatively fixed with a simple: - if not dironly or entry.is_dir(): + if not dironly or entry.is_dir(follow_symlinks=False): ... in glob.py#_iterdir(dirname, dironly): Later I found https://bugs.python.org/issue13968 in the git log and saw this is working as intended with even a symlink loop test.

marc-h38 · 2019-07-11T01:37:14Z

@ncoghlan , others: friendly ping? Should be a no-brainer now.

serhiy-storchaka · 2019-08-21T18:48:47Z

Symlinks to files will not match as well. But symlinks to directories will match.

aeros

Thanks for the PR @marc-h38!

The proposed changes look good to me, but I would recommend some minor formatting adjustments:

Doc/library/glob.rst

marc-h38

Symlinks to files will not match either (when there's a trailing '/')

The sole purpose of this commit is to highlight that recursive glob ** treats symlinks to directories like directories. That's because symlinks to directories are relatively "evil": for instance they can easily cause infinite loops. Learned the hard way. For comparison, pathlib does not recurse into symbolic links to directories.

On the other hand, there is nowhere near as much evil in symbolic link to files.

The current documentation does not make any reference to symbolic link to files, should I introduce this for the first time?

Doc/library/glob.rst

- ``os.sep`` or ``os.altsep`` + :data:`os.sep` or :data:`os.altsep` Co-Authored-By: Kyle Stanley <aeros167@gmail.com>

marc-h38 · 2019-09-03T23:25:27Z

@serhiy-storchaka do you think this should be expanded even more and should mention symbolic links to files too?

(The risk being of course that no correction at all ever happens due to loss of interest from all parties ;-)

miss-islington · 2019-09-11T17:17:08Z

Thanks @marc-h38 for the PR, and @JulienPalard for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7, 3.8.
🐍🍒⛏🤖

miss-islington · 2019-09-11T17:17:11Z

Sorry @marc-h38 and @JulienPalard, I had trouble checking out the 3.8 backport branch.
Please backport using cherry_picker on command line.
cherry_picker e24594bfe75aff3e654665cb940ddc4d4acffd2f 3.8

(cherry picked from commit e24594b) Co-authored-by: Marc <Marc.Herbert+github@gmail.com>

bedevere-bot · 2019-09-11T17:17:20Z

GH-15995 is a backport of this pull request to the 3.7 branch.

JulienPalard · 2019-09-11T17:18:00Z

@marc-h38 thanks for enhancing the documentation, the current state of it looks good to me. Telling further about glob not "following symlink files" would probably just add too much and make the paragraph less easy to understand. Thanks!

miss-islington · 2019-09-11T17:18:57Z

Thanks @marc-h38 for the PR, and @JulienPalard for merging it 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖

(cherry picked from commit e24594b) Co-authored-by: Marc <Marc.Herbert+github@gmail.com>

bedevere-bot · 2019-09-11T17:19:06Z

GH-15996 is a backport of this pull request to the 3.8 branch.

(cherry picked from commit e24594b) Co-authored-by: Marc <Marc.Herbert+github@gmail.com>

The initial purpose was to replace glob.glob(**, recursive=True) with pathlib.Path.glob(**) because the former follows symbolic links to directories in recursive mode (including "infinite" loops), whereas the latter (and more modern) doesn't. While trying to stick to this change, testing found a significant number of other issues and addressing all of them unfortunately turned into this significant rewrite. The entire Codeowners check is too small to isolate every change in separate commit, sorry for the resulting "bulk" review work load (~100 lines) Here's a simplified and extreme reproduction: - run a fairly extensive sanitycheck run. This creates 100,000s of files in sanity-out (thank you cmake) - observe how check_compliance.py -m Codeowners now takes minutes - create the following symbolic link: ln -s ../zephyr myself - observe how check_compliance.py -m Codeowners now takes hours or more. Only "hours" and not infinity because operating systems have limits to break infinite loops of symbolic links. Now considering Zephyr supports Windows and none of the multiple flavors of symbolic links offered by Windows is usable, it's unlikely Zephyr will ever _require_ symbolic links. However this shouldn't _forbid_ other users to use symbolic links privately. Funny enough neither glob nor pathlib documents whether they follow symbolic links to directories in recursive mode. "glob" does and "pathlib" doesn't. So I thought this was bug in glob and even had a tentative fix for it. Then I scanned the glob's git log and found it's actually a "feature": python/cpython#12918. glob's own documentation mentions pathlib as a higher-level and newer alternative: let's switch to pathlib. Other changes: - Fixed failing "path not found" detection of bogus lines in CODEOWNERS. glob seems to have a bug where glob.glob('doesntexist/**', recursive=True) returns 'doesntexit/' instead of '' (easter egg: there is one bogus line right now) - Successfully tested on Windows. Requires hack replacing sh with pbs and disabling a couple unrelated imports though. Previous glob version failed to match, new files were never owned. - Remove (accidental?) limitation to run from the git top level. Moved git top-level command to a global and remove the same limitation on -m Checkpatch too. - Tighten the regular expression parsing CODEOWNERS lines to actually report invalid lines: make sure pattern starts in first character; make sure the owner is not mister whitespace. Performance: - Down from 1 sec to less than 0.5s in the most common case: not adding any new file. Fix broken optimization that still scanned the whole tree even when there are no new files (hint: [''] is not false) - About 20% faster on large sets without symbolic links (up to "infinitely" faster *with* symlinks) One possible reason: the list of owned files is now split per CODEOWNERS line for better logging which also means scanning smaller lists. Other minor fixes: - Remove unused git ls-files invocation, replaced with an optimization TODO. - Use more portable splitlines() instead of split('\n') - renamed parse_codeowners() to ls_owned_files() because but it does far more than parsing CODEOWNERS - Use "early returns" and extract new function to reduce cyclomatic complexity (up to 9 levels of indentation before in parse_codeowners(). Other minor simplifications. - Add some pydocs and logging Signed-off-by: Marc Herbert <marc.herbert@intel.com>

bedevere-bot added the docs Documentation in the Doc dir label Apr 23, 2019

the-knights-who-say-ni added the CLA not signed label Apr 23, 2019

bedevere-bot added the awaiting review label Apr 23, 2019

marc-hb mentioned this pull request Apr 26, 2019

Codeowners: replace glob with pathlib. Many other fixes. zephyrproject-rtos/ci-tools#54

Merged

the-knights-who-say-ni added CLA signed and removed CLA not signed labels May 2, 2019

ncoghlan added needs backport to 3.7 skip issue skip news labels Jun 21, 2019

marc-h38 force-pushed the doc-glob-recursive-symlinks branch from 070276d to fd5babb Compare July 8, 2019 04:42

aeros approved these changes Aug 22, 2019

View reviewed changes

Doc/library/glob.rst Outdated Show resolved Hide resolved

bedevere-bot added awaiting core review and removed awaiting review labels Aug 22, 2019

marc-h38 commented Aug 22, 2019

View reviewed changes

Doc/library/glob.rst Outdated Show resolved Hide resolved

Update Doc/library/glob.rst

fd5eda2

- ``os.sep`` or ``os.altsep`` + :data:`os.sep` or :data:`os.altsep` Co-Authored-By: Kyle Stanley <aeros167@gmail.com>

JulienPalard merged commit e24594b into python:master Sep 11, 2019

bedevere-bot removed the awaiting core review label Sep 11, 2019

miss-islington assigned JulienPalard Sep 11, 2019

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 11, 2019

Doc: recursive glob ** follows symlinks to directories (pythonGH-12918)

0104dcb

(cherry picked from commit e24594b) Co-authored-by: Marc <Marc.Herbert+github@gmail.com>

bedevere-bot removed the needs backport to 3.7 label Sep 11, 2019

JulienPalard added needs backport to 3.8 and removed needs backport to 3.8 labels Sep 11, 2019

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 11, 2019

Doc: recursive glob ** follows symlinks to directories (pythonGH-12918)

9af08a8

(cherry picked from commit e24594b) Co-authored-by: Marc <Marc.Herbert+github@gmail.com>

bedevere-bot removed the needs backport to 3.8 label Sep 11, 2019

miss-islington added a commit that referenced this pull request Sep 11, 2019

Doc: recursive glob ** follows symlinks to directories (GH-12918)

9b46586

(cherry picked from commit e24594b) Co-authored-by: Marc <Marc.Herbert+github@gmail.com>

miss-islington added a commit that referenced this pull request Sep 11, 2019

Doc: recursive glob ** follows symlinks to directories (GH-12918)

d479936

(cherry picked from commit e24594b) Co-authored-by: Marc <Marc.Herbert+github@gmail.com>

DinoV pushed a commit to DinoV/cpython that referenced this pull request Sep 12, 2019

Doc: recursive glob ** follows symlinks to directories (pythonGH-12918)

b15aa44

Uh oh!

doc: recursive glob ** follows symlinks to directories #12918

doc: recursive glob ** follows symlinks to directories #12918

Uh oh!

Conversation

marc-h38 commented Apr 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

the-knights-who-say-ni commented Apr 23, 2019

Uh oh!

marc-h38 commented Apr 23, 2019

Uh oh!

serhiy-storchaka commented May 7, 2019

Uh oh!

marc-h38 commented Jun 7, 2019

Uh oh!

ncoghlan commented Jun 9, 2019

Uh oh!

marc-h38 commented Jun 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ncoghlan commented Jun 21, 2019

Uh oh!

marc-h38 commented Jul 11, 2019

Uh oh!

serhiy-storchaka commented Aug 21, 2019

Uh oh!

aeros left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

marc-h38 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

marc-h38 commented Sep 3, 2019

Uh oh!

miss-islington commented Sep 11, 2019

Uh oh!

miss-islington commented Sep 11, 2019

Uh oh!

bedevere-bot commented Sep 11, 2019

Uh oh!

JulienPalard commented Sep 11, 2019

Uh oh!

miss-islington commented Sep 11, 2019

Uh oh!

bedevere-bot commented Sep 11, 2019

Uh oh!

Uh oh!

marc-h38 commented Apr 23, 2019 •

edited

Loading

marc-h38 commented Jun 19, 2019 •

edited

Loading