Repository navigation
fix(cargo): reinstall when install options change - #11480
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughCargo installations now persist normalized install options and compare them when deciding whether a version is satisfied. Install orchestration handles shared paths and rolling checksums by concrete install path, with unit, end-to-end, and documentation coverage. ChangesInstall state and rolling install paths
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant BackendInstallVersion
participant CargoBackend
participant InstallStateFile
participant CargoInstaller
User->>BackendInstallVersion: run mise install
BackendInstallVersion->>CargoBackend: check version and normalized options
CargoBackend->>InstallStateFile: read persisted install state
alt state is not satisfied
BackendInstallVersion->>CargoInstaller: install Cargo version
CargoInstaller->>InstallStateFile: write normalized options
else state is satisfied
BackendInstallVersion-->>User: report already installed
end
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/backend/cargo.rs`:
- Line 265: Make all three write_install_state call sites in install_version_
best-effort: handle write errors by logging them and continue returning the
successful install result instead of propagating with ?. Apply the same behavior
to the additional state write path around lines 391-397, preserving normal error
handling for the actual installation commands.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 14f29d5d-c20b-4d05-a155-b42161b82e2a
📒 Files selected for processing (6)
docs/dev-tools/backends/cargo.mde2e/backend/test_cargo_binstall_tokene2e/backend/test_cargo_install_options_slowe2e/backend/test_cargo_install_options_stub_slowsrc/backend/cargo.rssrc/backend/mod.rs
Greptile SummaryThis PR makes Cargo installation identity sensitive to artifact-selection options and improves handling of shared rolling installs.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Important Files Changed
Reviews (3): Last reviewed commit: "fix(cargo): reinstall when install optio..." | Re-trigger Greptile |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/backend/mod.rs`:
- Around line 2389-2404: Confirm and encode the intended behavior around
install_satisfied and rolling_reinstall in the install flow: add coverage for
outdated rolling-channel installs using shared paths, or adjust the redirect
condition so rolling checksum staleness does not redirect an otherwise matching
shared install. Anchor the change in is_install_satisfied_or_false,
rolling_reinstall, and the shared-install redirect block, preserving forced and
genuinely unsatisfied install behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: f4a2ae12-48c1-40f1-a37d-3de4c0a9b880
📒 Files selected for processing (6)
docs/dev-tools/backends/cargo.mde2e/backend/test_cargo_binstall_tokene2e/backend/test_cargo_install_options_slowe2e/backend/test_cargo_install_options_stub_slowsrc/backend/cargo.rssrc/backend/mod.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/dev-tools/backends/cargo.md
Summary
.mise-cargo-install.tomlfor each installed versionfeatures,default-features,bin,crate, orlockedchangesProblem
Cargo artifacts are identified by more than the package version. The selected crate and binary determine what is installed, feature flags determine which code is compiled, and locked mode determines dependency resolution. Previously mise only checked whether the version directory existed, so changing any of these options could silently reuse the wrong artifact.
Approach
The Cargo backend now derives a normalized install state from the exact options passed to the installer. Feature strings and arrays are split on commas or whitespace, then sorted and deduplicated. Omitted
lockedanddefault-featuresvalues normalize to their existing true defaults, whilebinis resolved for the current platform andcraterecords the selected override.All successful external cargo-binstall, native binstall, and cargo install paths attempt to write the state file. A metadata write failure logs a warning and preserves the successfully installed artifact; the missing state then causes a non-default request to retry on the next install. A missing state file remains compatible with an all-default legacy install; missing state for non-default options, malformed state, option mismatches, and removal of non-default options trigger one reinstall.
When an incompatible artifact comes from a shared or system fallback, mise leaves that directory untouched and installs the requested variant into the primary user install directory instead.
install_env, registry selection, and cargo-binstall settings are intentionally excluded because they are execution inputs rather than public artifact-selection options and may contain secrets.Benefits of tracking all five options
featuresanddefault-featuresselect compiled code and capabilities.binselects which executable Cargo installs from a multi-binary package.crateselects the workspace package for Git sources.lockedcontrols whether Cargo uses the committed dependency graph.Treating the complete selection as the artifact identity prevents stale binaries without requiring users to remember
--force.Verification
mise exec -- cargo test backend::cargo::tests— 19 passedmise run test:e2e e2e/backend/test_cargo_install_options_stub_slow— passed; covers all five changes, option removal, feature normalization, and shared shadowingmise run test:e2e e2e/backend/test_cargo_install_options_slow— passed; recompiles the same local Git tag and changes runtime output after enabling a featuremise run test:e2e e2e/backend/test_cargo_binstall_token— passedmise run test:e2e e2e/cli/test_shared_install_dirs— passedmise exec -- cargo check --all-features— passedmise run lintcannot run as a wrapper in this Sapling working copy because hk reportsfailed to find git repository; the applicable checks above were run individuallyAddresses #5235
AI-assisted — Tool: Codex; model: OpenAI/GPT-5; version: unavailable.
Summary by CodeRabbit