[issue39899] `pathlib.Path.expanduser()` does not call `os.path.expanduser()`

[Andrei Kulakov]

Andrei Kulakov  added the comment:

To be more precise, this change fixes https://bugs.python.org/issue41082 by 
raising RuntimeError instead of KeyError and also by documenting it, which 
means matplotlib can fix it by either using os.path.expanduser or catching 
RuntimeError, whichever might work better in their case.

I will let them know once we sort this out.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue39899] `pathlib.Path.expanduser()` does not call `os.path.expanduser()`

[Andrei Kulakov]

Andrei Kulakov  added the comment:

Note this change also fixes https://bugs.python.org/issue41082 . I'm guessing 
it's too much of an edge case to backport this fix to 3.9, so I've put up a 
possible fix via docs update on that issue.

--
nosy: +andrei.avk, kj

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue39899] `pathlib.Path.expanduser()` does not call `os.path.expanduser()`

[Steve Dower]

Change by Steve Dower :


--
stage: patch review -> resolved
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue39899] `pathlib.Path.expanduser()` does not call `os.path.expanduser()`

[Steve Dower]

Steve Dower  added the comment:


New changeset ba1db571987c65672d9c06789e9852313ed2412a by Barney Gale in branch 
'master':
bpo-39899: Don't double-check directory name if we're requesting the current 
user's home directory in ntpath.expanduser() (GH-25277)
https://github.com/python/cpython/commit/ba1db571987c65672d9c06789e9852313ed2412a


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue39899] `pathlib.Path.expanduser()` does not call `os.path.expanduser()`

[Barney Gale]

Barney Gale  added the comment:

Good spot Eryk - I've put in another PR to address it.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue39899] `pathlib.Path.expanduser()` does not call `os.path.expanduser()`

[Barney Gale]

Change by Barney Gale :


--
pull_requests: +24014
stage: resolved -> patch review
pull_request: https://github.com/python/cpython/pull/25277

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue39899] `pathlib.Path.expanduser()` does not call `os.path.expanduser()`

[Eryk Sun]

Eryk Sun  added the comment:

> os.path.expanduser() has many flaws, and it just "guess" the 
> home directory for other users.

I'm fine with not guessing another user's profile directory (or home directory) 
in some cases, or even always. But the code that got committed bails out even 
if the target user is the same as the current user, according to the USERNAME 
environment variable. It shouldn't do that.

---

If we want something better than guessing, it's not very hard to get the real 
profile directory or home directory for another user on the current system. The 
profile directory is configured as the "ProfileImagePath" value in a subkey of 
"HKLM\Software\Microsoft\Windows NT\CurrentVersion\ProfileList". The subkey 
name is the user SID in string form. The SID can be looked up with 
LookupAccountNameW(NULL, target_user, , ...) and converted to string form 
with ConvertSidToStringSidW(, string_sid). If there's no local profile 
directory, try looking up the user's configured home_dir and/or home_dir_drive 
(a mapped drive for a remote home directory) via NetUserGetInfo(NULL, 
target_user, 4, ).

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue39899] `pathlib.Path.expanduser()` does not call `os.path.expanduser()`

[Serhiy Storchaka]

Serhiy Storchaka  added the comment:

AFAIK you can set arbitrary path as user home directory. So home directories of 
different users can even not be on the same disk, and the last component of the 
path can be different from the user name.

os.path.expanduser() has many flaws, and it just "guess" the home directory for 
other users. It is difficult to fix os.path.expanduser() due to backward 
compatibility, but we should do better in pathlib from start.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue39899] `pathlib.Path.expanduser()` does not call `os.path.expanduser()`

[Eryk Sun]

Eryk Sun  added the comment:

For a "~user" path, the value of userhome should always be used if target_user 
== current_user. If for some reason the USERPROFILE environment variable isn't 
defined, the fallback "%HOMEDRIVE%%HOMEPATH%" does not necessarily end in the 
user's name. Example rewrite:

if i != 1: #~user
target_user = path[1:i]
if isinstance(target_user, bytes):
target_user = os.fsdecode(target_user)
current_user = os.environ.get('USERNAME')

if target_user != current_user:
# Try to guess user home directory.  By default all user
# profile directories are located in the same place and are
# named by corresponding usernames.  If userhome isn't a
# normal profile directory, this guess is likely wrong,
# so we bail out.
if current_user != basename(userhome):
return path
userhome = join(dirname(userhome), target_user)

--
nosy: +eryksun
status: closed -> open

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue39899] `pathlib.Path.expanduser()` does not call `os.path.expanduser()`

[Steve Dower]

Change by Steve Dower :


--
resolution:  -> fixed
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue39899] `pathlib.Path.expanduser()` does not call `os.path.expanduser()`

[Steve Dower]

Steve Dower  added the comment:


New changeset 3f3d82b84823eb28abeedf317bbe107bbe7f6492 by Barney Gale in branch 
'master':
bpo-39899: os.path.expanduser(): don't guess other Windows users' home 
directories if the basename of the current user's home directory doesn't match 
their username. (GH-18841)
https://github.com/python/cpython/commit/3f3d82b84823eb28abeedf317bbe107bbe7f6492


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue39899] `pathlib.Path.expanduser()` does not call `os.path.expanduser()`

[Barney Gale]

Barney Gale  added the comment:

Apologies, I think I started writing a comment before your reply was posted, 
which undid your changes.

--
resolution: not a bug -> 
status: closed -> open
versions: +Python 3.10 -Python 3.9

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue39899] `pathlib.Path.expanduser()` does not call `os.path.expanduser()`

[Steve Dower]

Steve Dower  added the comment:

> Firstly, `os.path.expanduser()` is already documented to return the path 
> unchanged if the home directory can't be resolved:

Ah, too bad. Doesn't prevent us from changing it, but hopefully it means that 
everyone using it is already checking the result and not blindly using it.

> I can understand why this could be seen as change for change's sake. In fact 
> this code removal greatly aids my work towards addressing bpo-24132.

In light of this, did you mean to close the issue? Or do you still want to 
pursue making the change?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue39899] `pathlib.Path.expanduser()` does not call `os.path.expanduser()`

[Barney Gale]

Barney Gale  added the comment:

Thanks for taking a look, Steve.

A couple things maybe worth noting:

Firstly, `os.path.expanduser()` is already documented to return the path 
unchanged if the home directory can't be resolved:

> If the expansion fails or if the path does not begin with a tilde, the path 
> is returned unchanged.

Secondly, `ntpath.expanduser()` already returns the path unchanged if neither 
USERPROFILE nor HOMEPATH are in the environment.

An alternative would be to leave `ntpath.expanduser()` method alone, and forgo 
the slightly-improved error checking in `WindowsPath.expanduser()` in the name 
of conformity. Or perhaps we could add a `stict` parameter to `expanduser()`?

I can understand why this could be seen as change for change's sake. In fact 
this code removal greatly aids my work towards addressing bpo-24132.

Thanks again

--
resolution:  -> not a bug
stage: patch review -> resolved
status: open -> closed
versions: +Python 3.9 -Python 3.10

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue39899] `pathlib.Path.expanduser()` does not call `os.path.expanduser()`

[Steve Dower]

Steve Dower  added the comment:

I think this is worth unifying, but I'm concerned about making expanduser() 
return the original path on Windows - "~name" is a valid filename/relative 
path, and so code that does mkdir(expanduser("~nonuser/dir")) could create 
garbage in the current directory. I'd rather just raise OSError (or I guess 
RuntimeError, for consistency).

Long term, I'd like to see it switch to calling GetProfilesDirectory on 
Windows, but that's separate from this change, and doesn't invalidate this one.

Reading through the discussion, it seems like the primary concern about the 
change is "change for change sake". I think the amount and kind of code that's 
being removed is a good thing, and it's better represented as an "expand" step 
in the accessor than a "get" from the path.

So let's get it merged, probably(?) with a stronger error for the unknown 
users, but happy to be talked out of that. And only for 3.10.

--
nosy: +steve.dower
resolution: not a bug -> 
stage: resolved -> patch review
status: closed -> open
versions: +Python 3.10 -Python 3.9

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue39899] `pathlib.Path.expanduser()` does not call `os.path.expanduser()`

[Barney Gale]

Barney Gale  added the comment:

The only design flaw mentioned in that thread is that `os.path.expanduser()` 
returns the input unchanged if expansion fails, which is not very pythonic. 
However, such a problem doesn't necessitate a rewrite of 
`os.path.expanduser()`. Checking `result[:1]` is enough.

I think there's also a difference in the Windows heuristic in that pathlib 
checks whether `basename(%HOMEPATH%) == %USERNAME%` whereas `ntpath.expanduser` 
doesn't. But if that's really an issue it should probably be fixed in `ntpath` 
IMO, rather than having divergent implementations.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue39899] `pathlib.Path.expanduser()` does not call `os.path.expanduser()`

[Serhiy Storchaka]

Serhiy Storchaka  added the comment:

It was discussed in issue19776.

Having separate implementation we can avoid design flaws of 
os.path.expanduser().

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue39899] `pathlib.Path.expanduser()` does not call `os.path.expanduser()`

[Barney Gale]

Barney Gale  added the comment:

I see no reason for the duplication, and I can point to one concrete bug 
affecting your re-implementation of `expanduser` that doesn't affect the 
original, i.e. that a `KeyError` is raised on Windows when `"USERNAME"` is not 
present in `os.environ`, whereas all similar cases raise `RuntimeError`. These 
sorts of issues sneak in when you duplicate code - better to stick with the 
battle-hardened version rather than an inherently risky rewrite.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue39899] `pathlib.Path.expanduser()` does not call `os.path.expanduser()`

[Serhiy Storchaka]

Serhiy Storchaka  added the comment:

I see no reason to change the current code.

--
resolution:  -> not a bug
stage: patch review -> resolved
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue39899] `pathlib.Path.expanduser()` does not call `os.path.expanduser()`

[Barney Gale]

Change by Barney Gale :


--
keywords: +patch
pull_requests: +18198
stage:  -> patch review
pull_request: https://github.com/python/cpython/pull/18841

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue39899] `pathlib.Path.expanduser()` does not call `os.path.expanduser()`

[Barney Gale]

Barney Gale  added the comment:

We can check whether `os.path.expanduser()` returned a path beginning with "~" 
and raise a RuntimeError if so, right? On point #2, I'm not sure this 
optimization alone justifies the duplication. PR incoming...

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue39899] `pathlib.Path.expanduser()` does not call `os.path.expanduser()`

[Serhiy Storchaka]

Serhiy Storchaka  added the comment:

There are two reasons:

1. os.path.expanduser() returns the path unchanged when a home directory cannot 
be resolved, pathlib.Path.expanduser() raises an error. The latter behavior 
looks more robust, but we can't change os.path.expanduser().

2. os.path.expanduser() needs to split the path on components while 
pathlib.Path.expanduser() already has ready components. In some cases it may be 
more efficient.

--
nosy: +serhiy.storchaka

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue39899] `pathlib.Path.expanduser()` does not call `os.path.expanduser()`

[Barney Gale]

New submission from Barney Gale :

`pathlib.Path.expanduser()` does not call `os.path.expanduser()`, but instead 
re-implements it. The implementations look pretty similar and I can't see a 
good reason for the duplication. The only difference is that 
`pathlib.Path.expanduser()` raises `RuntimeError` when a home directory cannot 
be resolved, whereas `os.path.expanduser()` returns the path unchanged.

--
components: Library (Lib)
messages: 363635
nosy: barneygale
priority: normal
severity: normal
status: open
title: `pathlib.Path.expanduser()` does not call `os.path.expanduser()`
versions: Python 3.9

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com