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

Update to abscissa 0.6.0-beta.1 and clap 3.0.0-beta.5 #1576

Merged
merged 17 commits into from
Dec 1, 2021

Conversation

mzabaluev
Copy link
Contributor

@mzabaluev mzabaluev commented Nov 18, 2021

Closes: #134

Description

Overhaul command line argument processing to use the clap_derive macro instead of gumdrop.

Caveats

Usage of flags like -h and --version in subcommands is ambiguous with the flags automatically supported by clap for conventional help and version printouts. This is highlighted by FIXME comments and clap settings disabling the automatically generated flags for these select subcommands. --help is also not available for the subcommands that use -h for something else, because the clap setting can only suppress both long and short help flags.

gumdrop is still used by tendermint-testgen, so it's not removed from the dependency tree (there's one version less of it now, though)


For contributor use:

  • Added a changelog entry, using unclog.
  • If applicable: Unit tests written, added test to CI.
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Updated relevant documentation (docs/) and code comments.
  • Re-reviewed Files changed in the Github PR explorer.

@mzabaluev
Copy link
Contributor Author

mzabaluev commented Nov 18, 2021

Problems to be resolved:

  • It panics on any valid command that is not internally supported by clap, with some hoo-ha about setting the color theme twice.
  • The listen flag --event does not work the intended way due to a current limitation in clap: clap_derive: Vec/Option<Vec> behavior is inconsistent with other types clap-rs/clap#1772
  • abscissa_core 0.6.0-beta.1 wants clap =3.0.0-beta.5, which is not compatible with the requirements of modelator in either 0.2.1 or 0.3.1 versions.
  • Tests panic with 'couldn't install color-eyre: cannot install provided ErrorHook, a hook has already been installed'
  • An e2e test workflow fails with an error in decoding JSON output of a hermes command.

@adizere
Copy link
Member

adizere commented Nov 18, 2021

I had a cursory look over the PR. Great job so far, Mikhail!

I have a question regarding dependency management: is it appropriate for ibc-rs to release a version where we pin on a -beta dependency, concretely for abscissa and clap? I'm thinking in terms of risks of API instability of these beta versions. We'd like to avoid depending on APIs that shift too much, but not sure if this could be a problem here. WDYT?

@mzabaluev
Copy link
Contributor Author

I have a question regarding dependency management: is it appropriate for ibc-rs to release a version where we pin on a -beta dependency, concretely for abscissa and clap? I'm thinking in terms of risks of API instability of these beta versions.

As these are dependencies of the hermes executable and some tests, but not of the libraries, and the exact versions are locked in Cargo.lock, it's probably OK.

@mzabaluev mzabaluev force-pushed the mikhail/abscissa-0.6-with-clap branch 2 times, most recently from 9b75c6d to dd8c116 Compare November 22, 2021 20:33
@mzabaluev mzabaluev marked this pull request as ready for review November 22, 2021 20:57
@mzabaluev mzabaluev requested a review from adizere as a code owner November 22, 2021 20:57
@mzabaluev mzabaluev force-pushed the mikhail/abscissa-0.6-with-clap branch from 22e2ad8 to 7683fa4 Compare November 22, 2021 21:03
mzabaluev and others added 12 commits November 23, 2021 17:27
Overhaul command line argument processing to use clap instead of
gumdrop.
Need this to resolve the dependency conflict on clap.
Does not compile because of the API break.
The version string for chain config memos that was previously obtained
from abscissa is obtained with the crate_version! macro from clap.
Tests rely on the `version` subcommand being implemented.
This is cumbersome, but it's the only way to support multiple
occurrences of --event flag as of clap 3.0.0-beta.5.
Should be fixed by the clap 3.0.0 release.
To avoid a panic in the terminal component initialization over
an attempt to install a global eyre handler after the application has
already done so, we need to tell the terminal to not use colors.
@mzabaluev mzabaluev force-pushed the mikhail/abscissa-0.6-with-clap branch from e3451fe to 5cbf1be Compare November 23, 2021 15:32
@epage
Copy link

epage commented Nov 23, 2021

I came across this from the backlink in clap 1772. I'm curious, any feedback on clap in making this transition?

@mzabaluev
Copy link
Contributor Author

mzabaluev commented Nov 23, 2021

I came across this from the backlink in clap 1772. I'm curious, any feedback on clap in making this transition?

Overall, It's been smooth and, as previously experienced with clap 2.0/structopt, we got a lot of extra polish and convenience for free.

The derive macro is not documented in detail in the clap crate, so I had to extrapolate from the structopt docs.

One badly needed fix is clap-rs/clap#1772. I had to look into the code of clap-derive to understand how to manually write the Args and FromArgMatches impls in order to get multiple_occurrences(true) where it was needed. These traits are scantily documented and perhaps not intended to be implemented by hand.

It would be nice to have settings to disable only the small or the long flag for version and help: we've got subcommands that use -h for something else, but --help could still be auto-generated on the same command. Likewise there's one subcommand with a long --version flag, but that should arguably be renamed to unambiguously refer to the subject entity; also, clap-provided --version is not supported on subcommands by default.

Thank you @epage for your hard work!

@mzabaluev mzabaluev added I: CLI Internal: related to the relayer's CLI O: new-feature Objective: cause to add a new feature or support O: usability Objective: cause to improve the user experience (UX) and ease using the product labels Nov 23, 2021
@epage
Copy link

epage commented Nov 23, 2021

The derive macro is not documented in detail in the clap crate, so I had to extrapolate from the structopt docs.

Yes, this will be addressed before 3.0.

One badly needed fix is clap-rs/clap#1772. I had to look into the code of clap-derive to understand how to manually write the Args and FromArgMatches impls in order to get multiple_occurrences(true) where it was needed.

Mind providing feedback on what you want to see from the derives? I have clap-rs/clap#2993 as one proposal.

These traits are scantily documented and perhaps not intended to be implemented by hand.

I've been trying to move in this direction, trying to meet the goals:

  • crates like clap-verbosity-flag are usable by non-derive users
  • For build-time conscience, allow developers of crates like the above to hand-implement the traits so builder users don't have to pull in the proc macro machinery.

All the breaking changes to meet these goals are complete. The rest is in documentation which is a lower priority than just getting the derives out in a stable release.

Any feedback on what you want in documentation is welcome!

It would be nice to have settings to disable only the small or the long flag for version and help: we've got subcommands that use -h for something else, but --help could still be auto-generated on the same command.

Whats the use case for a non-help -h? Seems like -h is too tightly associated with help to reuse.

As a hack, you could re-assign the short with mut_arg("help", |a| a.short('t')). I've been contemplating having more functions take impl Into<Option<T>> so you can clear things make to default, like App::help_heading

also, clap-provided --version is not supported on subcommands by default.

Its been a while since I've dug into this part, is this just an issue of setting version or setting PropagateVersion

@mzabaluev mzabaluev changed the title Update to abscissa 0.6.0-beta.1 and clap 0.3 Update to abscissa 0.6.0-beta.1 and clap 3.0 Nov 24, 2021
@romac romac self-assigned this Nov 24, 2021
@mzabaluev
Copy link
Contributor Author

@epage

Mind providing feedback on what you want to see from the derives? I have clap-rs/clap#2993 as one proposal.

I support the changes outlined there. The main thing is, a Vec should be treated with multiple_occurrences(true).

There may be also a need to override the special type treatment and let the user provide a custom accumulating parser/validator for the strings produced by ArgMatches::values_of and friends. This could be also useful for user-supplied collection types that the macro does not recognize as special. But this could be done later.

Any feedback on what you want in documentation is welcome!

I think basic examples of hand-written implementation for Args and FromArgMatches would help a lot.

Whats the use case for a non-help -h? Seems like -h is too tightly associated with help to reuse.

Well, folks here have been using it as the short for --height without giving much thought to it 😁 But I agree, this could be construed as being at odds with the common convention, and the non-help short option should be renamed to -H to avoid ambiguity.

is this just an issue of setting version or setting PropagateVersion

Sorry, I was trying to say that the application-specified --version in a subcommand was not a problem in this case as the automatic top-level --version is not propagated. Still, the chosen name is confusing and IMHO --version should be reserved to just the version printout case.

@romac
Copy link
Member

romac commented Nov 24, 2021

But I agree, this could be construed as being at odds with the common convention, and the non-help short option should be renamed to -H to avoid ambiguity.

👍

@romac romac changed the title Update to abscissa 0.6.0-beta.1 and clap 3.0 Update to abscissa 0.6.0-beta.1 and clap 3.0.0-beta.5 Nov 24, 2021
@romac
Copy link
Member

romac commented Nov 24, 2021

Sorry, I was trying to say that the application-specified --version in a subcommand was not a problem in this case as the automatic top-level --version is not propagated. Still, the chosen name is confusing and IMHO --version should be reserved to just the version printout case.

@mzabaluev Can you point me to the place where we rely on --version being available in a subcommand?

@mzabaluev
Copy link
Contributor Author

Can you point me to the place where we rely on --version being available in a subcommand?

Sure, I have annotated it with a FIXME: https://github.com/informalsystems/ibc-rs/blob/c33820497a319779c4de4c52a0f0038298be1722/relayer-cli/src/commands/create/channel.rs#L60

To avoid confusion with the --version flag conventionally used to print
the program version, the new long name is --channel-version.
The --version flag is still supported with the present meaning on the
`create channel` subcommand, but this alias is no longer described
in built-in help.
Copy link
Member

@romac romac left a comment

Choose a reason for hiding this comment

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

Let's revert the guide updates and do them in the release PR, otherwise the guide will be out of sync with the latest published version of the relayer since it's published automatically from master, cf. #609

guide/src/commands/path-setup/channels.md Outdated Show resolved Hide resolved
guide/src/help.md Outdated Show resolved Hide resolved
guide/src/help.md Outdated Show resolved Hide resolved
Copy link
Member

@romac romac left a comment

Choose a reason for hiding this comment

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

@mzabaluev Thank you so much for stepping up to this tedious task 🌻

@romac romac merged commit 02776c8 into master Dec 1, 2021
@romac romac deleted the mikhail/abscissa-0.6-with-clap branch December 1, 2021 10:28
@mzabaluev
Copy link
Contributor Author

@romac No problem, some light hazing for a newcomer 😁

hu55a1n1 pushed a commit to hu55a1n1/hermes that referenced this pull request Sep 13, 2022
…s#1576)

* Update to abscissa 0.6.0-beta.1 and clap 0.3

Overhaul command line argument processing to use clap instead of
gumdrop.

* Attempt to update to modelator 0.3.2

Need this to resolve the dependency conflict on clap.
Does not compile because of the API break.

* refactor modelator code with latest api

* cli: De-hardcode crate version

The version string for chain config memos that was previously obtained
from abscissa is obtained with the crate_version! macro from clap.

* hermes: Fix panic in terminal color initialization

* Resurrect the version subcommand

Tests rely on the `version` subcommand being implemented.

* Manually implement clap::Parser for listen command

This is cumbersome, but it's the only way to support multiple
occurrences of --event flag as of clap 3.0.0-beta.5.
Should be fixed by the clap 3.0.0 release.

* Add changelog entry

* Fix `default_value` for `Order` in a couple commands

* Fix handling of timeout options in `ft-transfer`

* Wait a bit longer in passive connection test to avoid spurious failures

* cli: Suppress terminal color in abscissa

To avoid a panic in the terminal component initialization over
an attempt to install a global eyre handler after the application has
already done so, we need to tell the terminal to not use colors.

* Improve info message in `listen` command

* Rename --version flag of `create channel` cmd

To avoid confusion with the --version flag conventionally used to print
the program version, the new long name is --channel-version.
The --version flag is still supported with the present meaning on the
`create channel` subcommand, but this alias is no longer described
in built-in help.

* Updated the guide on availability of --help flags

* Revert guide updates, to be done in the release PR

Co-authored-by: Ranadeep Biswas <[email protected]>
Co-authored-by: Romain Ruetschi <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I: CLI Internal: related to the relayer's CLI O: new-feature Objective: cause to add a new feature or support O: usability Objective: cause to improve the user experience (UX) and ease using the product
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Relayer commands don't support help menu for subcommands
5 participants