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

fix(zebrad): accept default subcommand arguments and print consistent usage information for top-level 'help' subcommand #6801

Merged
merged 29 commits into from
Jun 7, 2023

Conversation

arya2
Copy link
Contributor

@arya2 arya2 commented May 31, 2023

Motivation

This PR is needed to avoid a compilation error that is likely to be added to the Rust 2024 edition, to make our dependency tree smaller.

Related: This makes the output of zebrad --help consistent with that of zebrad help.

Closes #5502
Closes #5707
Closes #5624

Breaking Changes

Major

  • The version subcommand has been replaced with a --version/-V flag
  • Zebra now accepts filters for the start command when no subcommand is provided

Minor

  • Zebra now ignores unrecognized arguments passed after a --version flag

Solution

  • Bumps abscissa_core from 0.5 to 0.7.0 in zebrad/Cargo.toml
  • Replaces gumdrop with the latest version of clap
  • Updates ZebradApp's impl of Application to match new method signatures
  • Updates commands to use clap::Parser instead of gumdrop::Options
  • Uses ColorChoice::Never when initializing the updated Terminal component to avoid panicking when color_spantrace::set_theme() is called after it's been set
  • Adds a EntryPoint::process_cli_args() method for inserting the default subcommand if one is not provided
  • Moves EntryPoint from the application module to commands now that it's been excluded from abscissa_core
  • Reverts fix(clippy): Silence future-incompat warnings until we upgrade Abscissa #6024

Review

Anyone can review.

Reviewer Checklist

  • Will the PR name make sense to users?
    • Does it need extra CHANGELOG info? (new features, breaking changes, large changes)
  • Are the PR labels correct?
  • Does the code do what the ticket and PR says?
    • Does it change concurrent code, unsafe code, or consensus rules?
  • How do you know it works? Does it have tests?

Follow Up Work

@arya2 arya2 added A-dependencies Area: Dependency file updates C-enhancement Category: This is an improvement P-Medium ⚡ C-security Category: Security issues labels May 31, 2023
@arya2 arya2 requested a review from a team as a code owner May 31, 2023 18:13
@arya2 arya2 self-assigned this May 31, 2023
@arya2 arya2 requested review from a team as code owners May 31, 2023 18:13
@arya2 arya2 requested review from dconnolly and upbqdn and removed request for a team May 31, 2023 18:13
@github-actions github-actions bot added the C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG label May 31, 2023
@teor2345
Copy link
Contributor

teor2345 commented Jun 5, 2023

I opened this PR in case other reviewers wanted a smaller diff for entry_point.rs:
#6826

arya2 and others added 2 commits June 5, 2023 22:02
Co-authored-by: teor <teor@riseup.net>
teor2345
teor2345 previously approved these changes Jun 6, 2023
Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

This seems to be working well enough for now.

@teor2345 teor2345 mentioned this pull request Jun 6, 2023
41 tasks
mergify bot added a commit that referenced this pull request Jun 6, 2023
@arya2
Copy link
Contributor Author

arya2 commented Jun 7, 2023

In the merge queue:

process didn't exit successfully: /home/runner/work/zebra/zebra/target/release/deps/zebrad-4d9e827350fddfd8 (signal: 11, SIGSEGV: invalid memory reference)
Error: Process completed with exit code 101.

https://github.com/ZcashFoundation/zebra/actions/runs/5194379481/jobs/9365975834#step:15:3531

Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

Conditional approval: if we see any memory errors in the zebrad lib tests, we should revert this PR.

@teor2345 teor2345 mentioned this pull request Jun 7, 2023
7 tasks
dconnolly added a commit that referenced this pull request Jun 7, 2023
mergify bot added a commit that referenced this pull request Jun 7, 2023
@mergify mergify bot merged commit 59086c7 into main Jun 7, 2023
@mergify mergify bot deleted the bump-abscissa branch June 7, 2023 06:03
dconnolly added a commit that referenced this pull request Jun 7, 2023
dconnolly added a commit that referenced this pull request Jun 7, 2023
mergify bot pushed a commit that referenced this pull request Jun 8, 2023
* Bump semvers

* Update zebra-utils/README.md

* Updated mainnet checkpoints against commit b7029b8

* Add testnet checkpoints from b7029b8

* Bump zebrad rust-version to 1.70

* rust-version 1.68

Co-authored-by: teor <teor@riseup.net>

* Add CHANGELOG for 1.0.0-rc.9

* Bump estimated release height to within june 7th 2023 utc-4

* Add #6801 to CHANGELOG in anticipation

* Update CHANGELOG.md

Co-authored-by: teor <teor@riseup.net>

* Update CHANGELOG.md

Co-authored-by: teor <teor@riseup.net>

* Update CHANGELOG.md

Co-authored-by: teor <teor@riseup.net>

* Update CHANGELOG.md

Co-authored-by: teor <teor@riseup.net>

* Update CHANGELOG.md

Co-authored-by: teor <teor@riseup.net>

* Update breaking changes in 1.0.0-rc.9 changelog

* changelog: move #6801 to Fix

* Update CHANGELOG.md

Co-authored-by: teor <teor@riseup.net>

* Include #6832 in the changelog

* Add missing changes to changelog

* Remove #6801 from known issues in the README

* Use the latest bug template link

---------

Co-authored-by: teor <teor@riseup.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-dependencies Area: Dependency file updates C-breaking Category: A breaking change for users C-bug Category: This is a bug C-enhancement Category: This is an improvement C-security Category: Security issues C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG
Projects
None yet
2 participants