Skip to content

Conversation

@cgwalters
Copy link
Collaborator

It wasn't a useful abstraction in the end for most cases.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request successfully removes the custom Task abstraction, replacing it with direct usage of std::process::Command and helper extension traits from bootc_utils::CommandRunExt. This is a positive change that simplifies the codebase. The new implementation generally improves error reporting by capturing stderr on command failures. I've identified one minor regression where an error context attribute was removed, which I've suggested restoring to maintain detailed error diagnostics. Otherwise, the changes are excellent.

}

#[context("Failed to wipe {dev}")]
pub(crate) fn wipefs(dev: &Utf8Path) -> Result<()> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The #[context("Failed to wipe {dev}")] attribute was removed. This attribute provides valuable context to error messages when the wipefs command fails. Without it, the error message will be less specific, making debugging harder. It would be beneficial to restore this attribute to improve error diagnostics.

Suggested change
pub(crate) fn wipefs(dev: &Utf8Path) -> Result<()> {
#[context("Failed to wipe {dev}")]
pub(crate) fn wipefs(dev: &Utf8Path) -> Result<()> {

It wasn't a useful abstraction in the end for most cases.

Signed-off-by: Colin Walters <[email protected]>
@ckyrouac ckyrouac merged commit 825f7ac into bootc-dev:main Aug 1, 2025
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants