Copilot commented on code in PR #865: URL: https://github.com/apache/pekko-grpc/pull/865#discussion_r3893639337
########## project/AssemblyLicenseCheck.scala: ########## @@ -0,0 +1,116 @@ +/* + * 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.util.zip.ZipFile + +import sbt._ +import sbt.Keys._ +import sbtassembly.AssemblyKeys._ +import org.mdedetrich.apache.sonatype.ApacheSonatypePlugin.autoImport.{ + apacheSonatypeLicenseFile, + apacheSonatypeNoticeFile +} + +/** + * Guards the LICENSE and NOTICE files that ship in the assembly jars. + * + * The assembly jars bundle classes from 3rd party projects, so their `META-INF/LICENSE` and + * `META-INF/NOTICE` have to account for those projects. Those files are curated by hand in + * `legal/`, because most of the bundled jars carry no NOTICE of their own and the text has to be + * taken from the upstream projects. These tasks stop the curated files from drifting away from + * what the jars actually bundle. + */ +object AssemblyLicenseCheck { + + val assemblyBundledModules = + taskKey[Seq[String]]("The `organization:name` ids of the 3rd party modules bundled into the assembly jar") + val assemblyLicenseCheck = + taskKey[Unit]("Check that every module bundled into the assembly jar is named in the assembly LICENSE file") + val assemblyMetaInfArchives = + taskKey[Seq[File]]("The published archives that should carry the assembly LICENSE and NOTICE files") + val assemblyMetaInfCheck = + taskKey[Unit]("Check that the published archives ship the assembly LICENSE and NOTICE files in META-INF") + + /** A module id as listed in the assembly LICENSE file: `organization:name`, optionally `:version`. */ + private val ModuleId = """^([a-zA-Z0-9][\w.\-]*):([a-zA-Z0-9][\w.\-]*)(?::.*)?$""".r + + lazy val settings: Seq[Setting[?]] = Seq( + assemblyBundledModules := { + val suffix = "_" + CrossVersion.binaryScalaVersion(scalaVersion.value) + val modules = (assembly / fullClasspath).value.flatMap(_.get(moduleID.key)) + modules + .filterNot(_.organization == "org.apache.pekko") // our own modules, covered by the ASF header + .map { module => + val name = if (module.name.endsWith(suffix)) module.name.dropRight(suffix.length) else module.name + s"${module.organization}:$name" + } + .distinct + .sorted + }, + assemblyLicenseCheck := { + val log = streams.value.log + val licenseFile = apacheSonatypeLicenseFile.value + val listed = IO.readLines(licenseFile).map(_.trim).collect { case ModuleId(org, module) => s"$org:$module" }.toSet + val bundled = assemblyBundledModules.value + + val extra = (listed -- bundled).toSeq.sorted + if (extra.nonEmpty) + // the assembly jars are cross built, so a module can be listed for a Scala version other than this one + log.warn( + s"${licenseFile.getName} lists modules that ${name.value} does not bundle for Scala ${scalaVersion.value}, " + + s"check whether they are still needed: ${extra.mkString(", ")}") + + val missing = bundled.filterNot(listed) + if (missing.nonEmpty) + sys.error( + s"${name.value} bundles modules that ${licenseFile.getName} does not account for: ${missing.mkString(", ")}") + + log.info(s"${licenseFile.getName} accounts for all ${bundled.size} modules bundled by ${name.value}") + }, + assemblyMetaInfArchives := Seq(assembly.value), + assemblyMetaInfCheck := { + val log = streams.value.log + val expected = + Seq("META-INF/LICENSE" -> apacheSonatypeLicenseFile.value, "META-INF/NOTICE" -> apacheSonatypeNoticeFile.value) + + assemblyMetaInfArchives.value.foreach { archive => + val zip = new ZipFile(archive) + val problems = + try expected.flatMap { case (path, file) => + Option(zip.getEntry(path)) match { + case None => + Some(s"$path is missing, expected a copy of $file") + case Some(entry) => + val actual = { + val in = zip.getInputStream(entry) + try in.readAllBytes() + finally in.close() + } Review Comment: Resource handling is correct but could be simplified and made more robust by using `scala.util.Using` (or similar) for both `ZipFile` and each entry `InputStream`. Also, if `new ZipFile(archive)` throws (e.g., corrupted archive), the error will be a raw exception without the contextual `archive.getName` message used later; consider wrapping zip-open failures to include the archive name for easier diagnostics in CI. ########## project/AssemblyLicenseCheck.scala: ########## @@ -0,0 +1,116 @@ +/* + * 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.util.zip.ZipFile + +import sbt._ +import sbt.Keys._ +import sbtassembly.AssemblyKeys._ +import org.mdedetrich.apache.sonatype.ApacheSonatypePlugin.autoImport.{ + apacheSonatypeLicenseFile, + apacheSonatypeNoticeFile +} + +/** + * Guards the LICENSE and NOTICE files that ship in the assembly jars. + * + * The assembly jars bundle classes from 3rd party projects, so their `META-INF/LICENSE` and + * `META-INF/NOTICE` have to account for those projects. Those files are curated by hand in + * `legal/`, because most of the bundled jars carry no NOTICE of their own and the text has to be + * taken from the upstream projects. These tasks stop the curated files from drifting away from + * what the jars actually bundle. + */ +object AssemblyLicenseCheck { + + val assemblyBundledModules = + taskKey[Seq[String]]("The `organization:name` ids of the 3rd party modules bundled into the assembly jar") + val assemblyLicenseCheck = + taskKey[Unit]("Check that every module bundled into the assembly jar is named in the assembly LICENSE file") + val assemblyMetaInfArchives = + taskKey[Seq[File]]("The published archives that should carry the assembly LICENSE and NOTICE files") + val assemblyMetaInfCheck = + taskKey[Unit]("Check that the published archives ship the assembly LICENSE and NOTICE files in META-INF") + + /** A module id as listed in the assembly LICENSE file: `organization:name`, optionally `:version`. */ + private val ModuleId = """^([a-zA-Z0-9][\w.\-]*):([a-zA-Z0-9][\w.\-]*)(?::.*)?$""".r Review Comment: `ModuleId` reads like a type (e.g., a case class) but is actually a line-matching `Regex`. Renaming to something like `moduleIdLineRegex` / `moduleIdPattern` would make call sites (`collect { case ... }`) clearer. ########## project/Dependencies.scala: ########## @@ -110,6 +110,10 @@ object Dependencies { Compile.grpcProtobuf, Test.scalaTest) + lazy val scalapbProtocPlugin = l ++= Seq( + Compile.scalapbCompilerPlugin, + Protobuf.protobufJava) // or else scalapb pulls older version in transitively Review Comment: Fix the typo/wording in the comment: remove 'in' so it reads correctly (e.g., 'pulls an older version transitively'). ########## project/AssemblyLicenseCheck.scala: ########## @@ -0,0 +1,116 @@ +/* + * 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.util.zip.ZipFile + +import sbt._ +import sbt.Keys._ +import sbtassembly.AssemblyKeys._ +import org.mdedetrich.apache.sonatype.ApacheSonatypePlugin.autoImport.{ + apacheSonatypeLicenseFile, + apacheSonatypeNoticeFile +} + +/** + * Guards the LICENSE and NOTICE files that ship in the assembly jars. + * + * The assembly jars bundle classes from 3rd party projects, so their `META-INF/LICENSE` and + * `META-INF/NOTICE` have to account for those projects. Those files are curated by hand in + * `legal/`, because most of the bundled jars carry no NOTICE of their own and the text has to be + * taken from the upstream projects. These tasks stop the curated files from drifting away from + * what the jars actually bundle. + */ +object AssemblyLicenseCheck { + + val assemblyBundledModules = + taskKey[Seq[String]]("The `organization:name` ids of the 3rd party modules bundled into the assembly jar") + val assemblyLicenseCheck = + taskKey[Unit]("Check that every module bundled into the assembly jar is named in the assembly LICENSE file") + val assemblyMetaInfArchives = + taskKey[Seq[File]]("The published archives that should carry the assembly LICENSE and NOTICE files") + val assemblyMetaInfCheck = + taskKey[Unit]("Check that the published archives ship the assembly LICENSE and NOTICE files in META-INF") + + /** A module id as listed in the assembly LICENSE file: `organization:name`, optionally `:version`. */ + private val ModuleId = """^([a-zA-Z0-9][\w.\-]*):([a-zA-Z0-9][\w.\-]*)(?::.*)?$""".r + + lazy val settings: Seq[Setting[?]] = Seq( + assemblyBundledModules := { + val suffix = "_" + CrossVersion.binaryScalaVersion(scalaVersion.value) + val modules = (assembly / fullClasspath).value.flatMap(_.get(moduleID.key)) + modules + .filterNot(_.organization == "org.apache.pekko") // our own modules, covered by the ASF header + .map { module => + val name = if (module.name.endsWith(suffix)) module.name.dropRight(suffix.length) else module.name + s"${module.organization}:$name" + } + .distinct + .sorted + }, + assemblyLicenseCheck := { + val log = streams.value.log + val licenseFile = apacheSonatypeLicenseFile.value + val listed = IO.readLines(licenseFile).map(_.trim).collect { case ModuleId(org, module) => s"$org:$module" }.toSet + val bundled = assemblyBundledModules.value + + val extra = (listed -- bundled).toSeq.sorted + if (extra.nonEmpty) + // the assembly jars are cross built, so a module can be listed for a Scala version other than this one + log.warn( + s"${licenseFile.getName} lists modules that ${name.value} does not bundle for Scala ${scalaVersion.value}, " + + s"check whether they are still needed: ${extra.mkString(", ")}") + + val missing = bundled.filterNot(listed) + if (missing.nonEmpty) + sys.error( + s"${name.value} bundles modules that ${licenseFile.getName} does not account for: ${missing.mkString(", ")}") + + log.info(s"${licenseFile.getName} accounts for all ${bundled.size} modules bundled by ${name.value}") + }, + assemblyMetaInfArchives := Seq(assembly.value), + assemblyMetaInfCheck := { + val log = streams.value.log + val expected = + Seq("META-INF/LICENSE" -> apacheSonatypeLicenseFile.value, "META-INF/NOTICE" -> apacheSonatypeNoticeFile.value) + + assemblyMetaInfArchives.value.foreach { archive => + val zip = new ZipFile(archive) + val problems = + try expected.flatMap { case (path, file) => + Option(zip.getEntry(path)) match { + case None => + Some(s"$path is missing, expected a copy of $file") + case Some(entry) => + val actual = { + val in = zip.getInputStream(entry) + try in.readAllBytes() + finally in.close() + } + if (actual.sameElements(IO.readBytes(file))) None + else Some(s"$path does not match $file") + } + } + finally zip.close() Review Comment: This re-reads the expected LICENSE/NOTICE files from disk for every archive *and* for each entry (`IO.readBytes(file)` inside the loop). Consider reading `apacheSonatypeLicenseFile`/`apacheSonatypeNoticeFile` once into memory (e.g., a `Map[path -> Array[Byte]]`) before iterating archives, then compare against those cached bytes. This reduces redundant IO in CI/local runs and makes the check cheaper as more archives are added. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
