jerryshao commented on PR #13257: URL: https://github.com/apache/gravitino/pull/13257#issuecomment-5754393473
**Verdict:** blocking issues — one artifact-content error in the Javadoc JARs, plus two gaps that let legal omissions pass unnoticed. ## Findings **1. `build.gradle.kts:815-820` — the Javadoc JARs of `:spark-connector:spark-3.5` / `:spark-connector:spark-4.0` drop the Iceberg notice on a false premise.** The comment says "The copied Spark transform implementation is private and absent from Javadoc", and `sourceNotices` filters `iceberg` out for those two modules. But `SparkTransformConverter` is declared `public class` (`spark-connector/spark-common/src/main/java/org/apache/gravitino/spark/connector/SparkTransformConverter.java:61`), `spark-connector/v3.5/spark/build.gradle.kts:50-58` puts `spark-common/src/main/java` on the module's main source set, and the module applies `maven-publish` (`:22`). The default `javadoc` task therefore documents that class, so `gravitino-spark-connector-runtime-3.5-*-javadoc.jar` will describe Iceberg-derived code with no Iceberg attribution in its `META-INF/NOTICE` — while this same PR adds that file to the root `LICENSE` Iceberg section (`LICENSE:233`). Either drop the `when` branch, or make the class non-public / exclude it from the Javadoc task. Note that `dev/release/maven/test-legal-files.gradle:69-71` asserts the exclusion, so the test enco des the same wrong premise and must change with it. (verified by: read the class declaration, the source-set wiring and the publishing block in the checked-out PR head; no javadoc exclusion exists anywhere in the spark module or in the root `tasks.withType<Javadoc>` block at `build.gradle.kts:760-775`) **2. `build.gradle.kts:215-226` — a bundled dependency that ships no legal file and has no supplement is silently absent from the generated `META-INF/LICENSE`.** The inventory is built from `documents.keys` only. A component appears solely if the loop at `:166-210` added at least one document for it, and `:202` only adds supplements `if (hasContent)` and only when `dependencies.txt` names files. The six label-only rows — `dev/release/maven/dependencies.txt:42-47` (`javax.activation`, `jaxb-api`, `checker-qual`, `stax2-api`, `org.slf4j:*`, `net.java.dev.jna:*`) — contribute the `| label` text through `:222` only when the dependency's own JAR already carries a legal resource. If any of them (or any dependency not listed at all) ships none, the shaded artifact bundles it with no mention in LICENSE and nothing fails. Add a check that every entry of `dependencyIds` ends up in the inventory, and fail the build for any bundled coordinate that yields neither a document nor a supplement. (verified by: traced `add()` → `documents` → `components` in `GenerateJarLegalFiles.generate()`; `TestRuntimeJarLegalFiles.testMainDocumentsIdentifyBundledComponentsAndNotices` only asserts the reverse direction — every `META-INF/licenses/` path is named in LICENSE) **3. `dev/release/maven/test-legal-files.gradle:58-66` — the source-mapping check can only confirm mappings that already exist, never detect a missing one.** `if (notice == null) return` skips every root-`LICENSE` section that has no matching `NOTICE.<name>` template, so a copied-source section whose notice was never authored is unreachable by the assertion at `:65`. Concretely, `LICENSE:266` lists `Lance Namespace` with two `src/main/java` files under `:lance:lance-common`, there is no `dev/release/maven/NOTICE.lance`, and `:lance:lance-common` is absent from `sourceNoticeNames` (`build.gradle.kts:790-805`) — so `lance-common`'s JARs carry no Lance attribution and the check stays green. Please confirm whether upstream `lance-namespace-impls` ships a NOTICE; if it does, add the template and the mapping. Either way, make the skip loud (e.g. list the sections deliberately without a notice, and fail on any section not in that list). (verified by: read the check's control flow, grepped `dev/release/maven/` for `NOTICE.lance`, and read the `sourceNoticeNames` map) **4. `dev/release/maven/dependencies.txt:48-49` — version-pinned supplement rows degrade silently on a dependency bump.** `supplement()` (`build.gradle.kts:161-165`) falls back `group:artifact:version` → `group:artifact` → `group:*` → `""`. There is no `com.fasterxml.jackson.core:jackson-core` or `com.fasterxml.jackson.core:*` row, so bumping `jackson` from `2.15.2` (`gradle/libs.versions.toml:26`) drops the FastDoubleParser and Schubfach texts from every shaded artifact with no error — `require` only fires for a *named* file that is missing. `TestRuntimeJarLegalFiles.testMissingBundledLicensesAreSupplemented` catches this for `client-java-runtime` alone. A catch-all `com.fasterxml.jackson.core:jackson-core=` row that fails with an explicit "unaudited version" message would make the bump self-reporting. (verified by: read `supplement()`, the two jackson rows, and the version catalog entry) ## Tests The generator itself is covered well: `test-legal-files.gradle` exercises selection, supplement precedence, nested-inventory regeneration, exclusions, determinism and the two failure paths against synthetic ZIPs, and `TestRuntimeJarLegalFiles` checks a real shaded artifact end to end. What is missing is breadth over the artifacts this PR actually changes — the four artifact tests all target `clients/client-java-runtime`. Nothing asserts the produced content of the CLI fat JAR (the only consumer of the new `:clients:cli` path at `build.gradle.kts:855-862`), of any of the other shaded bundles, or of the two WARs whose packaging changed at `build.gradle.kts:866-875` — in particular nothing asserts that `web-v2/web`'s WAR now carries `web-v2/web/LICENSE.bin` rather than `web/web`'s, which is one of the bugs this PR claims to fix. A single parameterised assertion over one bundle, the CLI JAR and both WARs would cover the three code paths that currently have none. Also missing: the coverage-direction gap in finding 2. I could not execute the build to confirm the generated artifacts (`./gradlew` requires JDK 17; only JDK 21 is installed here), so all findings above come from reading the code at `24fc0a2`. ## Nits - `build.gradle.kts:222` emits an em dash into `META-INF/LICENSE`; the surrounding legal texts are ASCII, and some release-inspection tooling reads these files as ASCII. - `catalogs/catalog-common/src/main/java/org/apache/gravitino/catalog/glue/GravitinoGlueCredentialsProvider.java` lost its "Adapted from Apache Doris's `CustomAwsCredentialsProvider`" class comment in #12575. This PR restores the root-`LICENSE` entry and documents the provenance in `dev/release/maven/README.md:97-100`; restoring the one-line comment in the source would keep the provenance visible where the next mover will see it. - `build.gradle.kts:166` uses `ids.getValue(it.name)`, which throws a bare `NoSuchElementException` if a file in `includedDependencies` has no resolved artifact. The neighbouring `require` calls give good messages; this one is worth matching. --- _Generated by [Claude Code](https://claude.ai/code)_ -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
