upgrade clap to v4.5 #5273

eserilev · 2024-02-20T21:52:53Z

Issue Addressed

Proposed Changes

This PR upgrades clap from version 2.x to 4.5.1. There are some breaking changes between the two versions, v4.x is a bit stricter when it comes to flag definitions and has a slightly different api. Once we upgrade clap to v4 we can begin working on migrating to clap derive, which gives us the following benefits

easier to read cli flag definitions
a bit more type safety/compile time guarantees
less verbose initialization logic
an easy interface to enable reading cli flags from a config file

We'll migrate to clap derive in separate PR's on a crate by crate basis, to make the PRs a bit more manageable

List of breaking changes:

latency-measurement-service flag is now deprecated in favor of disable-latency-measurement-service. By default the latency measurement service is enabled, and can be disabled by providing the new disable-latency-measurement-service flag.
The self-limiter flag no longer accepts arguments. Providing the self-limiter flag enables the outbound rate limiter service. A new flag self-limiter-protocols is introduced to allow for configuring the outbound rate limiter service per protocol. In order to use the new self-limiter-protocols flag, self-limiter must also be provided.
The inbound-rate-limiter flag is now deprecated in favor of disable-inbound-rate-limiter. By default the inbound rate limiter service is enabled and can be disabled by providing the new disable-inbound-rate-limiter flag. Also, an additional flag inbound-rate-limiter-protocols is introduced to allow for configuring the inbound rate limiter service per protocol. Note that the disable-inbound-rate-limiter flag conflicts with the inbound-rate-limiter-protocols flag, they cannot be used in tandem.
slasher-broadcast now requires either a true or false value. The flag cannot be used without providing a value.
The above should cover the big breaking changes introduced by this PR. However its important to note that cli flag definitions are a bit stricter now. Anything listed under Options in the help screen must have an argument provided. Anything listed under Flags in the help screen cannot have an argument provided. If those conditions are violated the program will fail and an error message will be returned specifying what cli rules were violated.

eserilev · 2024-02-20T21:53:23Z

There are still roughly 10 failing tests here that I need to resolve

…grade-clap-cli

validator_client/src/cli.rs

beacon_node/src/cli.rs

eserilev · 2024-02-23T07:49:48Z

Tests are passing, this should be ready for a review

…grade-clap-cli

AgeManning

I had a quick skim. Looks good to me!

I'm of the opinion that we make all the breaking changes we need right now. Adjust all the CLI flags we need, regardless if they break.

I'll try and do a follow up PR which tries to hide some flags and remove deprecated flags, then in the next release we can warn users of breaking CLI changes.

beacon_node/src/cli.rs

beacon_node/src/config.rs

boot_node/src/cli.rs

testing/simulator/src/cli.rs

account_manager/src/wallet/create.rs

beacon_node/src/cli.rs

boot_node/src/cli.rs

common/clap_utils/src/lib.rs

lighthouse/src/main.rs

testing/simulator/src/cli.rs

validator_manager/src/create_validators.rs

chong-he · 2024-04-04T00:47:22Z

I have done some testing on this and some comments below (current refers to the help texts that we have now, while new refers to the help texts from this PR):

The help texts work on both Linux and Windows. The help texts are displaying for all commands and subcommands, with green colour on the flag itself, and white colour for the description. Only some current help texts (such as bn and vc) have this colour distinction, while other commands like lighthouse a validator –help and lighthouse a wallet –help do not. So this is a great improvement.
The current help texts always generate the commit in the first line, in which @macladson removed it in the cli.sh when putting them in the Lighthouse Book, as if otherwise the cli-check would always fail. The new help texts do not have this commit in the first line anymore, and as such the removal of first line is no longer needed. If we run make cli-local now, the help texts will always start with this:

Usage: lighthouse beacon_node [OPTIONS]

which is not desirable as the important descriptive first line is removed. To solve this, we remove these lines:

lighthouse/scripts/cli.sh

Lines 15 to 16 in 3058b96

    
           # Remove first line of cmd to get rid of commit specific numbers. 
        
           cmd=${cmd#*$'\n'}

I have tested that after removing the above in cli.sh and running make cli-local, all help texts are reserved and shown.

Currently, the flags are separated into 2 groups: flags (do not require a value), options (require a value). With this PR, all flags are listed under options only. Is this an expected change?
Currently the flags are arranged in alphabetical order. I am not sure in what order is the new help texts arranged? For example, in lighthouse bn –help, it starts with the flags --network-dir, then --freezer-dir and --logfile which does not seem follow a certain order? It would be nice if they follow a certain order, e.g., the alphabetical order.
The new help texts do not start a new line until reaching the monitor width. This unfortunately breaks the word when starting a new line, referring to the screenshot below. The words exceeds and attestations are broken into two lines.

The current help texts do not have this problem because it starts a new line after a certain number of words. Because the new help texts is shown as a single line, when it is displayed in Lighthouse book, it will have a long horizontal scroll at the bottom, which is not very UX friendly:

I wonder if it is a good idea to break into new lines after reaching a certain number of words, like the current help texts?

Thanks!

beacon_node/src/config.rs

dapplion · 2024-04-04T06:00:41Z

beacon_node/src/config.rs

-            // Enabled by default, with default values
+    // The inbound rate limiter is enabled by default unless `disabled` via the
+    // `disable-inbound-rate-limiter` flag.
+    config.inbound_rate_limiter_config = if parse_flag(cli_args, "disable-inbound-rate-limiter") {


Changing from inbound-rate-limiter to disable-inbound-rate-limiter appears to be a breaking change on the beacon subcommand, is it necessary?

AFAIU the current flag usage is to disable is --inbound-rate-limiter=disable?

Currently this cli param is "bad" because it tries to do too many things. By default its value is set to true. if the cli param is provided without a value, its treated as true. On top of this the param can either accept the value disable OR a ; separated list of protocol values w/ the following format: <protocol_name>:<tokens>/<time_in_seconds>

Technically these breaking changes aren't necessary, however the new version of clap is a bit stricter with cli param definitions. We now either define the cli param as accepting a value (ArgAction::Set) or as a bool flag (ArgAction::SetTrue). The existing functionality requires us to treat this cli param as a hybrid of Set and SetTrue like so

.default_missing_value("true") .require_equals(true) .num_args(0..1)

which is also technically a breaking change since the cli param now requires an = sign if the user tries to provide it a value (currently the = isn't necessary). I think its a bit nicer to just keep all cli params as either Set or SetTrue, but I'm open to the hybrid option if we decide this breaking change isnt worth it.

Makes sense. If @AgeManning blesses this breaking change I'm okay with it

Yeah I'm down for breaking changes if it conforms better to the new clap. Makes sense to me.

AgeManning · 2024-05-07T04:42:36Z

Should we get this in for 5.2?

…grade-clap-cli

michaelsproul

LGTM.

I would like to see a collected list of breaking changes that we can add to the release notes.

Cargo.lock

lighthouse/tests/beacon_node.rs

eserilev · 2024-05-20T15:45:42Z

i'll put together a list of breaking changes

Squashed commit of the following: commit 9a39467 Author: Eitan Seri-Levi <[email protected]> Date: Mon May 20 07:24:00 2024 +0100 cli commit aa4334f Merge: 8180839 b5de925 Author: Eitan Seri-Levi <[email protected]> Date: Mon May 20 06:57:20 2024 +0100 Merge branch 'unstable' of https://github.com/sigp/lighthouse into upgrade-clap-cli commit 8180839 Author: Eitan Seri-Levi <[email protected]> Date: Tue May 14 12:24:53 2024 +0300 retry cli check commit 3c26b9a Author: Eitan Seri-Levi <[email protected]> Date: Tue May 14 12:24:39 2024 +0300 retry cli check commit 61da93f Author: Eitan Seri-Levi <[email protected]> Date: Tue May 14 12:10:10 2024 +0300 cli check commit f8a8536 Author: Eitan Seri-Levi <[email protected]> Date: Tue May 14 12:09:54 2024 +0300 cli check commit 0be7b71 Author: Eitan Seri-Levi <[email protected]> Date: Tue May 14 11:50:10 2024 +0300 md changes commit c5a3058 Author: Eitan Seri-Levi <[email protected]> Date: Sun May 12 15:18:48 2024 +0300 book changes commit 82d1bc3 Merge: be90ac7 6d792b4 Author: Eitan Seri-Levi <[email protected]> Date: Sun May 12 15:04:52 2024 +0300 resolve merge conflicts commit be90ac7 Author: Eitan Seri-Levi <[email protected]> Date: Tue Apr 30 17:35:55 2024 +0300 fix eth sim cli commit b8bea91 Author: Eitan Seri-Levi <[email protected]> Date: Sat Apr 27 15:37:41 2024 +0300 require at least one arg commit 6a81d12 Author: Eitan Seri-Levi <[email protected]> Date: Sat Apr 27 15:26:49 2024 +0300 revert simulator changes commit c6b9712 Merge: de33110 13f94ef Author: Eitan Seri-Levi <[email protected]> Date: Fri Apr 26 11:00:48 2024 +0300 merge conflicts commit de33110 Author: Eitan Seri-Levi <[email protected]> Date: Fri Apr 26 10:57:07 2024 +0300 resolve merge conflicts commit ac451a8 Author: Eitan Seri-Levi <[email protected]> Date: Thu Apr 25 23:43:24 2024 +0300 revert commit f5f114f Author: Eitan Seri-Levi <[email protected]> Date: Thu Apr 25 23:41:29 2024 +0300 revert commit fa7c4b3 Merge: bd4e5c9 3203456 Author: Eitan Seri-Levi <[email protected]> Date: Thu Apr 25 23:40:42 2024 +0300 merge commit bd4e5c9 Author: Eitan Seri-Levi <[email protected]> Date: Tue Apr 16 01:22:14 2024 +0300 fmt commit 7f4af13 Author: Eitan Seri-Levi <[email protected]> Date: Tue Apr 16 01:06:16 2024 +0300 help text, text width, and a few flag fixes commit e23b96b Author: Eitan Seri-Levi <[email protected]> Date: Fri Apr 12 10:52:28 2024 +0300 md commit 0a624c9 Author: Eitan Seri-Levi <[email protected]> Date: Fri Apr 12 10:35:46 2024 +0300 merge conflicts commit d19c9e7 Merge: 75c2c72 6bac5ce Author: Eitan Seri-Levi <[email protected]> Date: Fri Apr 12 10:23:55 2024 +0300 merge conflict commit 75c2c72 Author: Eitan Seri-Levi <[email protected]> Date: Fri Apr 12 10:20:02 2024 +0300 md files commit 568bf79 Author: realbigsean <[email protected]> Date: Wed Apr 10 17:28:27 2024 -0400 cli help updates commit ebdc22b Merge: a4c4bb5 54fbdda Author: realbigsean <[email protected]> Date: Wed Apr 10 17:21:44 2024 -0400 Merge branch 'unstable' of https://github.com/sigp/lighthouse into upgrade-clap-cli commit a4c4bb5 Author: realbigsean <[email protected]> Date: Wed Apr 10 15:54:49 2024 -0400 cli help updates commit 51c4419 Author: Eitan Seri-Levi <[email protected]> Date: Tue Apr 9 23:17:54 2024 +0300 update commit 9de4980 Author: Eitan Seri-Levi <[email protected]> Date: Tue Apr 9 23:16:22 2024 +0300 remove -e files commit 71ff732 Author: Eitan Seri-Levi <[email protected]> Date: Tue Apr 9 23:14:47 2024 +0300 update commit e4a4b88 Author: Eitan Seri-Levi <[email protected]> Date: Mon Apr 8 08:36:31 2024 +0300 make cli commit 8911e3c Author: Eitan Seri-Levi <[email protected]> Date: Fri Apr 5 08:57:49 2024 +0300 make cli commit daec018 Author: Eitan Seri-Levi <[email protected]> Date: Fri Apr 5 08:49:00 2024 +0300 fix broken flag commit 35bdf5e Author: Eitan Seri-Levi <[email protected]> Date: Fri Apr 5 00:06:02 2024 +0300 fmt got me again commit 4555fb2 Author: Eitan Seri-Levi <[email protected]> Date: Thu Apr 4 23:55:55 2024 +0300 revert removed if statement commit 71edfe8 Author: Eitan Seri-Levi <[email protected]> Date: Thu Apr 4 23:41:03 2024 +0300 fix test commit 0dd6423 Merge: 30556d0 b65daac Author: Eitan Seri-Levi <[email protected]> Date: Thu Apr 4 23:10:52 2024 +0300 resolve conflicts commit 30556d0 Author: Eitan Seri-Levi <[email protected]> Date: Thu Apr 4 23:05:22 2024 +0300 fix test commit f6653b0 Merge: 0c92aaa feb531f Author: Eitan Seri-Levi <[email protected]> Date: Thu Apr 4 21:49:31 2024 +0300 resolve merge conflict commit 0c92aaa Author: Eitan Seri-Levi <[email protected]> Date: Thu Apr 4 21:48:22 2024 +0300 alphabetic order commit e320c99 Author: Eitan Seri-Levi <[email protected]> Date: Thu Apr 4 21:06:39 2024 +0300 update commit 9b40a14 Author: Eitan Seri-Levi <[email protected]> Date: Thu Apr 4 00:56:37 2024 +0300 fmt commit fb3b8dd Author: Eitan Seri-Levi <[email protected]> Date: Thu Apr 4 00:52:05 2024 +0300 remove unneeded check commit 7b50109 Author: Eitan Seri-Levi <[email protected]> Date: Wed Apr 3 23:23:59 2024 +0300 add custom flag parser, make rate limiter flags clap friendly commit 7b558bd Author: Eitan Seri-Levi <[email protected]> Date: Wed Apr 3 15:02:58 2024 +0300 add num args, version commit d00855e Author: Eitan Seri-Levi <[email protected]> Date: Wed Apr 3 02:12:02 2024 +0300 resolve beta compiler issue commit 88f083a Merge: f4447b7 969d12d Author: Eitan Seri-Levi <[email protected]> Date: Wed Apr 3 01:50:11 2024 +0300 merge conflicts commit f4447b7 Author: Eitan Seri-Levi <[email protected]> Date: Wed Mar 13 15:42:14 2024 +0200 fix eth sim commit 1f45d1f Author: Eitan Seri-Levi <[email protected]> Date: Wed Mar 13 15:24:33 2024 +0200 fix eth sim commit acc8c6d Author: Eitan Seri-Levi <[email protected]> Date: Tue Mar 12 20:02:12 2024 +0200 default --format val commit c3d6ede Author: Eitan Seri-Levi <[email protected]> Date: Tue Mar 12 17:31:58 2024 +0200 merge unstable commit b34b11d Merge: e05b856 2a3c709 Author: Eitan Seri-Levi <[email protected]> Date: Tue Mar 12 17:12:10 2024 +0200 Merge branch 'unstable' of https://github.com/sigp/lighthouse into upgrade-clap-cli commit e05b856 Author: Eitan Seri-Levi <[email protected]> Date: Sat Feb 24 09:33:28 2024 +0200 value parser for mnemonic commit 4743a2c Merge: 1de4880 b5bae6e Author: Eitan Seri-Levi <[email protected]> Date: Thu Feb 22 09:14:23 2024 +0200 Merge branch 'unstable' of https://github.com/sigp/lighthouse into upgrade-clap-cli commit 1de4880 Author: Eitan Seri-Levi <[email protected]> Date: Thu Feb 22 09:14:20 2024 +0200 cli fixes commit 3b8fd4a Author: Eitan Seri-Levi <[email protected]> Date: Tue Feb 20 23:46:40 2024 +0200 upgrade clap to v4.5

michaelsproul · 2024-05-28T05:18:09Z

@Mergifyio queue

mergify · 2024-05-28T05:18:33Z

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at df983a8

upgrade clap to v4.5

3b8fd4a

eserilev mentioned this pull request Feb 20, 2024

Upgrade clap and move to derive #5231

Closed

jimmygchen added the work-in-progress PR is a work-in-progress label Feb 21, 2024

eserilev added 2 commits February 22, 2024 09:14

cli fixes

1de4880

Merge branch 'unstable' of https://github.com/sigp/lighthouse into up…

4743a2c

…grade-clap-cli

eserilev commented Feb 23, 2024

View reviewed changes

validator_client/src/cli.rs Outdated Show resolved Hide resolved

eserilev commented Feb 23, 2024

View reviewed changes

beacon_node/src/cli.rs Outdated Show resolved Hide resolved

value parser for mnemonic

e05b856

realbigsean added ready-for-review The code is ready for review and removed work-in-progress PR is a work-in-progress labels Mar 7, 2024

eserilev added 7 commits March 12, 2024 17:12

Merge branch 'unstable' of https://github.com/sigp/lighthouse into up…

b34b11d

…grade-clap-cli

merge unstable

c3d6ede

default --format val

acc8c6d

fix eth sim

1f45d1f

fix eth sim

f4447b7

merge conflicts

88f083a

resolve beta compiler issue

d00855e

AgeManning reviewed Apr 2, 2024

View reviewed changes

dapplion reviewed Apr 3, 2024

View reviewed changes

eserilev added 4 commits April 3, 2024 15:02

add num args, version

7b558bd

add custom flag parser, make rate limiter flags clap friendly

7b50109

remove unneeded check

fb3b8dd

fmt

9b40a14

dapplion reviewed Apr 4, 2024

View reviewed changes

eserilev added 3 commits April 4, 2024 21:06

update

e320c99

alphabetic order

0c92aaa

resolve merge conflict

f6653b0

eserilev added 2 commits April 27, 2024 15:37

require at least one arg

b8bea91

fix eth sim cli

be90ac7

eserilev added 9 commits May 12, 2024 15:04

resolve merge conflicts

82d1bc3

book changes

c5a3058

md changes

0be7b71

cli check

f8a8536

cli check

61da93f

retry cli check

3c26b9a

retry cli check

8180839

Merge branch 'unstable' of https://github.com/sigp/lighthouse into up…

aa4334f

…grade-clap-cli

cli

9a39467

michaelsproul approved these changes May 20, 2024

View reviewed changes

Cargo.lock Show resolved Hide resolved

lighthouse/tests/beacon_node.rs Show resolved Hide resolved

michaelsproul added the v5.2.0 Q2 2024 label May 20, 2024

eserilev mentioned this pull request May 21, 2024

Update Criterion #5819

Closed

michaelsproul mentioned this pull request May 23, 2024

Release v5.2.0 #5664

Merged

michaelsproul added 3 commits May 28, 2024 09:53

Merge remote-tracking branch 'origin/unstable' into upgrade-clap-cli

c54e39a

Update CLI docs for Goerli removal

5e0f2df

Fix cargo lock

23b0c70

michaelsproul added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels May 28, 2024

mergify bot added a commit that referenced this pull request May 28, 2024

Merge of #5273

3b15cf9

mergify bot mentioned this pull request May 28, 2024

merge queue: embarking unstable (6a7305a) and #5273 together #5851

Closed

5 tasks

mergify bot merged commit df983a8 into sigp:unstable May 28, 2024
29 checks passed

michaelsproul mentioned this pull request Jun 7, 2024

Undo some CLI flag breakages #5902

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

upgrade clap to v4.5 #5273

upgrade clap to v4.5 #5273

eserilev commented Feb 20, 2024 •

edited

Loading

eserilev commented Feb 20, 2024

eserilev commented Feb 23, 2024

AgeManning left a comment

chong-he commented Apr 4, 2024

dapplion Apr 4, 2024

eserilev Apr 4, 2024

dapplion Apr 6, 2024 •

edited

Loading

AgeManning May 22, 2024

AgeManning commented May 7, 2024

michaelsproul left a comment

eserilev commented May 20, 2024

michaelsproul commented May 28, 2024

mergify bot commented May 28, 2024 •

edited

Loading

upgrade clap to v4.5 #5273

upgrade clap to v4.5 #5273

Conversation

eserilev commented Feb 20, 2024 • edited Loading

Issue Addressed

Proposed Changes

List of breaking changes:

eserilev commented Feb 20, 2024

eserilev commented Feb 23, 2024

AgeManning left a comment

Choose a reason for hiding this comment

chong-he commented Apr 4, 2024

dapplion Apr 4, 2024

Choose a reason for hiding this comment

eserilev Apr 4, 2024

Choose a reason for hiding this comment

dapplion Apr 6, 2024 • edited Loading

Choose a reason for hiding this comment

AgeManning May 22, 2024

Choose a reason for hiding this comment

AgeManning commented May 7, 2024

michaelsproul left a comment

Choose a reason for hiding this comment

eserilev commented May 20, 2024

michaelsproul commented May 28, 2024

mergify bot commented May 28, 2024 • edited Loading

✅ The pull request has been merged automatically

eserilev commented Feb 20, 2024 •

edited

Loading

dapplion Apr 6, 2024 •

edited

Loading

mergify bot commented May 28, 2024 •

edited

Loading