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
10 changes: 3 additions & 7 deletions bundle/artifacts/whl/infer.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,6 @@ type infer struct {
func (m *infer) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics {
artifact := b.Config.Artifacts[m.name]

// TODO use python.DetectVEnvExecutable once bundle has a way to specify venv path
py, err := python.DetectExecutable(ctx)
if err != nil {
return diag.FromErr(err)
}

// Note: using --build-number (build tag) flag does not help with re-installing
// libraries on all-purpose clusters. The reason is that `pip` ignoring build tag
// when upgrading the library and only look at wheel version.
Expand All @@ -36,7 +30,9 @@ func (m *infer) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics {
// version=datetime.datetime.utcnow().strftime("%Y%m%d.%H%M%S"),
// ...
//)
artifact.BuildCommand = fmt.Sprintf(`"%s" setup.py bdist_wheel`, py)

py := python.GetExecutable()
artifact.BuildCommand = fmt.Sprintf(`%s setup.py bdist_wheel`, py)

return nil
}
Expand Down
4 changes: 2 additions & 2 deletions bundle/config/mutator/python/python_mutator_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -541,7 +541,7 @@ func TestLoadDiagnosticsFile_nonExistent(t *testing.T) {

func TestInterpreterPath(t *testing.T) {
if runtime.GOOS == "windows" {
assert.Equal(t, "venv\\Scripts\\python3.exe", interpreterPath("venv"))
assert.Equal(t, "venv\\Scripts\\python.exe", interpreterPath("venv"))
} else {
assert.Equal(t, "venv/bin/python3", interpreterPath("venv"))
}
Expand Down Expand Up @@ -673,7 +673,7 @@ func withFakeVEnv(t *testing.T, venvPath string) {

func interpreterPath(venvPath string) string {
if runtime.GOOS == "windows" {
return filepath.Join(venvPath, "Scripts", "python3.exe")
return filepath.Join(venvPath, "Scripts", "python.exe")
} else {
return filepath.Join(venvPath, "bin", "python3")
}
Expand Down
19 changes: 17 additions & 2 deletions libs/python/detect.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,19 @@ import (
"runtime"
)

// GetExecutable gets appropriate python binary name for the platform
func GetExecutable() string {
// On Windows when virtualenv is created, the <env>/Scripts directory
// contains python.exe but no python3.exe.
// Most installers (e.g. the ones from python.org) only install python.exe and not python3.exe

if runtime.GOOS == "windows" {
return "python"
} else {
return "python3"
}
}

// DetectExecutable looks up the path to the python3 executable from the PATH
// environment variable.
//
Expand All @@ -25,7 +38,9 @@ func DetectExecutable(ctx context.Context) (string, error) {
// the parent directory tree.
//
// See https://github.com/pyenv/pyenv#understanding-python-version-selection
out, err := exec.LookPath("python3")

out, err := exec.LookPath(GetExecutable())

// 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) {
Expand Down Expand Up @@ -54,7 +69,7 @@ func DetectExecutable(ctx context.Context) (string, error) {
func DetectVEnvExecutable(venvPath string) (string, error) {
interpreterPath := filepath.Join(venvPath, "bin", "python3")
if runtime.GOOS == "windows" {
interpreterPath = filepath.Join(venvPath, "Scripts", "python3.exe")
interpreterPath = filepath.Join(venvPath, "Scripts", "python.exe")
}

if _, err := os.Stat(interpreterPath); err != nil {
Expand Down
2 changes: 1 addition & 1 deletion libs/python/detect_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ func TestDetectVEnvExecutable_badLayout(t *testing.T) {

func interpreterPath(venvPath string) string {
if runtime.GOOS == "windows" {
return filepath.Join(venvPath, "Scripts", "python3.exe")
return filepath.Join(venvPath, "Scripts", "python.exe")
} else {
return filepath.Join(venvPath, "bin", "python3")
}
Expand Down
107 changes: 107 additions & 0 deletions libs/python/pythontest/pythontest.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,107 @@
package pythontest

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

"github.com/databricks/cli/internal/testutil"
"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-")
}

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
}
43 changes: 43 additions & 0 deletions libs/python/pythontest/pythontest_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
package pythontest

import (
"context"
"os/exec"
"path/filepath"
"testing"

"github.com/databricks/cli/libs/python"
"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()
dir := t.TempDir()
opts := VenvOpts{
PythonVersion: pythonVersion,
Dir: dir,
}
RequireActivatedPythonEnv(t, ctx, &opts)
require.DirExists(t, opts.EnvPath)
require.DirExists(t, opts.BinPath)
require.FileExists(t, opts.PythonExe)

pythonExe, err := exec.LookPath(python.GetExecutable())
require.NoError(t, err)
require.Equal(t, filepath.Dir(pythonExe), filepath.Dir(opts.PythonExe))
require.FileExists(t, 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