Repository navigation
chore: check for circular dependencies with cargo publish - #26582
Merged
Merged
Conversation
The Circular Dependency Check built dev/depcheck, which links the cargo crate (396 packages, including libgit2, curl, and sqlite) only to load the workspace and look for a cycle in the dependencies between DataFusion crates. The build and run took 130 seconds in a 4-vCPU container, and the tool's cargo dependency and toolchain pin needed their own updates (for example apache#24649 and apache#25935). Such a cycle blocks publishing to crates.io. `cargo publish` resolves all of a crate's dependencies, dev-dependencies included, so a crate can be published only after every DataFusion crate it depends on, and no crate in a cycle can go first (apache#9277). Cargo refuses to build a cycle of normal dependencies, but it allows one that passes through a dev-dependency, so that kind needs a check of its own. Since Rust 1.90, `cargo publish --workspace` publishes all of a workspace's crates in dependency order. check_circular_dependencies.sh now runs it with `--dry-run --no-verify`, which packages every published crate without building or uploading anything. Because it runs the publish step itself, it also catches other problems that would block a release, such as a dependency without a version. On main the dry run takes 13 to 30 seconds locally, including from a fresh checkout with an empty Cargo cache and no submodules, and it skips the 11 crates with `publish = false`. With a temporary dev-dependency of datafusion-common on datafusion-expr, `cargo check --tests` passes and the dry run fails. It fails whether or not the workspace version is already on crates.io, with different errors: "circular dependency detected while publishing" when it is, and "failed to select a version", as in apache#9277, when it is not. Neither error names the cycle itself. The job keeps its name, since catching cycles is its main purpose and renaming a required check would leave every open PR without it until the PR runs CI again. It now runs on ubuntu-slim with setup-rustup, without the amd64/rust container or submodules. Remove dev/depcheck, its Dependabot entry, and its workspace exclusion.
neilconway
changed the title
cargo publish
kumarUjjawal
approved these changes
Oct 22, 2026
kumarUjjawal
left a comment
kumarUjjawal
left a comment
Contributor
There was a problem hiding this comment.
Thanks @neilconway
LGTM!
github-actions
Bot
added
documentation
Contributor
|
Merged main to PR branch to trigger CI. |
Contributor
|
CI looks good so merging |
Contributor
|
awesome! |
lyne7-sc
pushed a commit
to lyne7-sc/datafusion
that referenced
this pull request
Oct 23, 2026
## Which issue does this PR close? - N/A ## Rationale for this change The semver check previously compared the PR against the current state of `apache/main`, and did so by doing a full `git` checkout, including the entire repository history. This approach has two downsides: 1. It is expensive: cloning the entire repo history takes ~9 sec locally, versus ~0.7 for a shallow checkout. 2. There's a race condition which results in comparing the PR against the wrong base when assessing whether the PR breaks semantic versioning. On (2): when a PR is opened or updated, GitHub creates a test merge commit that merges the PR into main as of that moment (T0). Now suppose that there is a delay between the CI jobs being queued and the semver job actually running, so that when the semver job runs, `HEAD` is now at T1. The previous version of this check would compare the PR with T1, not T0. For example, if a commit between T0 and T1 added a new API, the semver check would claim that the PR _removes_ that API. Fix this by comparing the merge commit with its first parent instead, which is T0 in the notation above. ## What changes are included in this PR? See above. ## What is the testing strategy for this PR? CI / dev tooling change only. Tested locally on six open PRs (apache#26565, apache#26000, apache#26582, apache#25874, apache#26534, apache#26576): fetched each PR's merge ref at depth 2 and ran both steps against `HEAD^1`. The crate lists and semver results match CI, including two PRs that branched 66 and 154 commits behind main. ## Are there any user-facing changes? No.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Rationale for this change
Normal
cargocommands reject circular dependencies between packages, but they allow cycles through dev-dependencies. This has bitten us in the past, which is why thedev/depchecktool and its associated CI job were added to detect dev-dependency cycles.Running
dev/depcheckin CI is relatively expensive, in part because it depends on thecargocrate, which has a large dependency tree. Building and runningdepchecktakes ~130 seconds on a 4 vCPU container, and also requires anubuntu-latestrunner.Cargo 1.90 added support for multiple packages in a single
cargo publish, which does check for dev-dependency cycles. It follows that we can use a wrapper script overcargo publish --workspace --dry-runto replacedepcheck. This is more efficient and can run onubuntu-slim. Moreover, it avoids the need to maintain a separate tool and ensures that the CI check is consistent with how packages are actually published. (We should probably also switch to using multi-packagecargo publishto make releases, but I'll defer that for now.)One downside is that error reporting when a cycle is found is less helpful, but I don't think that is a major concern.
What changes are included in this PR?
dev/depcheckci/scripts/check_circular_dependencies.shto callcargo publish --dry-run --workspaceubuntu-slimWhat is the testing strategy for this PR?
CI / tooling only. Manual local testing:
cargo testpasses, andcargo package/cargo publish --workspace --dry-runfail.publish = falsecrates.cargo check --testspasses, and the dry run fails.cargo checkfails by itself.Are there any user-facing changes?
No.