Skip to content

Fix finding Python within virtualenv on Windows - #2034

Merged
denik merged 12 commits into
mainfrom
DECO-24194--fix-venv-windows
Dec 20, 2024
Merged

Fix finding Python within virtualenv on Windows#2034
denik merged 12 commits into
mainfrom
DECO-24194--fix-venv-windows

Conversation

@denik

@denikdenik commented Dec 18, 2024•
edited
Loading

Copy link
Copy Markdown
Contributor

Changes

Simplify logic for selecting Python to run when calculating default whl build command: "python" on Windows and "python3" everywhere.

Python installers from python.org do not install python3.exe. In virtualenv there is no python3.exe.

Tests

Added new unit tests to create real venv with uv and simulate activation by prepending venv/bin to PATH.

@denik
denikforce-pushed the DECO-24194--fix-venv-windows branch from a1fc119 to 7fbd9feCompareDecember 18, 2024 20:14
@denik
denikforce-pushed the DECO-24194--fix-venv-windows branch from 7fbd9fe to 3c3c172CompareDecember 18, 2024 20:15
@denik
denik enabled auto-merge December 18, 2024 20:44
@denik
denik disabled auto-merge December 19, 2024 08:24
pietern
pietern reviewed Dec 19, 2024
View reviewed changes
Comment threadinternal/testutil/env.go Outdated
Comment threadinternal/testutil/env.go Outdated
Comment threadlibs/python/detect.go Outdated

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

@pieternpieternDec 19, 2024

Copy link
Copy Markdown
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?

@denikdenikDec 19, 2024

Copy link
Copy Markdown
ContributorAuthor

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?

@denikdenikDec 19, 2024

Copy link
Copy Markdown
ContributorAuthor

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.

@andrewnesterandrewnesterDec 19, 2024

Copy link
Copy Markdown
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?

@denikdenikDec 19, 2024

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

that should not matter

Comment threadlibs/python/detect.go Outdated
Comment threadlibs/python/pythontest/pythontest.go Outdated
Comment threadlibs/python/pythontest/pythontest.go Outdated
Comment threadlibs/python/pythontest/pythontest_test.go
)

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

@pieternpieternDec 19, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Smart :)

@denik
denik requested a review from pieternDecember 19, 2024 09:30
@eng-dev-ecosystem-bot

eng-dev-ecosystem-bot commented Dec 19, 2024

Copy link
Copy Markdown
Collaborator

Test Details: go/deco-tests/12409928666

andrewnester
andrewnester reviewed Dec 19, 2024
View reviewed changes
Comment threadlibs/python/pythontest/pythontest.go
Comment threadlibs/python/detect.go Outdated

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

@andrewnesterandrewnesterDec 19, 2024

Copy link
Copy Markdown
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?

Comment threadlibs/python/pythontest/pythontest.go
Comment threadlibs/python/pythontest/pythontest.go
@github-actions

github-actionsBot commented Dec 19, 2024

Copy link
Copy Markdown
Contributor

If integration tests don't run automatically, an authorized user can run them manually by following the instructions below:

Trigger:
go/deco-tests-run/cli

Inputs:

  • PR number: 2034
  • Commit SHA: 3bf36aaedd4a4f6eb166adc300a1f11ed6c65638

Checks will be approved automatically on success.

@denik
denik enabled auto-merge December 19, 2024 17:05
@denik
denik added this pull request to the merge queueDec 20, 2024
Merged via the queue into main with commit 2fee243Dec 20, 2024
@denik
denik deleted the DECO-24194--fix-venv-windows branch December 20, 2024 07:49
pietern added a commit that referenced this pull request Jan 8, 2025
@pietern
[Release] Release v0.238.0
019b7c3
Bundles:
* Fix finding Python within virtualenv on Windows ([#2034](#2034)).
* Include missing field descriptions in JSON schema ([#2045](#2045)).
* Add validation for volume referenced from `artifact_path` ([#2050](#2050)).
* Handle `${workspace.file_path}` references in source-linked deployments ([#2046](#2046)).
* Set the write bit for files written during template initialization ([#2068](#2068)).
@pieternpietern mentioned this pull request Jan 8, 2025
Merged
github-merge-queueBot pushed a commit that referenced this pull request Jan 8, 2025
@pietern
[Release] Release v0.238.0 (#2096)
b48fa70
Bundles:
* Fix finding Python within virtualenv on Windows
([#2034](#2034)).
* Include missing field descriptions in JSON schema
([#2045](#2045)).
* Add validation for volume referenced from `artifact_path`
([#2050](#2050)).
* Handle `${workspace.file_path}` references in source-linked
deployments ([#2046](#2046)).
* Set the write bit for files written during template initialization
([#2068](#2068)).
@denikdenik mentioned this pull request Jan 27, 2025
Merged
github-merge-queueBot pushed a commit that referenced this pull request Jan 27, 2025
@denik
libs/python: Remove DetectInterpreters (#2234)
65fbbd9
## Changes
- Remove DetectInterpreters from DetectExecutable call: python3 or
python should always be on on the PATH. We don't need to detect
non-standard situations like python3.10 is present but python3 is not.
- I moved DetectInterpreters to cmd/labs where it is still used.
This is a follow up to #2034
## Tests
Existing tests.
@denikdenik mentioned this pull request Mar 3, 2025
Merged
denik added a commit that referenced this pull request May 20, 2026
@denik
Fix finding Python within virtualenv on Windows (#2034)
e9d9caa
## Changes
Simplify logic for selecting Python to run when calculating default whl
build command: "python" on Windows and "python3" everywhere.
Python installers from python.org do not install python3.exe. In
virtualenv there is no python3.exe.
## Tests
Added new unit tests to create real venv with uv and simulate activation
by prepending venv/bin to PATH.
denik pushed a commit that referenced this pull request May 20, 2026
@pietern
[Release] Release v0.238.0 (#2096)
3406824
Bundles:
* Fix finding Python within virtualenv on Windows
([#2034](#2034)).
* Include missing field descriptions in JSON schema
([#2045](#2045)).
* Add validation for volume referenced from `artifact_path`
([#2050](#2050)).
* Handle `${workspace.file_path}` references in source-linked
deployments ([#2046](#2046)).
* Set the write bit for files written during template initialization
([#2068](#2068)).
denik added a commit that referenced this pull request May 20, 2026
@denik
libs/python: Remove DetectInterpreters (#2234)
5a60099
## Changes
- Remove DetectInterpreters from DetectExecutable call: python3 or
python should always be on on the PATH. We don't need to detect
non-standard situations like python3.10 is present but python3 is not.
- I moved DetectInterpreters to cmd/labs where it is still used.
This is a follow up to #2034
## Tests
Existing tests.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pieternpieternpietern approved these changes
@shreyas-goenkashreyas-goenkaAwaiting requested review from shreyas-goenka
@andrewnesterandrewnesterAwaiting requested review from andrewnester

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@denik@eng-dev-ecosystem-bot@pietern@andrewnester