7848: ci: fix ambiguity issue during tokio releases#56
7848: ci: fix ambiguity issue during tokio releases#56martin-augment wants to merge 81 commits intomasterfrom
Conversation
Co-authored-by: Alice Ryhl <aliceryhl@google.com>
…i/tokio into fix-release-ambiguity-issue
WalkthroughThis pull request implements a structured approach to managing local development dependencies in a multi-crate workspace. Changes include: adding a ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @martin-augment, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the dependency management within the Highlights
Changelog
Ignored Files
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Code ReviewOverviewThis PR addresses an important CI/release issue by adding a workspace patch for the Strengths✅ Clear Problem Solving: The workspace patch and dual-testing approach directly addresses the release ambiguity issue Code Quality & Best Practices✅ Good Practices
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request aims to fix dependency ambiguity issues by centralizing local path overrides into a [patch.crates-io] section in the root Cargo.toml. This is a good approach, but the implementation is incomplete. The patch section only includes tokio, while it should include all workspace crates to prevent pulling outdated versions from crates.io during local development. I've left a critical comment with a suggestion to fix this.
Additionally, the new contributing guide about dependency versions is not aligned with the changes in this PR and should be updated to reflect the new [patch]-based workflow. I've added a comment with suggestions for the documentation.
| [patch.crates-io] | ||
| tokio = { path = "tokio"} |
There was a problem hiding this comment.
The [patch.crates-io] section is incomplete. To resolve dependency ambiguity within the workspace for local development, it should include all workspace member crates that are dependencies of other members. Without this, builds will pull crates like tokio-util, tokio-stream, etc., from crates.io, which can lead to version conflicts and build failures when there are local changes.
You should patch all of tokio, tokio-macros, tokio-stream, tokio-test, and tokio-util.
Also, please ensure all path dependencies to other workspace crates are removed from the individual Cargo.toml files to rely solely on this patch mechanism. For example, the path dependency on tokio-macros in tokio/Cargo.toml was not removed.
| [patch.crates-io] | |
| tokio = { path = "tokio"} | |
| [patch.crates-io] | |
| tokio = { path = "tokio" } | |
| tokio-macros = { path = "tokio-macros" } | |
| tokio-stream = { path = "tokio-stream" } | |
| tokio-test = { path = "tokio-test" } | |
| tokio-util = { path = "tokio-util" } |
| # How to specify crates dependencies versions | ||
|
|
||
| Each crate (e.g., `tokio-util`, `tokio-stream`, etc.) should specify dependencies | ||
| according to these rules: | ||
|
|
||
| 1. The listed version should be the oldest version that the crate works with | ||
| (e.g., if `tokio-util` works with `tokio` version `1.44` but not `1.43`, then | ||
| `tokio-util` should specify version `1.44` for its `tokio` dependency). | ||
| We don't require users to use the latest version unnecessarily. | ||
| 2. When a crate starts using a newer feature in a dependency, the version | ||
| should be bumped to the version that introduced it. | ||
| 3. If a crate depends on an unreleased feature in a dependency, it may use | ||
| `path = ` dependency to specify this. Since path dependencies must be removed | ||
| during the release of the crate, this ensures that it can't be released until | ||
| the dependency has a new version. | ||
|
|
||
| Consider the following example from `tokio-stream`: | ||
|
|
||
| ```toml | ||
| [dependencies] | ||
| futures-core = { version = "0.3.0" } | ||
| pin-project-lite = "0.2.11" | ||
| tokio = { version = "1.15.0", path = "../tokio", features = ["sync"] } | ||
| ``` | ||
|
|
||
| In this case, local development of `tokio-stream` uses the local version | ||
| of `tokio` via the `path` dependency. This means that it's currently not | ||
| possible to release `tokio-stream`. Once a new version of `tokio` is | ||
| released, the path dependency will be removed from `tokio-stream`. | ||
| As mentioned before, this version should only be bumped when adding a new | ||
| feature in the crate that relies on a newer version. |
There was a problem hiding this comment.
This documentation is a great addition, but it's inconsistent with the changes made in this same pull request. This PR removes path dependencies in favor of a centralized [patch.crates-io] section in the root Cargo.toml. The documentation should be updated to reflect this as the new best practice for the repository.
Specifically:
- Rule 3 should explain the
[patch.crates-io]mechanism for local development instead ofpathdependencies. - The example should not use a
pathdependency. - The explanation of the example should refer to the workspace patch, not a
pathdependency.
Updating this will avoid confusion for future contributors and correctly document the new workflow.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@docs/contributing/how-to-specify-crates-dependencies-versions.md`:
- Around line 17-31: The example shows a crate-level path dependency
(tokio-stream using path = "../tokio") which is inconsistent with this PR's use
of a workspace-level [patch.crates-io] in the root Cargo.toml; update the docs
to reflect the actual pattern by replacing the inline path example with an
example that demonstrates using [patch.crates-io] in the workspace root (and
mention that individual crate manifests like tokio-util/Cargo.toml will not
contain path = entries), or alternatively add a short note showing both
approaches and stating that the workspace [patch.crates-io] is the preferred
method in this repository.
🧹 Nitpick comments (3)
docs/contributing/README.md (1)
40-40: Minor grammar nit in link text."crates dependencies versions" reads awkwardly — consider "crate dependency versions" for natural English phrasing.
-- [How to specify crates dependencies versions](how-to-specify-crates-dependencies-versions.md) +- [How to specify crate dependency versions](how-to-specify-crate-dependency-versions.md)(This would also require renaming the target
.mdfile accordingly.)Cargo.toml (1)
18-19: Thesedremoval patterns in CI are tightly coupled to this exact formatting.The CI workflow uses literal regex
^tokio = { path = "tokio"}$to strip these lines. Note the missing space before}— this is intentional because thesedpatterns must match exactly. However, this is fragile: any formatting change (e.g.,cargo fmton TOML, or adding a trailing space) will silently break the patch-removal step.Consider adding a CI comment near the
sedcalls referencing this line, or alternatively using a more resilient approach (e.g.,cargo-hack --remove-dev-depsstyle tooling, or atoml-aware edit).tokio-stream/Cargo.toml (1)
45-48: Dev-deptokioversion is lower than the main dependency version.Line 45 specifies
tokio = "1.2.0"while line 41 already requirestokio = "1.38.0". Cargo will unify to>=1.38.0, so this isn't a bug, but the dev-dep minimum is misleading. Consider aligning it to"1.38.0"for clarity and to catch accidental downgrades of the main dependency.Proposed fix
[dev-dependencies] -tokio = { version = "1.2.0", features = ["full", "test-util"] } +tokio = { version = "1.38.0", features = ["full", "test-util"] }
| Consider the following example from `tokio-stream`: | ||
|
|
||
| ```toml | ||
| [dependencies] | ||
| futures-core = { version = "0.3.0" } | ||
| pin-project-lite = "0.2.11" | ||
| tokio = { version = "1.15.0", path = "../tokio", features = ["sync"] } | ||
| ``` | ||
|
|
||
| In this case, local development of `tokio-stream` uses the local version | ||
| of `tokio` via the `path` dependency. This means that it's currently not | ||
| possible to release `tokio-stream`. Once a new version of `tokio` is | ||
| released, the path dependency will be removed from `tokio-stream`. | ||
| As mentioned before, this version should only be bumped when adding a new | ||
| feature in the crate that relies on a newer version. |
There was a problem hiding this comment.
The example doesn't match the actual pattern used in this PR.
The example on line 23 shows an inline path = "../tokio" in tokio-stream's [dependencies], but this PR actually uses a workspace-level [patch.crates-io] in the root Cargo.toml to achieve the same effect. The individual crate manifests (e.g., tokio-util/Cargo.toml) no longer carry path = entries.
This could confuse contributors who look at the actual Cargo.toml files and see no path dependencies, contrary to what this doc describes. Consider either:
- Updating the example to show the
[patch.crates-io]approach actually used, or - Mentioning both approaches and clarifying which is preferred.
🤖 Prompt for AI Agents
In `@docs/contributing/how-to-specify-crates-dependencies-versions.md` around
lines 17 - 31, The example shows a crate-level path dependency (tokio-stream
using path = "../tokio") which is inconsistent with this PR's use of a
workspace-level [patch.crates-io] in the root Cargo.toml; update the docs to
reflect the actual pattern by replacing the inline path example with an example
that demonstrates using [patch.crates-io] in the workspace root (and mention
that individual crate manifests like tokio-util/Cargo.toml will not contain path
= entries), or alternatively add a short note showing both approaches and
stating that the workspace [patch.crates-io] is the preferred method in this
repository.
There was a problem hiding this comment.
value:good-to-have; category:bug; feedback: The CodeRabbit AI reviewer is correct! The patch in the workspace Cargo.toml seems not really needed. The new .md file suggests to use path = "../tokio" when needed. So the global patch seems also unnecessary.
🤖 Augment PR SummarySummary: Updates CI and workspace dependency configuration to avoid Cargo package-selection ambiguity during Tokio releases. Changes:
🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| cargo nextest run --workspace --features $TOKIO_STABLE_FEATURES | ||
| # Removing workspace patches to run tests without path dependencies | ||
| # (if not specified differently in the crate) | ||
| sed -i.bak '/^\[patch\.crates-io\]$/d; /^tokio = { path = "tokio"}$/d' Cargo.toml && rm -f Cargo.toml.bak |
There was a problem hiding this comment.
The sed deletion here matches the [patch.crates-io] block very literally; small formatting changes (e.g., whitespace) or adding more entries under [patch.crates-io] could make the patch-removal silently fail or leave invalid TOML, which would undermine the “no path dependencies” checks.
Severity: medium
Other Locations
.github/workflows/ci.yml:229.github/workflows/ci.yml:827
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
There was a problem hiding this comment.
value:valid-but-wont-fix; category:bug; feedback: The Augment AI reviewer is not correct! The syntax [patch.crates-io] is the usually used one, without any whitespace characters. There is no need to add usage of \s* around the square brackets just in case someone decides to change it to non-idiomatic syntax.
| cargo check -p tokio --features $TOKIO_STABLE_FEATURES | ||
| # Other crates doesn't have unstable features, so we can use --all-features. | ||
| cargo check -p tokio-macros -p tokio-stream -p tokio-util -p tokio-test --all-features | ||
| cargo hack check -p tokio-macros -p tokio-stream -p tokio-util -p tokio-test --all-features |
There was a problem hiding this comment.
This still relies on -p tokio-stream/-p tokio-util package selection; after switching away from path deps (and given the earlier WASI step had to move to --manifest-path), this job may still run into Cargo “ambiguous package” selection when registry + workspace copies exist.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
There was a problem hiding this comment.
value:incorrect-but-reasonable; category:bug; feedback: The Augment AI reviewer is not correct! The cargo-hack check -p ... command behaves like cargo check --manifest-path ... and the passing CI check proves that it works as expected.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| --exclude examples \ | ||
| --features $TOKIO_STABLE_FEATURES | ||
| # Cargo nextest does not support doctest, so we run them separately | ||
| # (see https://github.com/nextest-rs/nextest/issues/16) |
There was a problem hiding this comment.
Doctest now runs without workspace patch unintentionally
Medium Severity
The sed command removing [patch.crates-io] was inserted above the pre-existing cargo test --doc --workspace line, changing its execution context. Previously, doctests ran with the workspace patch (against local tokio). Now they only run after the patch is removed, meaning non-tokio crate doctests resolve tokio from crates.io instead of using local code. There's no doctest run with the patch at all, since cargo nextest (line 114) doesn't support doctests. This creates a gap where doctests using unreleased tokio features would fail, and doctests are never validated against the local tokio source.
There was a problem hiding this comment.
value:annoying; category:bug; feedback: The Bugbot AI reviewer is not correct! All "tokio-**" crates depend on already released version of "tokio". If they need to use the currently in development version of "tokio" then they need to use path = "../tokio" until the next release, so they will be able to compile.
value:useful; category:bug; feedback: The CodeRabbit AI reviewer is correct! Cargo resolves to the bigger version when the same dependency is used in both |
value:useful; category:bug; feedback: The Claude AI reviewer is correct! The Github actions should be installed in a consistent way. |
2 participants
7848: To review by AI