-
Notifications
You must be signed in to change notification settings - Fork 180
Fix finding Python within virtualenv on Windows #2034
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
Changes from 1 commit
3c3c172
dcfeeac
5b4c762
640de90
2b89625
b50227a
21a4cd1
56785cf
1357f4a
5a931dd
094530a
3bf36aa
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| package testutil | ||
|
|
||
| import ( | ||
| "bytes" | ||
| "os" | ||
| "os/exec" | ||
|
|
||
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| func RunCommand(t TestingT, name string, args ...string) { | ||
| cmd := exec.Command(name, args...) | ||
| cmd.Stdout = os.Stdout | ||
| cmd.Stderr = os.Stderr | ||
| require.NoError(t, cmd.Run()) | ||
| } | ||
|
|
||
| func CaptureCommandOutput(t TestingT, name string, args ...string) string { | ||
| cmd := exec.Command(name, args...) | ||
| var stdout bytes.Buffer | ||
| cmd.Stdout = &stdout | ||
| cmd.Stderr = os.Stderr | ||
| err := cmd.Run() | ||
| require.NoError(t, err) | ||
| return stdout.String() | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,15 +25,32 @@ func DetectExecutable(ctx context.Context) (string, error) { | |
| // the parent directory tree. | ||
| // | ||
| // See https://github.com/pyenv/pyenv#understanding-python-version-selection | ||
|
|
||
| // On Windows when virtualenv is created, the <env>/Scripts directory | ||
| // contains python.exe but no python3.exe. However, system python does have python3 entry | ||
| // and it is also added to PATH, so it is found first. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could this regress the non-venv cases where
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe, if they somehow managed to have Python2 installed in the first place, but why would they have that?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see python3 mentioned in another place in this module - it also won't work.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does it matter which Windows terminal is being used? Some users might use Git Bash for example and maybe the behaviour is different?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. that should not matter |
||
| if runtime.GOOS == "windows" { | ||
| out, err := exec.LookPath("python.exe") | ||
|
denik marked this conversation as resolved.
Outdated
|
||
| if err == nil && out != "" { | ||
| return out, nil | ||
| } | ||
| if err != nil && !errors.Is(err, exec.ErrNotFound) { | ||
| return "", err | ||
| } | ||
| } | ||
|
|
||
| out, err := exec.LookPath("python3") | ||
|
|
||
| // most of the OS'es have python3 in $PATH, but for those which don't, | ||
| // we perform the latest version lookup | ||
| if err != nil && !errors.Is(err, exec.ErrNotFound) { | ||
| return "", err | ||
| } | ||
|
|
||
| if out != "" { | ||
| return out, nil | ||
| } | ||
|
|
||
| // otherwise, detect all interpreters and pick the least that satisfies | ||
| // minimal version requirements | ||
| all, err := DetectInterpreters(ctx) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,32 @@ | ||
| package pythontest | ||
|
|
||
| import ( | ||
| "context" | ||
| "path/filepath" | ||
| "strings" | ||
|
|
||
| "github.com/databricks/cli/internal/testutil" | ||
| "github.com/databricks/cli/libs/python" | ||
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| func RequirePythonVENV(t testutil.TestingT, ctx context.Context, pythonVersion string, checkVersion bool) string { | ||
|
denik marked this conversation as resolved.
Outdated
|
||
| tmpDir := t.TempDir() | ||
| testutil.Chdir(t, tmpDir) | ||
|
|
||
| venvName := testutil.RandomName("test-venv-") | ||
| testutil.RunCommand(t, "uv", "venv", venvName, "--python", pythonVersion, "--seed") | ||
|
denik marked this conversation as resolved.
Outdated
|
||
| testutil.InsertVirtualenvInPath(t, filepath.Join(tmpDir, venvName)) | ||
|
|
||
| pythonExe, err := python.DetectExecutable(ctx) | ||
| require.NoError(t, err) | ||
| require.Contains(t, pythonExe, venvName) | ||
|
|
||
| if checkVersion { | ||
| actualVersion := testutil.CaptureCommandOutput(t, pythonExe, "--version") | ||
| expectVersion := "Python " + pythonVersion | ||
| require.True(t, strings.HasPrefix(actualVersion, expectVersion), "Running %s --version: Expected %v, got %v", pythonExe, expectVersion, actualVersion) | ||
| } | ||
|
|
||
| return tmpDir | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| package pythontest | ||
|
|
||
| import ( | ||
| "context" | ||
| "testing" | ||
| ) | ||
|
|
||
| func TestVenv(t *testing.T) { | ||
| // Test at least two version to ensure we capture a case where venv version does not match system one | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Smart :) |
||
| for _, pythonVersion := range []string{"3.11", "3.12"} { | ||
| t.Run(pythonVersion, func(t *testing.T) { | ||
| ctx := context.Background() | ||
| RequirePythonVENV(t, ctx, pythonVersion, true) | ||
| }) | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.