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 clap to the latest v4 version #1580

Merged
merged 5 commits into from
Jul 3, 2023
Merged

Update clap to the latest v4 version #1580

merged 5 commits into from
Jul 3, 2023

Conversation

karbyshev
Copy link
Contributor

@karbyshev karbyshev commented Jun 16, 2023

Closes #64

@karbyshev
Copy link
Contributor Author

@tzemanovic
Copy link
Member

@tzemanovic Clap has removed colored help output in v4. https://stackoverflow.com/questions/74068168/clap-rs-not-printing-colors-during-help

That's a shame though overall I think it's being improved so good to stay up-to-date. Looks like a lot of the e2e tests are affected for some reason

@karbyshev
Copy link
Contributor Author

@tzemanovic > Looks like a lot of the e2e tests are affected for some reason
Might be fixed now.

@tzemanovic
Copy link
Member

@tzemanovic > Looks like a lot of the e2e tests are affected for some reason Might be fixed now.

thx! The last failed e2e test should be fixed in new base branch

tzemanovic
tzemanovic previously approved these changes Jun 16, 2023
Copy link
Member

@tzemanovic tzemanovic left a comment

Choose a reason for hiding this comment

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

pls add a changelog

@karbyshev
Copy link
Contributor Author

pls add a changelog

@tzemanovic Done

apps/src/lib/cli.rs Outdated Show resolved Hide resolved
apps/src/lib/cli.rs Outdated Show resolved Hide resolved
tzemanovic
tzemanovic previously approved these changes Jun 19, 2023
tzemanovic
tzemanovic previously approved these changes Jun 19, 2023
apps/Cargo.toml Outdated Show resolved Hide resolved
@tzemanovic
Copy link
Member

I'll rebase onto latest base branch and add it a new draft

tzemanovic
tzemanovic previously approved these changes Jun 21, 2023
tzemanovic added a commit that referenced this pull request Jun 21, 2023
* aleks/clap-up-v4:
  Add changelog
  Enable color output
  Update clap to latest v4
  Update clap to latest v3
@tzemanovic tzemanovic mentioned this pull request Jun 21, 2023
@tzemanovic
Copy link
Member

I'm having some trouble with this in #1603 draft. There's a failing e2e test in CI, but when I try to run it locally it fails for me earlier than that at the command namadac init-validator --alias new-validator --source Bertha --unsafe-dont-encrypt --gas-amount 0 --gas-limit 0 --gas-token NAM --commission-rate 0.05 --max-commission-rate-change 0.01 --node 127.0.0.1:26657. It panics with:

The application panicked (crashed).
Message:  Mismatch between definition and access of `chain-id`. Unknown argument or group id.  Make sure you are using the argument id and not the short or long flags

Location: apps/src/lib/cli/utils.rs:314

  ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ BACKTRACE ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
                                ⋮ 8 frames hidden ⋮
   9: clap_builder::parser::error::MatchesError::unwrap::h4377723c4aa77431
      at /home/tz/.cargo/registry/src/github.com-1ecc6299db9ec823/clap_builder-4.3.5/src/parser/error.rs:32
  10: clap_builder::parser::matches::arg_matches::ArgMatches::get_one::hf4b5105e4827a7bd
      at /home/tz/.cargo/registry/src/github.com-1ecc6299db9ec823/clap_builder-4.3.5/src/parser/matches/arg_matches.rs:115
  11: namada_apps::cli::utils::parse_opt::h33b8ec3b64ec1e79
      at /home/tz/dev/namada-2/apps/src/lib/cli/utils.rs:314
  12: namada_apps::cli::utils::ArgOpt<T>::parse::h3eff49e0d32aa3d1
      at /home/tz/dev/namada-2/apps/src/lib/cli/utils.rs:197
  13: namada_apps::cli::args::<impl namada_apps::cli::utils::Args for namada::ledger::args::Tx<namada_apps::cli::args::CliTypes>>::parse::h204b9eb3b4dfb1cc
      at /home/tz/dev/namada-2/apps/src/lib/cli.rs:3399
  14: namada_apps::cli::args::<impl namada_apps::cli::utils::Args for namada::ledger::args::TxInitValidator<namada_apps::cli::args::CliTypes>>::parse::h5b99dfc697be870c
      at /home/tz/dev/namada-2/apps/src/lib/cli.rs:2341
  15: <namada_apps::cli::cmds::TxInitValidator as namada_apps::cli::utils::SubCmd>::parse::{{closure}}::h9a2c31a6dc417cb0
      at /home/tz/dev/namada-2/apps/src/lib/cli.rs:1205

@tzemanovic
Copy link
Member

I'm having some trouble with this in #1603 draft.

Ah so it's not just draft, I'm also have the same problem on this branch

@tzemanovic
Copy link
Member

The problem was a missing arg definition for chain-id arg in Tx. It was introduced in merge of #925 but previous clap version wasn't able to detect it.

Clap only does the detection in debug build (when debug_assertions is enabled - https://github.com/clap-rs/clap/blob/master/clap_builder/src/parser/matches/arg_matches.rs#L115) so we're not catching these in CI that uses release build. I've opened #1610 to enable debug_assertions in the CI e2e tests.

@karbyshev
Copy link
Contributor Author

karbyshev commented Jun 22, 2023

The problem was a missing arg definition for chain-id arg in Tx. It was introduced in merge of #925 but previous clap version wasn't able to detect it.

Great that you found it!

tzemanovic added a commit that referenced this pull request Jun 22, 2023
* aleks/clap-up-v4:
  cli: add a missing chain ID arg
@brentstone brentstone merged commit f3dad3a into main Jul 3, 2023
12 checks passed
@brentstone brentstone deleted the aleks/clap-up-v4 branch July 3, 2023 16:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

update clap to stable release
3 participants