-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
feat(env): prefer embedded pip for poetry use #10091
Conversation
Presently, when Poetry dispatches pip commands for uninstallation, this uses the pip version installed in the active virtual environment. This can potentially cause issues like python-poetry#10089. This change, switches to preferring the embedded pip wheel and makes preferring the in-env version the exception, while preferring it when a user uses `poetry run pip`.
Reviewer's Guide by SourceryThis pull request modifies how Poetry invokes pip commands, preferring the embedded pip wheel over the in-environment version, except when the user explicitly uses Sequence diagram for Poetry's pip command execution flowsequenceDiagram
participant Poetry
participant BaseEnv
participant PipCommand
alt prefer_in_env=True and pip exists in environment
Poetry->>BaseEnv: get_pip_command(prefer_in_env=true)
BaseEnv->>PipCommand: Execute [python, -m, pip]
else Default case or pip not in environment
Poetry->>BaseEnv: get_pip_command(prefer_in_env=false)
BaseEnv->>PipCommand: Execute [python, embedded_pip]
end
Flow diagram for pip command selection logicflowchart TD
A[Start] --> B{prefer_in_env?}
B -->|Yes| C{Pip exists in env?}
C -->|Yes| D[Use in-env pip]
C -->|No| E[Use embedded pip]
B -->|No| E
D --> F[Execute pip command]
E --> F
F --> G[End]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @abn - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah #6060 and #6062 were what I was missing. I agree that we should really just drop the uninstall case. The build step is already using an isolated builder now in I'll see if I can implement some uninstaller based on pip's logic as a starter. If it's easy enough we can avoid reverting part of #9392. |
Presently, when Poetry dispatches pip commands for uninstallation, this uses the pip version installed in the active virtual environment. This can potentially cause issues like #10089.
This change, switches to preferring the embedded pip wheel and makes preferring the in-env version the exception, while preferring it when a user uses
poetry run pip
.See-also: #4011
Summary by Sourcery
Bug Fixes: