Improve CLI robustness, safety, and readability #14333
Conversation
- Ensure granular mutex locking in stdout/stderr threads to prevent deadlocks and panics. - Handle poisoned locks gracefully to improve CLI resilience. - Replace inefficient fold-based command argument formatting with join for better performance. - Simplify file extension handling using and_then(|ext| ext.to_str()) for clarity and cross-platform safety. - Introduce get_verbosity helper for consistent and maintainable verbosity handling. - Add Default derives to configuration structs for safer deserialization. - Add minimal CLI smoke test to catch regressions in subcommand help messages. - Standardize and clean up logging and error handling for better maintainability.
Package Changes Through dfcefc6No changes. Add a change file through the GitHub UI by following this link. Read about change files or the docs at github.com/jbolda/covector |
crates/tauri-cli/src/lib.rs
Outdated
| } | ||
|
|
||
| #[derive(Deserialize)] | ||
| #[derive(Deserialize, Default)] |
There was a problem hiding this comment.
Is that used anywhere? Same for PackageJson
| err.format(&mut app) | ||
| } | ||
|
|
||
| fn get_verbosity(cli_verbose: u8) -> u8 { |
There was a problem hiding this comment.
Verbosity consistency: Introduced a helper function to centralize verbosity parsing from environment variables and CLI flags, ensuring predictable logging levels across all subcommands.
This function is only used once, where the code was inlined before. I don't mind having this in a separate function but wanted to mention this.
There was a problem hiding this comment.
I made this function because the logic may be reused (e.g. if we later need verbosity in other commands),
but I’m fine inlining it if preferred.
| .get_args() | ||
| .map(|a| a.to_string_lossy()) | ||
| .collect::<Vec<_>>() | ||
| .join(" "); |
There was a problem hiding this comment.
i wonder if that's slower than fold but i agree that it's easier on the eyes.
| /// | ||
| /// The passed `bin_name` parameter should be how you want the help messages to display the command. | ||
| /// This defaults to `cargo-tauri`, but should be set to how the program was called, such as | ||
| /// `cargo tauri`. |
There was a problem hiding this comment.
Were these comments incorrect?
crates/tauri-cli/src/lib.rs
Outdated
| if let Ok(mut lines) = stdout_lines_.lock() { | ||
| lines.extend(line.as_bytes()); | ||
| } |
There was a problem hiding this comment.
i think we should keep the lock() where it was for performance reasons. the next time we will need a lock is after these threads are stopped.
crates/tauri-cli/src/lib.rs
Outdated
| } | ||
| } | ||
| Err(_) => (), | ||
| Err(_) => break, |
There was a problem hiding this comment.
probably not a good idea, just because one line may error out doesn't mean we should stop reading completely. Except of course if we've had issues with the old behavior which i'm not aware of.
crates/tauri-cli/src/lib.rs
Outdated
| if let Ok(mut lines) = stderr_lines_.lock() { | ||
| lines.extend(line.as_bytes()); | ||
| } | ||
| } | ||
| Err(_) => (), | ||
| Err(_) => break, |
| #[test] | ||
| fn help_output_includes_build() { | ||
| let help = Cli::command().render_help().to_string(); | ||
| assert!(help.contains("Build")); | ||
| } |
There was a problem hiding this comment.
i'm kinda failing to see the value of this test but it's not technically wrong so fair enough i guess
| // treat all other extensions as json | ||
| _ => Ok(Self( | ||
| // from tauri-utils/src/config/parse.rs: | ||
| // we also want to support **valid** json5 in the .json extension | ||
| // if the json5 is not valid the serde_json error for regular json will be returned. |
There was a problem hiding this comment.
since the logic is still the same we should probably keep the comments as well
There was a problem hiding this comment.
@KushalMeghani1644 can you copy these comments over as well?
There was a problem hiding this comment.
Oh yes! I missed these! though Now I have added them back now. Sorry for the inconsistency
|
Hey! @FabianLars I have made the changes, you reviewed/requested... I have added back the comments that I had blindly removed... removed the |
2 participants
This PR makes several critical improvements to the Tauri CLI to enhance stability, performance, and maintainability:
Thread safety and resilience: The stdout/stderr reader threads now lock the mutex only per iteration, and poisoned locks are handled gracefully. This prevents deadlocks and panics when running commands concurrently, especially on long-running build processes.
Performance and clarity: Command argument formatting was refactored from fold to join, reducing memory allocations and improving readability.
Cross-platform safety: File extension parsing now correctly handles
OsStrconversions, preventing potential panics or incorrect parsing on non-UTF-8 paths.Verbosity consistency: Introduced a helper function to centralize verbosity parsing from environment variables and CLI flags, ensuring predictable logging levels across all subcommands.
Default values: Added
Defaultderivations to config-related structs to make deserialization more robust and prevent unnecessary unwraps.Maintainability: Standardized error handling and logging formatting for clearer messages and easier future modifications.
Regression safety: Added a minimal CLI help test to catch accidental changes to subcommand names or descriptions.
These changes are necessary to ensure the CLI behaves reliably under all conditions, handles errors gracefully, and remains maintainable as Tauri continues to grow.