Skip to content
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
25 changes: 12 additions & 13 deletions crates/ostree-ext/src/sysroot.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ use std::ops::Deref;

use anyhow::Result;

use crate::utils::async_task_with_spinner;

/// A locked system root.
#[derive(Debug)]
pub struct SysrootLock {
Expand Down Expand Up @@ -35,20 +37,17 @@ impl SysrootLock {
/// immediately, a status message will be printed to standard output.
/// The lock will be unlocked when this object is dropped.
pub async fn new_from_sysroot(sysroot: &ostree::Sysroot) -> Result<Self> {
let mut printed = false;
loop {
if sysroot.try_lock()? {
return Ok(Self {
sysroot: sysroot.clone(),
unowned: false,
});
}
if !printed {
eprintln!("Waiting for sysroot lock...");
printed = true;
}
tokio::time::sleep(std::time::Duration::from_secs(3)).await;
if sysroot.try_lock()? {
return Ok(Self {
sysroot: sysroot.clone(),
unowned: false,
});
}
async_task_with_spinner("Waiting for sysroot lock...", sysroot.lock_future()).await?;
Ok(Self {
sysroot: sysroot.clone(),
unowned: false,
})
Comment on lines +40 to +50
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 logic to construct the SysrootLock is duplicated. You can refactor this to avoid repetition by handling the case where the lock isn't immediately available, and then having a single point of SysrootLock creation. This will make the code more maintainable.

Suggested change
if sysroot.try_lock()? {
return Ok(Self {
sysroot: sysroot.clone(),
unowned: false,
});
}
async_task_with_spinner("Waiting for sysroot lock...", sysroot.lock_future()).await?;
Ok(Self {
sysroot: sysroot.clone(),
unowned: false,
})
if !sysroot.try_lock()? {
async_task_with_spinner("Waiting for sysroot lock...", sysroot.lock_future()).await?;
}
Ok(Self {
sysroot: sysroot.clone(),
unowned: false,
})

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sure, but not worth blocking over

}

/// This function should only be used when you have locked the sysroot
Expand Down
28 changes: 28 additions & 0 deletions crates/ostree-ext/src/utils.rs
Original file line number Diff line number Diff line change
@@ -1 +1,29 @@
use std::{future::Future, time::Duration};

/// Call an async task function, and write a message to stdout
/// with an automatic spinner to show that we're not blocked.
/// Note that generally the called function should not output
/// anything to stdout as this will interfere with the spinner.
pub(crate) async fn async_task_with_spinner<F, T>(msg: &str, f: F) -> T
where
F: Future<Output = T>,
{
let pb = indicatif::ProgressBar::new_spinner();
let style = indicatif::ProgressStyle::default_bar();
pb.set_style(style.template("{spinner} {msg}").unwrap());
pb.set_message(msg.to_string());
pb.enable_steady_tick(Duration::from_millis(150));
let r = f.await;
pb.finish_and_clear();
r
Comment on lines +11 to +18
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

If the future f panics, the await will unwind the stack and pb.finish_and_clear() will not be called, leaving the spinner on the screen. Using a RAII guard to manage the ProgressBar's lifetime ensures that it's cleaned up correctly even in the case of a panic.

Suggested change
let pb = indicatif::ProgressBar::new_spinner();
let style = indicatif::ProgressStyle::default_bar();
pb.set_style(style.template("{spinner} {msg}").unwrap());
pb.set_message(msg.to_string());
pb.enable_steady_tick(Duration::from_millis(150));
let r = f.await;
pb.finish_and_clear();
r
let pb = indicatif::ProgressBar::new_spinner();
let style = indicatif::ProgressStyle::default_bar();
pb.set_style(style.template("{spinner} {msg}").unwrap());
pb.set_message(msg.to_string());
pb.enable_steady_tick(Duration::from_millis(150));
struct Guard(indicatif::ProgressBar);
impl Drop for Guard {
fn drop(&mut self) {
self.0.finish_and_clear();
}
}
let _guard = Guard(pb);
f.await

Copy link
Collaborator

Choose a reason for hiding this comment

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

Wow that's kinda wild that it noticed that. While I guess it's true... if we panic it's not like the fact that the spinner isn't cleared is the most jarring UX from the whole thing 😆

}

#[cfg(test)]
mod tests {
use super::*;

#[tokio::test]
async fn test_spinner() {
async_task_with_spinner("Testing...", tokio::time::sleep(Duration::from_secs(5))).await
}
}