jerryshao commented on code in PR #13257: URL: https://github.com/apache/gravitino/pull/13257#discussion_r4059826849
########## dev/release/maven/test-legal-files.gradle: ########## @@ -0,0 +1,296 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import java.nio.charset.StandardCharsets +import java.util.zip.ZipEntry +import java.util.zip.ZipFile +import java.util.zip.ZipOutputStream +import org.gradle.api.tasks.SourceSetContainer +import org.gradle.api.tasks.bundling.War + +// Synthetic JARs keep generator checks independent of dependency downloads. +def generatorType = project(':api').tasks.named('generateMavenLegalFiles').get().class.superclass +def fixtureGenerator = tasks.register('mavenLegalTestFixture', generatorType) +def legalTests = tasks.register('testMavenLegalFiles') { + group = 'verification' + description = 'Checks Maven legal templates, source mappings and packaging edge cases.' + doLast { + def templates = file('dev/release/maven') + def expectFailure = { String message, Closure action -> + boolean failed = false + try { + action.call() + } catch (IllegalArgumentException error) { + assert error.message.contains(message): error.message + failed = true + } + assert failed: "Expected failure: ${message}" + } + ['LICENSE', 'NOTICE'].each { name -> + assert file(name).text.trim().startsWith(new File(templates, name).text.trim()): "Maven ${name} differs from the root prefix" + } + def normalizedRootNotice = file('NOTICE').text.replaceAll(/\s+/, ' ') + def modules = subprojects.findAll { it.tasks.findByName('generateMavenLegalFiles') != null } + modules.each { module -> + module.tasks.named('generateMavenLegalFiles').get().sourceNotices.get().each { name -> + new File(templates, "NOTICE.${name}").readLines().findAll { it.trim() }.each { line -> + assert normalizedRootNotice.contains(line.trim().replaceAll(/\s+/, ' ')): "Root NOTICE is missing ${name}: ${line}" + } + } + def shadow = module.tasks.findByName('shadowJar') + def slf4jPatterns = shadow == null ? [] : shadow.excludes.findAll { it.startsWith('org/slf4j/') } + if (!slf4jPatterns.isEmpty()) { + assert slf4jPatterns.toSet() == ['org/slf4j/**'].toSet(): "${module.path}: review legal exclusions when changing the SLF4J pattern" + assert module.tasks.named('generateBundledLegalFiles').get().excludedGroups.get().contains('org.slf4j'): "${module.path}: excluded SLF4J classes must not retain dependency notices" + } + } + def sourceOwners = modules.collectEntries { module -> + [(module): module.extensions.getByType(SourceSetContainer).getByName('main').allJava.files.collect { it.canonicalFile }.toSet()] + } + def reviewedWithoutNotice = [ + // These upstream trees contain LICENSE but no NOTICE: + // https://github.com/lance-format/lance-namespace/tree/v0.0.20 + // https://github.com/lance-format/lance-namespace-impls/tree/e3e202f2f655c92ee9427ab671226e1ed945a629 + 'Lance Namespace (https://github.com/lance-format/lance-namespace-impls)': [ + 'lance/lance-common/src/main/java/org/apache/gravitino/lance/common/ops/gravitino/JsonArrowSchemaConverter.java', + 'lance/lance-common/src/main/java/org/apache/gravitino/lance/common/ops/gravitino/PageUtil.java' + ], + // https://github.com/trinodb/trino/tree/435 has only a server dependency NOTICE, + // which does not apply to these copied classes. + 'Trino': [ + 'trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/catalog/hive/SortingColumn.java', + 'trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/util/json/AbstractTypedJacksonModule.java', + 'trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/util/json/BlockJsonSerde.java', + 'trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/util/json/TypeDeserializer.java', + 'trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/util/json/TypeSignatureDeserializer.java' + ] + ].collectEntries { component, paths -> [(component): paths.collect { file(it).canonicalFile }.toSet()] } + def sourceNoticeFor = { String component, Collection sources -> + def notice = templates.listFiles().find { it.name.startsWith('NOTICE.') && it.readLines().first().startsWith(component) } + if (notice == null && !reviewedWithoutNotice.getOrDefault(component, Collections.emptySet()).containsAll(sources)) { + throw new IllegalArgumentException("Review upstream NOTICE for ${component}: add a template or document the source-specific absence of an applicable notice") + } + notice + } + expectFailure('Review upstream NOTICE for Unknown source') { + sourceNoticeFor('Unknown source', [file('unknown/Source.java').canonicalFile]) + } + expectFailure('Review upstream NOTICE for Lance Namespace') { + sourceNoticeFor('Lance Namespace (https://github.com/lance-format/lance-namespace-impls)', [file('lance/NewSource.java').canonicalFile]) + } + file('LICENSE').text.split(/\n\s*\n/).each { section -> + def lines = section.readLines().collect { it.trim() } + def sources = lines.findAll { it.startsWith('./') && it.contains('/src/main/java/') }.collect { file(it).canonicalFile } + def compiledSources = sources.findAll { source -> sourceOwners.values().any { it.contains(source) } } + if (compiledSources.isEmpty()) return Review Comment: [Important] This is the one skip left in the hardened check, and it is silent. `compiledSources` keeps only paths that some module's main source set actually contains, so a root-`LICENSE` section whose `src/main/java` paths have gone stale - file renamed, or moved to another module without the `LICENSE` entry being updated - produces an empty list and returns here, before `sourceNoticeFor` runs. The module that now compiles the copied code is never checked against its `sourceNotices`, and its published JAR ships without the upstream notice: precisely the omission `sourceNoticeFor` was added to make loud. Nothing else in the build verifies that the paths listed in `LICENSE` still exist. Suggest failing on a listed source no module owns, e.g. after line 99: `assert compiledSources.size() == sources.size(): "LICENSE lists sources no module compiles: ${sources - compiledSources}"`, with an explicit allowance for any path that is deliberately outside the Maven modules. Verified by: read the control flow at this head; `modules` at :48 covers every subproject except `client-python` and the two `hive-metastore*-libs` (`build.gradle.kts:582-592`), so a filtered path means a stale path rather than an unpublished module. I also checked every `./...` path listed in `LICENSE` - all of them exist today, so this is latent, not a present miss. ########## build.gradle.kts: ########## @@ -72,6 +81,230 @@ val sharedTestEnvironmentLock = gradle.sharedServices.registerIfAbsent( maxParallelUsages.set(1) } +/** Packages the legal documents for one Maven artifact, retaining dependency provenance. */ +@CacheableTask +abstract class GenerateJarLegalFiles : DefaultTask() { + companion object { + private const val LICENSE_INVENTORY = "\nBundled component licensing:\n" + private const val NOTICE_INVENTORY = "\nBundled component notices:\n" + private val noticeFileName = Regex("(?i)([A-Za-z0-9_]+-)*NOTICES?([.-].*)?") + private val legalFileName = Regex( + "(?i)([A-Za-z0-9_]+-)*(LICENSE|LICENCE|NOTICES?|COPYING|COPYRIGHT)(-[A-Za-z0-9_-]+)?(\\.(txt|md|markdown|adoc))?" + ) + + /** Selects legal resources without mistaking SDK models such as license-manager.json for licenses. */ + fun isLegalResource(path: String): Boolean = + path == "about.html" || path.startsWith("about_files/") || + path.startsWith("licenses/") || path.startsWith("license/") || + path.startsWith("META-INF/licenses/") || path.startsWith("META-INF/license/") || + path.startsWith("META-INF/licenses-binary/") || + legalFileName.matches(path.substringAfterLast('/')) + } + + @get:Internal + abstract val templates: DirectoryProperty + + @get:InputFiles + @get:PathSensitive(PathSensitivity.RELATIVE) + val templateFiles: FileTree + get() = templates.get().asFileTree + + @get:Input + abstract val sourceNotices: ListProperty<String> + + @get:InputFiles + @get:PathSensitive(PathSensitivity.NONE) + abstract val dependencyJars: ConfigurableFileCollection + + @get:Input + abstract val dependencyIds: MapProperty<String, String> + + @get:Input + abstract val excludedGroups: SetProperty<String> + + @get:InputFiles + @get:PathSensitive(PathSensitivity.RELATIVE) + abstract val javadocFiles: ConfigurableFileCollection + + @get:OutputFile + abstract val outputArchive: RegularFileProperty + + init { + sourceNotices.convention(emptyList()) + dependencyIds.convention(emptyMap()) + excludedGroups.convention(emptySet()) + } + + @TaskAction + fun generate() { + val directory = templates.get().asFile + val ids = dependencyIds.get() + val excluded = excludedGroups.get() + // ByteBuffer compares byte contents, so identical documents are retained only once. + val documents = sortedMapOf<String, LinkedHashSet<ByteBuffer>>() + fun add(path: String, bytes: ByteArray) { + require(!path.startsWith('/') && path.split('/').none { it == ".." }) { + "Invalid legal resource path: $path" + } + val group = path.removePrefix("META-INF/licenses/").substringBefore('/') + if (path.startsWith("META-INF/licenses/") && group in excluded) return + documents.getOrPut(path) { linkedSetOf() }.add(ByteBuffer.wrap(bytes)) + } + val overrides = mutableMapOf<String, String>() + directory.resolve("dependencies.txt").readLines() + .filter { it.isNotBlank() && !it.startsWith('#') } + .forEach { line -> + val fields = line.split('=', limit = 2) + require(fields.size == 2) { "Invalid Maven legal supplement: $line" } + require(overrides.put(fields[0], fields[1]) == null) { "Duplicate Maven legal supplement: ${fields[0]}" } + } + fun supplement(id: String): String { + val parts = id.split('/') + val coordinate = "${parts[0]}:${parts[1]}" + val selected = overrides["$coordinate:${parts[2]}"] ?: overrides[coordinate] + require(selected != null || overrides.keys.none { it.startsWith("$coordinate:") }) { + "Unaudited Maven legal supplement version: $coordinate:${parts[2]}. Review dependencies.txt and upstream legal documents." Review Comment: [Nit] This failure will usually be triggered by a routine dependency bump, in whichever shaded module first resolves the new version, and the message names only the coordinate. The caller at :170-173 holds the jar file and the task knows its project, so threading those through (`"... $coordinate:${parts[2]} bundled as ${jar.name} in ${path}"`) would let whoever does the bump act on it without first tracing which configuration pulled the artifact. The inventory-loop caller at :232 has only the document prefix, so it can at least say which artifact's inventory the component came from. Verified by: read `supplement()` and both call sites at this head - `dependencies` at :169-173 pairs each id with its jar, and the failure is thrown from a plain `require` with no project or file context. ########## build.gradle.kts: ########## @@ -565,27 +798,111 @@ subprojects { from(tasks["javadoc"]) } - tasks.withType<Jar> { - into("META-INF") { - from(rootDir) { - if (name == "sourcesJar") { - include("LICENSE") - include("NOTICE") - } else if (project.name == "web") { - include("web/web/LICENSE.bin") - rename("LICENSE.bin", "LICENSE") - include("web/web/NOTICE.bin") - rename("NOTICE.bin", "NOTICE") - } else { - include("LICENSE.bin") - rename("LICENSE.bin", "LICENSE") - include("NOTICE.bin") - rename("NOTICE.bin", "NOTICE") + // These notices cover copied production sources, not dependencies declared only in the POM. + val sourceNoticeNames = mapOf( + ":api" to listOf("spark", "iceberg"), + ":common" to listOf("iceberg", "hadoop"), + ":core" to listOf("spark", "iceberg", "kafka", "aws"), + ":clients:client-java" to listOf("iceberg"), + ":catalogs:catalog-common" to listOf("doris"), + ":catalogs:hive-metastore-common" to listOf("iceberg"), + ":catalogs:catalog-lakehouse-iceberg" to listOf("iceberg"), + ":catalogs:catalog-lakehouse-paimon" to listOf("paimon"), + ":spark-connector:spark-3.5" to listOf("iceberg"), + ":spark-connector:spark-4.0" to listOf("iceberg"), + ":server-common" to listOf("hadoop"), + ":iceberg:iceberg-common" to listOf("iceberg"), + ":iceberg:iceberg-rest-server" to listOf("iceberg"), + ":authorizations:authorization-ranger" to listOf("ranger") + )[project.path].orEmpty() + + val mavenLegalFiles = tasks.register<GenerateJarLegalFiles>("generateMavenLegalFiles") { + templates.set(rootProject.layout.projectDirectory.dir("dev/release/maven")) + sourceNotices.set(sourceNoticeNames) + outputArchive.set(layout.buildDirectory.file("generated/maven-legal/main.zip")) + } + val javadocLegalFiles = tasks.register<GenerateJarLegalFiles>("generateJavadocLegalFiles") { + templates.set(rootProject.layout.projectDirectory.dir("dev/release/maven")) + // SparkTransformConverter's Iceberg-derived findWidth method is private and absent from Javadoc. + sourceNotices.set( + when (project.path) { + ":spark-connector:spark-3.5", ":spark-connector:spark-4.0" -> sourceNoticeNames.filterNot { it == "iceberg" } Review Comment: [Nit] The corrected premise holds: `SparkTransformConverter` marks only the private static `findWidth` as Iceberg-derived, and the standard doclet does not document private members, so these two Javadoc JARs really do carry no Iceberg-derived API. What changed in 8f0ae3f is that the assertion protecting that invariant is gone. Nothing now fails if `findWidth` later becomes package-private or protected, or if another Iceberg-derived member is added to the class - the Javadoc JAR would quietly ship without the Iceberg attribution while `LICENSE:233` still lists the file. Over-attribution in a Javadoc JAR costs nothing, so the cheapest fix is to drop this `when` branch; if the exclusion is worth keeping, assert the modifier instead of the task configuration (parse the `findWidth` declaration in `testMavenLegalFiles` and require `private`). Verified by: read the class at this head - line 61 `public class`, line 402 `// Referred from org.apache.iceberg.spark.Spark3Util` above `private static int findWidth`, and `grep -n -i iceberg` over the file shows no other reference; `git show 8f0ae3f` removed the `generateJavadocLegalFiles` assertion from `dev/release/maven/test-legal-files.gradle`. -- 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]
