classification
Title: os.path.expanduser should not use HOME on windows
Type: behavior Stage: resolved
Components: Library (Lib), Windows Versions: Python 3.8
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: steve.dower Nosy List: Anthony Sottile, blueyed, eryksun, paul.moore, steve.dower, tim.golden, zach.ware
Priority: normal Keywords: patch

Created on 2019-03-11 16:09 by Anthony Sottile, last changed 2019-03-18 05:50 by blueyed. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 12282 merged Anthony Sottile, 2019-03-11 16:38
PR 12294 merged steve.dower, 2019-03-12 16:04
Messages (11)
msg337683 - (view) Author: Anthony Sottile (Anthony Sottile) * Date: 2019-03-11 16:09
The current code for `os.path.expanduser` in `ntpath` uses `HOME` in preference to `USERPROFILE` / `HOMEDRIVE\\HOMEPATH`

I can't find any documentation of `HOME` being relevant on windows (only on posix).  For example, wikipedia only mentions `USERPROFILE` / `HOMEDRIVE\\HOMEPATH` options: https://en.wikipedia.org/wiki/Home_directory#Default_home_directory_per_operating_system

Seems to be (one of) the direct causes of a downstream bug for me: https://github.com/pre-commit/pre-commit/issues/960

(msys sets `HOME` to a bogus value if `HOMEDRIVE` is set, then python uses it)

My proposal is to remove these lines and change the `elif` to an `if`:

https://github.com/python/cpython/blob/d9bd8ec2a40ea67bc4248a72943a409ee645ddf3/Lib/ntpath.py#L302-L304
msg337750 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2019-03-12 15:17
I like the patch, but I'm not sure all the tests are properly preserving the real value of USERPROFILE.

Modifying this value could have a real impact on the rest of the process, so we should be very careful to undo it regardless of test result. (Modifying HOME is not a as big a deal since, as you point out, it's not "real" ;) )

Also, it's probably better to get the current value and check against that, rather than setting it to a known value. One day we might do the right thing and switch expanduser() from the environment variables to the correct API, which would break the test.
msg337752 - (view) Author: Anthony Sottile (Anthony Sottile) * Date: 2019-03-12 15:28
> Modifying this value could have a real impact on the rest of the process, so we should be very careful to undo it regardless of test result. (Modifying HOME is not a as big a deal since, as you point out, it's not "real" ;) )

Agreed, though I'd like to write it off as an existing problem (if it is a problem, it looks to me like the tests are doing proper environment cleanup).  If they are broken, they'd already be leaking environment variables on posix since these tests run on both

Auditing the tests I modified:

- test_netrc: uses `with support.EnvironmentVarGuard() as environ:` (OK!)
- test_ntpath: uses `with support.EnvironmentVarGuard() as env:` (OK!)
- test_dist: `MetadataTestCase` extends `support.EnvironGuard` (OK!)
- test_config: `BasePyPIRCCommandTestCase` extends `support.EnvironGuard` (OK!)

so actually, looks like this is already good to go on that front!

Definitely agree on using the true value in the long term -- I'd like to defer that until that's actually happening though as it'll be a much more involved patch!
msg337754 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2019-03-12 15:39
Great, thanks for checking! I'll merge.
msg337755 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2019-03-12 15:40
New changeset 25ec4a45dcc36c8087f93bd1634b311613244fc6 by Steve Dower (Anthony Sottile) in branch 'master':
bpo-36264: Don't honor POSIX HOME in os.path.expanduser on Windows (GH-12282)
https://github.com/python/cpython/commit/25ec4a45dcc36c8087f93bd1634b311613244fc6
msg337756 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2019-03-12 15:42
If we're going ahead with this, it's worth a mention in whatsnew as this is going to break things for some users.
msg337763 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2019-03-12 15:54
> If we're going ahead with this, it's worth a mention in whatsnew

Good call. I'll do it
msg337781 - (view) Author: Eryk Sun (eryksun) * (Python triager) Date: 2019-03-12 18:01
> `os.path.expanduser` in `ntpath` uses `HOME` in preference to 
> `USERPROFILE` / `HOMEDRIVE\\HOMEPATH`

Guido intentionally added support for HOME in ntpath.expanduser way back in Python 1.5 (circa 1997), and now we're removing it over 20 years later. I expect this to break some deployments, but it's not a serious problem. 

My feeling here is "meh". Practically all use of expanduser() is dubious in Windows, varying from going against convention to being completely wrong. We're long due for a module in the standard library that abstracts access to platform-specific configuration and special directories, but attempts to adopt one keep losing momentum. I guess it's too prone to bike shedding and arguments.
msg337784 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2019-03-12 18:33
> Guido intentionally added support for HOME in ntpath.expanduser way back in Python 1.5 (circa 1997), and now we're removing it over 20 years later.

Wouldn't be the first thing to be removed :)
msg337813 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2019-03-12 22:15
New changeset 8ef864d50fb847cf15d5717c0db04fd60fb13d8d by Steve Dower in branch 'master':
bpo-36264: Updates documentation for change to expanduser on Windows (GH-12294)
https://github.com/python/cpython/commit/8ef864d50fb847cf15d5717c0db04fd60fb13d8d
msg338162 - (view) Author: daniel hahler (blueyed) * Date: 2019-03-18 05:50
Just as a sidenote: the other day I was checking Python's source to see how to override a user's home (in tests, when os.path.expanduser` is used in code), and found it easy to just set $HOME in the environment in the end.

I guess this change will cause quite some regressions in this regard - even though $HOME might not be used in practice on Windows.
History
Date User Action Args
2019-03-18 05:50:30blueyedsetnosy: + blueyed
messages: + msg338162
2019-03-12 22:16:16steve.dowersetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2019-03-12 22:15:29steve.dowersetmessages: + msg337813
2019-03-12 18:33:18steve.dowersetmessages: + msg337784
2019-03-12 18:01:18eryksunsetnosy: + eryksun
messages: + msg337781
2019-03-12 16:04:12steve.dowersetpull_requests: + pull_request12273
2019-03-12 15:54:13steve.dowersetmessages: + msg337763
2019-03-12 15:42:13zach.waresetmessages: + msg337756
2019-03-12 15:40:21steve.dowersetmessages: + msg337755
2019-03-12 15:39:21steve.dowersetassignee: steve.dower
messages: + msg337754
2019-03-12 15:28:40Anthony Sottilesetmessages: + msg337752
2019-03-12 15:17:55steve.dowersetmessages: + msg337750
2019-03-11 16:38:11Anthony Sottilesetkeywords: + patch
stage: patch review
pull_requests: + pull_request12262
2019-03-11 16:09:57Anthony Sottilecreate