linebender / vello

A GPU compute-centric 2D renderer.
http://linebender.org/vello/
Apache License 2.0
2.41k stars 140 forks source link

Switch from `dialoguer` to `inquire` #511

Closed simbleau closed 8 months ago

simbleau commented 8 months ago

I can't compile vello anymore on 1.76 stable for very strange reasons related to lazy_static, see https://github.com/console-rs/dialoguer/issues/306

dialoguer is less maintained, less stars, etc. than inquire, and dialoguer never made the change to once_cell and I think in recent versions of rust (1.75+?) a dep with lifetimes in lazy_static are no longer producing macro-expansion that is valid.

The issue is simple... replace dialoguer with inquire. Should take 5 minutes, but I wanted to explain the purpose here.

We only use dialoguer on native to assist with downloads, which we could easily replace with the "Confirm" equivalent on inquire: https://github.com/mikaelmello/inquire?tab=readme-ov-file#Confirm

xStrom commented 8 months ago

What cargo command do you use that fails? I tried both cargo build -p vello and cargo build -p scenes on my macOS 10.15 with Rust 1.76 and it succeeded. The build also succeeds in the GitHub CI with both macOS 12 and macOS 14 on M1.

Have you tried cargo clean ?

simbleau commented 8 months ago

Same specs, M1, 1.76 stable. I did a cargo clean. It's hard for me to figure out.

Vello runs, it's just the with_winit example that didn't. Not sure if that's CI tested. It's also new behavior... not sure when exactly or what caused it.

Either way, I don't think there's a good reason not to switch. :)

xStrom commented 8 months ago

Vello runs, it's just the with_winit example that didn't.

Does with_winit still not run? What is the cargo command that fails?

simbleau commented 8 months ago

It happens when compiling console, which is a dependant of dialoguer. The error seems unhelpful...

16:27 simbleau on main ~ cargo run -p with_winit
    Updating crates.io index
  Downloaded skrifa v0.16.0
  Downloaded read-fonts v0.16.0
  Downloaded 2 crates (385.7 KB) in 1.01s
    Blocking waiting for file lock on package cache
   Compiling bitflags v2.4.2
   Compiling libc v0.2.153
   Compiling font-types v0.4.3
   Compiling core-graphics-types v0.1.3

   ... lots of crates

   Compiling rand_chacha v0.3.1
   Compiling fsevent-sys v4.1.0
   Compiling console v0.15.8            <--- This right here officer
   Compiling filetime v0.2.23
error[E0195]: lifetime parameters or bounds on type `Target` do not match the trait declaration
  --> /Users/simbleau/.cargo/registry/src/index.crates.io-6f17d22bba15001f/console-0.15.8/src/utils.rs:25:1
   |
25 | / lazy_static! {
26 | |     static ref STDOUT_COLORS: AtomicBool = AtomicBool::new(default_colors_enabled(&Term::stdout()));
27 | |     static ref STDERR_COLORS: AtomicBool = AtomicBool::new(default_colors_enabled(&Term::stderr()));
28 | | }
   | |_^ lifetimes do not match type in trait
   |
   = note: this error originates in the macro `__lazy_static_internal` which comes from the expansion of the macro `lazy_static` (in Nightly builds, run with -Z macro-backtrace for more info)

   Compiling rand v0.8.5
error[E0599]: no method named `load` found for struct `STDOUT_COLORS` in the current scope
  --> /Users/simbleau/.cargo/registry/src/index.crates.io-6f17d22bba15001f/console-0.15.8/src/utils.rs:39:19
   |
25 | / lazy_static! {
26 | |     static ref STDOUT_COLORS: AtomicBool = AtomicBool::new(default_colors_enabled(&Term::stdout()));
27 | |     static ref STDERR_COLORS: AtomicBool = AtomicBool::new(default_colors_enabled(&Term::stderr()));
28 | | }
   | |_- method `load` not found for this struct
...
39 |       STDOUT_COLORS.load(Ordering::Relaxed)
   |                     ^^^^ method not found in `STDOUT_COLORS`

error[E0599]: no method named `store` found for struct `STDOUT_COLORS` in the current scope
  --> /Users/simbleau/.cargo/registry/src/index.crates.io-6f17d22bba15001f/console-0.15.8/src/utils.rs:48:19
   |
25 | / lazy_static! {
26 | |     static ref STDOUT_COLORS: AtomicBool = AtomicBool::new(default_colors_enabled(&Term::stdout()));
27 | |     static ref STDERR_COLORS: AtomicBool = AtomicBool::new(default_colors_enabled(&Term::stderr()));
28 | | }
   | |_- method `store` not found for this struct
...
48 |       STDOUT_COLORS.store(val, Ordering::Relaxed)
   |                     ^^^^^ method not found in `STDOUT_COLORS`

error[E0599]: no method named `load` found for struct `STDERR_COLORS` in the current scope
  --> /Users/simbleau/.cargo/registry/src/index.crates.io-6f17d22bba15001f/console-0.15.8/src/utils.rs:60:19
   |
25 | / lazy_static! {
26 | |     static ref STDOUT_COLORS: AtomicBool = AtomicBool::new(default_colors_enabled(&Term::stdout()));
27 | |     static ref STDERR_COLORS: AtomicBool = AtomicBool::new(default_colors_enabled(&Term::stderr()));
28 | | }
   | |_- method `load` not found for this struct
...
60 |       STDERR_COLORS.load(Ordering::Relaxed)
   |                     ^^^^ method not found in `STDERR_COLORS`

error[E0599]: no method named `store` found for struct `STDERR_COLORS` in the current scope
  --> /Users/simbleau/.cargo/registry/src/index.crates.io-6f17d22bba15001f/console-0.15.8/src/utils.rs:69:19
   |
25 | / lazy_static! {
26 | |     static ref STDOUT_COLORS: AtomicBool = AtomicBool::new(default_colors_enabled(&Term::stdout()));
27 | |     static ref STDERR_COLORS: AtomicBool = AtomicBool::new(default_colors_enabled(&Term::stderr()));
28 | | }
   | |_- method `store` not found for this struct
...
69 |       STDERR_COLORS.store(val, Ordering::Relaxed)
   |                     ^^^^^ method not found in `STDERR_COLORS`

Some errors have detailed explanations: E0195, E0599.
For more information about an error, try `rustc --explain E0195`.
error: could not compile `console` (lib) due to 6 previous errors
warning: build failed, waiting for other jobs to finish...
xStrom commented 8 months ago

Very curious indeed.

I have reasonable confidence that this is not a general problem with the source code, as evidenced by it compiling fine on a bunch of macOS systems.

Smells more like a Rust toolchain bug, maybe somehow related to the new parallelism work, in which case the manifestation may depend on a certain number of threads being used.

Sure we can "fix" it by merging #512 but we can easily end up depending on console again via some dependency in the future. Even worse if this is as more generic toolchain bug that can start causing issues elsewhere. This time it resulted in a compilation error, but other times it might just result in buggy machine instructions that can silently corrupt data at runtime. Not great, but also not easy to track down. Luckily there are a bunch of other people seeing the same issue, so hopefully someone will put in the effort to figure it out.