console-rs / indicatif

A command line progress reporting library for Rust
MIT License
4.23k stars 240 forks source link

indicatif::style::ProgressTracker is not `Sync` #467

Closed rctlmk closed 1 year ago

rctlmk commented 1 year ago

Is Sync bound missing by mistake? After updating my project's dependency from "0.17.0-rc.8" to "0.17.0" it doesn't compile anymore because of this:

lazy_static! {
    static ref IDLE_STYLE: ProgressStyle = ProgressStyle::with_template("{prefix:.dim} {msg:.cyan.dim}").unwrap();
}
486 | / lazy_static! {
487 | |     static ref IDLE_STYLE: ProgressStyle = ProgressStyle::with_template("{prefix:.dim} {msg:.cyan.dim}").unwrap();
488 | | }
    | |_^ `(dyn ProgressTracker + 'static)` cannot be shared between threads safely

Considering this struct from test_stateful_tracker():

#[derive(Debug, Clone)]
struct TestTracker(Arc<Mutex<String>>);

doesn't this mean it needs to be Sync anyway? I'm new at this Rust thread safety thing, I'm sorry if I'm missing something obvious.

djc commented 1 year ago

Yeah, we should probably fix that... It's technically a semver-incompatible change though. @chris-laplante what do you think, should we try to sneak this into 0.17.1 and hope no one has written a !Send ProgressTracker or bump to 0.18 instead?

chris-laplante commented 1 year ago

Yeah, we should probably fix that... It's technically a semver-incompatible change though. @chris-laplante what do you think, should we try to sneak this into 0.17.1 and hope no one has written a !Send ProgressTracker or bump to 0.18 instead?

I really doubt anyone has written a !Send let alone found out about and used ProgressTracker yet. It's probably fine to sneak it into 0.17.1.

rctlmk commented 1 year ago

hope no one has written !Send ProgressTracker

@djc, I hope you mean !Sync, because pub trait ProgressTracker: Send. If not, then it is really confusing. Also, I made a little fix (potentially without breaking anything else), should I make a PR?

djc commented 1 year ago

PR would be great!