fix: Add package prefix to uv output #843
Conversation
iangelak
force-pushed
the
753-uv-prefix-debug-output
branch
from
7e826d6 to
f791867
Compare
|
When I run an e2e test with this change, I do see the prefix as expected: What can we do to make it more obvious that those lines are the output of the uv command, and not fromager itself? |
|
I'm worried that this change is going to make the logs even harder to read, especially when we build packages in parallel. Currently the entire output of an external command is logged as one multi-line string. With your change, the output is split into individual lines and every line is logged on its own. If Fromager is building 10 packages in parallel, the output of all 10 builds are multiplexed. It makes the output very hard to read. |
|
@dhellmann I' ll try to add UV in the prefix to distinguish its printouts from Fromager's. |
|
You could add a lock, but then you'd have to figure out how to do proper locking without blocking other threads. There is a much, much simpler way to solve the problem: rewrite the |
Yeah, I like this approach. |
|
If I understood correctly, @tiran you are suggesting to first split the output to lines, then add the prefix to each line and then concatenate all lines back to one string and print that right? |
|
Yes, something like that. But instead of splitting the string, you can archive the goal with just str method call. |
iangelak
force-pushed
the
753-uv-prefix-debug-output
branch
from
f791867 to
c7ddc85
Compare
iangelak
force-pushed
the
753-uv-prefix-debug-output
branch
from
c7ddc85 to
70ba61e
Compare
|
Let me know if these changes are good. |
iangelak
force-pushed
the
753-uv-prefix-debug-output
branch
3 times, most recently
from
55c47b0 to
d0dd682
Compare
rd4398
left a comment
rd4398
left a comment
There was a problem hiding this comment.
This looks okay to me. I see Doug and Christian had comments on previous versions of this MR. I will leave it to them for final approval
|
@iangelak The commit message is too long, also there are more comments in the code than required. We do not need comments unless it is a must have. Also lets reduce the commit message to 2 to 3 lines which is more human readable than long texts. |
|
@iangelak Do you have logs to compare before and after (this patch)? If yes, please put it in the PR so that it is easier for the reviewers to see the difference. |
iangelak
force-pushed
the
753-uv-prefix-debug-output
branch
2 times, most recently
from
78a4950 to
b792bd6
Compare
Yes of course here is an example before and after the patch: and |
iangelak
force-pushed
the
753-uv-prefix-debug-output
branch
from
b792bd6 to
ec8aada
Compare
|
This implementation looks good to me. I'll leave the PR open because @LalatenduMohanty and @tiran both had comments as well, but let's try to get this merged by early next week so we can cut a release. |
|
@iangelak Thanks for fixing the commits. Appreciate it. |
External command output lacked package prefixes on continuation lines, making logs hard to grep in parallel builds. Solution: Added get_log_prefix() helper to extract package context. FromagerLogRecord prefixes the first line, str.replace() handles continuation lines. Fixes: python-wheel-build#753 Co-authored-by: Claude <[email protected]> Signed-off-by: Ioannis Angelakopoulos <[email protected]>
iangelak
force-pushed
the
753-uv-prefix-debug-output
branch
from
ec8aada to
792432c
Compare
dhellmann
dismissed
tiran’s stale review
I think all of the review comments from @tiran are addressed so I am clearing the change request so we can merge this and cut a release.
|
@Mergifyio refresh |
✅ Pull request refreshed |
5 participants
External command output lacked package prefixes on continuation lines, making logs hard to grep in parallel builds.
Solution: Added get_log_prefix() helper to extract package context. FromagerLogRecord prefixes the first line, str.replace() handles continuation lines.
Fixes: #753