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

Feature/clap v4 wip #3514

Closed
wants to merge 10 commits into from
Closed

Conversation

hargut
Copy link
Contributor

@hargut hargut commented Sep 22, 2022

Current Behavior

clap & clap_complete v3 are used

Proposed Changes

upgrade clap & clap_complete to v4

  • address v3 deprecation warnings
  • upgrade to v4 & fixing compiler errors
  • address v4 deprecation warnings

Notes

this PR is not ready as the upgrade guide was only partially done

Checks

address v3 deprecation warnings
upgrade clap to v4.0.0.0-rc.2 & clap_complete to v4.0.0-rc.1
address v4 deprecation warnings
@hargut
Copy link
Contributor Author

hargut commented Sep 22, 2022

#3506

@mrinalwadhwa
Copy link
Member

@hargut awesome thank you for starting this. Let us know if you have questions along the way.

finish transition to command/arg
deactivate mut_subcommand/mut_arg for "help" (does not work as expected with upgraded version)
@@ -148,8 +148,12 @@ EXAMPLES:
version,
long_version = Version::long(),
next_help_heading = "GLOBAL OPTIONS",
mut_arg("help", |a| a.help_heading("GLOBAL OPTIONS")),
mut_subcommand("help", |c| c.about("Print help information"))
// TODO: mut_arg & mut_subcommand for help
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mrinalwadhwa
The previously used mut_arg & mut_subcommand for help lead with the new version to runtime errors like:

thread 'main' panicked at 'Argument `help` is undefined'
thread 'main' panicked at 'Command `help` is undefined'

So it seems that mut_ on "help" does not work any more as in the previous version. I've check the docs, but could not locate any indication that this was removed or disabled. Further the upgrade guide is not providing an information this is no longer possible.

https://docs.rs/clap/4.0.0-rc.2/clap/_derive/index.html
https://docs.rs/clap/4.0.0-rc.2/clap/struct.Command.html#method.mut_subcommand

Did I miss something?

It looks like "help" is no longer part of the clap::builder::command::Command subcommands Vec and therefore mut_subcommand fails on trying to mutate "help".
At top level with the mut_arg it seems to be a similar situation. Trying to add "help" leads to a duplicate error, and only works out after disable_help_flag = true.

Best regards,
Harald

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Thank you for researching that, makes sense. In the refactor to clap v4, I think we can disable the use of mut_arg and mut_subcommand ... get everything working in the new version. And then add the change suggested in clap-rs/clap#4056

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just had a look at the first test run results, but cannot understand what was going on:

The checkout action states:
https://github.com/build-trust/ockam/actions/runs/3108456670/jobs/5043177038#step:3:485

478:  HEAD is now at 0207e5a Merge 40df091a79a724158272f306e4a88f46279cfc98 into fe6e34b8f824ff3b56a3482a62c9ba8e652c7d64

One error from the build is:
https://github.com/build-trust/ockam/actions/runs/3108456670/jobs/5043177038#step:7:57

 56: error: Unknown `#[clap(long)]` attribute (`#[arg(long)] exists)
  --> implementations/rust/ockam/ockam_command/src/node/create.rs:87:12
   |
87 |     #[clap(long, hide = true)]
   |            ^^^^

The file does not contain any #[clap( at commit 40df091.

😕 😲

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, the part originates from a commit that was not there when the change was created:

fe6e34b#diff-afac108b9e4f2377c7932bef242708078b72d74836268ccc0ff4a09a24f67a53R87

@hargut hargut closed this Sep 24, 2022
@hargut hargut deleted the feature/clap-v4-wip branch September 24, 2022 13:59
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.

None yet

2 participants