Skip to content

fix(spm): classify artifact bundle lock options#9986

Closed
risu729 wants to merge 12 commits into
jdx:mainfrom
risu729:fix/spm-filter-bins-manifest-options
Closed

fix(spm): classify artifact bundle lock options#9986
risu729 wants to merge 12 commits into
jdx:mainfrom
risu729:fix/spm-filter-bins-manifest-options

Conversation

@risu729

@risu729 risu729 commented May 19, 2026

Copy link
Copy Markdown
Contributor

Summary

  • include SPM provider/API/artifact bundle inputs in lock identity
  • keep filter_bins out of lock identity while preserving it as an install/layout option
  • wire SPM install-time option keys into the shared manifest precedence filter

Stacked on #9959. Target branch remains main.

Verification

  • cargo fmt --check
  • cargo test options_include

@greptile-apps

greptile-apps Bot commented May 19, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR classifies SPM backend options into two categories — lock-identity options (provider, api_url, artifactbundle, artifactbundle_asset) and install/layout-only options (filter_bins) — and hooks SPM into the shared install_time_option_keys_for_type dispatch. The raw ToolVersionOptions is now wrapped in a typed SpmOptions<'_> struct that centralises all per-platform option lookups.

  • Introduces SpmOptions<'a> with lockfile_options(&PlatformTarget) that emits only the options that form the artifact-download identity, correctly scoping each lookup to the requested target platform rather than the running host.
  • Removes the standalone resolve_artifactbundle_mode / requires_artifactbundle free functions and migrates all callers to SpmOptions methods; artifactbundle_mode still returns eyre::Result and bails on unrecognised values.
  • Registers spm::install_time_option_keys() in install_time_option_keys_for_type, ensuring the manifest precedence filter treats all five SPM options correctly.

Confidence Score: 5/5

Safe to merge; the changes are well-scoped refactoring with comprehensive tests, and no correctness regressions were found.

The refactoring correctly encapsulates platform-aware option lookups in SpmOptions, the lockfile_options implementation uses target-scoped lookups without current-host fallback, and all callers are updated consistently. The one gap — no diagnostic output when an invalid artifactbundle value is silently omitted from the lockfile — is a minor quality concern that does not affect runtime correctness since the install path still bails on the same invalid value.

src/backend/spm.rs — the Err arm in lockfile_options

Important Files Changed

Filename Overview
src/backend/spm.rs Introduces SpmOptions wrapper, lockfile_options, and install_time_option_keys; the Err(_) arm in lockfile_options silently drops invalid artifactbundle values without any diagnostic output
src/backend/mod.rs Wires BackendType::Spm into install_time_option_keys_for_type; one-line change, straightforward

Reviews (3): Last reviewed commit: "fix(spm): avoid host fallback for target..." | Re-trigger Greptile

Comment thread src/backend/spm.rs Outdated

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces the SpmOptions struct to handle SPM-specific backend configurations and implements the resolve_lockfile_options method for SPMBackend. It refactors the codebase to utilize this new helper for retrieving settings like provider, API URL, and artifact bundle modes, while also adding comprehensive unit tests. Review feedback recommends extending SpmOptions to support platform-specific overrides using BackendOptions helpers and updating the lockfile_options signature to accept a PlatformTarget for improved cross-platform locking accuracy.

Comment thread src/backend/spm.rs Outdated
Comment thread src/backend/spm.rs Outdated
Comment thread src/backend/spm.rs
Comment thread src/backend/spm.rs Outdated
Comment thread src/backend/spm.rs
@risu729

risu729 commented May 19, 2026

Copy link
Copy Markdown
Contributor Author

CI note: lint/test-ci are failing because cargo deny check now detects RUSTSEC-2026-0145 for astral-tokio-tar 0.6.1 via rattler_package_streaming. This appears unrelated to this PR: the current main branch is failing the same cargo-deny advisory in the test workflow, and test-ci only failed because lint failed.

This comment was generated by an AI coding assistant.

@github-actions

Copy link
Copy Markdown

This PR has had failing checks for more than 7 days. Feel free to reopen or create a new PR if you'd like to continue working on this.

@github-actions github-actions Bot closed this May 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant