Skip to content

MNT: Change config home path logic#10705

Merged
embray merged 2 commits intoastropy:masterfrom
pllim:win-cfg-path-logic
Sep 22, 2020
Merged

MNT: Change config home path logic#10705
embray merged 2 commits intoastropy:masterfrom
pllim:win-cfg-path-logic

Conversation

@pllim
Copy link
Member

@pllim pllim commented Sep 2, 2020

Description

This pull request is to address the problems Windows users encounter because config looks in HOMESHARE (either inaccessible or simply missing) too early. Now, it only looks for HOMESHARE if all the other options are exhausted.

I thought about removing it altogether but maybe some people do use HOMESHARE to store config; who knows. 🤷

If this is too controversial, we can move the milestone down to 5.0.

Fixes #10661
Fixes #8731

@pllim pllim added config Windows API change PRs and issues that change an existing API, possibly requiring a deprecation period labels Sep 2, 2020
@pllim pllim added this to the v4.2 milestone Sep 2, 2020
@embray
Copy link
Member

embray commented Sep 5, 2020

As I wrote on #10661, I wonder if we shouldn't replace this logic entirely to just use os.path.expanduser('~') as the default on both POSIX and Windows systems. By doing so we can delegate this logic to what Python already provides, which is probably better battle-tested and thought out by experts. According to the docs:

On Windows, HOME and USERPROFILE will be used if set, otherwise a combination of HOMEPATH and HOMEDRIVE will be used. An initial ~user is handled by stripping the last directory component from the created user path derived above.

It appears HOMESHARE is not used at all in this logic. According to this page which documents Windows environment variables better than anything I could find directly from Microsoft, HOMESHARE is only meaningful if HOMEDRIVE happens to be a UNC path (and I guess in this case they would be the same), in which case HOMESHARE points to directory under HOMEDRIVE that contains the user's home. A lot of this probably exists for legacy purposes that aren't very meaningful to a typical Windows user outside some specialized enterprise deployment.

Also worth noting that as of Python 3.8:

Changed in version 3.8: No longer uses HOME on Windows.

That change was motivated by this issue.

@pllim pllim changed the title MNT: Only look for HOMESHARE on Windows if no other option MNT: Change config home path logic Sep 8, 2020
@pllim pllim removed the Windows label Sep 8, 2020
@pllim pllim force-pushed the win-cfg-path-logic branch from 9e295e2 to dee80de Compare September 8, 2020 20:43
@pllim pllim force-pushed the win-cfg-path-logic branch from dee80de to 059a434 Compare September 8, 2020 20:45
@pllim
Copy link
Member Author

pllim commented Sep 8, 2020

@embray , I tried to implement your suggestion in the second commit but there is a chance I completely misunderstood, so please review. Does the doc at https://docs.astropy.org/en/latest/config/index.html need updating too?

@embray
Copy link
Member

embray commented Sep 16, 2020

@pllim Sorry for the late reply. The last week has been kind of crazy for me; I will be around more after this week.

Looking at the documentation, it looks like it conflicts with my previous recommendation that Windows users not use XDG_CONFIG_HOME, but now I can see where someone might have gotten the idea to if their "default" home directory was not being found properly.

I would argue we change that recommendation, but actually in retrospect there is not currently any other way to specify a different astropy config path via the environment except for setting XDG_CONFIG_HOME, since not every system uses the XDG base directory specification--depending on this is a bit system-biased (of course it's just a specification that one can in principle use on any system).

Perhaps we should take a cue here from matplotlib's MPLCONFIGDIR. I will open a separate issue for this, if there isn't already.

@embray
Copy link
Member

embray commented Sep 16, 2020

I'm otherwise 👍 on this as-is.

@pllim pllim requested a review from embray September 17, 2020 15:38
@pllim
Copy link
Member Author

pllim commented Sep 17, 2020

@embray , thanks for opening the meta issue, it was really illuminating. If you are okay with this, would you mind approving and merging? If you need write access, please let Kelle know.

Copy link
Member

@embray embray left a comment

Choose a reason for hiding this comment

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

LGTM

@embray
Copy link
Member

embray commented Sep 22, 2020

Cool, looks like I have access to merge. Just had to approve my review.

@embray embray merged commit 7962503 into astropy:master Sep 22, 2020
@pllim pllim deleted the win-cfg-path-logic branch September 22, 2020 17:24
@pllim
Copy link
Member Author

pllim commented Sep 22, 2020

Thanks! 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

API change PRs and issues that change an existing API, possibly requiring a deprecation period config

Projects

None yet

Development

Successfully merging this pull request may close these issues.

config folder/file not found config.paths._find_home() doesn't check HOMESHARE correctly

2 participants