Skip to content
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

utils: Add a little CommandRunExt helper trait #733

Merged
merged 1 commit into from
Jul 26, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions lib/src/install.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ use crate::mount::Filesystem;
use crate::spec::ImageReference;
use crate::store::Storage;
use crate::task::Task;
use crate::utils::sigpolicy_from_opts;
use crate::utils::{sigpolicy_from_opts, CommandRunExt};

/// The default "stateroot" or "osname"; see https://github.com/ostreedev/ostree/issues/2794
const STATEROOT_DEFAULT: &str = "default";
Expand Down Expand Up @@ -584,10 +584,9 @@ async fn initialize_ostree_root(state: &State, root_setup: &RootSetup) -> Result
("sysroot.bootprefix", "true"),
("sysroot.readonly", "true"),
] {
Task::new("Configuring ostree repo", "ostree")
Command::new("ostree")
.args(["config", "--repo", "ostree/repo", "set", k, v])
.cwd(rootfs_dir)?
.quiet()
.cwd_dir(rootfs_dir.try_clone()?)
.run()?;
}
Task::new("Initializing sysroot", "ostree")
Expand Down
25 changes: 25 additions & 0 deletions lib/src/utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,25 @@ use ostree::glib;
use ostree_ext::container::SignatureSource;
use ostree_ext::ostree;

/// Helpers intended for [`std::process::Command`].
pub(crate) trait CommandRunExt {
fn run(&mut self) -> Result<()>;
}

impl CommandRunExt for Command {
Copy link
Member

Choose a reason for hiding this comment

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

I see you are using Command::new and not CommandRunExt above. I am not sure I follow how this gets wired. I imagine that now everytime someone uses Command::new it will use this instead, but have no idea how that is happening.

Copy link
Member

Choose a reason for hiding this comment

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

mmm I guess it's using trait and then the impl that wires this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right, the trait adds a new method.

/// Synchronously execute the child, and return an error if the child exited unsuccessfully.
fn run(&mut self) -> Result<()> {
let st = self.status()?;
if !st.success() {
// Note that we intentionally *don't* include the command string
// in the output; we leave it to the caller to add that if they want,
// as it may be verbose.
anyhow::bail!(format!("Subprocess failed: {st:?}"))
}
Ok(())
}
}

/// Try to look for keys injected by e.g. rpm-ostree requesting machine-local
/// changes; if any are present, return `true`.
pub(crate) fn origin_has_rpmostree_stuff(kf: &glib::KeyFile) -> bool {
Expand Down Expand Up @@ -190,3 +209,9 @@ fn test_sigpolicy_from_opts() {
SignatureSource::ContainerPolicyAllowInsecure
);
}

#[test]
fn command_run_ext() {
Command::new("true").run().unwrap();
assert!(Command::new("false").run().is_err());
}