Conversation
Run `dev/release/dependencies.sh check` in the `lint` job, so a dependency whose license is not allowed by `deny.toml` fails the pull request instead of being found at release time. Set `[graph] all-features = true` in `deny.toml`. Without it, cargo-deny only follows default features. Since apache#3143 that graph no longer includes the optional OpenDAL backends, so the check skipped the MPL-2.0 crates that `deny.toml` has exceptions for. Closes apache#3234.
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Release-time cargo-deny version validation is not aligned with the CI and local tooling pin.
Review effort: Lite
Findings: None
What changed in this PR
Adds PR-time and local cargo-deny license checks, including all dependency features, with updated contributor and release documentation.
Changes:
- Pins
cargo-deny0.19.9 for CI and local checks. - Enables all-feature license validation.
- Updates license policy and release guidance.
| File | Summary |
|---|---|
website/src/release.md |
Updates release tooling and license-check guidance. |
Makefile |
Adds local license-check targets. |
dev/release/README.md |
Documents pinned cargo-deny installation and CI checks. |
dev/release/dependencies.sh |
Updates cargo-deny version references and validation. |
deny.toml |
Enables all-feature dependency analysis. |
CONTRIBUTING.md |
Documents dependency license policy. |
.github/workflows/ci.yml |
Adds the CI license-check step. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Thanks for looking at this, both you and I ended up implementing similar changes. (I should have posted on #3234, my bad!) I only opened #3299 today however my approach has been to move the invocation of cargo-deny into a Python script, as I found there were other gaps with it. It's a precursor to addressing #3239. Let me know what you think. I do particularly like that there is guidance added to CONTRIBUTING.md here, I will take a closer look. |
dannycjones
left a comment
dannycjones
left a comment
There was a problem hiding this comment.
Specifically took a look at the changes different from #3299.
| [graph] | ||
| # Also check dependencies that only optional features pull in, such as the | ||
| # OpenDAL storage backends. Otherwise cargo-deny only follows default features. | ||
| all-features = true |
There was a problem hiding this comment.
Good catch - I do think we need this.
| Every dependency must have a license that is compatible with the | ||
| [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html). `deny.toml` lists the allowed licenses | ||
| and the per-crate exceptions. CI checks every pull request against it with `cargo deny`, and you can run the same | ||
| check locally with `make check-dependency-licenses`. | ||
|
|
||
| If the check rejects a license, look up its category in the ASF policy: | ||
|
|
||
| - Category A licenses can be added to `allow` in `deny.toml`. | ||
| - Category B licenses are added to `exceptions` in `deny.toml`, with one entry per crate that uses them. | ||
| - Category X licenses are not allowed, so the dependency must be replaced. | ||
|
|
||
| Explain any change to `deny.toml` in the pull request description. |
There was a problem hiding this comment.
Thanks for adding this, looks good
There was a problem hiding this comment.
LGTM! We can see the check working here: https://github.com/apache/iceberg-rust/actions/runs/36485758300/job/109142138283?pr=3293#step:11:16
Let's merge this, I'll work on dropping TSVs and then rebasing #3299 on top here. (More info: #3299 (comment))
|
@kevinjqliu @laskoviymishka if either of you can help here with review and merge, it'd be much appreciated! |
Which issue does this PR close?
What changes are included in this PR?
The
lintjob inci.ymlnow runsdev/release/dependencies.sh check, the samecargo denylicense check the release process runs. A PR that adds a dependency whose licensedeny.tomldoes not allow now fails CI instead of being caught at release time. cargo-deny 0.19.9 is installed by the existingtaiki-e/install-actionstep, matching the version pinned independencies.sh.lintalready feedsci-required, so.asf.yamldoes not change.deny.tomlnow sets[graph] all-features = true. Without it, cargo-deny only follows default features. #3143 removedopendal-allfrom the Python bindings, so the default graph no longer includes the optional OpenDAL backends (gcs, oss, azdls, hf). Today the default graph has 503 crates and the all-features graph has 619. The missing crates include the two MPL-2.0 crates thatdeny.tomlhas exceptions for (coloredandoption-ext, pulled in byopendal-service-hf). This setting also applies to the release-time check.Other changes:
make check-dependency-licensesruns the check locally, andmake checknow includes it.CONTRIBUTING.mddescribes the license policy and what to do when the check rejects a license.make install-cargo-denyfor the pinned version and note that CI already runs the check.dependencies.shno longer refer to the CI TSV check removed in chore: Move dependency list generation back to release manager task #2706.As suggested in #3233, the release-time check in
create_rc.shstays. TSV generation is unchanged.Unlike the TSV check reverted in #2706, this check does not ask PRs to regenerate any files. Dependabot PRs pass unless an update brings in a license that
deny.tomldoes not allow.Are these changes tested?
Tested locally with cargo-deny 0.19.9:
dev/release/dependencies.sh checkandmake check-dependency-licensespass on this branch.coloredexception fromdeny.tomlmakes the check fail and namecolored v3.1.1. Onmain, the same edit still passes, because the default-feature graph does not includecolored.dev/release/dependencies.sh generateproduces identical TSV files with and without the[graph]setting.taplo fmt --checkpasses.The new CI step has not run in GitHub Actions yet. This PR is its first run.
AI Disclosure
I used Claude Code, an AI coding assistant, to investigate the issue, write the changes and this description, and run the local tests above.
Areas for reviewers to check:
CONTRIBUTING.mdguidance on Category A, B, and X licenses describes howdeny.tomlhandles them today. It is not a legal interpretation of the ASF policy.