defer to stdlib for path.get_home_dir() #998

Merged
merged 3 commits into from Nov 24, 2011

3 participants

@minrk
IPython member

We have elaborate and fragile logic for determining
home dir, and it is ultimately less reliable than the stdlib behavior
used for os.path.expanduser('~'). This commit defers to that in
all cases other than a bundled Python in py2exe/py2app environments.

The one case where the default guess will not be correct, based on
inline comments, is on WinHPC, where all paths must be UNC (\\foo), and
thus HOMESHARE is the logical first choice. However, HOMESHARE is
the wrong answer in approximately all other cases where it is defined,
and the fix for WinHPC users is the trivial HOME=%HOMESHARE%.

This removes the various tests of our Windows path resolution logic,
which are no longer relevant. Further, $HOME is used by the stdlib
as first priority on all platforms, so tests for this behavior are
no longer posix-specific.

closes gh-970
closes gh-747

@minrk
IPython member

I also made a small change, allowing IPython to start without requiring that $HOME is actually writable, because we almost never write anything there. Note that we already handle the actually important case of the ipython dir being writable, falling back on a temp dir if not.

If HOME does not exist, the only two references to HOME that I found (%cd with no args, and %logstart global) will raise proper, informative 'no such file' errors, but IPython is fully functional otherwise, so it seemed silly to have a fatal error on something that isn't actually a requirement for reasonably well behaved IPython.

@minrk
IPython member

pinging @ellisonbg as the individual with the most experience on the one system that will be adversely affected by this change: Windows-based clusters. Do you still have a test system, where we can check what needs to be done when HOMESHARE is not the default choice? Is it difficult to set HOME or IPYTHON_DIR for jobs?

@fperez fperez commented on an outdated diff Nov 20, 2011
IPython/utils/path.py
else:
- raise HomeDirError('No valid home directory could be found for your OS')
+ raise HomeDirError('%s is not a writable dir, set $HOME env to override' % homedir)
@fperez
IPython member
fperez added a note Nov 20, 2011

spell out 'environment variable' in full: windows users may be confused if they only see 'env', while the full word is easy to google for even if they don't know how/where to configure the environment in windows.

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

I definitely like the simplification this gives, though I'm a little skittish at jettisoning logic that might actually protect a valid corner case perhaps not covered by expanduser('~'). What do you think of #154, for example?

I'd also like to hear @ellisonbg's opinion: after all the pain of getting all that working on the winhpc server he went through, the last thing we want is to break that!

So let's be cautious with this one, but if indeed we can remove those hacks for such a vastly simpler replacement, I'd be delighted! My only other feedback is a tiny fix on a user-facing message.

Finally, this should also (once we settle the policy decision) have a paragraph in the docs explaining how to configure things (and remove any possibly outdated info that could refer to the old logic).

But thanks for the big cleanup, I hope we can indeed merge the whole thing!

@ellisonbg
IPython member
@minrk
IPython member

I definitely don't trust expanduser to get all of the corner cases
correct unless we can verify that it handles everything that our
current code does.

No, expanduser will not get all of the corner cases, but it does a better job than our current code in almost all normal cases. The problem is that we simply cannot get the right answer on both WinHPC and the rest of Windows, because WinHPC wants HOMESHARE for its UNC path, which is the wrong answer approximately everywhere else that it is defined.

An advantage of expanduser is that HOME is first priority on all platforms, so it is the easiest and most natural to manually specify.

Using expanduser means that covering every possible cornercase is handled with one line of user code, setting $HOME prior to launching IPython, in all environments on all platforms.

Our choice is between:

A) requiring all WinHPC users to specify HOME=%HOMESHARE% (or just use IPYTHON_DIR, which is enough for the parallel apps)
B) requiring most non-WinHPC users to specify HOMESHARE=something (this is not guaranteed to be possible, and just using IPYTHON_DIR does not cut it for interactive IPython users).

I understand it would be nice to simplify our code, but before we do
that, let's figure out exactly what expanduser does.

os.path.expanduser priority on Windows:

  1. $HOME
  2. $USERPROFILE
  3. $HOMEDRIVE\$HOMEPATH

and on Unix:

  1. $HOME
  2. query passwd database via pwd module
@ellisonbg
IPython member
@minrk
IPython member

IIRC, HOMESHARE is not usually set on Windows and the current logic
will ignore it if it is not set and move onto the other options
(HOMEDRIVE+HOMEPATH, USERROFILE, My Documents, HOME). So the only
time HOMESHARE is used is when it is set and is actually the option
that likely should be used. We know this approach works as this is
how our code has done it for about 2 years now.

The fact that we have had multiple bug reports (including #747) of cases where HOMESHARE is the wrong choice would suggest otherwise.

I don't see how these are our only two options. If non-WinHPC users
don't set HOMESHARE, it is ignored and the other options are
attempted, which is what we do today.

The problem is the enterprise/computer lab environments, which regularly do set this env, where it is not the right answer. It would appear from the user reports that HOMESHARE is never right outside WinHPC even when defined, but obviously we won't be getting reports that we are doing the right thing. Sysadmins, not users, set HOMESHARE, but users can set HOME. So if HOMESHARE is wrong, there isn't much recourse if it is the first priority, but if HOME is wrong or undefined, it should be safe to override.

So the logical choices for first priority are to match the rest of the Python universe and use HOME, or use HOMESHARE, which has the benefit of guaranteeing UNC paths on WinHPC, but the disadvantages of being the wrong answer everywhere else that it is defined, along with being internally inconsistent with the rest of Python.

The fact is that both of these cases are guaranteed to get the wrong answer in some cases, so we just have to pick the one that is going to require some extra config. WinHPC seems the more logical choice to me, because it is already the more complicated and unconventional use case, and the fix is standard and less intrusive.

OK, thanks for tracking this down. Quite different logic than we have
though. This makes me wonder (even aside from the HOMESHARE issue)
why our ordering is so different.

Yes, I think we are exactly backwards. If we do restore HOMESHARE as first priority, we should use expanduser after that, and can still fall back on My Documents in the end, if we want.

@ellisonbg
IPython member
@minrk
IPython member

Given the situation, I think that using expanduser makes sense.

Okay, should I add the My Documents wreg bit back in as a fallback? One case we no longer handle on Windows is the lack of any environment, but I don't know when/where that might come up, if ever.

@fperez
IPython member
@minrk
IPython member

Okay, My Documents fallback is back (along with its test), and @fperez's comment on the error message is addressed.

minrk added some commits Nov 13, 2011
@minrk minrk defer to stdlib for path.get_home_dir()
We have elaborate and fragile logic for determining
home dir, and it is ultimately less reliable than the stdlib behavior
used for `os.path.expanduser('~')`.  This commit defers to that in
all cases other than a bundled Python in py2exe/py2app environments.

The one case where the default guess will *not* be correct, based on
inline comments, is on WinHPC, where all paths must be UNC (`\\foo`), and
thus HOMESHARE is the logical first choice.  However, HOMESHARE is
the wrong answer in approximately all other cases where it is defined,
and the fix for WinHPC users is the trivial `HOME=%HOMESHARE%`.

This removes the various tests of our Windows path resolution logic,
which are no longer relevant. Further, $HOME is used by the stdlib
as first priority on *all* platforms, so tests for this behavior are
no longer posix-specific.

closes gh-970
closes gh-747
b5ca646
@minrk minrk allow IPython to run without writable home dir
get_ipython_dir() ensures that the *IPython* dir is writable, which is more relevant, but the home dir need not be writable. Some optional behaviors (e.g. `%logstart global`) will not work if the home dir is not writable, but IPython should not crash.  Approximately no other operations actually depend on writing directly to $HOME.
db42b5f
@minrk minrk restore My Documents fallback for get_home_dir on Windows 9df2cbb
@fperez
IPython member

@minrk, this looks pretty much ready to merge, no? If so, feel free to go ahead with it, the last commits look fine.

Thanks for the cleanup and being patient to work through the issues on WinHPC with @ellisonbg!

@minrk
IPython member

Sure, it seems well behaved to me. I'll give it a few iptests, then merge if nothing pops up.

@fperez
IPython member
@minrk minrk merged commit 351c8fc into ipython:master Nov 24, 2011
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment