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

Non-release profiles of lcli panic by default due to argument processing failure #4488

Closed
jmcph4 opened this issue Jul 11, 2023 · 0 comments
Closed
Assignees
Labels
bug Something isn't working low-hanging-fruit Easy to resolve, get it before someone else does! v4.4.1 ETA August 2023

Comments

@jmcph4
Copy link
Member

jmcph4 commented Jul 11, 2023

Description

When invoked in non-release profiles, lcli panics by default during argument parsing. This is most likely due to an error in Clap: clap-rs/clap#1546.

Credit to @jimmygchen for the discovery.

Version

$ rustc --version
rustc 1.70.0 (90c541806 2023-05-31)
$ git rev-parse HEAD
dfcb3363c757671eb19d5f8e519b4b94ac74677a
$ cargo run --bin lighthouse -- --version
Lighthouse v4.3.0-dfcb336
BLS library: blst
SHA256 hardware acceleration: true
Allocator: system
Profile: debug
Specs: mainnet (true), minimal (false), gnosis (false)

Present Behaviour

$ cargo run --profile dev --bin lcli
thread 'main' panicked at 'Global arguments cannot be required.

	'spec' is marked as global and required', /home/jmcph4/.cargo/registry/src/index.crates.io-6f17d22bba15001f/clap-2.34.0/src/app/parser.rs:208:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
$ echo $?
101

Expected Behaviour

Lighthouse should not panic on this condition, but instead display the default help text and return a 1 exit code. Compare the above behaviour to the release profile behaviour:

$ cargo run --profile release --bin lcli
Failed to run lcli: Unknown subcommand . See --help.
$ echo $?
1

Steps to resolve

As per this comment on the Clap ticket, removing all instances of both required and default_value should work:

diff --git a/lcli/src/main.rs b/lcli/src/main.rs
index d072beaa4..4206a2f47 100644
--- a/lcli/src/main.rs
+++ b/lcli/src/main.rs
@@ -38,7 +38,6 @@ fn main() {
                 .long("spec")
                 .value_name("STRING")
                 .takes_value(true)
-                .required(true)
                 .possible_values(&["minimal", "mainnet", "gnosis"])
                 .default_value("mainnet")
                 .global(true),
@@ -361,7 +360,6 @@ fn main() {
                         .index(2)
                         .value_name("BIP39_MNENMONIC")
                         .takes_value(true)
-                        .required(true)
                         .default_value(
                             "replace nephew blur decorate waste convince soup column \
                             orient excite play baby",
@@ -382,7 +380,6 @@ fn main() {
                         .help("The block hash used when generating an execution payload. This \
                             value is used for `execution_payload_header.block_hash` as well as \
                             `execution_payload_header.random`")
-                        .required(true)
                         .default_value(
                             "0x0000000000000000000000000000000000000000000000000000000000000000",
                         ),
@@ -400,7 +397,6 @@ fn main() {
                         .value_name("INTEGER")
                         .takes_value(true)
                         .help("The base fee per gas field in the execution payload generated.")
-                        .required(true)
                         .default_value("1000000000"),
                 )
                 .arg(
@@ -409,7 +405,6 @@ fn main() {
                         .value_name("INTEGER")
                         .takes_value(true)
                         .help("The gas limit field in the execution payload generated.")
-                        .required(true)
                         .default_value("30000000"),
@jmcph4 jmcph4 added bug Something isn't working v4.4.1 ETA August 2023 low-hanging-fruit Easy to resolve, get it before someone else does! labels Jul 11, 2023
@jmcph4 jmcph4 self-assigned this Jul 11, 2023
bors bot pushed a commit that referenced this issue Jul 11, 2023
## Issue Addressed

#4488 

## Proposed Changes

 - Remove all instances of the `required` modifier where we have a default value specified for a subcommand

## Additional Info

N/A
bors bot pushed a commit that referenced this issue Jul 12, 2023
## Issue Addressed

#4488 

## Proposed Changes

 - Remove all instances of the `required` modifier where we have a default value specified for a subcommand

## Additional Info

N/A
bors bot pushed a commit that referenced this issue Jul 12, 2023
## Issue Addressed

#4488 

## Proposed Changes

 - Remove all instances of the `required` modifier where we have a default value specified for a subcommand

## Additional Info

N/A
bors bot pushed a commit that referenced this issue Jul 12, 2023
## Issue Addressed

#4488 

## Proposed Changes

 - Remove all instances of the `required` modifier where we have a default value specified for a subcommand

## Additional Info

N/A
bors bot pushed a commit that referenced this issue Jul 12, 2023
## Issue Addressed

#4488 

## Proposed Changes

 - Remove all instances of the `required` modifier where we have a default value specified for a subcommand

## Additional Info

N/A
bors bot pushed a commit that referenced this issue Jul 13, 2023
## Issue Addressed

#4488 

## Proposed Changes

 - Remove all instances of the `required` modifier where we have a default value specified for a subcommand

## Additional Info

N/A
bors bot pushed a commit that referenced this issue Jul 13, 2023
## Issue Addressed

#4488 

## Proposed Changes

 - Remove all instances of the `required` modifier where we have a default value specified for a subcommand

## Additional Info

N/A
bors bot pushed a commit that referenced this issue Jul 13, 2023
## Issue Addressed

#4488 

## Proposed Changes

 - Remove all instances of the `required` modifier where we have a default value specified for a subcommand

## Additional Info

N/A
bors bot pushed a commit that referenced this issue Jul 13, 2023
## Issue Addressed

#4488 

## Proposed Changes

 - Remove all instances of the `required` modifier where we have a default value specified for a subcommand

## Additional Info

N/A
bors bot pushed a commit that referenced this issue Jul 13, 2023
## Issue Addressed

#4488 

## Proposed Changes

 - Remove all instances of the `required` modifier where we have a default value specified for a subcommand

## Additional Info

N/A
bors bot pushed a commit that referenced this issue Jul 14, 2023
## Issue Addressed

#4488 

## Proposed Changes

 - Remove all instances of the `required` modifier where we have a default value specified for a subcommand

## Additional Info

N/A
bors bot pushed a commit that referenced this issue Jul 17, 2023
## Issue Addressed

#4488 

## Proposed Changes

 - Remove all instances of the `required` modifier where we have a default value specified for a subcommand

## Additional Info

N/A
@jmcph4 jmcph4 closed this as completed Jul 17, 2023
Woodpile37 pushed a commit to Woodpile37/lighthouse that referenced this issue Jan 6, 2024
## Issue Addressed

sigp#4488 

## Proposed Changes

 - Remove all instances of the `required` modifier where we have a default value specified for a subcommand

## Additional Info

N/A
Woodpile37 pushed a commit to Woodpile37/lighthouse that referenced this issue Jan 6, 2024
## Issue Addressed

sigp#4488 

## Proposed Changes

 - Remove all instances of the `required` modifier where we have a default value specified for a subcommand

## Additional Info

N/A
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working low-hanging-fruit Easy to resolve, get it before someone else does! v4.4.1 ETA August 2023
Projects
None yet
Development

No branches or pull requests

1 participant