Removed mechanism that change $PATH #148

xmnlab · 2019-04-04T00:35:20Z

resolves #145

xmnlab · 2019-04-04T22:52:03Z

the error on CI for python 3.3 says: RuntimeError: Python 3.4 or later is required

any thoughts?

It seems it is done for review.

gnestor · 2019-04-09T00:56:41Z

@xmnlab Your PR looks good.

It looks like master is failing CI: https://travis-ci.org/jupyter/jupyter_core/builds/443142041

@minrk Any ideas?

minrk · 2019-04-09T06:52:27Z

We need to make sure that this doesn't change the resolution of executables, which it looks like it does, currently. I think this change currently requires all jupyter-foo to be in the same directory as jupyter, which would not be correct.

The behavior should be the same as if dirname(jupyter) is on PATH. So what we need to do is find jupyter-foo on $PATH+dirname(jupyter) and then resolve the absolute path, but without mutating $PATH. This needs to be careful to properly resolve with $PATHEXT on Windows.

One option, I believe, should be to use shutil.which to resolve the path:

with mock.patch($PATH with self):
    abs_exe = shutil.which(jupyter-foo)
if abs_exe:
    exec(abs_exe)

xmnlab · 2019-04-12T22:31:47Z

thanks @minrk for the feedback. I hope to have captured your thoughts. let me know if I understood that correctly.

xmnlab · 2019-04-13T15:57:04Z

regarding to https://pypi.org/project/setuptools/ it seems py3.3 is not supported any more.

xmnlab · 2019-04-13T15:57:48Z

it is done for a new review.

xmnlab · 2019-04-13T15:59:42Z

jupyter_core/command.py

+    # set new PATH with self
+    os.environ['PATH'] = os.pathsep.join(_path_with_self())
+    # get the real path for the jupyter-<subcommand>
+    real_path = find_executable('{}-{}'.format(path, subcommand))


shutil.which is not available for py2

But good news, this repo already has a backport - from .utils.shutil_which import which. This is a bit more complex than find_executable, e.g. it handles PATHEXT on Windows, so it would be good to use that (and the real shutil.which on Python 3, of course).

xmnlab · 2019-04-17T18:50:32Z

any feedback?

xmnlab · 2019-04-22T14:07:30Z

hey everyone, any feedback about this PR?

xmnlab · 2019-04-24T20:12:59Z

hey @Carreau ! @gnestor told me maybe you could help with some review/feedback

xmnlab · 2019-04-30T14:21:37Z

hey everyone! any thoughts here?

gnestor · 2019-04-30T15:09:29Z

@takluyver Would you care to review?

takluyver · 2019-05-02T07:28:30Z

Yup, I'll review. While we're looking at this repository, can I encourage people to consider PR #143?

takluyver

Thanks, the idea looks fine, but I've left a few notes about the implementation.

takluyver · 2019-05-02T08:36:11Z

jupyter_core/command.py

+    # set new PATH with self
+    os.environ['PATH'] = os.pathsep.join(_path_with_self())
+    # get the real path for the jupyter-<subcommand>
+    real_path = find_executable('{}-{}'.format(path, subcommand))


But good news, this repo already has a backport - from .utils.shutil_which import which. This is a bit more complex than find_executable, e.g. it handles PATHEXT on Windows, so it would be good to use that (and the real shutil.which on Python 3, of course).

jupyter_core/command.py

takluyver · 2019-05-02T08:39:54Z

jupyter_core/command.py

@@ -113,9 +115,24 @@ def _execvp(cmd, argv):
        os.execvp(cmd, argv)


+def _jupyter_realpath(path, subcommand):


This should have a brief docstring to explain what it is.

xmnlab · 2019-06-11T21:08:57Z

thanks for the feedback @takluyver i will work on that!

xmnlab · 2019-06-11T23:57:14Z

@takluyver @minrk @gnestor it is done for a new review :)

jupyter_core/command.py

takluyver · 2019-06-12T15:26:59Z

jupyter_core/command.py

-
-    command = 'jupyter-' + subcommand
+
+    command = _jupyter_abspath(sys.argv[0], subcommand)


I think we always want to be looking up jupyter-xyz, regardless of what sys.argv[0] is.

@takluyver I changed that considering the follow case reported by @mlucool

Setup:

Set your PATH to have python 2 as your default version of python (prepend the path)

Run jupyterlab from a python 3 location (e.g. `path/to/python-3/bin/jupyterlab')

In a python 2 kernel run the following:

import sys print(sys.version) import os print(os.environ['PATH'])

As expected, sys.version will be python 2. But now you can see that the PATH is modified with python3 in preprended. So if I were to do:

import subprocess print(subprocess.Popen("python -c 'import sys; print(sys.version)'", shell=True, stdout=subprocess.PIPE).stdout.read())

This would show python 3.

This is really just a problem as we are migrating from python 2 to 3 and we want to run the server with 3 (as we want to use latest) but anything that is called via subprocess should use whatever you had in your PATH. We are planning to locally path ipykernel_launcher.py in a hacky way. This is suboptiomal so we'd like to revert this as soon as we can.

maybe it will not be a problem considering that there is no jupyterlab for py2 .. but if path/to/python-3/bin/jupyterlab is not in $PATH even if I run jupyerlab using the full path it will not be found.

do you have any thoughts here, considering this case?

I'm fine with not modifying $PATH.

The sys.argv[0] that you pass in here is only used, as far as I can see, in this line:

jupyter_subcommand = '{}-{}'.format(path, subcommand)

If sys.argv[0] is, e.g. /home/takluyver/.local/bin/jupyter, then you construct a subcommand like /home/takluyver/.local/bin/jupyter-notebook. That short-circuits shutil.which(), so it will only work if that's the location of jupyter-notebook.

If the subcommand is notebook, then we should always be looking up jupyter-notebook, not {sys.argv[0]}-notebook.

I see .. OK .. so maybe related to the scenario presented by @mlucool the workaround should be call /path/jupyterlab-subcommand directly

@takluyver it is done for a new review! thanks!

takluyver · 2019-06-18T20:38:55Z

jupyter_core/command.py

+    # get env PATH with self
+    search_path = os.pathsep.join(_path_with_self())
+    # get the abs path for the jupyter-<subcommand>
+    jupyter_subcommand = '{}-{}'.format(path, subcommand)


Thanks. Now you don't need to pass an argument, you can just hardcode it in this line. That's one less thing to follow around when reading the code. :-)

Ok :) done!

takluyver · 2019-06-18T21:42:01Z

jupyter_core/command.py

@@ -113,9 +119,29 @@ def _execvp(cmd, argv):
        os.execvp(cmd, argv)


+def _jupyter_abspath(subcommand):
+    """This method get the abspath of jupyter with no changes on ENV."""


Nitpick: "...the abspath of a specified jupyter-subcommand..."

ok! done! thanks for the feedback!

takluyver · 2019-06-18T21:44:20Z

Thanks! I've just spotted that the docstring should be a bit clearer. Other than that, I think this looks good.

@gnestor @minrk - once this is landed, I'll probably try to make a 4.5.0 release soon.

gnestor approved these changes Apr 9, 2019

View reviewed changes

xmnlab commented Apr 13, 2019

View reviewed changes

takluyver reviewed May 2, 2019

View reviewed changes

xmnlab force-pushed the mutating-env-path branch from 00a623c to 7fe4c25 Compare June 11, 2019 23:06

gnestor approved these changes Jun 12, 2019

View reviewed changes

takluyver reviewed Jun 12, 2019

View reviewed changes

jupyter_core/command.py Outdated Show resolved Hide resolved

takluyver reviewed Jun 12, 2019

View reviewed changes

takluyver added this to the 4.5 milestone Jun 13, 2019

xmnlab added 2 commits June 17, 2019 17:29

Removed env.path changing

07d15a8

Applied recommendations from review

b229405

xmnlab force-pushed the mutating-env-path branch from 7fe4c25 to b229405 Compare June 17, 2019 21:57

Applied recommendation from review

2626a39

takluyver reviewed Jun 18, 2019

View reviewed changes

Applied recommendations from review

53fc3fc

takluyver reviewed Jun 18, 2019

View reviewed changes

Improved docstring

eac6fa8

takluyver merged commit 2ae0172 into jupyter:master Jun 19, 2019

xmnlab deleted the mutating-env-path branch June 19, 2019 14:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removed mechanism that change $PATH #148

Removed mechanism that change $PATH #148

xmnlab commented Apr 4, 2019

xmnlab commented Apr 4, 2019

gnestor commented Apr 9, 2019

minrk commented Apr 9, 2019

xmnlab commented Apr 12, 2019

xmnlab commented Apr 13, 2019

xmnlab commented Apr 13, 2019

xmnlab Apr 13, 2019

takluyver May 2, 2019

xmnlab commented Apr 17, 2019

xmnlab commented Apr 22, 2019

xmnlab commented Apr 24, 2019

xmnlab commented Apr 30, 2019

gnestor commented Apr 30, 2019

takluyver commented May 2, 2019

takluyver left a comment

takluyver May 2, 2019

takluyver May 2, 2019

xmnlab commented Jun 11, 2019

xmnlab commented Jun 11, 2019

takluyver Jun 12, 2019

xmnlab Jun 17, 2019 •

edited

Loading

takluyver Jun 18, 2019

xmnlab Jun 18, 2019

xmnlab Jun 18, 2019

takluyver Jun 18, 2019

xmnlab Jun 18, 2019

takluyver Jun 18, 2019

xmnlab Jun 18, 2019

takluyver commented Jun 18, 2019

		@@ -113,9 +115,24 @@ def _execvp(cmd, argv):
		os.execvp(cmd, argv)


		def _jupyter_realpath(path, subcommand):


		command = 'jupyter-' + subcommand

		command = _jupyter_abspath(sys.argv[0], subcommand)

Removed mechanism that change $PATH #148

Removed mechanism that change $PATH #148

Conversation

xmnlab commented Apr 4, 2019

xmnlab commented Apr 4, 2019

gnestor commented Apr 9, 2019

minrk commented Apr 9, 2019

xmnlab commented Apr 12, 2019

xmnlab commented Apr 13, 2019

xmnlab commented Apr 13, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xmnlab commented Apr 17, 2019

xmnlab commented Apr 22, 2019

xmnlab commented Apr 24, 2019

xmnlab commented Apr 30, 2019

gnestor commented Apr 30, 2019

takluyver commented May 2, 2019

takluyver left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xmnlab commented Jun 11, 2019

xmnlab commented Jun 11, 2019

Choose a reason for hiding this comment

xmnlab Jun 17, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

takluyver commented Jun 18, 2019

xmnlab Jun 17, 2019 •

edited

Loading