Skip to content

fix(l1): make ethrex-rlp bench dev-deps path-only so the first crates.io publish succeeds#6897

Merged
ilitteri merged 6 commits into
mainfrom
fix-publish-rlp-devdeps
Jun 20, 2026
Merged

fix(l1): make ethrex-rlp bench dev-deps path-only so the first crates.io publish succeeds#6897
ilitteri merged 6 commits into
mainfrom
fix-publish-rlp-devdeps

Conversation

@ilitteri

Copy link
Copy Markdown
Collaborator

Motivation

The manual Publish crates to crates.io run failed on the very first crate:

error: failed to prepare local package for uploading
Caused by:
  no matching package named `ethrex-common` found
  location searched: crates.io index
  required by package `ethrex-rlp v17.0.0`

ethrex-rlp is published first, but its bench dev-dependencies on ethrex-common and ethrex-trie became versioned when version = "17.0.0" was added to the [workspace.dependencies] entries (via .workspace = true). cargo publish keeps versioned dev-deps in the published manifest and resolves them against crates.io — but those crates aren't published yet (they come later in the order), so packaging ethrex-rlp fails.

Description

Declare ethrex-rlp's bench dev-deps as path-only (no version):

ethrex-common = { path = ".." }
ethrex-trie = { path = "../trie" }

Path-only dev-deps are stripped from the published manifest (the pre-publishing behavior), so cargo publish no longer requires them from the index. Benches still build locally via the path.

This was the only affected crate: ethrex-l2-rpc's dev-dep on ethrex-rpc and ethrex-sdk's build-dep on ethrex-sdk-contract-utils are forward edges (the dep publishes before the consumer), so they resolve in order.

Validation

cargo publish --dry-run now succeeds for all three leaf crates — each reaches "Uploading … aborting upload due to dry run":

  • ethrex-rlp, ethrex-crypto, ethrex-sdk-contract-utils

(The dependents can't be dry-run'd locally until their deps are on crates.io, but the topological order is unchanged and no other versioned sibling dev/build-deps point forward.)

How to test

After merge, re-run the Publish crates to crates.io workflow (dry-run first, then real). The first crate ethrex-rlp will package and upload instead of erroring.

Checklist

  • Updated STORE_SCHEMA_VERSION (crates/storage/lib.rs) if the PR includes breaking changes to the Store requiring a re-sync. — N/A.

…h succeeds

ethrex-rlp is the first crate published to crates.io, but its bench
dev-dependencies on ethrex-common and ethrex-trie became versioned once
version = "17.0.0" was added to the [workspace.dependencies] entries. cargo
publish then required ethrex-common 17.0.0 from the registry before it existed
(ethrex-rlp publishes before them), failing with
"no matching package named `ethrex-common` found".

Declare those bench dev-deps as path-only (no version) so they are stripped from
the published manifest, restoring the pre-publishing behavior. Verified with
`cargo publish --dry-run` for the three leaf crates (ethrex-rlp, ethrex-crypto,
ethrex-sdk-contract-utils).
@ilitteri ilitteri requested a review from a team as a code owner June 19, 2026 23:31
Copilot AI review requested due to automatic review settings June 19, 2026 23:31
@github-actions github-actions Bot added the L1 Ethereum client label Jun 19, 2026
@github-actions

Copy link
Copy Markdown

🤖 Kimi Code Review

The change is correct and solves a legitimate workspace publishing ordering issue.

Analysis:

  1. Issue Identification: Since ethrex-rlp is published before ethrex-common and ethrex-trie (likely due to being a lower-level dependency), using workspace dependencies (which imply versioning) would cause cargo publish to fail when validating dependencies because the higher-level crates don't yet exist on crates.io.

  2. Solution Validity: Path-only dev-dependencies (without version) are indeed stripped from the published manifest and don't require the target crate to exist on the registry. This is the standard Rust pattern for handling dev-dependencies in crates that publish before their dev-dependency peers.

  3. Path Verification (lines 21-22):

    • path = ".." correctly points to crates/common/ (assuming this file is at crates/common/rlp/Cargo.toml)
    • path = "../trie" correctly points to crates/common/trie/

Minor Suggestion:

The comment on lines 18-20 is excellent and explains the "why" clearly. Consider also adding a brief note that these are only used in benches/ to reinforce why stripping them from the published manifest is safe, though the current comment is sufficient.

No issues found - this is the idiomatic fix for this scenario.


Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt

@github-actions

Copy link
Copy Markdown

🤖 Claude Code Review

Here is my review of PR #6897:


Code Review: fix(l1): make ethrex-rlp bench dev-deps path-only so the first crates.io publish succeeds

Overview

This is a surgical one-file fix to crates/common/rlp/Cargo.toml. It converts two bench dev-dependencies (ethrex-common, ethrex-trie) from workspace-inherited entries (which carry a version) to path-only entries (no version). This prevents cargo publish from attempting to resolve those sibling crates from crates.io before they have been uploaded.


Correctness

  • The fix is correct. When a dev-dependency has a version field — even one inherited via .workspace = true — Cargo includes it in the published manifest. At publish time, Cargo resolves it from crates.io, which fails if the dep isn't published yet. Path-only dev-deps are stripped from the published manifest entirely.
  • The relative paths are correct: ".." resolves to crates/common/ and "../trie" resolves to crates/common/trie/ from crates/common/rlp/.
  • Local and CI builds are unaffected since Cargo prefers path deps when available.

Comment Quality

The 4-line comment explaining the intent is justified here. This is precisely the kind of non-obvious invariant that a future developer might "clean up" by converting back to .workspace = true, which would silently reintroduce the publish failure. Keeping the explanation inline is the right call.

Scope

The PR description correctly identifies this as the only affected crate:

  • ethrex-l2-rpc dev-dep on ethrex-rpc and ethrex-sdk build-dep on ethrex-sdk-contract-utils are both forward edges (dep publishes before consumer), so they resolve in order without this problem.

Minor Observations

  • The rand = "0.9.2" and once_cell = "1" dev-deps immediately below are external crates and don't benefit from path-only treatment — they are correct as-is.
  • There is no workspace version guard on rand or criterion in this block either; that's a pre-existing pattern unrelated to this fix.
  • Future-proofing: if someone later adds version = "..." to these path-only entries, the problem silently returns. The comment mitigates this, but a CI step that validates publish ordering (e.g., cargo publish --dry-run for leaf crates on every PR) would provide a stronger guard. That is out of scope for this fix but worth considering.

Summary

The fix is minimal, correct, and well-justified. No issues found. The dry-run validation described in the PR body is the right way to confirm it before merging.


Automated review by Claude (Anthropic) · sonnet · custom prompt

@greptile-apps

greptile-apps Bot commented Jun 19, 2026

Copy link
Copy Markdown

Greptile Summary

Fixes the cargo publish failure for ethrex-rlp by replacing workspace-inherited (versioned) bench dev-dependencies on ethrex-common and ethrex-trie with path-only entries. Path-only dev-deps are stripped from the published manifest, so cargo no longer attempts to resolve these not-yet-published sibling crates from the crates.io index.

  • ethrex-common = { path = \"..\" } and ethrex-trie = { path = \"../trie\" } replace .workspace = true entries, which inherited a version and caused packaging to fail when those crates weren't on crates.io yet.
  • A clear explanatory comment is added inline documenting the intent, which is good for future maintainers.

Confidence Score: 5/5

Safe to merge — the change is a one-file, two-line fix to dev-dependency declarations with no impact on the published library code or its runtime behaviour.

The paths .. (ethrex-common) and ../trie (ethrex-trie) are correct relative to crates/common/rlp/, benches continue to build locally via the path, and the published manifest will no longer include these deps at all. The inline comment documents the intent clearly for future maintainers. No production code is touched.

No files require special attention.

Important Files Changed

Filename Overview
crates/common/rlp/Cargo.toml Replaces workspace-inherited (versioned) bench dev-deps on ethrex-common and ethrex-trie with path-only declarations so cargo publish strips them from the published manifest, fixing first-crate publish failure.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant CI as CI Workflow
    participant cargo as cargo publish
    participant index as crates.io index

    Note over CI,index: Before fix (broken)
    CI->>cargo: publish ethrex-rlp
    cargo->>index: resolve ethrex-common v17.0.0
    index-->>cargo: ❌ not found (not yet published)
    cargo-->>CI: error: no matching package

    Note over CI,index: After fix
    CI->>cargo: publish ethrex-rlp
    Note over cargo: path-only dev-deps stripped from manifest
    cargo->>index: (no lookup for ethrex-common / ethrex-trie)
    cargo-->>CI: ✅ Uploading ethrex-rlp v17.0.0
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant CI as CI Workflow
    participant cargo as cargo publish
    participant index as crates.io index

    Note over CI,index: Before fix (broken)
    CI->>cargo: publish ethrex-rlp
    cargo->>index: resolve ethrex-common v17.0.0
    index-->>cargo: ❌ not found (not yet published)
    cargo-->>CI: error: no matching package

    Note over CI,index: After fix
    CI->>cargo: publish ethrex-rlp
    Note over cargo: path-only dev-deps stripped from manifest
    cargo->>index: (no lookup for ethrex-common / ethrex-trie)
    cargo-->>CI: ✅ Uploading ethrex-rlp v17.0.0
Loading

Reviews (1): Last reviewed commit: "fix(l1): make ethrex-rlp bench dev-deps ..." | Re-trigger Greptile

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes the crates.io publishing order issue for the ethrex-rlp crate by ensuring its bench-only dev-dependencies on sibling workspace crates don’t get published as versioned dependencies that must already exist on crates.io.

Changes:

  • Converted ethrex-rlp’s dev-dependencies on ethrex-common and ethrex-trie from workspace = true (versioned) to path-only dependencies.
  • Added an in-file comment explaining why these dev-dependencies must remain path-only to allow the first publish in the sequence to succeed.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@github-actions

Copy link
Copy Markdown

🤖 Codex Code Review

No findings.

This is a manifest-only change in crates/common/rlp/Cargo.toml:19-24: it moves ethrex-common and ethrex-trie from workspace-managed dev-deps to explicit local path dev-deps, and those deps are only used by the RLP benches. I don’t see any impact on runtime code, EVM behavior, gas accounting, trie/state logic, or RLP encode/decode correctness.

Residual verification gap: I could not complete cargo check/cargo package in this environment because Cargo tried to use read-only ~/.cargo/~/.rustup locations and then hit workspace dependency resolution for a git dependency outside this crate. I did confirm via cargo metadata --manifest-path crates/common/rlp/Cargo.toml --no-deps that the manifest parses and the new paths resolve. If you want belt-and-suspenders validation, I’d still run cargo publish -p ethrex-rlp --dry-run in CI or a writable local environment to confirm Cargo’s publish-time handling of these stripped bench-only dev-deps.


Automated review by OpenAI Codex · gpt-5.4 · custom prompt

…e in CI

REVERT BEFORE MERGE. Temporarily:
- adds a `push` trigger for this branch so the workflow runs in CI on each push,
- forces dry-run for every non-release event (the push trigger never publishes),
- upgrades the dry-run to `cargo publish --dry-run` (packaging + index resolution
  + verify build), tolerating only the expected "no matching package" error for
  siblings not yet on crates.io.

Validates the workflow mechanics, the crates-release-prod environment, and the
leaf crates' full packaging + verify-build on a clean CI runner.
@ilitteri ilitteri temporarily deployed to crates-release-prod June 19, 2026 23:46 — with GitHub Actions Inactive
ilitteri added 4 commits June 19, 2026 20:52
…t dep cycles

The workflow_dispatch dry-run used `cargo package --list`, which only lists
tarball files and never resolves against the crates.io index -- so it could not
catch the unpublishable-dependency bug this PR fixes.

- Replace the dry-run with `cargo publish --dry-run` (packaging + index
  resolution + verify build), tolerating only the expected "no matching package"
  for siblings not yet published during a chain dry-run.
- Add a guard step (runs for both dry-run and real publish) that fails fast if
  the edges `cargo publish` keeps form a dependency cycle among the published
  crates -- exactly the bug class this PR fixes (a versioned dev/build-dep
  pointing at a crate published later). Deterministic; the dry-run alone can't
  distinguish it from the expected chain ordering, so the guard is what catches it.
Remove the embedded Python step (keeps the workflow free of inline scripts).
The workflow_dispatch dry-run still uses `cargo publish --dry-run` (packaging +
index resolution + verify build) instead of the previous `cargo package --list`,
so on-demand dry-runs are meaningful; it tolerates only the expected
"no matching package" for siblings not yet published during a chain dry-run.
Add a `run-name` that appends " (dry run)" when triggered via workflow_dispatch
with the dry_run input, so the Actions run list clearly distinguishes a dry run
from a real publish.
@ilitteri ilitteri enabled auto-merge June 20, 2026 00:13
@github-project-automation github-project-automation Bot moved this to In Review in ethrex_l1 Jun 20, 2026
@ilitteri ilitteri added this pull request to the merge queue Jun 20, 2026
Merged via the queue into main with commit 1a55752 Jun 20, 2026
59 checks passed
@ilitteri ilitteri deleted the fix-publish-rlp-devdeps branch June 20, 2026 01:32
@github-project-automation github-project-automation Bot moved this from In Review to Done in ethrex_l1 Jun 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

L1 Ethereum client

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

5 participants