Repository navigation
Migrate CLI argument parsing to clap (fixes #101) - #108
Merged
Merged
Conversation
Replaces main.rs's ~120-line hand-rolled parse_args_raw loop and ~200-line hand-maintained print_usage with a clap 4 derive Cli struct. Every flag keeps its exact spelling and semantics; config::Overrides's Option<T>-based "explicitly set on the CLI vs. left at a built-in default" merge contract is unchanged -- clap is never given a default value for anything, and cli_to_raw() converts the parsed Cli into parse_args_raw's existing (Overrides, InstrumentationOptions, Option<f64>, String, Vec<String>) return shape, so resolve()/main's config-file layering is untouched. Key implementation points: - --fpu/--no-fpu, --jit/--no-jit, --defaults/--no-defaults each use two ArgAction::SetTrue flags with bidirectional overrides_with, so the last one given on the command line wins (matching the old parser), then collapse to Option<bool> by hand in cli_to_raw. - --sanitize-uninit implies --sanitize, applied in cli_to_raw rather than via clap (clap has no simple "set this other field" action). - Existing value parsers (parse_byte_size, parse_cpu_type, parse_clock_mhz, split_name_value, the 0x-hex ignore-pc parser) are reused as-is via thin per-flag wrapper functions passed as clap value_parsers, so their unit tests keep exercising the real logic. - Help text carries over print_usage's full prose (including the two trailing "SYS:/C:/S:/... resolve out of the box" and "~/.volamos supplies defaults" paragraphs via after_long_help), grouped into Logging/Filesystem/Machine/Execution/Instrumentation/Networking help_headings. print_usage is deleted. - Added --version (long-only; clap's auto -V short alias collides with our existing -V/--volume, so disable_version_flag + a manual ArgAction::Version field). Trailing-args semantics (requirement: `volamos prog -v --stack` must pass -v and --stack to the guest verbatim, not re-parse them) turned out to need more than trailing_var_arg + allow_hyphen_values: empirically, clap 4.6's trailing_var_arg only stops an *unrecognized* token from erroring once the variadic positional starts consuming -- it still matches a flag clap knows about (e.g. --stack) anywhere in the whole argument list. Fixed with a small hand-written split_program_boundary() that finds the <program> boundary before clap ever runs (using a VALUE_FLAGS table to know which flags consume a following token), feeding clap only the option-prefix + <program>, and setting guest_args on the parsed Cli by hand afterward. Intentional behavior change: an unrecognized --flag before <program> is now a clean clap error, instead of the old parser's silent fallback of treating any unrecognized token as <program> itself. One related edge case this changes: a program file whose name itself starts with a `-` would previously have been accepted as <program>; it's now treated as an unknown flag (an extremely unlikely real-world name). -h/--help and --version now go through clap's own ClapError::exit() in main() for the real binary (full formatted help, exit 0), while the test-only parse_args_raw/parse_args wrapper keeps the pre-clap contract of Err(String::new()) for help so the existing test suite's plumbing needed only minimal adjustment (error-message substring assertions updated to match clap's wording; the exact-string "missing <program> argument" assertion loosened to a substring check against clap's standard required-argument error). Added tests: unknown flag before <program> errors; flags after <program> reach the guest untouched even when they look like recognized flags (the specific case split_program_boundary exists for); --fpu/--no-fpu last-wins in both directions; -V/-a repeated-flag order preservation. cargo test -p volamos: 171/171 passing. cargo clippy -p volamos --all-targets -- -D warnings: clean. cargo fmt -p volamos: clean. Manually smoke-tested --help, an unknown-flag error, --version, and fixtures/echoargs end-to-end (plain and with -v). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GNNXMtYaZVHe8ihLL2GsrY
…llings from clap metadata Review fixes on top of the clap migration: - split_program_boundary() now treats a standalone -- as clap's end-of-options escape: the next token is <program> no matter how flag-like it looks, and everything after it is the guest's argv. Previously the scanner pushed a dash-leading program into the half clap parses and mistook the first real guest argument for <program>, so 'volamos -- -prog one two' ran -prog with argv 'two' -- 'one' was silently dropped (and clap's own unknown-flag error actively recommends the -- spelling, so the escape has to work). - The hand-synced VALUE_FLAGS table is gone: value_flag_spellings() derives the -X/--long spellings of every value-taking flag from clap's own Command metadata (CommandFactory), so the boundary scan can never drift out of sync with Cli's flag declarations. A test pins the current set so an unexpected change is still visible. - New regression tests: the -- escape (with and without preceding flags), the spellings pin, and --clock-mhz=25/--stack=256K =-form values (a user-reported failure of the old hand-rolled parser, which treated '--clock-mhz=25' as the program name). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GNNXMtYaZVHe8ihLL2GsrY
CLI-Reference: sync the usage block with the full current flag surface (--defaults/--no-defaults, --volumes-dir, --dirty-heap, --net were missing even before this branch), document --version, the --flag=value spellings, the unknown-flag error + ./-foo / -- -foo escape for dash-leading program names, and the new exit-code convention (2 for a CLI parse error, 0 for --help/--version). Changelog: 0.8 entry for the migration's user-visible changes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GNNXMtYaZVHe8ihLL2GsrY
Four previously-working command-line behaviors change: a bare dash-leading program path is now an unknown-flag error (./-foo or -- -foo still work), parse errors exit 2 instead of 1, --help prints to stdout instead of stderr, and parse-error wording is clap's. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GNNXMtYaZVHe8ihLL2GsrY
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Replaces the hand-rolled argument parser and the ~200-line hand-maintained
print_usagewith clap 4 (derive API). Every flag keeps its exact spelling and semantics; the config-file layering (config::Overrides/resolve()) is untouched — clap is given no defaults, so "explicitly set on the CLI" vs "left at default" merging works exactly as before.Fixes #101:
--version(long-only —-Vstays--volume; clap's auto-Vversion alias is disabled), printing to stdout and exiting 0, soV=$(volamos --version)works.--flagbefore<program>is now a clean error with a did-you-mean suggestion, instead of being silently treated as the program path and surfacing as "couldn't read '--sanitze': No such file or directory".Also fixes a user-reported failure:
--clock-mhz=25(and every other--flag=valuespelling) now parses — the old parser only matched exact--flagtokens followed by a separate value, so the=form fell through to the catch-all and was treated as the program name. Regression test included.Details:
long_help), now grouped into Logging / Filesystem / Machine / Execution / Instrumentation / Networking headings, with the standard-volumes and config-file paragraphs inafter_long_help.parse_byte_size,parse_cpu_type,parse_clock_mhz, NAME:value splitting, 0x-hex ignore-pc) are reused as clapvalue_parsers; their unit tests carry over.--fpu/--no-fpu,--jit/--no-jit,--defaults/--no-defaultspairs use bidirectionaloverrides_with, preserving last-one-wins.<program>boundary ("everything after the program is the guest's argv, verbatim, even-v") is found by a pre-clap scan: clap'strailing_var_argalone still matches recognized flag spellings after the positional, so the split has to happen first. The scan derives the set of value-taking flag spellings from clap's ownCommandmetadata (no hand-synced table to drift), honors clap's--end-of-options escape (the token after--is<program>even if it starts with-), and a test pins the spelling set.Breaking / behavior changes (intentional, also listed in the Changelog):
<program>flags error instead of being treated as the program name (No --version flag: 'volamos --version' reports a missing file instead #101) — so a program path given bare with a leading-(volamos -foo) no longer runs; it's still expressible as./-fooor-- -foo.--help/--versionexit 0.--helpprints to stdout (previously stderr).Tests: 174 passing (
cargo test -p volamos), clippy and fmt clean. New coverage: unknown-flag error, recognized-looking flags after<program>pass through untouched, bool-pair last-wins both directions, repeatable-V/-aorder preservation, the--escape,=-form values, and the value-flag spelling pin.🤖 Generated with Claude Code
https://claude.ai/code/session_01GNNXMtYaZVHe8ihLL2GsrY