Skip to content
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 finding Python within virtualenv on Windows #2034

Merged
merged 12 commits into from
Dec 20, 2024
3 changes: 3 additions & 0 deletions .github/workflows/push.yml
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,9 @@ jobs:
with:
python-version: '3.9'

- name: Install uv
uses: astral-sh/setup-uv@v4

- name: Set go env
run: |
echo "GOPATH=$(go env GOPATH)" >> $GITHUB_ENV
Expand Down
17 changes: 17 additions & 0 deletions libs/python/detect.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this regress the non-venv cases where python.exe resolves to a system-wide Python 2 installation?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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")
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)
Expand Down
115 changes: 115 additions & 0 deletions libs/python/pythontest/pythontest.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,115 @@
package pythontest

import (
"context"
"errors"
"fmt"
"os"
"os/exec"
"path/filepath"
"runtime"
"strings"
"testing"

"github.com/databricks/cli/internal/testutil"
"github.com/databricks/cli/libs/python"
"github.com/stretchr/testify/require"
)

type VenvOpts struct {
// input
PythonVersion string
skipVersionCheck bool

// input/output
Dir string
Name string

// output:
// Absolute path to venv
EnvPath string

// Absolute path to venv/bin or venv/Scripts, depending on OS
BinPath string

// Absolute path to python binary
PythonExe string
}

func CreatePythonEnv(opts *VenvOpts) error {
if opts == nil || opts.PythonVersion == "" {
return errors.New("PythonVersion must be provided")
}
if opts.Name == "" {
opts.Name = testutil.RandomName("test-venv-")
}
if opts.Dir != "" {
opts.Dir = "."
}

cmd := exec.Command("uv", "venv", opts.Name, "--python", opts.PythonVersion, "--seed", "-q")
cmd.Stdout = os.Stdout
denik marked this conversation as resolved.
Show resolved Hide resolved
cmd.Stderr = os.Stderr
cmd.Dir = opts.Dir
err := cmd.Run()
if err != nil {
return err
}

opts.EnvPath, err = filepath.Abs(filepath.Join(opts.Dir, opts.Name))
if err != nil {
return err
}

_, err = os.Stat(opts.EnvPath)
if err != nil {
return fmt.Errorf("cannot stat EnvPath %s: %s", opts.EnvPath, err)
}

if runtime.GOOS == "windows" {
// https://github.com/pypa/virtualenv/commit/993ba1316a83b760370f5a3872b3f5ef4dd904c1
opts.BinPath = filepath.Join(opts.EnvPath, "Scripts")
opts.PythonExe = filepath.Join(opts.BinPath, "python.exe")
} else {
opts.BinPath = filepath.Join(opts.EnvPath, "bin")
opts.PythonExe = filepath.Join(opts.BinPath, "python3")
}

_, err = os.Stat(opts.BinPath)
if err != nil {
return fmt.Errorf("cannot stat BinPath %s: %s", opts.BinPath, err)
}

_, err = os.Stat(opts.PythonExe)
if err != nil {
return fmt.Errorf("cannot stat PythonExe %s: %s", opts.PythonExe, err)
}

if !opts.skipVersionCheck {
cmd := exec.Command(opts.PythonExe, "--version")
out, err := cmd.CombinedOutput()
if err != nil {
return fmt.Errorf("Failed to run %s --version: %s", opts.PythonExe, err)
}
outString := string(out)
expectVersion := "Python " + opts.PythonVersion
if !strings.HasPrefix(outString, expectVersion) {
return fmt.Errorf("Unexpected output from %s --version: %v (expected %v)", opts.PythonExe, outString, expectVersion)
}
}

return nil
}

func RequireActivatedPythonEnv(t *testing.T, ctx context.Context, opts *VenvOpts) {
err := CreatePythonEnv(opts)
require.NoError(t, err)
require.DirExists(t, opts.BinPath)

newPath := fmt.Sprintf("%s%c%s", opts.BinPath, os.PathListSeparator, os.Getenv("PATH"))
denik marked this conversation as resolved.
Show resolved Hide resolved
t.Setenv("PATH", newPath)
denik marked this conversation as resolved.
Show resolved Hide resolved

pythonExe, err := python.DetectExecutable(ctx)
require.NoError(t, err)
require.Equal(t, filepath.Dir(pythonExe), filepath.Dir(opts.PythonExe))
}
31 changes: 31 additions & 0 deletions libs/python/pythontest/pythontest_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
package pythontest

import (
"context"
"testing"

"github.com/stretchr/testify/require"
)

func TestVenvSuccess(t *testing.T) {
// Test at least two version to ensure we capture a case where venv version does not match system one
Copy link
Contributor

Choose a reason for hiding this comment

The 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()
opts := VenvOpts{PythonVersion: pythonVersion}
RequireActivatedPythonEnv(t, ctx, &opts)
require.DirExists(t, opts.EnvPath)
require.DirExists(t, opts.BinPath)
require.FileExists(t, opts.PythonExe)
})
}
}

func TestWrongVersion(t *testing.T) {
require.Error(t, CreatePythonEnv(&VenvOpts{PythonVersion: "4.0"}))
}

func TestMissingVersion(t *testing.T) {
require.Error(t, CreatePythonEnv(nil))
require.Error(t, CreatePythonEnv(&VenvOpts{}))
}
Loading