Conversation
This reverts commit f4aecb9. It causes ill-defined behavior with older glib, resulting in file descriptor mis-assignments.
There was a problem hiding this comment.
Code Review
This pull request reverts a problematic change and adds thorough testing to prevent future regressions. The new tests for bwrap's stdout/stderr handling are a great addition. I have a couple of minor suggestions for the new test scripts to improve consistency and conciseness.
This was a regression from 057ccd9 Signed-off-by: Colin Walters <walters@verbum.org>
This backs out prior changes to use `.run()` in most cases where we're invoking nontrivial commands. Yeah, the `run` API here is probably too much of a trap and we should go back to not swallowing stderr. Or at least have very explicit APIs for the two cases. Signed-off-by: Colin Walters <walters@verbum.org>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request reverts a change that merged STDERR and STDOUT for command execution in bwrap, which caused issues with older glib versions. The revert is accompanied by updates to error handling to accommodate separate output streams and new tests to verify that both STDOUT and STDERR are correctly captured. The changes look good overall. I've added a few minor suggestions to improve the consistency of error messages across the modified files.
There really isn't any kind of single default way to run a subprocess, that's why it's tricky. Sometimes one wants to have them be async, sometimes synchronous. Sometimes one wants to capture stdout, other times not etc. The `run()` name implies it's a default but it can't really be because some use cases we really do want to directly copy stderr instead of capturing it. It happens that *most* cases here inside bootc we're fine to only show stderr on error I think; I only changed the editor case to use the new `run_inherited()`. But in contrast many use cases in e.g. coreos/rpm-ostree#5439 wanted `run_inherited()`. Unit tests: Assisted-by: Claude Code Signed-off-by: Colin Walters <walters@verbum.org>
There really isn't any kind of single default way to run a subprocess, that's why it's tricky. Sometimes one wants to have them be async, sometimes synchronous. Sometimes one wants to capture stdout, other times not etc. The `run()` name implies it's a default but it can't really be because some use cases we really do want to directly copy stderr instead of capturing it. It happens that *most* cases here inside bootc we're fine to only show stderr on error I think; I only changed the editor case to use the new `run_inherited()`. But in contrast many use cases in e.g. coreos/rpm-ostree#5439 wanted `run_inherited()`. Unit tests: Assisted-by: Claude Code Signed-off-by: Colin Walters <walters@verbum.org>
|
/override ci/prow/fcos-e2e |
|
@cgwalters: Overrode contexts on behalf of cgwalters: ci/prow/fcos-e2e DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
There really isn't any kind of single default way to run a subprocess, that's why it's tricky. Sometimes one wants to have them be async, sometimes synchronous. Sometimes one wants to capture stdout, other times not etc. The `run()` name implies it's a default but it can't really be because some use cases we really do want to directly copy stderr instead of capturing it. It happens that *most* cases here inside bootc we're fine to only show stderr on error I think; I only changed the editor case to use the new `run_inherited()`. But in contrast many use cases in e.g. coreos/rpm-ostree#5439 wanted `run_inherited()`. Unit tests: Assisted-by: Claude Code Signed-off-by: Colin Walters <walters@verbum.org>
This reverts commit f4aecb9.
It causes ill-defined behavior with older glib, resulting in
file descriptor mis-assignments.
Also add tests that verify that we do get both stdout and stderr from both postprocess scripts and package scripts.
Closes: #5436