-
Notifications
You must be signed in to change notification settings - Fork 307
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
Publish workspace to crates.io #4978
Comments
Thanks for writing this up and tracking it, fwiw:
I think this is already done in v0.82.x branch. The release branch should have all of the ecosystem bumps (tonic, ibc-types, tendermint, tower, etc.) |
That's true, the release branch does contain a commit that bumps the deps (#4973), but unfortunately it contains regressions like breaking HTTPS support for tonic clients. Fixing that regression is a bit of a side quest from the described goal of publishing the workspace, but worthwhile to get things straightened out. I'll follow up with PRs into main that describe the problem in detail. |
## Describe your changes This test is off-by-default, given that it talks to a remote endpoint, and is rather slow. It can be run directly via: cargo nextest run --release -p pcli --features integration-testnet sync_wallet_on_public_testnet As usual, make sure to include the `--release` flag, otherwise it'll be much slower. ## Issue ticket number and link Related to changes described in #4978. Specifically, there were changes made in #4973 that broke ClientTls configs for tonic connections, but our test suite didn't catch that. ## Testing and review 1. Check this branch out locally and run `cargo nextest run --release -p pcli --features integration-testnet sync_wallet_on_public_testnet` 2. Confirm that test passes! It's also worth considering clicking this on in CI so we get assurance; I left it off by default but plan to ask for it to be used in subsequent testing towards #4978. Maybe we should just stick it in CI, at least temporarily? ## Checklist before requesting a review - [x] I have added guiding text to explain how a reviewer should test these changes. - [x] If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason: > Test-only, no changes to app code. Intended to reduce the odds of breaking changes being merged.
Thanks for the write-up, my understanding is that this is caused by the
I am not entirely sure that this is sufficient to unblock, but that's what I got from hacking on this a bit. |
## Describe your changes This PR re-implements the changes from #4973, which were merged into the `release/v0.82.x` branch, but never landed on main. I'm resubmitting them so that we can address the HTTPS breakage, documented below, prior to tackling the rest of the changes required for getting the workspace crates published (#4978). Continuation of #4963, into a release branch `v0.82.x` before tagging a release candidate at that version and publishing the workspace using an `alpha` version. This handles the domain type change for upgradeable channels (penumbra-zone/ibc-types#84) smoothly. It makes sure to write default values to the new fields, which avoids wire protocol changes, and makes this PR non consensus/state breaking. Includes substantial version changes to: * tendermint-rs * tonic #4400 * ibc-types #4682 * cnidarium #4956 ## Issue ticket number and link This PR resubmits the changes in #4973, in an attempt to isolate problematic behavior. ## Testing and review The primary motivation for this changeset was to address the following error, which occurred when I tried to sync a wallet against testnet (using an HTTPS connection): ``` 2025-01-10T22:41:38.135876Z DEBUG load_or_initialize{path=Some("/tmp/nix-shell.5VH2AV/tmp.4Ay22mKmyY/pcli-view.sqlite") url=https://testnet.plinfra.net/}: penumbra_view::storage: database does not exist path="/tmp/nix-shell.5VH2AV/tmp.4Ay22mKmyY/pcli-view.sqlite" thread 'main' panicked at /home/conor/.cargo/registry/src/index.crates.io-6f17d22bba15001f/rustls-0.23.20/src/crypto/mod.rs:249:14: no process-level CryptoProvider available -- call CryptoProvider::install_default() before this point note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace ``` The most recent commit on this branch addresses that problem, by 1. explicitly configuring a default crypto provider via [rustls](https://docs.rs/rustls) 2. reusing a new `ViewServer::get_pd_channel` method throughout the codebase to handle conditional TLS config Feedback welcome on whether the new logic is clearly documented and stored in the right place. ## Checklist before requesting a review - [x] I have added guiding text to explain how a reviewer should test these changes. - [x] If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason: > There are significant version bumps in this patch set, but previous discussion concluded that the changes are not consensus-breaking; see #4682 (comment) --------- Co-authored-by: Erwan Or <[email protected]> Co-authored-by: Richard Janis Goldschmidt <[email protected]>
Overview
We want to publish the crates in the protocol repo workspace to https://crates.io, so that external developers can depend on Penumbra code when writing downstream tooling. To date, we've recommended that external developers use git dependendencies in Rust projects, e.g.
but doing so is not possible for projects already publishing their crates to crates.io. Two notable projects we intend to support are Astria (#3125) and Hermes. We're already maintaining a fork of Hermes, and publishing the crates would unblock us from submitting the Penumbra integration for Hermes to upstream.
Work to date
There's currently some WIP around the workspace consolidation happening in the following branches:
That work should be appended to, and organized in a feature branch that can be PR'd into main. In the process, we'll likely attempt to resolve both #4400 & #4682.
The text was updated successfully, but these errors were encountered: