GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — GH-78079: Fix UNC device path root normalization in pathlib by barneygale · Pull Request #102003 · python/cpython

Conversation

barneygale commented Feb 17, 2023 •

edited by bedevere-bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

We no longer add a root to device paths such as //./PhysicalDrive0, //?/BootPartition and //./c: while normalizing. We also avoid adding a root to incomplete UNC share paths, like //, //a and //a/.

This isn't easy to backport as the the surrounded code has changed a lot since 3.11.

Issue: [pathlib] issues with Windows device paths #78079

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          pythonGH-78079: Fix UNC device path root normalization in pathlib

99f62bc

We no longer add a root to device paths such as `//./PhysicalDrive0`,
`//?/BootPartition` and `//./c:` while normalizing. We also avoid adding a
root to incomplete UNC share paths, like `//`, `//a` and `//a/`.

barneygale added OS-windows topic-pathlib labels

Feb 17, 2023

bedevere-bot added the awaiting review label

Feb 17, 2023

bedevere-bot mentioned this pull request

Feb 17, 2023

[pathlib] issues with Windows device paths #78079

Closed


          A few more test cases

6651b67

zooba approved these changes

Feb 20, 2023

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting review labels

Feb 20, 2023


          Merge branch 'main' into pythongh-78079-win-device-path-roots

87519ba

barneygale commented Mar 10, 2023

Copy link

Copy Markdown

Contributor Author

Hey @eryksun, does this PR look OK to you?

As I mentioned on the issue, it does not fix //./Global and //?/Global; those are somewhat complicated and I'd like to tackle them in a follow-up. Hope that makes sense?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

eryksun commented Mar 10, 2023

Copy link

Copy Markdown

Contributor

As I mentioned on the issue, it does not fix //./Global and //?/Global; those are somewhat complicated and I'd like to tackle them in a follow-up. Hope that makes sense?

Handling "Global" is a low priority. It turns out that handling "GLOBALROOT" is a more pressing concern, but it's a small enough niche and such a complicated problem that I think leaving the drive as "\\?\GLOBALROOT" is good enough.

os.readlink() has to be updated to support returning "\\?\GLOBALROOT" paths because custom symlinks can target arbitrary NT object paths. For example, if SYMLINK_FLAG_RELATIVE isn't set in the symlink reparse buffer, then the target path "\ContainerMappedDirectories" has to be returned as "\\?\GLOBALROOT\ContainerMappedDirectories". Refer to the discussion in issue #102440 and the sample code I posted in this comment.

barneygale reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

eryksun reviewed

Mar 10, 2023

View reviewed changes

Comment thread

Lib/pathlib.py Resolved

Uh oh!

There was an error while loading. Please reload this page.


          Update Lib/pathlib.py

c444a3c

Co-authored-by: Eryk Sun <[email protected]>

eryksun reviewed

Mar 10, 2023

View reviewed changes

Comment thread

Lib/pathlib.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

barneygale marked this pull request as draft

March 10, 2023 20:57

barneygale added 3 commits

March 10, 2023 22:10


          Fix handling of UNC paths with incomplete drives in glob(), walk(), etc.

36623c0


          Test handling of UNC paths with incomplete drives in joinpath()

aea236e


          Merge branch 'main' into pythongh-78079-win-device-path-roots

bb0af91

barneygale commented Mar 24, 2023

Copy link

Copy Markdown

Contributor Author

This is presently blocked on #102789.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          Merge branch 'main' into pythongh-78079-win-device-path-roots

f412f87

barneygale commented Apr 3, 2023

Copy link

Copy Markdown

Contributor Author

#102789 has now landed, but it turns out we're blocked on another bug! PR here: #103221

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          Merge branch 'main' into pythongh-78079-win-device-path-roots

8ff07fc

zooba reviewed

Apr 11, 2023

View reviewed changes

Comment thread

Lib/pathlib.py

zooba Apr 11, 2023

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Whoa, we sys.intern everything? That seems like a painful memory leak. Would we be better off with a class level lru_cache'd function¹?

Don't we already know that rel will be str here? If it's bytes, str(x) is the wrong conversion anyway. Maybe the intern function should be cls._flavour.str_and_intern?

Footnotes

@lru_cache(max_size=REASONABLE_VALUE) def intern(x): return x ↩

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

barneygale Apr 11, 2023 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

The interning of the path parts is longstanding pathlib behaviour. Honestly I don't know enough about the benefits/drawbacks of interning to say whether it's reasonable.

Don't we already know that rel will be str here?

We know that it's an instance of str (and not bytes), but we don't know whether it's a true str object (required by sys.intern()).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

eryksun Apr 11, 2023

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Whoa, we sys.intern everything? That seems like a painful memory leak.

Is it really all that bad? sys.intern() calls PyUnicode_InternInPlace(), not PyUnicode_InternImmortal(). An interned string gets deallocated based on its reference count, like any other object. For example:

>>> s = sys.intern('spam and eggs')
>>> t = sys.intern('spam and eggs')
>>> s is t
True
>>> id(s)
139835455199152
>>> del s, t
>>> s = sys.intern('spam and eggs')
>>> id(s)
139835455198912

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          Merge branch 'main' into pythongh-78079-win-device-path-roots

723a5aa

barneygale marked this pull request as ready for review

April 11, 2023 16:52

barneygale commented Apr 14, 2023

Copy link

Copy Markdown

Contributor Author

Hey @eryksun, are you happy for this to land? Thanks

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

eryksun commented Apr 14, 2023

Copy link

Copy Markdown

Contributor

LGTM. The implementation of pathlib has changed a lot since I opened #78079 almost 5 years ago.

barneygale reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

barneygale merged commit 8af8f52 into python:main

Apr 14, 2023

bedevere-bot removed the awaiting merge label

Apr 14, 2023

barneygale commented Apr 14, 2023

Copy link

Copy Markdown

Contributor Author

Thank you both for the reviews + advice!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bedevere-bot commented Apr 14, 2023

Copy link

Copy Markdown

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot ARM Raspbian 3.x has failed when building commit 8af8f52.

What do you need to do:

Don't panic.
Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/424/builds/3700) and take a look at the build logs.
Check if the failure is related to this commit (8af8f52) or if it is a false positive.
If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/424/builds/3700

Summary of the results of the build (if available):

== Tests result: ENV CHANGED ==

413 tests OK.

10 slowest tests:

test_venv: 7 min 28 sec
test_asyncio: 6 min 41 sec
test_largefile: 5 min 30 sec
test_math: 4 min 26 sec
test_multiprocessing_spawn: 4 min 24 sec
test_concurrent_futures: 3 min 8 sec
test___all__: 2 min 27 sec
test_cppext: 2 min 13 sec
test_tokenize: 2 min 7 sec
test_multiprocessing_forkserver: 2 min 7 sec

1 test altered the execution environment:
test_concurrent_futures

20 tests skipped:
test_devpoll test_idle test_ioctl test_kqueue test_launcher
test_msilib test_peg_generator test_perf_profiler test_startfile
test_tcl test_tix test_tkinter test_ttk test_ttk_textonly
test_turtle test_winconsoleio test_winreg test_winsound test_wmi
test_zipfile64

Total duration: 30 min 33 sec

Click to see traceback logs

remote: Enumerating objects: 22, done.        
remote: Counting objects:   4% (1/22)        
remote: Counting objects:   9% (2/22)        
remote: Counting objects:  13% (3/22)        
remote: Counting objects:  18% (4/22)        
remote: Counting objects:  22% (5/22)        
remote: Counting objects:  27% (6/22)        
remote: Counting objects:  31% (7/22)        
remote: Counting objects:  36% (8/22)        
remote: Counting objects:  40% (9/22)        
remote: Counting objects:  45% (10/22)        
remote: Counting objects:  50% (11/22)        
remote: Counting objects:  54% (12/22)        
remote: Counting objects:  59% (13/22)        
remote: Counting objects:  63% (14/22)        
remote: Counting objects:  68% (15/22)        
remote: Counting objects:  72% (16/22)        
remote: Counting objects:  77% (17/22)        
remote: Counting objects:  81% (18/22)        
remote: Counting objects:  86% (19/22)        
remote: Counting objects:  90% (20/22)        
remote: Counting objects:  95% (21/22)        
remote: Counting objects: 100% (22/22)        
remote: Counting objects: 100% (22/22), done.        
remote: Compressing objects:   9% (1/11)        
remote: Compressing objects:  18% (2/11)        
remote: Compressing objects:  27% (3/11)        
remote: Compressing objects:  36% (4/11)        
remote: Compressing objects:  45% (5/11)        
remote: Compressing objects:  54% (6/11)        
remote: Compressing objects:  63% (7/11)        
remote: Compressing objects:  72% (8/11)        
remote: Compressing objects:  81% (9/11)        
remote: Compressing objects:  90% (10/11)        
remote: Compressing objects: 100% (11/11)        
remote: Compressing objects: 100% (11/11), done.        
remote: Total 12 (delta 10), reused 2 (delta 1), pack-reused 0        
From https://github.com/python/cpython
 * branch                  main       -> FETCH_HEAD
Note: switching to '8af8f52d175959f03cf4a2786b6a81ec09e15e95'.

You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by switching back to a branch.

If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -c with the switch command. Example:

  git switch -c <new-branch-name>

Or undo this operation with:

  git switch -

Turn off this advice by setting config variable advice.detachedHead to false

HEAD is now at 8af8f52d17 GH-78079: Fix UNC device path root normalization in pathlib (GH-102003)
Switched to and reset branch 'main'

Objects/obmalloc.c:775:1: warning: ‘always_inline’ function might not be inlinable [-Wattributes]
  775 | arena_map_get(pymem_block *p, int create)
      | ^~~~~~~~~~~~~

make: *** [Makefile:1950: buildbottest] Error 3

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

carljm added a commit to carljm/cpython that referenced this pull request

Apr 17, 2023


          Merge branch 'main' into superopt

* main:
  Remove `expert-*` from `project-updater` GH workflow (python#103579)
  pythongh-103583: Add codecs and maps to _codecs_* module state (python#103540)
  pythongh-48330: address review comments to PR-12271 (python#103209)
  pythongh-103527: Add multibytecodec.h as make dep for _codecs_* (python#103567)
  pythongh-103553: Improve `test_inspect`: add more assertions, remove unused (python#103554)
  pythonGH-103517: Improve tests for `pathlib.Path.walk()` (pythonGH-103518)
  pythongh-102114: Make dis print more concise tracebacks for syntax errors in str inputs (python#102115)
  pythonGH-78079: Fix UNC device path root normalization in pathlib (pythonGH-102003)
  pythongh-101517: Add regression test for a lineno bug in try/except* impacting pdb (python#103547)
  pythongh-103527: Add make deps for _codecs_* and _multibytecodec (python#103528)
  pythongh-103532: Fix reST syntax in NEWS entry (pythonGH-103544)
  pythongh-103532: Add NEWS entry (python#103542)

carljm added a commit to carljm/cpython that referenced this pull request

Apr 17, 2023


          Merge branch 'superopt' into superopt_spec

079c65a

* superopt:
  update generated cases with new comment
  review comments
  Remove `expert-*` from `project-updater` GH workflow (python#103579)
  pythongh-103583: Add codecs and maps to _codecs_* module state (python#103540)
  pythongh-48330: address review comments to PR-12271 (python#103209)
  pythongh-103527: Add multibytecodec.h as make dep for _codecs_* (python#103567)
  pythongh-103553: Improve `test_inspect`: add more assertions, remove unused (python#103554)
  pythonGH-103517: Improve tests for `pathlib.Path.walk()` (pythonGH-103518)
  pythongh-102114: Make dis print more concise tracebacks for syntax errors in str inputs (python#102115)
  pythonGH-78079: Fix UNC device path root normalization in pathlib (pythonGH-102003)
  pythongh-101517: Add regression test for a lineno bug in try/except* impacting pdb (python#103547)
  pythongh-103527: Add make deps for _codecs_* and _multibytecodec (python#103528)
  pythongh-103532: Fix reST syntax in NEWS entry (pythonGH-103544)
  pythongh-103532: Add NEWS entry (python#103542)

This was referenced Jun 17, 2023

add a feature on my updater billbill #105884

Closed

Difference in behaviour of PureWindowsPath in 3.12.0b2 with multiple slashes #105881

Closed

Reviewers

eryksun

eryksun left review comments

zooba

zooba approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

OS-windows topic-pathlib

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

barneygale commented Feb 17, 2023 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

barneygale commented Mar 10, 2023

Uh oh!

eryksun commented Mar 10, 2023

Uh oh!

Uh oh!

Uh oh!

barneygale commented Mar 24, 2023

Uh oh!

barneygale commented Apr 3, 2023

Uh oh!

zooba Apr 11, 2023

Choose a reason for hiding this comment

Footnotes

Uh oh!

barneygale Apr 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eryksun Apr 11, 2023

Choose a reason for hiding this comment

Uh oh!

barneygale commented Apr 14, 2023

Uh oh!

eryksun commented Apr 14, 2023

Uh oh!

barneygale commented Apr 14, 2023

Uh oh!

bedevere-bot commented Apr 14, 2023

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

barneygale commented Feb 17, 2023 •

edited by bedevere-bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

barneygale Apr 11, 2023 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.