You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "comphead (via GitHub)" <gi...@apache.org> on 2026/09/28 21:23:22 UTC
[PR] ci: check dependency licenses with cargo-deny on every PR [iceberg-rust]
comphead opened a new pull request, #3293:
URL: https://github.com/apache/iceberg-rust/pull/3293
## Which issue does this PR close?
- Closes #3234.
## What changes are included in this PR?
The `lint` job in `ci.yml` now runs `dev/release/dependencies.sh check`, the same `cargo deny` license check the release process runs. A PR that adds a dependency whose license `deny.toml` does not allow now fails CI instead of being caught at release time. cargo-deny 0.19.9 is installed by the existing `taiki-e/install-action` step, matching the version pinned in `dependencies.sh`. `lint` already feeds `ci-required`, so `.asf.yaml` does not change.
`deny.toml` now sets `[graph] all-features = true`. Without it, cargo-deny only follows default features. #3143 removed `opendal-all` from 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 that `deny.toml` has exceptions for (`colored` and `option-ext`, pulled in by `opendal-service-hf`). This setting also applies to the release-time check.
Other changes:
- `make check-dependency-licenses` runs the check locally, and `make check` now includes it.
- `CONTRIBUTING.md` describes the license policy and what to do when the check rejects a license.
- The release docs point to `make install-cargo-deny` for the pinned version and note that CI already runs the check.
- Two comments in `dependencies.sh` no longer refer to the CI TSV check removed in #2706.
As suggested in #3233, the release-time check in `create_rc.sh` stays. 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.toml` does not allow.
## Are these changes tested?
Tested locally with cargo-deny 0.19.9:
- `dev/release/dependencies.sh check` and `make check-dependency-licenses` pass on this branch.
- Removing the `colored` exception from `deny.toml` makes the check fail and name `colored v3.1.1`. On `main`, the same edit still passes, because the default-feature graph does not include `colored`.
- `dev/release/dependencies.sh generate` produces identical TSV files with and without the `[graph]` setting.
- `taplo fmt --check` passes.
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:
- The `CONTRIBUTING.md` guidance on Category A, B, and X licenses describes how `deny.toml` handles them today. It is not a legal interpretation of the ASF policy.
- The new CI step has only been tested locally so far.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] ci: check dependency licenses with cargo-deny on every PR [iceberg-rust]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on code in PR #3293:
URL: https://github.com/apache/iceberg-rust/pull/3293#discussion_r4178260908
##########
dev/release/create_rc.sh:
##########
@@ -317,12 +307,7 @@ check_rc_tag_available() {
}
check_dependency_licenses() {
- require_cargo_deny
- (
- trap - ERR
- cd "${REPO_ROOT}"
- cargo deny check license
- )
+ "${SCRIPT_DIR}/dependencies.sh" check
Review Comment:
use the same script for `create_rc.sh`
##########
.github/workflows/ci.yml:
##########
@@ -59,7 +59,7 @@ jobs:
# Keep versions in sync with the local install targets in Makefile.
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
Review 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 😄
##########
deny.toml:
##########
@@ -26,7 +30,6 @@ allow = [
"CC0-1.0",
"Zlib",
"CDLA-Permissive-2.0",
- "bzip2-1.0.6",
Review 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.
##########
deny.toml:
##########
@@ -15,6 +15,15 @@
# specific language governing permissions and limitations
# under the License.
+# Dependency license policy. CI checks it on every pull request, and the
+# release scripts check it again before creating a release candidate. See
+# "Dependencies" in CONTRIBUTING.md for what to do when a license is rejected.
+
+[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
Review Comment:
💯 ty for this! i didn't know about this option.
##########
CONTRIBUTING.md:
##########
@@ -118,6 +118,14 @@ tested in CI and developers have reproducible builds.
In `Cargo.toml`, we specify the minimum version required to use iceberg-rust. This allows users to choose their
dependency versions without always upgrading to the latest.
+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:
Review Comment:
keeping this part concise
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] ci: check dependency licenses with cargo-deny on every PR [iceberg-rust]
Posted by "dannycjones (via GitHub)" <gi...@apache.org>.
dannycjones commented on PR #3293:
URL: https://github.com/apache/iceberg-rust/pull/3293#issuecomment-5894177170
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] ci: check dependency licenses with cargo-deny on every PR [iceberg-rust]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on PR #3293:
URL: https://github.com/apache/iceberg-rust/pull/3293#issuecomment-5981663819
Thanks for the change @comphead and thank you @dannycjones for the review
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] ci: check dependency licenses with cargo-deny on every PR [iceberg-rust]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu merged PR #3293:
URL: https://github.com/apache/iceberg-rust/pull/3293
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] ci: check dependency licenses with cargo-deny on every PR [iceberg-rust]
Posted by "dannycjones (via GitHub)" <gi...@apache.org>.
dannycjones commented on PR #3293:
URL: https://github.com/apache/iceberg-rust/pull/3293#issuecomment-5939803332
@kevinjqliu @laskoviymishka if either of you can help here with review and merge, it'd be much appreciated!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] ci: check dependency licenses with cargo-deny on every PR [iceberg-rust]
Posted by "dannycjones (via GitHub)" <gi...@apache.org>.
dannycjones commented on code in PR #3293:
URL: https://github.com/apache/iceberg-rust/pull/3293#discussion_r4135873175
##########
deny.toml:
##########
@@ -15,6 +15,15 @@
# specific language governing permissions and limitations
# under the License.
+# Dependency license policy. CI checks it on every pull request, and the
+# release scripts check it again before creating a release candidate. See
+# "Dependencies" in CONTRIBUTING.md for what to do when a license is rejected.
+
+[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
Review Comment:
Good catch - I do think we need this.
##########
CONTRIBUTING.md:
##########
@@ -118,6 +118,19 @@ tested in CI and developers have reproducible builds.
In `Cargo.toml`, we specify the minimum version required to use iceberg-rust. This allows users to choose their
dependency versions without always upgrading to the latest.
+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.
Review Comment:
Thanks for adding this, looks good
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org