From 5910212f7d4fe59b210e18293bfbe8ba97f93ca4 Mon Sep 17 00:00:00 2001 From: Michael Marchetti Date: Wed, 4 Oct 2023 15:46:16 -0400 Subject: [PATCH 1/7] use exec.LookPath to find Python binaries --- internal/environment/python.go | 30 ++++++++++++++++++++++++----- internal/environment/python_test.go | 2 +- 2 files changed, 26 insertions(+), 6 deletions(-) diff --git a/internal/environment/python.go b/internal/environment/python.go index 97964f7f2..6522c9207 100644 --- a/internal/environment/python.go +++ b/internal/environment/python.go @@ -3,6 +3,7 @@ package environment import ( "bytes" "fmt" + "io/fs" "os/exec" "strings" @@ -54,18 +55,34 @@ func NewPythonInspector(projectDir util.Path, pythonPath util.Path, log logging. } } -func (i *defaultPythonInspector) getPythonExecutable() string { +func (i *defaultPythonInspector) getPythonExecutable() (string, error) { if i.pythonPath.Path() != "" { // User-provided python executable - return i.pythonPath.Path() + exists, err := i.pythonPath.Exists() + if err != nil { + return "", err + } + if exists { + return i.pythonPath.Path(), nil + } + return "", fmt.Errorf( + "cannot find the specified Python executable %s: %w", + i.pythonPath, fs.ErrNotExist) } else { // Use whatever is on PATH - return "python3" + path, err := exec.LookPath("python3") + if err != nil { + return exec.LookPath("python") + } + return path, err } } func (i *defaultPythonInspector) GetPythonVersion() (string, error) { - pythonExecutable := i.getPythonExecutable() + pythonExecutable, err := i.getPythonExecutable() + if err != nil { + return "", err + } args := []string{ `-E`, // ignore python-specific environment variables `-c`, // execute the next argument as python code @@ -90,7 +107,10 @@ func (i *defaultPythonInspector) GetPythonRequirements() ([]byte, error) { i.log.Info("Using Python packages", "source", requirementsFilename) return requirementsFilename.ReadFile() } - pythonExecutable := i.getPythonExecutable() + pythonExecutable, err := i.getPythonExecutable() + if err != nil { + return nil, err + } source := fmt.Sprintf("'%s -m pip freeze'", pythonExecutable) i.log.Info("Using Python packages", "source", source) args := []string{"-m", "pip", "freeze"} diff --git a/internal/environment/python_test.go b/internal/environment/python_test.go index 6d0c5660c..87d3ba5e6 100644 --- a/internal/environment/python_test.go +++ b/internal/environment/python_test.go @@ -84,7 +84,7 @@ func (s *PythonSuite) TestGetPythonVersionFromPATH() { log := logging.New() inspector := NewPythonInspector(util.Path{}, util.Path{}, log) executor := NewMockPythonExecutor() - executor.On("runPythonCommand", "python3", mock.Anything).Return([]byte("3.10.4"), nil) + executor.On("runPythonCommand", mock.Anything, mock.Anything).Return([]byte("3.10.4"), nil) inspector.executor = executor version, err := inspector.GetPythonVersion() s.Nil(err) From a6b7ddd33fd2eb8befc6d03f1fdd3423fc7e5dfe Mon Sep 17 00:00:00 2001 From: Michael Marchetti Date: Wed, 4 Oct 2023 15:55:28 -0400 Subject: [PATCH 2/7] update test --- internal/environment/python_test.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/internal/environment/python_test.go b/internal/environment/python_test.go index 87d3ba5e6..0824f9aa7 100644 --- a/internal/environment/python_test.go +++ b/internal/environment/python_test.go @@ -92,11 +92,14 @@ func (s *PythonSuite) TestGetPythonVersionFromPATH() { executor.AssertExpectations(s.T()) } -func (s *PythonSuite) TestGetPythonVersionFromRealDefaultPython() { +func (s *PythonSuite) TestGetPythonVersionFromRealDefaultPython3() { // This test can only run if python3 is on the PATH. _, err := exec.LookPath("python3") if err != nil { - s.T().Skip("python3 isn't available on PATH") + _, err := exec.LookPath("python") + if err != nil { + s.T().Skip("This test requires python or python3 to be available on PATH") + } } log := logging.New() inspector := NewPythonInspector(util.Path{}, util.Path{}, log) From 6ac4e8c2ec2edaacff73b0139498d1d195a1ad67 Mon Sep 17 00:00:00 2001 From: Michael Marchetti Date: Wed, 4 Oct 2023 16:12:11 -0400 Subject: [PATCH 3/7] test for python fallback --- internal/environment/python.go | 6 +++--- internal/environment/python_test.go | 16 ++++++++++++++-- 2 files changed, 17 insertions(+), 5 deletions(-) diff --git a/internal/environment/python.go b/internal/environment/python.go index 6522c9207..59fb6516f 100644 --- a/internal/environment/python.go +++ b/internal/environment/python.go @@ -55,7 +55,7 @@ func NewPythonInspector(projectDir util.Path, pythonPath util.Path, log logging. } } -func (i *defaultPythonInspector) getPythonExecutable() (string, error) { +func (i *defaultPythonInspector) getPythonExecutable(exec util.PathLooker) (string, error) { if i.pythonPath.Path() != "" { // User-provided python executable exists, err := i.pythonPath.Exists() @@ -79,7 +79,7 @@ func (i *defaultPythonInspector) getPythonExecutable() (string, error) { } func (i *defaultPythonInspector) GetPythonVersion() (string, error) { - pythonExecutable, err := i.getPythonExecutable() + pythonExecutable, err := i.getPythonExecutable(util.NewPathLooker()) if err != nil { return "", err } @@ -107,7 +107,7 @@ func (i *defaultPythonInspector) GetPythonRequirements() ([]byte, error) { i.log.Info("Using Python packages", "source", requirementsFilename) return requirementsFilename.ReadFile() } - pythonExecutable, err := i.getPythonExecutable() + pythonExecutable, err := i.getPythonExecutable(util.NewPathLooker()) if err != nil { return nil, err } diff --git a/internal/environment/python_test.go b/internal/environment/python_test.go index 0824f9aa7..37bd8df6c 100644 --- a/internal/environment/python_test.go +++ b/internal/environment/python_test.go @@ -92,8 +92,8 @@ func (s *PythonSuite) TestGetPythonVersionFromPATH() { executor.AssertExpectations(s.T()) } -func (s *PythonSuite) TestGetPythonVersionFromRealDefaultPython3() { - // This test can only run if python3 is on the PATH. +func (s *PythonSuite) TestGetPythonVersionFromRealDefaultPython() { + // This test can only run if python3 or python is on the PATH. _, err := exec.LookPath("python3") if err != nil { _, err := exec.LookPath("python") @@ -108,6 +108,18 @@ func (s *PythonSuite) TestGetPythonVersionFromRealDefaultPython3() { s.True(strings.HasPrefix(version, "3.")) } +func (s *PythonSuite) TestGetPythonExecutableFallbackPython() { + log := logging.New() + inspector := NewPythonInspector(util.Path{}, util.Path{}, log) + + exec := util.NewMockPathLooker() + exec.On("LookPath", "python3").Return("", os.ErrNotExist) + exec.On("LookPath", "python").Return("/usr/bin/python", nil) + executable, err := inspector.getPythonExecutable(exec) + s.NoError(err) + s.Equal("/usr/bin/python", executable) +} + func (s *PythonSuite) TestGetRequirementsFromFile() { baseDir, err := util.Getwd(afero.NewMemMapFs()) s.Nil(err) From 174fdab0d5e4d22f8d736aca8aa8647c2fb66260 Mon Sep 17 00:00:00 2001 From: Michael Marchetti Date: Wed, 4 Oct 2023 17:09:37 -0400 Subject: [PATCH 4/7] add process.go --- internal/util/process.go | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) create mode 100644 internal/util/process.go diff --git a/internal/util/process.go b/internal/util/process.go new file mode 100644 index 000000000..252a64061 --- /dev/null +++ b/internal/util/process.go @@ -0,0 +1,36 @@ +package util + +// Copyright (C) 2023 by Posit Software, PBC. + +import ( + "os/exec" + + "github.com/stretchr/testify/mock" +) + +type PathLooker interface { + LookPath(name string) (string, error) +} + +type defaultPathLooker struct{} + +func NewPathLooker() PathLooker { + return &defaultPathLooker{} +} + +func (p *defaultPathLooker) LookPath(name string) (string, error) { + return exec.LookPath(name) +} + +type mockPathLooker struct { + mock.Mock +} + +func NewMockPathLooker() *mockPathLooker { + return &mockPathLooker{} +} + +func (m *mockPathLooker) LookPath(name string) (string, error) { + args := m.Called(name) + return args.String(0), args.Error(1) +} From f785ac817135a0b696db7f2e4c63609565cb7800 Mon Sep 17 00:00:00 2001 From: tdstein Date: Mon, 16 Oct 2023 11:00:07 -0400 Subject: [PATCH 5/7] Modifies type declarations to resolve mypy --- .../connect_jupyterlab/connect_jupyterlab/handlers.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/extensions/jupyterlab/connect_jupyterlab/connect_jupyterlab/handlers.py b/extensions/jupyterlab/connect_jupyterlab/connect_jupyterlab/handlers.py index 5650ee411..02c94dca8 100644 --- a/extensions/jupyterlab/connect_jupyterlab/connect_jupyterlab/handlers.py +++ b/extensions/jupyterlab/connect_jupyterlab/connect_jupyterlab/handlers.py @@ -5,7 +5,7 @@ import shlex import subprocess from urllib.parse import urlparse -from typing import Set, Dict, Tuple +from typing import Set, Dict, Tuple, Any from jupyter_server.base.handlers import APIHandler from jupyter_server.utils import url_path_join @@ -14,7 +14,7 @@ from tornado.httputil import HTTPServerRequest from tornado.web import authenticated -base_url = None +base_url: str = "" EXECUTABLE = "connect-client" known_ports: Set[int] = set() @@ -26,7 +26,7 @@ class PublishHandler(APIHandler): def post(self) -> None: """post initiates the publishing process. Details TBD.""" self.log.info("Launching publishing UI") - data: Dict[str, str] = self.get_json_body() + data: Any = self.get_json_body() notebookPath = os.path.abspath(data["notebookPath"]) pythonPath = data["pythonPath"] pythonVersion = data["pythonVersion"] From 9cab7dd866959fef01b491d6bf88a5a1547f9de9 Mon Sep 17 00:00:00 2001 From: Michael Marchetti Date: Mon, 16 Oct 2023 13:44:36 -0400 Subject: [PATCH 6/7] Log the python executable we are running --- internal/environment/python.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/internal/environment/python.go b/internal/environment/python.go index 59fb6516f..d16e56e23 100644 --- a/internal/environment/python.go +++ b/internal/environment/python.go @@ -83,6 +83,7 @@ func (i *defaultPythonInspector) GetPythonVersion() (string, error) { if err != nil { return "", err } + i.log.Info("Running Python", "python", pythonExecutable) args := []string{ `-E`, // ignore python-specific environment variables `-c`, // execute the next argument as python code @@ -111,6 +112,7 @@ func (i *defaultPythonInspector) GetPythonRequirements() ([]byte, error) { if err != nil { return nil, err } + i.log.Info("Running Python", "python", pythonExecutable) source := fmt.Sprintf("'%s -m pip freeze'", pythonExecutable) i.log.Info("Using Python packages", "source", source) args := []string{"-m", "pip", "freeze"} From ae3d263e65fea7acf0255cb558bdc815f1164d28 Mon Sep 17 00:00:00 2001 From: Michael Marchetti Date: Mon, 16 Oct 2023 15:02:30 -0400 Subject: [PATCH 7/7] verify Python is runnable --- internal/environment/python.go | 23 ++++++++- internal/environment/python_test.go | 73 +++++++++++++++++++++++++++-- 2 files changed, 91 insertions(+), 5 deletions(-) diff --git a/internal/environment/python.go b/internal/environment/python.go index d16e56e23..9f26fa428 100644 --- a/internal/environment/python.go +++ b/internal/environment/python.go @@ -55,6 +55,12 @@ func NewPythonInspector(projectDir util.Path, pythonPath util.Path, log logging. } } +func (i *defaultPythonInspector) validatePythonExecutable(pythonExecutable string) error { + args := []string{"--version"} + _, err := i.executor.runPythonCommand(pythonExecutable, args) + return err +} + func (i *defaultPythonInspector) getPythonExecutable(exec util.PathLooker) (string, error) { if i.pythonPath.Path() != "" { // User-provided python executable @@ -71,10 +77,23 @@ func (i *defaultPythonInspector) getPythonExecutable(exec util.PathLooker) (stri } else { // Use whatever is on PATH path, err := exec.LookPath("python3") + if err == nil { + // Ensure the Python is actually runnable. This is especially + // needed on Windows, where `python3` is (by default) + // an app execution alias. Also, installing Python from + // python.org does not disable the built-in app execution aliases. + err = i.validatePythonExecutable(path) + } if err != nil { - return exec.LookPath("python") + path, err = exec.LookPath("python") + if err == nil { + err = i.validatePythonExecutable(path) + } + } + if err != nil { + return "", err } - return path, err + return path, nil } } diff --git a/internal/environment/python_test.go b/internal/environment/python_test.go index 37bd8df6c..29aa6d0c1 100644 --- a/internal/environment/python_test.go +++ b/internal/environment/python_test.go @@ -108,16 +108,83 @@ func (s *PythonSuite) TestGetPythonVersionFromRealDefaultPython() { s.True(strings.HasPrefix(version, "3.")) } +type mockPythonExecutor struct { + mock.Mock +} + +var _ pythonExecutor = &mockPythonExecutor{} + +func (m *mockPythonExecutor) runPythonCommand(pythonExecutable string, args []string) ([]byte, error) { + mockArgs := m.Called(pythonExecutable, args) + out := mockArgs.Get(0) + if out != nil { + return out.([]byte), mockArgs.Error(1) + } else { + return nil, mockArgs.Error(1) + } +} + func (s *PythonSuite) TestGetPythonExecutableFallbackPython() { + // python3 does not exist + // python exists and is runnable log := logging.New() - inspector := NewPythonInspector(util.Path{}, util.Path{}, log) + executor := &mockPythonExecutor{} + executor.On("runPythonCommand", "/some/python", mock.Anything).Return(nil, nil) + inspector := &defaultPythonInspector{ + executor: executor, + log: log, + } exec := util.NewMockPathLooker() exec.On("LookPath", "python3").Return("", os.ErrNotExist) - exec.On("LookPath", "python").Return("/usr/bin/python", nil) + exec.On("LookPath", "python").Return("/some/python", nil) + executable, err := inspector.getPythonExecutable(exec) + s.NoError(err) + s.Equal("/some/python", executable) +} + +func (s *PythonSuite) TestGetPythonExecutablePython3NotRunnable() { + // python3 exists but is not runnable + // python exists and is runnable + log := logging.New() + executor := &mockPythonExecutor{} + testError := errors.New("exit status 9009") + executor.On("runPythonCommand", "/some/python3", mock.Anything).Return(nil, testError) + executor.On("runPythonCommand", "/some/python", mock.Anything).Return(nil, nil) + + inspector := &defaultPythonInspector{ + executor: executor, + log: log, + } + + exec := util.NewMockPathLooker() + exec.On("LookPath", "python3").Return("/some/python3", nil) + exec.On("LookPath", "python").Return("/some/python", nil) executable, err := inspector.getPythonExecutable(exec) s.NoError(err) - s.Equal("/usr/bin/python", executable) + s.Equal("/some/python", executable) +} + +func (s *PythonSuite) TestGetPythonExecutableNoRunnablePython() { + // python3 exists but is not runnable + // python exists but is not runnable + log := logging.New() + executor := &mockPythonExecutor{} + testError := errors.New("exit status 9009") + executor.On("runPythonCommand", "/some/python3", mock.Anything).Return(nil, testError) + executor.On("runPythonCommand", "/some/python", mock.Anything).Return(nil, testError) + + inspector := &defaultPythonInspector{ + executor: executor, + log: log, + } + + exec := util.NewMockPathLooker() + exec.On("LookPath", "python3").Return("/some/python3", nil) + exec.On("LookPath", "python").Return("/some/python", nil) + executable, err := inspector.getPythonExecutable(exec) + s.NotNil(err) + s.Equal("", executable) } func (s *PythonSuite) TestGetRequirementsFromFile() {