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

build(deps): bump prost, tonic, tonic-build and console-subscriber #5009

Merged
merged 11 commits into from
Sep 6, 2022

Conversation

oxarbitrage
Copy link
Contributor

@oxarbitrage oxarbitrage commented Aug 30, 2022

Motivation

We want to update prost, tonic, tonic-build and console-subscriber at the same time.

Replaces #4853

Solution

Upgrade the 4 crates manually, clippy issues that were blocking this were resolved.

Review

I think anyone can review.

Reviewer Checklist

  • PR builds
  • All tests pass

@oxarbitrage oxarbitrage requested a review from a team as a code owner August 30, 2022 17:57
@oxarbitrage oxarbitrage requested review from upbqdn and removed request for a team August 30, 2022 17:57
@oxarbitrage
Copy link
Contributor Author

Hmm, this needs a new dependency, more info in hyperium/tonic#1047

Ill try to do this locally and then try to figure out where in the CI we need to add the new dependency. Also this will need changes to the README so is a bit more work than i was expecting but ill work on it.

@oxarbitrage
Copy link
Contributor Author

Ok, so i fixed this locally. As it says in the issue i posted above this now requires protoc to be in your OS. I installed this by using apt-get in my linux but this installed protoc 3.0 which is too old and gives an error when running clippy on zebra:

  thread 'main' panicked at 'Failed to generate lightwalletd gRPC files: Custom { kind: Other, error: "protoc failed: compact_formats.proto:8:8: Option \"swift_prefix\" unknown.\nservice.proto: Import \"compact_formats.proto\" was not found or had errors.\nservice.proto:143:36: \"CompactBlock\" is not defined.\nservice.proto:145:51: \"CompactBlock\" is not defined.\nservice.proto:166:47: \"CompactTx\" is not defined.\n" }', zebrad/build.rs:71:10
  note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

So i installed the last version using the binaries provided at https://github.com/protocolbuffers/protobuf/releases/tag/v21.5

With this version everything looks good.

We need to install this in the CI, what other projects are doing is adding https://github.com/arduino/setup-protoc to the workflow.

for example https://github.com/hyperium/tonic/blob/master/.github/workflows/CI.yml#L29

@teor2345
Copy link
Contributor

Also this will need changes to the README so is a bit more work than i was expecting but ill work on it.

We're only using these dependencies in tests, so we can update the developer documentation and link to it from the README. (We're trying to keep the README short.)

It's also ok to just link to the line in the Dockerfile or GitHub workflow that installs dependencies, because they change pretty often.

Also this PR removes the need for cmake in CI.

@oxarbitrage oxarbitrage requested a review from a team as a code owner August 30, 2022 23:57
@oxarbitrage oxarbitrage requested review from teor2345 and removed request for a team August 30, 2022 23:57
@codecov
Copy link

codecov bot commented Aug 31, 2022

Codecov Report

Merging #5009 (4d5ea31) into main (fc624d0) will decrease coverage by 0.04%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #5009      +/-   ##
==========================================
- Coverage   79.25%   79.20%   -0.05%     
==========================================
  Files         310      310              
  Lines       38901    38901              
==========================================
- Hits        30831    30812      -19     
- Misses       8070     8089      +19     

@teor2345 teor2345 removed their request for review August 31, 2022 05:36
@teor2345
Copy link
Contributor

The cargo deny output shows a duplicate dependency, with the older version coming via console-subscriber:

= prost v0.10.4
├── console-api v0.3.0
│ └── console-subscriber v0.1.6
│ └── zebrad v1.0.0-beta.13

https://github.com/ZcashFoundation/zebra/runs/8103900323?check_suite_focus=true#step:4:24

Try upgrading console-subscriber in this PR as well.

@oxarbitrage oxarbitrage changed the title build(deps): bump prost, tonic and tonic-build build(deps): bump prost, tonic, tonic-build and console-subscriber Aug 31, 2022
Copy link
Member

@upbqdn upbqdn left a comment

Choose a reason for hiding this comment

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

All looks good. I tested this PR locally, and the following checks passed with the protoc binary installed:

cargo fmt --all -- --check
cargo deny --workspace --all-features check bans sources
cargo clippy --all-targets --release --all-features
cargo test --release

I looked into the book to check where we could document that Zebra devs need protoc, but I didn't find any suitable place where this could be mentioned. Maybe a subsection here https://zebra.zfnd.org/CONTRIBUTING.html?

@oxarbitrage
Copy link
Contributor Author

@upbqdn
Copy link
Member

upbqdn commented Sep 1, 2022

Not sure what is this one: https://github.com/ZcashFoundation/zebra/runs/8142400656?check_suite_focus=true#step:3:12

Did we really reach the rate limit?
https://docs.github.com/en/rest/overview/resources-in-the-rest-api#requests-from-github-actions

The error also says that authenticated users get higher limit.

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.

Not sure what is this one: https://github.com/ZcashFoundation/zebra/runs/8142400656?check_suite_focus=true#step:3:12

Did we really reach the rate limit? https://docs.github.com/en/rest/overview/resources-in-the-rest-api#requests-from-github-actions

The error also says that authenticated users get higher limit.

Maybe @gustavovalverde can help, it seems like we need to fix the rate-limit before we merge this PR.

@teor2345 teor2345 added A-dependencies Area: Dependency file updates A-rust Area: Updates to Rust code C-enhancement Category: This is an improvement P-Low ❄️ labels Sep 2, 2022
@gustavovalverde
Copy link
Member

Sorry for the delay here @oxarbitrage. The fix has been applied for the actions, just a Cargo.lock conflict is missing. I'll leave the rest to you.

I pinned the version to 3.x so we don't have issues in the future with this.

teor2345
teor2345 previously approved these changes Sep 6, 2022
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.

I bumped the console-subscriber version again, and resolved the Cargo.lock conflict.

mergify bot added a commit that referenced this pull request Sep 6, 2022
mergify bot added a commit that referenced this pull request Sep 6, 2022
mergify bot added a commit that referenced this pull request Sep 6, 2022
mergify bot added a commit that referenced this pull request Sep 6, 2022
@mergify mergify bot merged commit 6cb9c52 into main Sep 6, 2022
@mergify mergify bot deleted the upgrade-tonic-prost branch September 6, 2022 14:49
@teor2345 teor2345 mentioned this pull request Sep 19, 2022
36 tasks
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 A-rust Area: Updates to Rust code C-enhancement Category: This is an improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants