Repository navigation
ci: check dependency licenses with cargo-deny on every PR - #3293
Conversation
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.
There was a problem hiding this comment.
💯 ty for this! i didn't know about this option.
| 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! |
kevinjqliu
left a comment
kevinjqliu
left a comment
There was a problem hiding this comment.
LGTM!
I updated the pr with some nits and documented the current changes 😄
| cd "${REPO_ROOT}" | ||
| cargo deny check license | ||
| ) | ||
| "${SCRIPT_DIR}/dependencies.sh" check |
There was a problem hiding this comment.
use the same script for create_rc.sh
| uses: taiki-e/install-action@d438492cf8a250514fa2d34b30bc3c0dc37c65ff # v2.87.8 | ||
| with: | ||
| tool: taplo@0.9.3,cargo-machete@0.7.0 | ||
| tool: taplo@0.9.3,cargo-machete@0.7.0,cargo-deny@0.19.9 |
There was a problem hiding this comment.
i think we might be able to get rid of the "keep versions in sync" pattern, now that we have make install-* in Makefile.
this can be a follow up 😄
| [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.
💯 ty for this! i didn't know about this option.
|
|
||
| Dependency licenses must comply with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html). | ||
| CI checks them against `deny.toml` with `cargo deny`; run `make check-dependency-licenses` to check locally. If a | ||
| license is rejected, find its category in the policy: |
There was a problem hiding this comment.
keeping this part concise
| "CC0-1.0", | ||
| "Zlib", | ||
| "CDLA-Permissive-2.0", | ||
| "bzip2-1.0.6", |
There was a problem hiding this comment.
this is flagged by my agent: removing bzip2-1.0.6 since nothing uses it anymore.
cargo deny flags it as license-not-encountered even with all-features, and there's no bzip2 crate in Cargo.lock. looks like it was added back in the 0.8.0 bump (#1938) for a dep we've since dropped. if it comes back, the check will fail and we can re-add it then.
|
Thanks for the change @comphead and thank you @dannycjones for the review |
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.
- Trim comments and docs that over-explain. - Condense the CONTRIBUTING.md license guidance. - create_rc.sh calls dependencies.sh check so the release uses the pinned cargo-deny version, same as CI. - Drop unused bzip2-1.0.6 from the deny.toml allow-list. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
e8e693a to
06402e7
Compare
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.dependencies.shno longer refers to the CI TSV check removed in chore: Move dependency list generation back to release manager task #2706.bzip2-1.0.6is removed fromallowindeny.toml. No dependency uses it, and cargo-deny warnedlicense-not-encountered.As suggested in #3233, the release-time check in
create_rc.shstays. It now callsdependencies.sh check, so it uses the same pinned cargo-deny version as CI. 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.create_rc.shwith only the dependency check enabled passes, and fails when thecoloredexception is removed.The new CI step also passes in GitHub Actions on this PR.
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.