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

feat: publish workspace crates #4986

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Conversation

conorsch
Copy link
Contributor

@conorsch conorsch commented Jan 15, 2025

Describe your changes

Workspace refactor, preparing to publish the Penumbra crates. In doing so, renames all crates to have a prefix penumbra-sdk-, which is unique and controlled by PL. There's also a custom script that handles the publication action, which is non-standard.

The major obstacle is that the penumbra-sdk-proof-params crate contains binary keyfiles, which are managed in git-lfs. We cannot upload the raw keyfiles to crates.io, because that'd result in a 100MB crate. Instead, we use a custom script to revert the binary keyfiles to plaintext lfs pointers immediately prior to publishing to crates.io, which stays under the limit, and allows third-party tools to opt into downloading the key material via the download-proving-keys feature.

Given that docs.rs doesn't permit network access during crate build, we default to warning about the missing keys during build of proof-params, recommending enabling the option. We could go further add fail the build, conditionally warning only in the docsrs build context. I chose to minimize special-casing as much as possible.

Issue ticket number and link

Towards #4978.

Testing and review

  1. Browse the recently published "alpha" crates: https://crates.io/search?q=penumbra-sdk
  2. Try updating a Rust project that depends on Penumbra, as the hermes fork or the reindexer, to depend on the published crates.
  3. Confirm that docs are available: https://docs.rs/penumbra-sdk-app/latest/penumbra_sdk_app/

Checklist before requesting a review

  • I have added guiding text to explain how a reviewer should test these changes.

  • 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:

    Careful attention was paid to make this a non-breaking change. All the crates have been renamed, but the code behaves the same. Testing should focus on making sure that key material for the proof params crate is fetched correctly, to avoid breakage.

erwanor and others added 2 commits January 15, 2025 09:39
This moves our workspace into a `penumbra-sdk` namespace ahead of
publishing `v0.82.0-alpha.0` on crates.io. We're using the `penumbra-sdk`
prefix for all crate names because it's unique. (The `penumbra`) crate
name is already taken on crates.io and belongs to an unrelated project.

In addition to a renaming, several other changes are made for crates.io
compatibility:

  * fills out crate metadata fields to meet spec
  * breaks out app-tests to separate crate
We want all the crates Penumbra protocol workspace to be published to
crates.io, so that external developers can depend on them in downstream
projects. For now, we'll skip doing so for the binaries, but we can
circle back on them.

The major obstacle is that the `penumbra-sdk-proof-params` create
contains binary keyfiles, which are managed in git-lfs. We cannot upload
the raw keyfiles to crates.io, because that'd result in a 100MB crate.
Instead, we use a custom script to revert the binary keyfiles to
plaintext lfs pointers immediately prior to publishing to crates.io,
which stays under the limit, and allows third-party tools to opt into
downloading the key material via the `download-proving-keys` feature.

The ratelimit of "20s" between crates publishing has been working
reliably when publishing alpha versions (i.e. an HTTP 429 is avoided).
Publishing all crates takes ~12m.

This commit also condenses several "alpha" series releases, culminating
in `0.82.0-alpha.15`. Most of these versions have been published to
crates.io, to evaluate CI behavior on publishing crates.

build: warn-only if download-proving-keys not set

The goal is to warn if keys are not set, but still permit the build.
@conorsch conorsch force-pushed the 4978-crate-publishing-take-2 branch from ef841dd to e6f5c30 Compare January 15, 2025 18:54
Copy link
Contributor Author

@conorsch conorsch left a comment

Choose a reason for hiding this comment

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

Added some comments in-line to guide review.

//!
//! Instead, we'll upload just git raw git-lfs pointer files when publishing to crates.io,
//! then use the build.rs logic to fetch the assets ahead of compilation. Use the feature
//! `download-proving-keys` to enable the auto-download behavior.
use anyhow::Context;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The changes to the proof-params build.rs file warrants close scrutiny.

# https://github.com/git-lfs/git-lfs/issues/951#issuecomment-581477084
# This is a destructive action! Which is why we checked for dirty tree above.
git read-tree HEAD && GIT_LFS_SKIP_SMUDGE=1 git checkout -f HEAD

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The GIT_LFS_SKIP_SMUDGE=1 approach is slightly cursed: we're altering in-repo assets immediately prior to crate publication. This works™, though, so I'm suggesting it as the right-sized solution to get the workspace published.

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.

2 participants