Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixup cli to handle parsing matches when called as a cargo subcommand #119

Merged
merged 2 commits into from
Jul 19, 2024

Conversation

dsteeley
Copy link
Contributor

@dsteeley dsteeley commented Jul 16, 2024

Fixes #118.
Expect this issue can be fixed in multiple ways and happy for an alternative to be suggested. The underlying issue is that we're not correctly parsing the argument matches when cargo-generate-rpm is called as a cargo subcommand.

To regain the previous behaviour the arguments of the generate-rpm subcommand need to be reparsed. The issue can be reproduced by running cargo run generate-rpm on the latest main.

thread 'main' panicked at src/cli.rs:134:40:
`"metadata_overwrite"` is not an id of an argument or a group.

@dsteeley
Copy link
Contributor Author

Debug output of matches at the point of a panic. Demonstrating that the arguments of the subcommand generate-rpm need to be parsed to match the args of the Cli struct.

ArgMatches { 
valid_args: ["help"], 
valid_subcommands: ["generate-rpm", "help"], 
args: FlatMap { keys: [], values: [] }, 
subcommand: Some(SubCommand { 
  name: "generate-rpm", 
  matches: ArgMatches { 
    valid_args: ["arch", "output", "package", "auto_req", "target", "target_dir", "profile", "payload_compress", "source_date", "metadata_overwrite", "set_metadata", "variant", "help", "version", "Cli"], 
    valid_subcommands: [], 
    args: FlatMap { 
      keys: ["auto_req", "profile", "payload_compress"], 
      values: [MatchedArg { 
        source: Some(DefaultValue), indices: [1], type_id: Some(cargo_generate_rpm::cli::AutoReqMode), vals: [[AnyValue { inner: cargo_generate_rpm::cli::AutoReqMode }]], 
      raw_vals: [["auto"]], 
      ignore_case: false }, 
    MatchedArg { 
      source: Some(DefaultValue), 
      indices: [2], 
      type_id: Some(alloc::string::String), 
      vals: [[AnyValue { inner: alloc::string::String }]], 
      raw_vals: [["release"]], 
      ignore_case: false }, 
    MatchedArg { 
      source: Some(DefaultValue), 
      indices: [3], 
      type_id: Some(cargo_generate_rpm::cli::Compression), 
      vals: [[AnyValue { inner: cargo_generate_rpm::cli::Compression }]], 
      raw_vals: [["zstd"]], 
      ignore_case: false }] }, 
    subcommand: None } }) }

@cat-in-136
Copy link
Owner

To my surprise, it turned out that the contents of matches were different between debug and release target: presence of valid_args and valid_subcommands. This is the reason why the release build does not reproduce the issue.
Anyway, as you indicates, I agree with matches.subcommand_matches("generate-rpm") would be preferred (regardless release target).

@cat-in-136
Copy link
Owner

I confirmed that its behavior itself works fine.

build Cargo.toml release --set-metadata subcommand? Result with fix candidate Note
debug PASS Release=1
debug generate-rpm PASS Release=1
debug 1.el9 PASS Release=1.el9
debug 1.el9 generate-rpm PASS Release=1.el9
debug 1.el8 PASS Release=1.el8
debug 1.el8 generate-rpm PASS Release=1.el8
debug 1.el8 1.el9 PASS Release=1.el9
debug 1.el8 1.el9 generate-rpm PASS Release=1.el9
release PASS Release=1
release generate-rpm PASS Release=1
release 1.el9 PASS Release=1.el9
release 1.el9 generate-rpm PASS Release=1.el9
release 1.el8 PASS Release=1.el8
release 1.el8 generate-rpm PASS Release=1.el8
release 1.el8 1.el9 PASS Release=1.el9
release 1.el8 1.el9 generate-rpm PASS Release=1.el9

FYI: Behavior before the fix (v0.15.1)

build Cargo.toml release --set-metadata subcommand? Actual result Note
debug PASS Release=1
debug generate-rpm FAIL(panicked) "metadata_overwrite" is not an id of an argument or a group.
debug 1.el9 PASS Release=1.el9
debug 1.el9 generate-rpm FAIL(panicked) "metadata_overwrite" is not an id of an argument or a group.
debug 1.el8 PASS Release=1.el8
debug 1.el8 generate-rpm FAIL(panicked) "metadata_overwrite" is not an id of an argument or a group.
debug 1.el8 1.el9 PASS Release=1.el9
debug 1.el8 1.el9 generate-rpm FAIL(panicked) "metadata_overwrite" is not an id of an argument or a group.
release PASS Release=1
release generate-rpm PASS Release=1
release 1.el9 PASS Release=1.el9
release 1.el9 generate-rpm FAIL Release=1
release 1.el8 PASS Release=1.el8
release 1.el8 generate-rpm PASS Release=1.el8
release 1.el8 1.el9 PASS Release=1.el9
release 1.el8 1.el9 generate-rpm FAIL Release=1.el8

@dsteeley
Copy link
Contributor Author

@cat-in-136 Thank you for the thorough testing that you've done here. Are there any changes you'd like me to make before getting the fix merged and released?

@cat-in-136
Copy link
Owner

Please check my review. (I've added @ mention to the review)

@dsteeley
Copy link
Contributor Author

Please check my review. (I've added @ mention to the review)

Am I looking in the wrong place? I'm not able to see any review comment on this PR.

src/cli.rs Outdated Show resolved Hide resolved
@cat-in-136 cat-in-136 merged commit 2073357 into cat-in-136:master Jul 19, 2024
1 check passed
@cat-in-136 cat-in-136 added the bug Something isn't working label Jul 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The --set-metadata option does not overwrite/set metadata anymore
2 participants