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
Fixes for ipython unhandeled OSError exception on failure of os.getcwdu() #811
Conversation
@@ -31,6 +31,7 @@ from __future__ import generators | |||
|
|||
import sys, warnings, os, fnmatch, glob, shutil, codecs | |||
from hashlib import md5 | |||
from IPython.utils.path import getcwdu |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should avoid importing other parts of IPython in the externals - these are separate packages that we include. If the problem affects this package, it should be fixed separately and sent upstream.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although looking at what this does, in this case I don't think it needs a fix. path just provides an OO representation of a file path, so it doesn't guarantee that a given path exists.
Overall, I think this warrants a closer look at what each case should be doing if the cwd doesn't exist, rather than simply replacing all calls with the new function. E.g. where cwd is used in tests, we might not want to catch the error, because that could mask something that's going wrong. And there may be other cases where it makes more sense to fail (with a helpful message) than to carry on with the user's home directory. |
BTW, this PR can't currently be merged due to conflicts. But I agree with @takluyver, a blanket catch of this error is the wrong approach. Let us know if you think you can work on this one further... |
The conflicts are probably due to a big pull request I merged without enough consideration - let me know if you want a hand rebasing it. |
@dhananjaysathe, do you think you'll have a chance to address the feedback here and rebase it? We'd love to have your contributions, but right now they need some fixing up first. If you think you'll have a chance to work on it let us know. Otherwise we'll close the PR for now, as it's problematic to keep open PRs that linger for a long time. You're always welcome to reopen it at any time in the future if you find the time to return to the work, and we'd be happy to help you with the necessary rebase and other feedback. |
Yep ill look into it and re-base, a bit tight on my schedule at the Cheers !!! Dhananjay Deepak Sathe | +919764871950 On Sun, Nov 27, 2011 at 2:24 AM, Fernando Perez <
|
I know this is an old PR, but I was just wondering how frequently (and when at all) such a situation would occur? i.e. "launch from locations that are not present" - is it when you started IPython and the directory you started from was moved or deleted right after that from under you? |
I've done that a few times - deleted a folder from one program while |
Hi, to give you an example me and some of my friends collaborate over an Cheers!!!
|
except OSError ,e : | ||
if e.errno is 2: | ||
warn.warn("Path does not exist , defaulting to Home Directory") | ||
current_dir_path = get_home_dir() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it seems that if we're going to report home as the cwd on error 2, we should CD to it, so that if the user ends up doing things using relative paths there are no surprises for him/her. And now I'm not even sure I like that solution either, because now something that ought not change state but only report the current working directory, sometimes (depending on whether or not the error is thrown) will change the state. The alternative of leaving this as is is also confusing, because getcwdu will report /home/user, but none of the standard python calls or system calls issued will actually have that as the current working directory.
I'm generally against this --- it's not surprising that IPython will crash if there is no current working directory. Most other command line tools will break, often with weird messages. For example, after a quick setup:
And
|
@dhananjaysathe : Not crashing is a reasonable aim, but we don't need to protect every call to Building on @ivanov's suggestion, I suggest you implement an IPython extension. There's a hook which is called before each code execution (you can call I'm going to close this pull request, as it's been sitting around for ages, and it's got various issues. You're welcome to implement a better solution and open a new PR, or talk to us on the mailing lists or on IRC if you want to know more. |
I followed the suggestion of @takluyver and made this little extension: It should be able to install with this: |
Thanks @marceloslacerda , I've left you a few suggestions in a comment on your gist. |
IPython.utils.path.getcwdu() added to prevent OSError on launch from locations that are not present
Fixed the OSError on launch of ipython to where thw os.getcwdu() function caused the exception to bubble and crash on launch.
It handles the OSError with errno 2 : corresponding to directory not present, and defaults it to the user home directory.
It also warns the user about its actions using the IPython.utils.warn.warn() about the action it takes hence maintaining transparency to the users.