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

Disable the default features of the tonic and tonic-build crates to allow downstream consumers to build in Wasm #1270

Merged

Conversation

willemolding
Copy link
Contributor

@willemolding willemolding commented Mar 14, 2024

Closes #1269

Note this regenerates the code-generated proto/service.rs file and the newer version is included in this PR. This is a minor change to the zcash_client_backend public API as these are exposed.

Copy link
Contributor

@str4d str4d left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

zcash_client_backend/src/proto/service.rs Show resolved Hide resolved
@str4d
Copy link
Contributor

str4d commented Mar 14, 2024

Also, in addition to making the above change, add the following change to .github/workflows/ci.yml so we are testing this change:

diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
index 6263019a6..ebf418cfe 100644
--- a/.github/workflows/ci.yml
+++ b/.github/workflows/ci.yml
@@ -195,7 +195,7 @@ jobs:
         run: cargo add --no-default-features --path ../crates/zcash_proofs
       - name: Add zcash_client_backend as a dependency of the synthetic crate
         working-directory: ./ci-build
-        run: cargo add --no-default-features --path ../crates/zcash_client_backend
+        run: cargo add --features lightwalletd-tonic --path ../crates/zcash_client_backend
       - name: Copy pinned dependencies into synthetic crate
         run: cp crates/Cargo.lock ci-build/
       - name: Add target

@willemolding
Copy link
Contributor Author

Thanks for the review @str4d !

I moved the impl of CompactTxStreamerClient<tonic::transport::Channel> to proto.rs as suggested but put it behind just a feature flag without a target_arch check.

I think this makes more sense as it it feels a little weird to have the API change depending on the build target. Instead builds will fail if the feature is enabled and then Wasm builds can disable it to have it work. I think more useful information is communicated this way.

@willemolding willemolding requested a review from str4d March 14, 2024 07:25
Copy link
Contributor

@str4d str4d left a comment

Choose a reason for hiding this comment

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

Reviewed 61447d9

.github/workflows/ci.yml Outdated Show resolved Hide resolved
Cargo.lock Outdated Show resolved Hide resolved
Copy link

codecov bot commented Mar 14, 2024

Codecov Report

Attention: Patch coverage is 0% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 63.42%. Comparing base (1c72b0b) to head (d702aea).
Report is 11 commits behind head on main.

Files Patch % Lines
zcash_client_backend/src/proto.rs 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1270      +/-   ##
==========================================
- Coverage   63.55%   63.42%   -0.13%     
==========================================
  Files         121      121              
  Lines       13661    13705      +44     
==========================================
+ Hits         8682     8693      +11     
- Misses       4979     5012      +33     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@str4d str4d merged commit 1775f65 into zcash:main Mar 15, 2024
20 of 25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

zcash_client_backend: unused tonic crate features prevent Wasm builds with feature lightwalletd-tonic
3 participants