executor: propagate original error even if pkg.to_dependency() fails#10340
executor: propagate original error even if pkg.to_dependency() fails#10340lotheac wants to merge 1 commit intopython-poetry:mainfrom
Conversation
Reviewer's Guide by SourceryThis pull request fixes a regression where the underlying build error of a git dependency was not outputted when the wheel build failed. The issue was caused by exception handling in Sequence diagram for error handling in _execute_operationsequenceDiagram
participant Executor
participant Package
participant Pip
Executor->>Package: pkg.to_dependency()
alt ValueError
Package-->>Executor: Raises ValueError
Executor->>Executor: Catch ValueError
Executor-->>Executor: Propagate original error
else No ValueError
Package-->>Executor: Returns Dependency
Executor->>Pip: pip wheel --no-cache-dir --use-pep517 <requirement>
alt Pip wheel fails
Pip-->>Executor: Raises BuildError
Executor-->>Executor: Propagate BuildError
else Pip wheel succeeds
Pip-->>Executor: Returns Wheel
end
end
File-Level Changes
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 @lotheac - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding a comment explaining why the
ValueErrormight occur and why it's safe to ignore in this context.
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.
lotheac
force-pushed
the
push-mmvstsnzlvmq
branch
2 times, most recently
from
7c9271e to
5a8d695
Compare
this fixes the following regression, introduced somewhere between 1.8.4 and 2.0.1: a git dependency which fails wheel build does not output the underlying error that caused the wheel build failure. eg. podman run -ti -w /tmp --entrypoint /bin/sh --rm python:3.13.3-alpine3.21 -c "pip install poetry && poetry new . && poetry add 'python-kadmin @ git+https://github.com/authentik-community/[email protected]'" the root cause is that the exception handling in _execute_operation() calls pkg.to_dependency(), which fails like: ValueError: Invalid git url "/root/.cache/pypoetry/virtualenvs/tmp-6WcazSRI-py3.13/src/python-kadmin" and the underlying build error is therefore never output.
lotheac
force-pushed
the
push-mmvstsnzlvmq
branch
from
5a8d695 to
6ec31e2
Compare
radoering
left a comment
radoering
left a comment
There was a problem hiding this comment.
At first, thanks for providing an exact repro of the issue. 👍
Although the change clearly improves the behavior, I think it is more a workaround than a valid fix. The suppressed exception is the real bug. When testing the repro with the change, you can see the following line at the end of the output:
You can verify this by running pip wheel --no-cache-dir --use-pep517 "/root/.cache/pypoetry/virtualenvs/tmp-6WcazSRI-py3.13/src/python-kadmin".
which is wrong because it references the cache directory instead of the git URL.
The reason is probably the following hack:
poetry/src/poetry/installation/executor.py
Line 674 in 297ae7d
We overwrite the source URL and reset it later:
poetry/src/poetry/installation/executor.py
Lines 685 to 686 in 297ae7d
Maybe, it is enough if we put some lines in a try-except and always reset the source URL in case of an exception.
Further, we should probably add a test. Maybe, the following test can be extended (or at least used as a starting point):
poetry/tests/installation/test_executor.py
Line 1370 in 297ae7d
The test will probably be more complicated than the fix. I may take a look for myself later.
|
@radoering thank you for your review. I'm not super familiar with the poetry code, so I would appreciate it if you could take a look at a better fix. If that's ok with you, I think we can close this PR. |
|
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
2 participants
this fixes the following regression, introduced somewhere between 1.8.4 and 2.0.1:
a git dependency which fails wheel build does not output the underlying error that caused the wheel build failure. eg.
the root cause is that the exception handling in _execute_operation() calls pkg.to_dependency(), which fails like:
and the underlying build error is therefore never output.
Pull Request Check List
Summary by Sourcery
Bug Fixes: