-
Notifications
You must be signed in to change notification settings - Fork 42
fix: include package name in exception messages #846
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: include package name in exception messages #846
Conversation
87b96b3 to
dd1a51d
Compare
LalatenduMohanty
left a comment
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.
@andre-motta src/fromager/commands/download_sequence.py do not use RuntimeError() wrapper. Lets fix that as well
|
@andre-motta Also 3 e2e tests are failing. PTAL |
|
Here is the fix to the e2e failures |
dd1a51d to
06414b5
Compare
|
Addressed all feedback |
06414b5 to
d976e00
Compare
dhellmann
left a comment
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.
It looks like the test job failures are due to the changes in the message format. That should be easy enough to fix. The change itself looks good.
d976e00 to
3e59569
Compare
Fixed the format, I also made some other changes as the fix I did to the raises actually mean I don't need the suggested changes in Heres an example of the duplication: |
3e59569 to
36d97da
Compare
|
@andre-motta I am wondering if the current proposed fix is relatively fragile. Also I am also re-thinking about use of |
|
A while ago I had the idea to wrap our hooks to have more detailed exceptions as well as separate exception classes for resolver, sdist, wheel build, and so on. Please take a look at the rough prototype https://github.com/tiran/fromager/pull/new/hook-exceptiongroup . |
rd4398
left a comment
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.
The changes look okay but looks like a rebase is needed.
|
I think we could land this if it was rebased. Some of the other suggested improvements could come later. |
|
Rebased on main. |
b273355 to
f0a5467
Compare
|
@andre-motta There are two commits now, do we need two commits? |
|
Fixing it now @LalatenduMohanty |
Add package name context to exception formatting to identify which package failed during build errors. The package name is added by FromagerLogRecord when messages are logged, and _format_exception handles formatting chained exceptions with 'because' syntax. Improve parallel build error handling to preserve package context across thread boundaries using RuntimeError wrapper. Tests added to test_external_commands.py verify error logging and exception formatting include package names and handle chained exceptions. Update e2e test pattern to match new log format that includes package names. Closes: python-wheel-build#845 Signed-off-by: Andre Lustosa <[email protected]>
f0a5467 to
2a5acda
Compare
Add package name context to exception formatting to identify which
package failed during build errors. The package name is added by
FromagerLogRecord when messages are logged, and _format_exception
handles formatting chained exceptions with 'because' syntax.
Improve parallel build error handling to preserve package context
across thread boundaries using RuntimeError wrapper.
Tests added to test_external_commands.py verify error logging and
exception formatting include package names and handle chained exceptions.
Update e2e test pattern to match new log format that includes package names.
Closes: #845