ostree: Use indicatif + async for lock wait #1536
Conversation
cgwalters
commented
Aug 20, 2025
cgwalters
commented
- Indicatif instead of eprintln! ensures we handle ttys vs not always; on a non-tty we're silent
- Use a proper async task instead of a try+sleep loop
- But do try once outside of the await to make the happy path avoid emitting a wait message
- Indicatif instead of eprintln! ensures we handle ttys vs not always; on a non-tty we're silent - Use a proper async task instead of a try+sleep loop - But do try once outside of the await to make the happy path avoid emitting a wait message Signed-off-by: Colin Walters <walters@verbum.org>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a nice improvement by using indicatif for a better user experience when waiting for a sysroot lock, replacing a manual print and sleep loop. The new async_task_with_spinner utility is a good abstraction. My review includes a suggestion to refactor the lock acquisition logic to reduce code duplication and another to improve the panic safety of the new spinner utility.
| 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, | ||
| }) |
There was a problem hiding this comment.
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.
| 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, | |
| }) |
There was a problem hiding this comment.
Sure, but not worth blocking over
| 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 |
There was a problem hiding this comment.
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.
| 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 |
There was a problem hiding this comment.
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 😆
|
Fallout in #1547 |
