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

mzabaluev · 2021-11-18T10:14:38Z

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 · 2021-11-18T10:18:56Z

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 · 2021-11-18T14:18:43Z

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 · 2021-11-18T15:29:46Z

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.

relayer-cli/src/bin/hermes/main.rs

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.

epage · 2021-11-23T19:10:08Z

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

mzabaluev · 2021-11-23T22:23:29Z

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!

epage · 2021-11-23T23:02:47Z

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 · 2021-11-24T09:42:55Z

@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 · 2021-11-24T09:54:20Z

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 · 2021-11-24T09:58:19Z

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 · 2021-11-24T10:14:35Z

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.

romac

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

guide/src/help.md

romac

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

mzabaluev · 2021-12-01T10:30:11Z

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

…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 <mail@rnbguy.at> Co-authored-by: Romain Ruetschi <romain@informal.systems>

mzabaluev requested review from romac and ancazamfir November 18, 2021 10:14

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 marked this pull request as ready for review November 22, 2021 20:57

mzabaluev requested a review from adizere as a code owner November 22, 2021 20:57

mzabaluev force-pushed the mikhail/abscissa-0.6-with-clap branch from 22e2ad8 to 7683fa4 Compare November 22, 2021 21:03

mzabaluev commented Nov 22, 2021

View reviewed changes

relayer-cli/src/bin/hermes/main.rs Outdated Show resolved Hide resolved

mzabaluev mentioned this pull request Nov 22, 2021

An option to seed the entry from GitHub informalsystems/unclog#28

Open

mzabaluev and others added 12 commits November 23, 2021 17:27

Update to abscissa 0.6.0-beta.1 and clap 0.3

f7236a1

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

Attempt to update to modelator 0.3.2

4d6818b

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

refactor modelator code with latest api

4a6c87b

cli: De-hardcode crate version

2e8a724

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

20e6f45

Resurrect the version subcommand

37bb743

Tests rely on the `version` subcommand being implemented.

Manually implement clap::Parser for listen command

0087287

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

35835da

Fix default_value for Order in a couple commands

92ed854

Fix handling of timeout options in ft-transfer

8ac4626

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

9c58777

cli: Suppress terminal color in abscissa

5cbf1be

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 force-pushed the mikhail/abscissa-0.6-with-clap branch from e3451fe to 5cbf1be Compare November 23, 2021 15:32

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

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 self-assigned this Nov 24, 2021

romac mentioned this pull request Nov 24, 2021

Upgrade gaia release on CI and allow multiple e2e tests (current and future release) #1557

Merged

5 tasks

Improve info message in listen command

c338204

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

mzabaluev mentioned this pull request Nov 24, 2021

Rename CLI options ambiguous with clap-provided -h/--help and -V/--version #1611

Closed

mzabaluev added 3 commits November 25, 2021 11:53

Merge branch 'master' into mikhail/abscissa-0.6-with-clap

0ff2d52

Updated the guide on availability of --help flags

7e9c0f0

mzabaluev added the needs guide update label Nov 25, 2021

romac requested changes Dec 1, 2021

View reviewed changes

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

Revert guide updates, to be done in the release PR

2690ba8

romac approved these changes Dec 1, 2021

View reviewed changes

romac merged commit 02776c8 into master Dec 1, 2021

romac deleted the mikhail/abscissa-0.6-with-clap branch December 1, 2021 10:28

mzabaluev mentioned this pull request Jan 4, 2022

Disable clap-provided -h flags on two subcommands #1739

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

mzabaluev commented Nov 18, 2021 •

edited by romac

Loading

mzabaluev commented Nov 18, 2021 •

edited

Loading

adizere commented Nov 18, 2021

mzabaluev commented Nov 18, 2021

epage commented Nov 23, 2021

mzabaluev commented Nov 23, 2021 •

edited

Loading

epage commented Nov 23, 2021

mzabaluev commented Nov 24, 2021

romac commented Nov 24, 2021

romac commented Nov 24, 2021

mzabaluev commented Nov 24, 2021

romac left a comment

romac left a comment

mzabaluev commented Dec 1, 2021

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

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

Conversation

mzabaluev commented Nov 18, 2021 • edited by romac Loading

Description

Caveats

mzabaluev commented Nov 18, 2021 • edited Loading

adizere commented Nov 18, 2021

mzabaluev commented Nov 18, 2021

epage commented Nov 23, 2021

mzabaluev commented Nov 23, 2021 • edited Loading

epage commented Nov 23, 2021

mzabaluev commented Nov 24, 2021

romac commented Nov 24, 2021

romac commented Nov 24, 2021

mzabaluev commented Nov 24, 2021

romac left a comment

Choose a reason for hiding this comment

romac left a comment

Choose a reason for hiding this comment

mzabaluev commented Dec 1, 2021

mzabaluev commented Nov 18, 2021 •

edited by romac

Loading

mzabaluev commented Nov 18, 2021 •

edited

Loading

mzabaluev commented Nov 23, 2021 •

edited

Loading