-
Notifications
You must be signed in to change notification settings - Fork 284
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
Fix launching non python kernels from a windows store installed jupyter #568
Conversation
jupyter_client/launcher.py
Outdated
@@ -87,6 +93,10 @@ def launch_kernel(cmd, stdin=None, stdout=None, stderr=None, env=None, | |||
cwd = cast_bytes_py2(cwd, sys.getfilesystemencoding() or 'ascii') | |||
kwargs['cwd'] = cwd | |||
|
|||
# Windows store python will pass invalid paths for the connection file. | |||
# Turn the connection file into a real path | |||
cmd = [ force_real_path(c) for c in cmd] |
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.
Note that cmd
represents Python code, not all elements of the list are paths. I don't think it is safe to assume that an element that is not a path won't exist as a path by chance.
Thanks for reporting the issue and submitting a PR @rchiodo. I agree replacing the path to the connection file with the "real path" can solve the issue, however it might be better to do it in: jupyter_client/jupyter_client/manager.py Lines 175 to 208 in 778fa85
|
I'm not sure that would work (I can try) but when I did something similar in multikernelmanager.py it broke python kernels (as I believe other spots must have the connection_file with the old path). I think it would be better to just change the code in the launcher.py to instead verify it's the kernel-.json file. Then there wouldn't be a problem with possibly changing paths on things that aren't files. |
No your suggested change did work. Thanks. I'll update to that. |
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.
Thanks for tackling this! I didn't have a good answer for the issue last time it was raised so I'm glad someone understood it better and had a solution that's simple in mind.
Any chance we could update or add a test to cover functionality? The code below the change does some complex manipulations and I think it'd be a risk that someone refactoring that could break behavior unintentionally for windows store installations and not know.
@MSeal I'd be happy to add a test but it would have to install the window's store python (or perhaps mock realpath to force it to something different for a unit test). |
Do you have any suggestions on where to start with say a unit test? That seems like the easier way to verify this doesn't regress. |
I would mock realpath to set the path to a fake location and test that the command returned by There's no existing tests on |
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.
Thanks for adding the test
Fixes #567
The root cause of this problem is the path of the connection file that's passed to an external process.
From within the window's store python (where jupyter_client is running), the path looks like so:
However this path doesn't actually exist. It's a redirected path caused by the window's store version of python (see this bug here: https://bugs.python.org/issue41196)
What the actual path is this:
This change 'fixes' the path to the real path before launching the kernel.
Sorry but not sure how I'd write a test for this other than manually testing it with a Windows store python.