fix(sozo): fix sozo --version #3305
Conversation
|
ohayo sensei! WalkthroughAdds dynamic version generation for the Sozo CLI using a new utility that queries Scarb; removes the explicit CLI version flag; allows manifest_path to be provided via DOJO_MANIFEST_PATH environment variable; and implements Scarb interop to run Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Clap as Clap/SozoArgs
participant Utils as utils::generate_version
participant Scarb as scarb_interop::Scarb
participant OS as scarb process
User->>Clap: sozo --version
Clap->>Utils: generate_version()
Utils->>Scarb: version()
Scarb->>OS: run "scarb --version"
OS-->>Scarb: stdout or stderr
Scarb-->>Utils: Some(version) / None
Utils-->>Clap: "<dojo_version>\nscarb: <ver|not found>"
Clap-->>User: print version banner
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
crates/sozo/scarb_interop/src/scarb.rs (1)
18-24: Ohayo, sensei — trim stdout and check exit status to avoid extra newline and false positivesRight now we don't verify the process exit status and we propagate the raw stdout (which typically includes a trailing newline). This can lead to funky formatting in
--versionand to returning junk on failure. Suggest checkingstatus.success()and trimming.Apply this diff:
- pub fn version() -> Option<String> { - Command::new("scarb") - .args(["--version"]) - .output() - .ok() - .and_then(|output| String::from_utf8(output.stdout).ok()) - } + pub fn version() -> Option<String> { + let output = Command::new("scarb").args(["--version"]).output().ok()?; + if !output.status.success() { + return None; + } + let stdout = String::from_utf8(output.stdout).ok()?; + Some(stdout.trim().to_owned()) + }bin/sozo/src/utils.rs (1)
212-223: Make version banner stable: remove embedded newline and trim Scarb output
Scarb::version()(and the fallback) can include trailing newlines, yielding awkward--versionoutput like extra blank lines. Trim the Scarb output and drop the newline from the fallback.Apply this diff:
pub fn generate_version() -> String { const DOJO_VERSION: &str = env!("CARGO_PKG_VERSION"); - let scarb_version = if let Some(scarb) = Scarb::version() { - scarb - } else { - "not found in your PATH\n".to_string() - }; + let scarb_version = Scarb::version() + .map(|s| s.trim().to_string()) + .unwrap_or_else(|| "not found in your PATH".to_string()); format!("{}\nscarb: {}", DOJO_VERSION, scarb_version) }bin/sozo/src/args.rs (2)
15-16: Use long_version for the multi-line banner; keep version to the Dojo semverFeeding a multi-line String into
versionworks, but clap traditionally useslong_versionfor extended, multi-line info whileversionis the short semver. This keepssozo <semver>tidy and still shows Scarb details.Additionally, if clap derive ever tightens what it accepts for
version, this avoids potential macro parsing issues around calling a function in the attribute.Consider this tweak:
-#[command(author, version=generate_version(), about, long_about = None)] +#[command( + author, + version = env!("CARGO_PKG_VERSION"), + long_version = generate_version(), + about, + long_about = None +)]Would you confirm your clap version supports expressions in derive attributes here? If not, the above change will be safer across versions.
17-22: Env support for manifest_path: nice usability win; optional UX hintBinding
DOJO_MANIFEST_PATHis solid. As a tiny UX boost, you can hint shells that this expects a directory path.You could add:
#[arg(long)] #[arg(global = true)] #[arg(env = "DOJO_MANIFEST_PATH")] +#[arg(value_hint = clap::ValueHint::DirPath)] #[arg(help = "Override path to a directory containing a Scarb.toml file.")] pub manifest_path: Option<Utf8PathBuf>,Note: this is purely a nicety for completion helpers; feel free to skip.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
bin/sozo/src/args.rs(1 hunks)bin/sozo/src/utils.rs(2 hunks)crates/sozo/scarb_interop/src/scarb.rs(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
bin/sozo/src/utils.rs (1)
crates/sozo/scarb_interop/src/scarb.rs (1)
version(18-24)
bin/sozo/src/args.rs (2)
bin/sozo/src/utils.rs (1)
generate_version(212-222)crates/sozo/scarb_interop/src/scarb.rs (1)
version(18-24)
🔇 Additional comments (2)
bin/sozo/src/utils.rs (1)
12-12: Ohayo, sensei — import looks goodImporting
Scarbhere is appropriate for the version banner generation.bin/sozo/src/args.rs (1)
12-12: Ohayo, sensei — dependency on generate_version is wired correctlyBringing
generate_versioninto scope here is needed for the command metadata.
remybar
force-pushed
the
fix-sozo_version
branch
from
19cf5ad to
fa6c189
Compare
| pub fn generate_version() -> String { | ||
| const DOJO_VERSION: &str = env!("CARGO_PKG_VERSION"); | ||
|
|
||
| let scarb_version = if let Some(scarb) = Scarb::version() { | ||
| scarb | ||
| } else { | ||
| "not found in your PATH\n".to_string() | ||
| }; | ||
|
|
||
| format!("{}\nscarb: {}", DOJO_VERSION, scarb_version) | ||
| } | ||
|
|
There was a problem hiding this comment.
I think considering we have strong dependency on scarb, it may be more appropriate to return this as a concrete error (or panic).
Description
Fix
sozo --versionoutput.Tests
Added to documentation?
Checklist
scripts/rust_fmt.sh,scripts/cairo_fmt.sh)scripts/clippy.sh,scripts/docs.sh)Summary by CodeRabbit