Fix poetry env activate for Bash on Windows #10722
Closed
+5
−1
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's GuideAdjusts virtualenv activation behavior so POSIX-style shells on Windows receive a proper 'source' prefix, introduces validation for 'poetry update' package arguments, and makes lockfile dependency constraints output deterministic via sorting, with accompanying tests. Sequence diagram for the updated poetry env activate behavior on different shellssequenceDiagram
actor User
participant Shell
participant PoetryEnvActivate as PoetryEnvActivateCommand
participant Env
rect rgb(235, 245, 255)
User->>Shell: eval $(poetry env activate)
Shell->>PoetryEnvActivate: poetry env activate --shell bash
PoetryEnvActivate->>Env: resolve_activation_script()
Env-->>PoetryEnvActivate: bin_dir/activate exists
PoetryEnvActivate-->>Shell: source /path/to/activate
Shell->>Env: source /path/to/activate
end
rect rgb(245, 235, 255)
User->>Shell: poetry env activate --shell powershell
Shell->>PoetryEnvActivate: poetry env activate --shell powershell
PoetryEnvActivate->>Env: resolve_activation_script()
Env-->>PoetryEnvActivate: bin_dir/activate.ps1 exists
PoetryEnvActivate-->>Shell: /path/to/activate.ps1
Shell->>Env: . /path/to/activate.ps1
end
rect rgb(235, 255, 235)
User->>Shell: poetry env activate --shell cmd
Shell->>PoetryEnvActivate: poetry env activate --shell cmd
PoetryEnvActivate->>Env: resolve_activation_script()
Env-->>PoetryEnvActivate: bin_dir/activate.bat exists
PoetryEnvActivate-->>Shell: C:\\path\\to\\activate.bat
Shell->>Env: C:\\path\\to\\activate.bat
end
Updated class diagram for commands and locker behaviorclassDiagram
class UpdateCommand {
+handle() int
}
class EnvActivateCommand {
+_get_activate_command(env Env, shell str) str
}
class Locker {
+_dump_package(package Package, category str) dict
}
class Env {
+bin_dir Path
}
class Package {
+all_requires list
+extras dict
}
UpdateCommand --> Package : validates_update_packages_against
Locker --> Package : serializes_dependencies_of
EnvActivateCommand --> Env : reads_activation_script_from
UpdateCommand --> EnvActivateCommand : indirectly_uses_env_activation
class UpdateCommand {
+handle() int
+validate_packages(packages list) list
}
class EnvActivateCommand {
+_get_activate_command(env Env, shell str) str
+_quote(path str, shell str) str
}
class Locker {
+_dump_package(package Package, category str) dict
+sort_dependency_constraints(constraints list) list
}
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- The package-name parsing in
UpdateCommand.handleis quite brittle (manualspliton>,<,=,!); consider usingpackaging.requirements.Requirement(or a similar existing parser in the codebase) so that extras, spaces, and more complex version specifiers are handled correctly. - When validating packages in
UpdateCommand.handle, you might want to strip extras ([extra]) before canonicalization, otherwise valid dependency names with extras may be incorrectly treated as invalid.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The package-name parsing in `UpdateCommand.handle` is quite brittle (manual `split` on `>`, `<`, `=`, `!`); consider using `packaging.requirements.Requirement` (or a similar existing parser in the codebase) so that extras, spaces, and more complex version specifiers are handled correctly.
- When validating packages in `UpdateCommand.handle`, you might want to strip extras (`[extra]`) before canonicalization, otherwise valid dependency names with extras may be incorrectly treated as invalid.
## Individual Comments
### Comment 1
<location> `src/poetry/console/commands/update.py:55-58` </location>
<code_context>
+ }
+
+ invalid_packages = []
+ for package in packages:
+ # Check if package name contains version constraint
+ # (e.g., "package==1.0.0" or "package>=1.0")
+ package_name = package.split(">")[0].split("<")[0].split("=")[0].split("!")[0].strip()
+ canonical_name = canonicalize_name(package_name)
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Manual splitting of version constraints is brittle and will mis-handle more complex specifiers.
This chained `split` approach will break on common valid forms like multiple constraints (`pkg>=1,<2`), markers (`pkg; python_version<'3.11'`), extras (`pkg[extra]==1.0`), or varied spacing. Prefer using an existing parser such as Poetry’s requirement/specifier utilities or `packaging.requirements.Requirement` to derive the canonical package name so all valid syntaxes remain supported without custom parsing updates.
</issue_to_address>
### Comment 2
<location> `src/poetry/console/commands/update.py:58-59` </location>
<code_context>
+ for package in packages:
+ # Check if package name contains version constraint
+ # (e.g., "package==1.0.0" or "package>=1.0")
+ package_name = package.split(">")[0].split("<")[0].split("=")[0].split("!")[0].strip()
+ canonical_name = canonicalize_name(package_name)
+
+ if canonical_name not in all_dependencies:
</code_context>
<issue_to_address>
**issue (bug_risk):** Extras in package specs will be treated as invalid dependencies.
For inputs like `poetry update foo[bar]==1.0`, `package_name` becomes `foo[bar]`. `canonicalize_name` will preserve the brackets, but `all_dependencies` is keyed by `dep.name` (typically just `foo`), so dependencies with extras will be incorrectly flagged as invalid. When deriving `package_name`, strip any extras segment (e.g. split on `[` before canonicalization) or use a requirement parser that gives you `.name` separately from extras.
</issue_to_address>
### Comment 3
<location> `src/poetry/console/commands/env/activate.py:60-61` </location>
<code_context>
if (activation_script := env.bin_dir / filename).exists():
- if WINDOWS:
+ # Only omit the command prefix for PowerShell and cmd on Windows
+ if not command:
return f"{self._quote(str(activation_script), shell)}"
return f"{command} {self._quote(str(activation_script), shell)}"
</code_context>
<issue_to_address>
**issue (bug_risk):** Condition now omits the command prefix for PowerShell/cmd on non-Windows as well.
With this change, any shell mapped to an empty `command` (PowerShell/pwsh or cmd) now omits the prefix on all platforms. On non-Windows systems where `powershell`/`pwsh` is used (PowerShell Core), running a `.ps1` without `pwsh -File`, `.` or `source` will typically fail. To keep the previous cross-platform behavior and match the comment, this condition likely needs to remain `if WINDOWS and not command:` instead of `if not command:`.
</issue_to_address>
### Comment 4
<location> `tests/console/commands/test_update.py:105-114` </location>
<code_context>
assert tester.command.installer._requires_synchronization
+
+
+def test_update_with_invalid_package_name_shows_error(
+ poetry_with_outdated_lockfile: Poetry,
+ command_tester_factory: CommandTesterFactory,
+) -> None:
+ """
+ Providing non-existent package names should raise an error.
+ """
+ tester = command_tester_factory("update", poetry=poetry_with_outdated_lockfile)
+
+ status = tester.execute("nonexistent-package")
+
+ assert status == 1
+ assert (
+ "The following packages are not dependencies of this project: nonexistent-package"
+ in tester.io.fetch_error()
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests for invalid package names that include version specifiers and comparison operators
Since the implementation now strips version/comparison parts from user input (e.g., `foo==1.0`, `foo>=1.0`, `foo<2`, `foo!=1.0`), please add tests that pass invalid package names with such constraints (e.g., `nonexistent==1.0`, `nonexistent>=2`, `nonexistent!=1.0`) and assert they’re correctly rejected. This will lock the tests to the new parsing behavior and guard against regressions.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
55
to
58
| for package in packages: | ||
| # Check if package name contains version constraint | ||
| # (e.g., "package==1.0.0" or "package>=1.0") | ||
| package_name = package.split(">")[0].split("<")[0].split("=")[0].split("!")[0].strip() |
There was a problem hiding this comment.
issue (bug_risk): Manual splitting of version constraints is brittle and will mis-handle more complex specifiers.
This chained split approach will break on common valid forms like multiple constraints (pkg>=1,<2), markers (pkg; python_version<'3.11'), extras (pkg[extra]==1.0), or varied spacing. Prefer using an existing parser such as Poetry’s requirement/specifier utilities or packaging.requirements.Requirement to derive the canonical package name so all valid syntaxes remain supported without custom parsing updates.
Comment on lines
58
to
59
| package_name = package.split(">")[0].split("<")[0].split("=")[0].split("!")[0].strip() | ||
| canonical_name = canonicalize_name(package_name) |
There was a problem hiding this comment.
issue (bug_risk): Extras in package specs will be treated as invalid dependencies.
For inputs like poetry update foo[bar]==1.0, package_name becomes foo[bar]. canonicalize_name will preserve the brackets, but all_dependencies is keyed by dep.name (typically just foo), so dependencies with extras will be incorrectly flagged as invalid. When deriving package_name, strip any extras segment (e.g. split on [ before canonicalization) or use a requirement parser that gives you .name separately from extras.
Comment on lines
105
to
114
| def test_update_with_invalid_package_name_shows_error( | ||
| poetry_with_outdated_lockfile: Poetry, | ||
| command_tester_factory: CommandTesterFactory, | ||
| ) -> None: | ||
| """ | ||
| Providing non-existent package names should raise an error. | ||
| """ | ||
| tester = command_tester_factory("update", poetry=poetry_with_outdated_lockfile) | ||
|
|
||
| status = tester.execute("nonexistent-package") |
There was a problem hiding this comment.
suggestion (testing): Add tests for invalid package names that include version specifiers and comparison operators
Since the implementation now strips version/comparison parts from user input (e.g., foo==1.0, foo>=1.0, foo<2, foo!=1.0), please add tests that pass invalid package names with such constraints (e.g., nonexistent==1.0, nonexistent>=2, nonexistent!=1.0) and assert they’re correctly rejected. This will lock the tests to the new parsing behavior and guard against regressions.
Contributor
|
you have made a mess of your branches, this pull request is mixed up with changes from another |
Author
|
I’ll correct it
…________________________________
From: David Hotham ***@***.***>
Sent: Sunday, February 8, 2026 3:16:02 AM
To: python-poetry/poetry ***@***.***>
Cc: Varun Chawla ***@***.***>; Author ***@***.***>
Subject: Re: [python-poetry/poetry] Fix poetry env activate for Bash on Windows (PR #10722)
[https://avatars.githubusercontent.com/u/875184?s=20&v=4]dimbleby left a comment (python-poetry/poetry#10722)<#10722 (comment)>
you have made a mess of your branches, this pull request is mixed up with changes from another
—
Reply to this email directly, view it on GitHub<#10722 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AIE72BCBPLUFJXQWLQETWIL4K4LHFAVCNFSM6AAAAACULZ5OHSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTQNRXGAYTQNJQGU>.
You are receiving this because you authored the thread.Message ID: ***@***.***>
|
The `poetry env activate` command now works correctly with Bash (and similar shells) on Windows by including the source command prefix. Previously, the code checked if the OS was Windows and skipped the command prefix for all shells, but this was incorrect for Bash/Zsh/etc. running on Windows (e.g., Git Bash, WSL). This fix changes the logic to only skip the command prefix for PowerShell and cmd shells, which are the only Windows-specific shells that don't need it. All other shells (bash, zsh, etc.) now correctly get the "source" prefix regardless of the OS. Fixes python-poetry#10395
veeceey
force-pushed
the
fix/issue-10395-env-activate-bash-windows
branch
from
cd6b269 to
d3540e2
Compare
Author
|
Fixed - rebased the branch so it only contains the changes for this PR. Sorry about the branch mix-up. |
Restrict the no-prefix condition to PowerShell/cmd on Windows only,
rather than using an empty command string check. This ensures
PowerShell Core on non-Windows platforms still receives the '.'
prefix needed to source activation scripts correctly.
Changes:
- Restore '.' as the command for PowerShell/pwsh and cmd (matching
upstream) instead of empty string
- Replace 'if not command' with explicit
'if WINDOWS and shell in ("powershell", "pwsh", "cmd")' check
- Add clarifying comment explaining why the prefix is omitted only
for these specific shells on Windows
Author
|
Verified the condition now correctly limits the prefix omission to Windows PowerShell/cmd/pwsh shells only. Bash/zsh on Windows retain their 'source' prefix as intended. All 11 activate tests pass. |
Contributor
|
cf #10716 |
Author
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #10395
This PR fixes the
poetry env activatecommand to work correctly with Bash (and similar shells) on Windows.Problem
The command
eval $(poetry env activate)didn't work in Bash running on Windows (e.g., Git Bash, WSL) because the output was missing thesourcecommand prefix. The code incorrectly assumed that all shells on Windows should skip the command prefix, but this is only true for PowerShell and cmd.Changes
"") only for PowerShell and cmd shellsif not command:instead ofif WINDOWS:Testing
source /path/to/activate/path/to/activate(or with appropriate quoting)Summary by Sourcery
Validate package names for
poetry update, ensure deterministic dependency constraint ordering in the lockfile, and fix environment activation output for POSIX shells on Windows.New Features:
poetry updateinvocations that specify packages not declared as project dependencies.Bug Fixes:
poetry env activateprints asourceprefix for POSIX-style shells on Windows while keeping bare script paths for PowerShell and cmd.