Repository navigation
Conversation
Include reviewed native dependency license texts and notices in shaded artifacts and validate the generated bundle and packaged legal entries. Co-authored-by: Codex <codex@openai.com>
Select shared and platform-specific legal materials from actual native payloads, review code-generation output separately from host tooling, consolidate Apache terms with pinned exception review, and curate required notices. Co-authored-by: Codex <codex@openai.com>
Replace the broad dependency inventory and generation tooling with a maintained binary LICENSE for the confirmed lz4_flex and Zstandard omissions. Check the shaded main and sources JARs using the existing Maven verification path. Co-authored-by: Codex <codex@openai.com>
Remove the Binary license materials section while preserving the binary LICENSE, Maven packaging and artifact checks. Co-authored-by: Codex <codex@openai.com>
andygrove
left a comment
andygrove
left a comment
There was a problem hiding this comment.
Thanks for catching this. I checked the lz4_flex and Zstandard texts against upstream (0.14.0 and v1.5.7) and they match. The versions also line up with native/Cargo.lock, and the Apache section matches LICENSE.txt.
Three things I'd like to see addressed.
The audit only covers two crates, which the description says openly. The native library also statically includes the Rust zstd, zstd-safe and zstd-sys crates and the other permissively licensed crates that parquet pulls in. I opened #7383 for the rest of the audit. Could you link it from the description so it doesn't get lost?
In dev/ensure-jars-have-correct-contents.sh, the new check only compares the JAR's META-INF/LICENSE with LICENSE-binary.txt. The text names lz4_flex 0.14.0 and Zstandard 1.5.7, so a bump in native/Cargo.lock would keep passing with the wrong version in the license. Would it be possible to also check those versions against the lock file?
In spark-integration/pom.xml, the sources JAR path is built from spark.version.short, scala.binary.version and project.version. Is that file guaranteed to exist in every profile that runs check-jar-contents? I'm wondering what happens with -Dmaven.source.skip=true or if the artifact name changes. Could the path come from a property, or could the script handle a missing sources JAR explicitly?
Check the two bundled compression license versions against Cargo.lock. Make the sources-JAR check configurable, report a missing expected source artifact explicitly, and honor maven.source.skip without skipping main-JAR checks. Co-authored-by: Codex <codex@openai.com>
sunchao
left a comment
sunchao
left a comment
There was a problem hiding this comment.
The license texts match the pinned upstream versions, and the checker rejects a truncated license. I found one regression in the existing binary JAR contents check.
Keep the binary allowlist strict while checking the explicit sources JAR with its three additional source paths. Track artifact roles by position so supplying the same file in both roles does not relax the binary check. Add paired artifact regression tests to the existing CI preflight. Co-authored-by: Codex <codex@openai.com>
andygrove
left a comment
andygrove
left a comment
There was a problem hiding this comment.
Looks like all review comments are addressed. Thanks @manuzhang
sunchao
left a comment
sunchao
left a comment
There was a problem hiding this comment.
Reviewed 74ad7e4 across correctness, resources, design, tests and release packaging. The source-only allowlist exceptions are now restricted to the explicitly supplied source JAR, addressing the previous finding. All four checker regression tests passed locally; the bundled license texts match the pinned upstream sources. Exact-head fork CI is green. No remaining actionable issue found.
Which issue does this PR close?
No separate issue. This addresses two native binary licensing omissions found while verifying 1.2.0-rc1.
Rationale for this change
The 1.2.0-rc1 JARs bundle lz4_flex and Zstandard code through libcomet, but their license and copyright texts are absent from the JAR legal resources.
What changes are included in this PR?
native/Cargo.lock. The sources-JAR path is configurable throughcomet.spark.sources.jar; an expected missing JAR fails explicitly, whilemaven.source.skip=trueintentionally skips its verification.This fixes the two confirmed omissions. It does not claim a complete audit of every bundled native dependency. The remaining audit is tracked in #7383.
How are these changes tested?
clean verify -DskipTestspassed with real macOS native payloads, as did incremental Spark 4.1 packaging. All four main/sources JARs have exactly one matching LICENSE and the unchanged project NOTICE. Committed-source Apache RAT and formatting passed.82b1ed3ba615997d4f2a77b301104ded557c794fon attempt 1: 23 successful jobs, 18 skipped and no failures. Native build, Clippy, Rust tests (2,732 default; 15 Python-UDF; five jemalloc), all four Spark 4.1 shards (5,531 passed; 31 canceled; 18 ignored), TPC-H/TPC-DS result verification, and real Linux main/sources JAR license checks passed.