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

Migrate from structopt to clap. #124

Merged
merged 10 commits into from
Nov 29, 2022
Merged

Conversation

rajarshimaitra
Copy link
Contributor

Description

Fixes #113.

This is an attempt to migrate from structopt to clap v0.3 which provides very similar kind of derives as structopt. Changes are straight forward. But this comes with few more problems.

  • with clap v3.2.22 the MSRV pushes up to 1.57.0.. The last clap of MSRV 1.56.0 was clap 3.2.5.. But even that doesn't seem to be working at MSRV 1.56.0 anymore, as bunch of underlying lib has upgraded.

  • clap v3.0 doesn't seem to support custom vector parsing well, reported here Handle parsing vector of custom structures clap-rs/clap#1704. This is required for recipient parsing which is a Vec<(Script, u32)>. Workaround for that is to use vecs of strings and parse them at runtime in create_tx handler. Included in the PR.

Notes to the reviewers

structopt is currently freezed at clap 2.0 and doesn't seem to intend on updating and currently its has unmaintained vulnerability. And clap v3.0 onward seems to replacing everything that structopt did before. So this means we should also look for ways to migrate from strcutopt to clap.. But clap seemed to have moved ahead than our MSRV 1.56.0.. So we need to take up a call on that.. Opened this PR to facilitate that discussion..

This is draft until we figure what to do..

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing

@rajarshimaitra rajarshimaitra marked this pull request as draft October 2, 2022 14:45
@notmandatory
Copy link
Member

Thanks for starting this! I don't see a problem updating bdk-cli MSRV to 1.57.0. It's only the core bdk libs that we want to be able to support projects using older rust versions.

@rajarshimaitra
Copy link
Contributor Author

Thanks @notmandatory .. If we can move up to 1.57 for bdk-cli then that resolves major chunk of the issue.. Will update the PR accordingly..

@rajarshimaitra
Copy link
Contributor Author

@notmandatory updated with few more fixes..

  • The clap vector of string parsing is not working properly.. Creating a new issue for it..
  • The 1.56 tests are still running.. Anyway to disable them in this PR?

@rajarshimaitra
Copy link
Contributor Author

Documented the issue here #126

@rajarshimaitra rajarshimaitra marked this pull request as ready for review October 13, 2022 07:37
@notmandatory
Copy link
Member

I'll need to change the branch protection rules to require 1.57 instead of 1.56. That means this this PR would need to get merged next and any others rebased on it. Is that the order you want to do things?

@rajarshimaitra
Copy link
Contributor Author

I think #123 should go in first.. This can go after.. Or maybe we can also combine this and #125 together and update the CI stuffs in one go?

@rajarshimaitra
Copy link
Contributor Author

@notmandatory Updated this to bdk v0.23.. Fixed #126 .. Turns out the only way to parse Vec is by using multiple flags.. This is now good to go.. And other PRs would be rebased..

@rajarshimaitra rajarshimaitra linked an issue Oct 30, 2022 that may be closed by this pull request
@notmandatory notmandatory added this to the Release 0.7.0 milestone Nov 16, 2022
This commit include changes to move from structopt to clap.
This includes:
 - Update Cargo.toml.
 - Update commands from structopt to clap.
 - Some auxiliary fixes in commands.
 - Updates main.
Clap doesn't support custom vector parsing. Not so ideal work around is
to handle recipient parsing in create_tx handler at run time.
Vector parsing is possible with multiple flags.
@notmandatory
Copy link
Member

I updated the branch protection rules to check for the tests run against 1.57 instead of 1.56. Looks like you just need to fix up the WASM.

@rajarshimaitra
Copy link
Contributor Author

Sorry its taking some time and iteration.. I am not being able to play the wasm run in local.. So trying up with the CI outputs here..

Seems like it fixed the issue but now clippy is throwing an unused import warning, which is error is CI.

  cargo clippy -- -D warnings
  shell: /usr/bin/bash -e {0}
   Compiling bdk-cli v0.6.0 (/home/runner/work/bdk-cli/bdk-cli)
error: unused import: `Args`
  --> src/commands.rs:16:25
   |
16 | use clap::{AppSettings, Args, Parser, Subcommand};
   |                         ^^^^
   |
   = note: `-D unused-imports` implied by `-D warnings`

error: could not compile `bdk-cli` due to previous error
Error: Process completed with exit code 101.

But that import Args is definitely used, and removing it causes build failure.. Any idea??

@rajarshimaitra
Copy link
Contributor Author

🎉 🎉 It worked.. @notmandatory this is now good for merge..

Copy link
Member

@notmandatory notmandatory left a comment

Choose a reason for hiding this comment

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

ACK b20a140

Did a little manual testing also and everything worked.

@notmandatory notmandatory merged commit 18ee6b2 into bitcoindevkit:master Nov 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Vec<String> aren't parsing in clap 3.0 RUSTSEC-2021-0139: ansi_term is Unmaintained
2 participants