Repository navigation
crates/minimal: cmd_dump accepts --arch to override target - #146
Conversation
Closes #125. Previously `minimal dump --packages` always evaluated Nickel against the host system's target, so packages whose `build_deps` use `match { arch = 'Amd64 } => ..., { arch = 'Arm64 } => ... } target` only surfaced one arm's data in the dump output — the other arm was erased at evaluation time with no breadcrumb. `mctx::graph_from_all_packages_with_target(target)` already exists; cmd_dump simply wasn't plumbed to it. This commit adds a clap arg: minimal dump --packages --arch amd64 minimal dump --packages --arch arm64 Parsing follows `cmd_materialize`'s existing `--arch` convention (arm64/aarch64 → Arm64, amd64/x86_64 → Amd64). OS is always Linux; dump consumers want per-arch evaluation of the same pkgs repo, not cross-OS targets. When `--arch` is absent, defaults to `Target::host()` (preserves current behavior). Unblocks the downstream consumers listed in the issue — pkgmgr's updater can now see per-arch source data for arch-matched packages (deno, go, gcloud, etc.) by running the dump once per arch, and supply-chain's scanner gets the same visibility. Tests: resolve_target helper has unit tests covering the default path, both arch aliases (amd64/x86_64, arm64/aarch64), and the error case for unsupported values.
| /// parsing (arm64/aarch64 → Arm64, amd64/x86_64 → Amd64). When the flag is | ||
| /// absent, defaults to [`Target::host`]. OS is always Linux — dump consumers | ||
| /// want per-arch evaluation of the same pkgs repo, not cross-OS. | ||
| fn resolve_target(arch: Option<&str>) -> Result<Target, Error> { |
There was a problem hiding this comment.
Make or use an existing method on Target or Arch: the generic problem of going from an &str into the specific type is something we are bound to use half a dozen times in various places.
Addresses review feedback: the str-to-Arch parsing (arm64/aarch64,
amd64/x86_64) was open-coded separately in cmd_dump and cmd_materialize
— moving it to a shared `impl FromStr for Arch` in common means:
- Single source of truth for which aliases we accept. uname -m output
works verbatim on both macOS (arm64/x86_64) and Linux (aarch64/amd64).
- New consumers can use `"amd64".parse::<Arch>()` instead of
re-implementing the match.
- Error formatting ("unsupported architecture: X. Use 'amd64' or
'arm64'.") lives in one place alongside the parse logic.
Both cmd_dump::resolve_target and cmd_materialize's arch resolution
now delegate to this via `str.parse::<Arch>()`. cmd_materialize loses
the anyhow import shadow (only uses anyhow! from below now). Tests
for the parser move to common/target.rs; cmd_dump tests retain their
integration coverage (resolve_target defaults, Linux-OS wrapping,
error propagation).
|
Good call on the shared helper — pushed 5aca2a3: added FromStr for Arch in common with full alias set and tests, migrated cmd_dump and cmd_materialize to str.parse. The arch parsing duplication in cmd_materialize is cleaned up as a drive-by since the same point applied there. |
twitchyliquid64
left a comment
There was a problem hiding this comment.
Lastly, update the other place where we go from a str to a Target, in the nickel-parsing code.
Idk why i thought looping over all representations was a good idea at the time lol
minimal/crates/decode/src/builds.rs
Lines 910 to 916 in 3109695
Addresses the second review point on #146: decode/src/builds.rs was looping over Target::all() to find a target by its as_ref() string ("amd64/linux", etc.). Now that Arch has FromStr, Target can too — consumers outside cmd_dump should use the shared impl instead. Adds: - FromStr for OS (linux/macos) with OsParseError - FromStr for Target, delegates arch/os segment parsing to the component FromStrs. Round-trips with Target::as_ref(). - TargetParseError surfaces which segment failed (shape / arch / os) so users get a specific message. decode/src/builds.rs's "target" field parser now does `target_str.parse::<Target>()` and folds into the existing Error::InvalidTarget variant on parse failure. No behavior change for valid inputs. Also fixes CI fmt-check failure: rustfmt on CI formats the cmd_materialize arch-resolver chain differently than my initial layout. Reformatted to match. Tests: 7 new in target.rs — OS both variants + unknown rejection, Target round-trip over every Target::all() value, missing-separator error, arch error propagation, OS error propagation.
|
Pushed 0926f00: added FromStr for Target (and OS as a building block) and migrated the ad-hoc Target::all() loop in decode/src/builds.rs. Also fixed the rustfmt diff CI flagged on cmd_materialize — chain layout preference. The new Target::FromStr has a roundtrip test over every Target::all() value so that impl and as_ref stay locked together. |
* crates/minimal: cmd_dump accepts --arch to override target Closes #125. Previously `minimal dump --packages` always evaluated Nickel against the host system's target, so packages whose `build_deps` use `match { arch = 'Amd64 } => ..., { arch = 'Arm64 } => ... } target` only surfaced one arm's data in the dump output — the other arm was erased at evaluation time with no breadcrumb. `mctx::graph_from_all_packages_with_target(target)` already exists; cmd_dump simply wasn't plumbed to it. This commit adds a clap arg: minimal dump --packages --arch amd64 minimal dump --packages --arch arm64 Parsing follows `cmd_materialize`'s existing `--arch` convention (arm64/aarch64 → Arm64, amd64/x86_64 → Amd64). OS is always Linux; dump consumers want per-arch evaluation of the same pkgs repo, not cross-OS targets. When `--arch` is absent, defaults to `Target::host()` (preserves current behavior). Unblocks the downstream consumers listed in the issue — pkgmgr's updater can now see per-arch source data for arch-matched packages (deno, go, gcloud, etc.) by running the dump once per arch, and supply-chain's scanner gets the same visibility. Tests: resolve_target helper has unit tests covering the default path, both arch aliases (amd64/x86_64, arm64/aarch64), and the error case for unsupported values. * cmd_dump, cmd_materialize: centralize arch parsing in common::Arch Addresses review feedback: the str-to-Arch parsing (arm64/aarch64, amd64/x86_64) was open-coded separately in cmd_dump and cmd_materialize — moving it to a shared `impl FromStr for Arch` in common means: - Single source of truth for which aliases we accept. uname -m output works verbatim on both macOS (arm64/x86_64) and Linux (aarch64/amd64). - New consumers can use `"amd64".parse::<Arch>()` instead of re-implementing the match. - Error formatting ("unsupported architecture: X. Use 'amd64' or 'arm64'.") lives in one place alongside the parse logic. Both cmd_dump::resolve_target and cmd_materialize's arch resolution now delegate to this via `str.parse::<Arch>()`. cmd_materialize loses the anyhow import shadow (only uses anyhow! from below now). Tests for the parser move to common/target.rs; cmd_dump tests retain their integration coverage (resolve_target defaults, Linux-OS wrapping, error propagation). * common, decode: FromStr for Target, replace ad-hoc loop in builds.rs Addresses the second review point on #146: decode/src/builds.rs was looping over Target::all() to find a target by its as_ref() string ("amd64/linux", etc.). Now that Arch has FromStr, Target can too — consumers outside cmd_dump should use the shared impl instead. Adds: - FromStr for OS (linux/macos) with OsParseError - FromStr for Target, delegates arch/os segment parsing to the component FromStrs. Round-trips with Target::as_ref(). - TargetParseError surfaces which segment failed (shape / arch / os) so users get a specific message. decode/src/builds.rs's "target" field parser now does `target_str.parse::<Target>()` and folds into the existing Error::InvalidTarget variant on parse failure. No behavior change for valid inputs. Also fixes CI fmt-check failure: rustfmt on CI formats the cmd_materialize arch-resolver chain differently than my initial layout. Reformatted to match. Tests: 7 new in target.rs — OS both variants + unknown rejection, Target round-trip over every Target::all() value, missing-separator error, arch error propagation, OS error propagation.
) Removes the agentic-workflow artifacts that landed here via the old sync / caller-wrapper install model (gominimal/min-aw PR #146 retired that path). This repo will be re-bootstrapped via `gh aw add gominimal/min-aw/<chore>@<release-tag>` once this PR merges — the gh-aw [sharing-workflows](https://github.github.com/gh-aw/guides/organization-practices/sharing-workflows/) pattern. Removed (agentic only): - `.github/workflows/sync-from-source-of-truth.yml` — retired sync workflow. - `.github/workflows/worker-fix.yml` + `worker-iterate.yml` — retired caller wrappers (no compiled `.lock.yml` was ever committed on this repo). - `AGENTS.md`, `.github/agents/`, `.github/labels.yml`, `.github/copilot-instructions.md` — contract template; will be reinstalled by `quick-setup.sh` at re-bootstrap. - `.github/ISSUE_TEMPLATE/{bug,chore,feature,feedback}.md` — installed by the min-aw contract template. The pre-existing `bug_report.md` and `feature_request.md` are NOT touched. Kept (native CI + pre-existing): - `.github/workflows/ci.yml` — Minimal CI gate. - `.github/workflows/promote.yml` — release promotion workflow. - `.github/ISSUE_TEMPLATE/bug_report.md` + `feature_request.md` — pre-existing GitHub-default issue templates. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
Closes #125. Adds a
--archflag tominimal dumpso downstream tools can evaluate arch-matched packages for both architectures.When
--archis absent, defaults toTarget::host()(preserves current behavior).Why
mctx::graph_from_all_packages_with_target(target)has existed for a while butcmd_dumpwas hard-coded toTarget::host(). For packages that usematch { arch = 'Amd64 } => ..., { arch = 'Arm64 } => ... } targetto select between per-arch source URLs (claude-code, go, deno, gcloud, etc.), this meant only the host arm's data surfaced in the dump — the other arm was erased at evaluation time.That blind spot cascades:
Exposing the target at the CLI lets consumers run the dump twice and aggregate.
Design notes
cmd_materialize's existing--archpattern verbatim: acceptsamd64/x86_64→Arch::Amd64,arm64/aarch64→Arch::Arm64. Rejects anything else with an error message naming the bad input and listing valid options.OS::Linux. Dump consumers want per-arch evaluation of the same pkgs repo, not cross-OS targets. Easy to extend later if a cross-OS case emerges.resolve_targetis factored out so it's unit-testable without spinning up aContext. Extracted into a private helper matching the codebase's existing style of small helpers colocated with their command.Test plan
cmd_dump::testscover the four paths: default (host), amd64 aliases, arm64 aliases, unknown-arch errorminimal test— full suite passes, including the new cmd_dump testsminimal run fmt-check— cleanminimal run clippy— clean (no new warnings)Follow-ups (not this PR)
sha256/urldata, and pkgmgr rewrites all arms in one pass.