GV

GeezerVison

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

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — GH-89812: Add `pathlib._PathBase` by barneygale · Pull Request #106337 · python/cpython

Conversation

@barneygale

@barneygale barneygale commented Jul 2, 2023 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

Add private pathlib._PathBase class. This will be used by an experimental PyPI package to incubate a tarfile.TarPath class.

Implementation notes

In pathlib.py:

  • Split Path into _PathBase and Path
  • Implement _PathBase.is_junction() and is_mount() using stat(). In the latter case, restore the implementation deleted in gh-86943: implement pathlib.WindowsPath.is_mount() #31458
  • Implement _PathBase.resolve() using absolute() and readlink()
  • Implement _PathBase._scandir() using iterdir()

In test_pathlib.py:

  • Add PathBaseTest to exercise the _PathBase class directly
  • Split PathTest into DummyPathTest and PathTest. The former exercises a representative subclass of _PathBase.
  • Add DummyPathWithSymlinksTest to exercise a similar subclass, this time with symlink support.

Questionable bits

In pathlib.py, the _PathBase.is_junction() and _PathBase.resolve() methods are new code. The latter is very performance-sensitive and may need more work.

Future work

  • Make pathlib._PathBase public. There's still at least a couple of things to do first:
    • Make PurePath._flavour public
    • Figure out what to do with _PathBase.__hash__(), __eq__(), and other comparison methods.

  • Issue: Add tarfile.TarPath #89812

Sorry, something went wrong.

Uh oh!

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

CAM-Gerlach reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@barneygale
Add pathlib._VirtualPath
4b29e2e
@barneygale barneygale added the topic-pathlib label Jul 2, 2023
@bedevere-bot bedevere-bot mentioned this pull request Jul 2, 2023
Add tarfile.TarPath #89812
Open
barneygale added 9 commits July 2, 2023 19:42
@barneygale
Add tests for pathlib._VirtualPath
8ce0139
@barneygale
Fix tests on Windows
b850d11
@barneygale
Fix tests on Windows (take 2)
39bf6b3
@barneygale
Fix tests on Windows (take 3)
0515dea
@barneygale
Fix tests on Windows (take 4)
596016f
@barneygale
Add tarfile.TarPath
1a6122b
@barneygale
Add docs for tarfile.TarPath
6833ed8
@barneygale
Add tests for tarfile.TarPath
4d2e8a9
@barneygale
Merge branch 'main' into pythongh-89812-omgtarpath
e4daac9
@barneygale
barneygale marked this pull request as ready for review July 3, 2023 20:44
@barneygale
barneygale requested a review from ethanfurman as a code owner July 3, 2023 20:44
@bedevere-bot bedevere-bot added the awaiting core review label Jul 3, 2023
This was referenced Jul 6, 2023
Heads up: pathlib._PathBase may be coming soon fsspec/universal_pathlib#114
Closed
Heads up: pathlib.VirtualPath may be coming soon drivendataorg/cloudpathlib#347
Open
Heads up: pathlib.VirtualPath may be coming soon justindujardin/pathy#105
Closed
Heads up: pathlib.VirtualPath may be coming soon liormizr/s3path#140
Open
@barneygale

barneygale commented Jul 7, 2023 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor Author

@brettcannon a long time ago in a PR far, far away, you said:

My current thinking (while I work through my PR review backlog to reach this PR), is this should be used for zipfile.Path. I also wouldn't be opposed to that happening this PR if @barneygale wanted to give that a shot while he waits for a review from me.

I couldn't make this work for zipfile.Path due to a couple of fundamental differences in the interface, e.g. the root attribute stores a string in pathlib.PurePath, but a ZipFile instance in zipfile.Path. It's possible to make it work with a couple of deprecations/removals, or an entirely new ZipPath class.

So in this PR I've chosen tarfile.TarPath as the initial target. I think it does a good job of showing the viability of the pathlib._VirtualPath interface.

However, I have a bunch of open questions in the PR description about how TarPath itself should work. It's not as straightforward as .zip files, because tarballs support symlinks and hardlinks, and because tarfile.TarFile doesn't support reading and writing the same file.

These questions make me think that tarfile.TarPath should first be gestated in a PyPI package before we add it to the standard library. But that leaves a bit of a chicken-and-egg problem with getting pathlib._VirtualPath in.

Do you have any advice on the best way forward?

Thanks!

All reactions

Sorry, something went wrong.

Uh oh!

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

@brettcannon

brettcannon commented Jul 7, 2023

Copy link
Copy Markdown
Member

These questions make me think that tarfile.TarPath should first be gestated in a PyPI package before we add it to the standard library. But that leaves a bit of a chicken-and-egg problem with getting pathlib._VirtualPath in.

You have a couple of options:

  1. Start a discussion on discuss.python.org for tarfile and see if there's clear consensus on what to do.
  2. Make _VirtualPath as private and do your experimental PyPI project knowing it might be perpetually tied to a specific Python version (and make that clear to users of the package).
  3. Go with your gut and hope for the best (and err on the side of leaving things out, especially if they can be added in later). 😅
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.

@merwok

merwok commented Jul 8, 2023

Copy link
Copy Markdown
Member

I am unsure about the name VirtualPath!

The way I’ve seen it used, a TarPath class, or SshPath, or S3Path would be examples of virtual filesystems: I give them paths that look like regular filesystem paths and they do custom operations to return info or data.

Their base class is not itself a virtual path, it’s an (abstract?) base class!

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

barneygale commented Jul 8, 2023

Copy link
Copy Markdown
Contributor Author

Fair point! How about PathBase? It squares with the names of base classes in io, I think.

merwok reacted with hooray emoji
All reactions
  • 🎉 1 reaction

Sorry, something went wrong.

Uh oh!

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

@barneygale

barneygale commented Jul 9, 2023

Copy link
Copy Markdown
Contributor Author

Alternatively, pathlib.abc.VirtualPath?

All reactions

Sorry, something went wrong.

Uh oh!

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

@merwok

merwok commented Jul 10, 2023 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

Not a fan of deep modules, and even .abc.VirtualPath doesn’t seem great to me.
I have posted on discuss about VirtualPath vs PathBase!

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 added 3 commits July 12, 2023 19:51
@barneygale
Merge branch 'main' into pythongh-89812-omgtarpath
e3f2509
@barneygale
Undo changes to tarfile.
508cabe
@barneygale
_VirtualPath --> _PathBase
2c56591
@barneygale barneygale changed the title GH-89812: Add tarfile.TarPath GH-89812: Add pathlib._PathBase Jul 12, 2023
@barneygale
barneygale removed the request for review from ethanfurman July 12, 2023 19:04
@barneygale

barneygale commented Sep 11, 2023

Copy link
Copy Markdown
Contributor Author

just stumbled uppon this and im wondering if a concept of a public Accessor of a path might make sense where an accessor provides the implementation, then for example tarfile/zipfile paths could just have the accessor

We actually used to have accessor objects, but their interface was much the same as Path objects, and so they amounted to nothing more than a layer of indirection. They were removed in #25701.

If we add accessors back in, most users would need to subclass two classes to implement their own path objects: the accessor class and the path class. The latter would be necessary for any user who wants to add their own path methods, for example.

All reactions

Sorry, something went wrong.

Uh oh!

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

@RonnyPfannschmidt

RonnyPfannschmidt commented Sep 11, 2023

Copy link
Copy Markdown

those "accessors" where nothing more than aliases, after all the implementation as done had no power over what the pure path portion can do with the real world

a actualy "accessor" would be able to be tied to something like a directory file descriptor, or a path object, or a entirely different thing thats not a filesystem

and yes there is some need for synchronization between concrete path capabilities and the accessors,

however there is certain equivalence classes to keep in mind (Posix, Windows), so in a lot of cases my impression is that there will be more "accessors" than paths

All reactions

Sorry, something went wrong.

Uh oh!

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

@barneygale

barneygale commented Sep 11, 2023

Copy link
Copy Markdown
Contributor Author

Intriguing. Could you sketch out the accessor API?

All reactions

Sorry, something went wrong.

Uh oh!

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

@merwok

merwok commented Sep 11, 2023

Copy link
Copy Markdown
Member

(in the forums rather than a PR discussion please)

ethanfurman 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.

@RonnyPfannschmidt

RonnyPfannschmidt commented Sep 11, 2023

Copy link
Copy Markdown

i can dedicate some time to it later

a basic concept of the accessor version ought to consider something like:

  • windows paths

  • posix paths

  • unc paths

  • directory file descriptor paths (both secured and unsecured)

  • paths that pretend to be a chroot on top of another path/pathlike

  • archives

  • ssh/scp

  • urls/http

  • distinctions between readonly and readwrite accessors

  • capabilities of accessors (like users/perms/xattrs)

  • wrappers (like union fs) to support interesting testing
    (imagine a path that maps onto the real fs but pretends certain files are deleted/replaced)

    additionally for purely virtual accessors it might be interesting if parts of the path could be objects (like dates), but thats currently out of scope

All reactions

Sorry, something went wrong.

Uh oh!

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

@barneygale

barneygale commented Sep 13, 2023 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor Author

To summarise my thoughts on the above: in many cases this PR helps bring those things about, and I don't think it hinders any of those things. But let's chat more in a forum thread.

What's left in this PR:

  • Land GH-109187: Improve symlink loop handling in pathlib.Path.resolve() #109192 to align handling of symlink loops in Path.resolve()'s with this implementation of _PathBase.resolve(), and with os.path.realpath()
  • Get review/approval for this _PathBase.resolve() implementation.

On the latter point, there are still two subtle and related problems:

  • _PathBase.resolve(strict=True) can raise OSError with the wrong values (errno, path). For example, resolve('/etc/hosts/foo') should raise NotADirectoryError(path='/etc/hosts'), but instead raises FileNotFoundError(path='/etc/hosts/foo')
  • To avoid an infinite loop, the implementation of _PathBase.readlink() must not call self.resolve(), and so users must supply a path with no symlinks, except in the final position. This is a bit different to Path.readlink().

Both of these issues could be solved if we can provide some way to stat() a path without resolving any symlinks. The resolve() method would call this stat() method first, and only call readlink() if the stat result indicates a symlink. A couple of options spring to mind:

  • We could allow _PathBase.stat(follow_symlinks=None), meaning "where possible, don't follow any symlinks in the path; the path I've provided is the direct physical path". But it's not particularly obvious, and follow_symlinks=None means something different in glob() and rglob().
  • We could add some other keyword-only argument to _PathBase.stat() with the same effect.
  • We could add a new method -- direct_stat(), stat1(), dstat()?
All reactions

Sorry, something went wrong.

Uh oh!

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

@barneygale
WIP
a51d7a0
@barneygale

barneygale commented Sep 15, 2023

Copy link
Copy Markdown
Contributor Author

Bah, pushed a commit labelled "WIP". In fact it contains a solution to the above problem: stash a _resolving flag on path objects, which means resolve() can safely call stat() and readlink() without hitting infinite recursion. No weird APIs for users.

All reactions

Sorry, something went wrong.

Uh oh!

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

@barneygale
Undo test change.
7e3729e
This will require further refactoring in another PR.
@barneygale

barneygale commented Sep 23, 2023

Copy link
Copy Markdown
Contributor Author

Hey @AA-Turner and @gst, thank you for your earlier reviews, very much appreciated. Please could you give it another pass, if you can spare the time?

I feel OK about _PathBase.resolve(): it's entirely private for the timebeing, and I intend to add further tests and optimization before we drop the underscore from _PathBase, so if we can't spot any clear defects I suspect it's fine to land? Grateful for your guidance!

gst 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.

gst
gst approved these changes Sep 23, 2023
View reviewed changes
@AA-Turner
AA-Turner self-requested a review September 24, 2023 03:04
AA-Turner
AA-Turner reviewed Sep 26, 2023
View reviewed changes
Comment thread Lib/pathlib.py Resolved

Uh oh!

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

Comment thread Lib/pathlib.py Resolved

Uh oh!

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

Comment thread Lib/pathlib.py Resolved

Uh oh!

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

Comment thread Lib/pathlib.py Resolved

Uh oh!

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

barneygale added 6 commits September 26, 2023 20:05
@barneygale
Merge branch 'main' into pythongh-89812-omgtarpath
b945cf8
@barneygale
Ensure .. segments are resolved in non-strict mode
703fe5c
@barneygale
Move symlink loop resolution test from PosixPathTest to `DummyPathT…
e5e5be5
…est`
@barneygale
Add PathBase._split_stack() helper method.
38769a0
@barneygale
Use path object as stat/link target cache key
7c78952
@barneygale
Optimise resolve(): skip stat() in non-strict mode if readlink() is u…
fe57725
…nsupported.
@barneygale
barneygale requested a review from AA-Turner September 29, 2023 13:33
@barneygale barneygale mentioned this pull request Sep 29, 2023
User-defined virtual path objects #110109
Closed
8 tasks done
AA-Turner
AA-Turner approved these changes Sep 29, 2023
View reviewed changes

@AA-Turner AA-Turner left a comment

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.

Thanks! Just two notes. I think _split_stack makes sense a method.

A

Sorry, something went wrong.

Uh oh!

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

barneygale reacted with heart emoji
All reactions
  • ❤️ 1 reaction
Comment thread Lib/pathlib.py Resolved

Uh oh!

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

Comment thread Lib/pathlib.py Resolved

Uh oh!

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

@barneygale
Address code review comments
cf9c8b6
@barneygale
barneygale merged commit 89966a6 into python:main Sep 30, 2023
@bedevere-app bedevere-app Bot removed the awaiting core review label Sep 30, 2023
@barneygale

barneygale commented Sep 30, 2023

Copy link
Copy Markdown
Contributor Author

Thank you so much for the reviews, everyone :-)

ap-- and ofek reacted with hooray emoji
All reactions
  • 🎉 2 reactions

Sorry, something went wrong.

Uh oh!

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

@AA-Turner

AA-Turner commented Sep 30, 2023

Copy link
Copy Markdown
Member

Congratulations Barney, a lot of hard work on your part paid off!

A

ofek, barneygale, and jamesmyatt reacted with heart emoji
All reactions
  • ❤️ 3 reactions

Sorry, something went wrong.

Uh oh!

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

@barneygale barneygale mentioned this pull request Oct 11, 2023
GH-110109: Add pathlib._PurePathBase #110670
Merged
Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024
@barneygale @AA-Turner @Glyphack
pythonGH-89812: Add pathlib._PathBase (python#106337)
9e32fba
Add private `pathlib._PathBase` class. This will be used by an experimental PyPI package to incubate a `tarfile.TarPath` class.

Co-authored-by: Adam Turner <[email protected]>
@brianschubert brianschubert mentioned this pull request Aug 6, 2025
[pathlib] Add owner, group methods on Windows python/typeshed#14536
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@AA-Turner AA-Turner AA-Turner approved these changes
+2 more reviewers
@ofek ofek ofek left review comments
@gst gst gst approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

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

Assignees

No one assigned

Labels

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.

9 participants

@barneygale @brettcannon @merwok @ap-- @RonnyPfannschmidt @AA-Turner @gst @ofek @bedevere-bot
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 01402503 — 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.