Repository navigation
Make finding home directory more general - #10570
Conversation
| else: | ||
| raise OSError('Could not find unix home directory to search for ' | ||
| 'astropy config dir') | ||
| try: |
There was a problem hiding this comment.
Can this entire function be effectively replaced by os.expanduser('~')? This claims to be applicable for all platforms so it seems best to delegate to this core library function. I guess it isn't clear if all the custom logic below for Windows is reflected in that function.
There was a problem hiding this comment.
Looking at the code for expanduser it takes a very different approach and uses the pwd module (password database) to get the home directory if HOME is not defined. That seems at first glance reliable, maybe?
There was a problem hiding this comment.
I think it could - I took the current approach to maintain any previous behaviour, but in ipython they just use os.expanduser. If there's openess to changing it I'm happy to modify in that direction?
There was a problem hiding this comment.
I'm not exactly sure what you're asking me, but IIRC using os.expanduser on Windows is reasonable.
There was a problem hiding this comment.
👍 if there is consensus that os.expanduser('~') is a reasonable approach to take then I will change the PR and see if CI is happy.
There was a problem hiding this comment.
@embray - my question is whether we can replace the entire _get_home code with a single line return os.expanduser('~'). Are all those special cases on Windows still necessary or are they a relic of how things were 10 years ago?
There was a problem hiding this comment.
Is there any chance this change can be merged (to fix the issues we are having in sunpy, whilst definitely maintaining back-compat), and discussion about whether to replace the whole function with os.path.expanduser('~') moved to a new issue?
There was a problem hiding this comment.
Yes, good plan, sorry for the delay. Can you open that issue? For merging this needs a bugfix changelog entry indicating fix of problems in MacOS with no HOME dir var. I can't see any reason not to backport to LTS, so put it in the 4.0.2 section.
|
Not sure why CI not kicking off properly. Maybe a change log under "API change" in 4.2? Is there a way to add a test? |
|
@pllim - do you agree this could be called a bug fix and go to LTS 4.0.2? |
|
For testing, maybe a test could temporarily unset HOME to exercise the new functionality? (Or whatever configuration is needed to force getting into that branch). |
|
Milestone moved. Needs a change log. Thanks! 😄 |
|
👍 changelog added |
Make finding home directory more general
Make finding home directory more general
Make finding home directory more general
Make finding home directory more general
4 participants
In sunpy we are seeing CI failures due to astropy not being able to find the home directory on macos: https://dev.azure.com/sunpy/sunpy/_build/results?buildId=8869&view=logs&jobId=6e7bd578-a810-548b-8fbf-8fa6b1a2d4f6&j=8b26c057-390d-57e7-9a94-ed463b57befe&t=7cf2f17d-5054-541f-491b-2e6078562aef
This seems to be because the 'HOME' environment variable isn't set. This PR makes finding home more lenient by trying to use
os.path.expanduser('~')as a backup if the 'HOME' environment variable isn't set. This is in line with what IPython now do, which according to the comments this function is based onhttps://github.com/ipython/ipython/blob/f8c9ea7db42d9830f16318a4ceca0ac1c3688697/IPython/utils/path.py#L190