-
-
Notifications
You must be signed in to change notification settings - Fork 31.8k
pathlib.Path.expanduser()
does not call os.path.expanduser()
#84080
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.
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
Comments
|
There are two reasons:
|
We can check whether |
I see no reason to change the current code. |
I see no reason for the duplication, and I can point to one concrete bug affecting your re-implementation of |
It was discussed in bpo-19776. Having separate implementation we can avoid design flaws of os.path.expanduser(). |
The only design flaw mentioned in that thread is that I think there's also a difference in the Windows heuristic in that pathlib checks whether |
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. |
Thanks for taking a look, Steve. A couple things maybe worth noting: Firstly,
Secondly, An alternative would be to leave 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 |
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.
In light of this, did you mean to close the issue? Or do you still want to pursue making the change? |
Apologies, I think I started writing a comment before your reply was posted, which undid your changes. |
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) |
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. |
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, &sid, ...) and converted to string form with ConvertSidToStringSidW(&sid, 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, &info). |
Good spot Eryk - I've put in another PR to address it. |
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. |
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. |
os.path.expanduser()
to expand home directories #18841Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: