Repository navigation
fix(cargo-deny): clarify zstd-sys and brotli-decompressor - #3414
dannycjones wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
7 open findings
declared_clarifications()indexesblock[\"crate\"]directly. Ifdeny.tomlhas a malformed… · Newdeclared_clarifications()indexesblock[\"crate\"]directly. Ifdeny.tomlhas a malformed… · Newdeclared_clarifications()indexesblock[\"crate\"]directly. Ifdeny.tomlhas a malformed… · Newcommand_check_dependenciesrunscargo deny check licenseand then always re-runs it again… · Newcommand_check_dependenciesrunscargo deny check licenseand then always re-runs it again… · New#(likely a non-breaking… · New
What changed in this PR
This PR adds license clarification entries for zstd-sys and brotli-decompressor in deny.toml, and enhances the dev/release/licenses.py helper to validate that cargo-deny clarifications are actually applied (and not stale) by parsing cargo-deny’s JSON diagnostics.
Changes:
- Add a PEP 723-style script header and new validation logic in
dev/release/licenses.pyto detect stale/ignored cargo-deny clarifications and to compute clarification file hashes. - Update
deny.tomlwith configuration to handle unused allowlist items and with new[[licenses.clarify]]blocks forzstd-sysandbrotli-decompressor.
| File | Description |
|---|---|
| dev/release/licenses.py | Adds clarification validation by re-running cargo-deny with JSON output parsing, plus a helper command to compute clarification hashes. |
| deny.toml | Adds license clarification blocks for two crates and tweaks settings around unused allowlist/exception entries. |
🧠 Review effort: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| with DENY_TOML.open("rb") as handle: | ||
| config = tomllib.load(handle) | ||
|
|
||
| blocks = config.get("licenses", {}).get("clarify", []) |
There was a problem hiding this comment.
I actually had implemented checking here, but I removed it to keep the code simpler and easier to follow.
I'm happy to be challenged on this, and we can reintroduce the checks. Reviewers, please let me know if this is a concern.
9c7cdfb to
677ae8f
Compare
677ae8f to
3f1ce2c
Compare
3f1ce2c to
683d60c
Compare
|
@CTTY or @kevinjqliu, please would you be able to take a pass and maybe merge this one? Then there's one last PR to actually generate the attribution notices. From there, we can focus on 0.11! |
blackmwk
left a comment
blackmwk
left a comment
There was a problem hiding this comment.
Reviewed at 683d60c194d243ccc3cf61946e877d2196212c62. I found one omission in the license clarification and one provenance question; both are attached inline.
The GPL-2.0-only OR BSD-3-Clause alternative is compatible with selecting BSD under ASF's license-choice policy. Likewise, the Brotli BSD-3-Clause AND MIT expression correctly represents cumulative obligations; the planned attribution follow-up still needs to provide the required texts and notices in distributed wheels.
Validation: the dependency check passes with CI-pinned cargo-deny 0.19.9, and all six configured hashes match the crate sources. Additional probes correctly rejected mismatching hashes, stale clarifications, disallowed license expressions, version pins, and a clarification missing from only one evaluated version.
This review was drafted by an AI-assisted tool and
confirmed by an Apache Iceberg Rust maintainer. The findings
below are observations, not blockers; an Apache Iceberg Rust
maintainer — a real person — will take the next look at the
PR. If you think a finding is mis-applied, please reply on
the PR and a maintainer will weigh in.More on how Apache Iceberg Rust handles maintainer review:
Contributing.
| # https://github.com/apache/iceberg-rust/issues/3307 | ||
| [[licenses.clarify]] | ||
| crate = "zstd-sys" | ||
| expression = "(MIT OR Apache-2.0) AND BSD-3-Clause AND (GPL-2.0-only OR BSD-3-Clause)" |
There was a problem hiding this comment.
Include the embedded BSD-2-Clause code in the zstd clarification (deny.toml:59). The packaged legacy decoders contain BSD-2-Clause licenses and Yann Collet copyright notices, for example zstd_v05.c. I checked that the packaged source matches upstream v1.5.7 and that the dependency graph enables zstd-sys/legacy. Please account for these embedded licenses in the expression and supporting evidence, and preserve the corresponding notices in the planned binary attribution output. ASF requires that every third-party work include its associated license; checking the crate's top-level license files alone misses these notices. See ASF's third-party source policy and binary distribution guidance.
There was a problem hiding this comment.
Nice catch - wow this complicates things :)
I've found a way to do the attribution in the later PR, not too complicated. I'll update this expression now.
I will need to hash the C file itself.
The C file's content does change from time to time (twice last year: https://github.com/facebook/zstd/commits/dev/lib/legacy/zstd_v05.c) - I think it will be a necessary cost to revalidate when we update zstd unfortunately.
Note, I will include only the one file for now. I think its important that we understand where we can remove clarifications when the license no longer applies. New licenses appearing is a moving target which we can't reliably verify when they're not declared.
There was a problem hiding this comment.
It sounds like we will run binary attribution verification in a different way other than the existing script?
| XxHash32 with seed 0, over the file with every line terminated by exactly one LF. | ||
|
|
||
| cargo-deny appears to offer no way to compute this value, | ||
| so this command is added to make it easy (based on the cargo-deny source code). |
There was a problem hiding this comment.
Clarify the hashing helper's provenance (licenses.py:323). The docstring says it is "based on the cargo-deny source code." Does that mean independently implementing the algorithm, or copying/translating upstream code? If code was copied or adapted, please document its origin and selected upstream license and preserve applicable notices. This is a provenance question, not a confirmed license violation. ASF's policy says: "Do not modify or remove any copyright notices or licenses within third-party works."
There was a problem hiding this comment.
I'll reword to clarify its not copied.
750c56b to
68ea8c6
Compare
CTTY
left a comment
CTTY
left a comment
There was a problem hiding this comment.
LGTM, thanks Danny for plumbing through this!
kevinjqliu
left a comment
kevinjqliu
left a comment
There was a problem hiding this comment.
i havent reviewed this fully but the size of the diff concerns me a bit 😅
|
do we really need these clarifications? im worried about the maintenance burden of adding this PR. From what I've gathered, we're doing this for the python wheel attribution. Could we take a look at datafusion-python and follow its model?
|
Which issue does this PR close?
Kind of related to #3239.
What changes are included in this PR?
Introduce clarifications for
zstd-sysandbrotli-decompressor. This doesn't impact the actual result - we're still meeting our allowlist of licenses. However, this accurately represents the dependencies we have.Due to having no way to fail when clarifications go stale or fail to apply, the Python script checks diagnostic output to fail builds in these cases.
Are these changes tested?
N/A
AI Disclosure
Used LLM to help generate these scripts - especially how to calculate the hash for clarifications. I have reviewed, modified, and stand by the changes.