From d7d1da0c9c0d60ee3d399626817110043a2edbbe Mon Sep 17 00:00:00 2001 From: Jan Chyb Date: Thu, 18 Aug 2022 13:39:46 +0200 Subject: [PATCH] Address review comments Also as suggested in the review, ResourceMappers were adjusted to save only output file paths in the mapping (now renamed to registry) file, as that is all we need. --- .../src/main/scala/scala/build/Build.scala | 1 + .../src/main/scala/scala/build/Inputs.scala | 14 +- .../scala/scala/build/ResourceMapper.scala | 122 ------------------ .../resource/BaseResourceMapper.scala | 47 +++++++ .../resource/NativeResourceMapper.scala | 28 ++++ .../internal/resource/ResourceMapper.scala | 40 ++++++ .../cli/commands/ScalaNativeOptions.scala | 4 +- .../scala/scala/cli/commands/Package.scala | 3 +- .../scala/cli/internal/CachedBinary.scala | 38 +++++- .../cli/integration/RunTestDefinitions.scala | 7 +- .../build/options/ScalaNativeOptions.scala | 2 +- 11 files changed, 161 insertions(+), 145 deletions(-) delete mode 100644 modules/build/src/main/scala/scala/build/ResourceMapper.scala create mode 100644 modules/build/src/main/scala/scala/build/internal/resource/BaseResourceMapper.scala create mode 100644 modules/build/src/main/scala/scala/build/internal/resource/NativeResourceMapper.scala create mode 100644 modules/build/src/main/scala/scala/build/internal/resource/ResourceMapper.scala diff --git a/modules/build/src/main/scala/scala/build/Build.scala b/modules/build/src/main/scala/scala/build/Build.scala index 26baf406fe..3c463dd9ef 100644 --- a/modules/build/src/main/scala/scala/build/Build.scala +++ b/modules/build/src/main/scala/scala/build/Build.scala @@ -16,6 +16,7 @@ import scala.build.Ops.* import scala.build.compiler.{ScalaCompiler, ScalaCompilerMaker} import scala.build.errors.* import scala.build.internal.{Constants, CustomCodeWrapper, MainClass, Util} +import scala.build.internal.resource.ResourceMapper import scala.build.options.* import scala.build.options.validation.ValidationException import scala.build.postprocessing.* diff --git a/modules/build/src/main/scala/scala/build/Inputs.scala b/modules/build/src/main/scala/scala/build/Inputs.scala index 3e5365482c..c3ec9d01dc 100644 --- a/modules/build/src/main/scala/scala/build/Inputs.scala +++ b/modules/build/src/main/scala/scala/build/Inputs.scala @@ -103,7 +103,7 @@ final case class Inputs( if (canWrite) this else inHomeDir(directories) } - def sourceHash(resourceDirs: Seq[os.Path] = Seq.empty): String = { + def sourceHash(): String = { def bytes(s: String): Array[Byte] = s.getBytes(StandardCharsets.UTF_8) val it = elements.iterator.flatMap { case elem: Inputs.OnDisk => @@ -111,26 +111,14 @@ final case class Inputs( case dirInput: Inputs.Directory => Seq("dir:") ++ Inputs.singleFilesFromDirectory(dirInput) .map(file => s"${file.path}:" + os.read(file.path)) - case resDirInput: Inputs.ResourceDirectory => - // Resource changes for SN require relinking, so they should also be hashed - Seq("resource-dir:") ++ os.walk(resDirInput.path) - .filter(os.isFile(_)) - .map(filePath => s"$filePath:" + os.read(filePath)) case _ => Seq(os.read(elem.path)) } (Iterator(elem.path.toString) ++ content.iterator ++ Iterator("\n")).map(bytes) case v: Inputs.Virtual => Iterator(v.content, bytes("\n")) } - val resourceDirsIt = (resourceDirs.flatMap { dir => - os.walk(dir) - .filter(os.isFile(_)) - .map(filePath => s"$filePath:" + os.read(filePath)) - }.iterator ++ Iterator("\n")).map(bytes) - val md = MessageDigest.getInstance("SHA-1") it.foreach(md.update) - resourceDirsIt.foreach(md.update) val digest = md.digest() val calculatedSum = new BigInteger(1, digest) String.format(s"%040x", calculatedSum) diff --git a/modules/build/src/main/scala/scala/build/ResourceMapper.scala b/modules/build/src/main/scala/scala/build/ResourceMapper.scala deleted file mode 100644 index e361a10d2a..0000000000 --- a/modules/build/src/main/scala/scala/build/ResourceMapper.scala +++ /dev/null @@ -1,122 +0,0 @@ -package scala.build - -import scala.collection.Map -import scala.jdk.CollectionConverters._ -import scala.build.internal.Constants - -trait BaseResourceMapper { - private def readMappingIfExists(mappingFile: os.Path): Option[Map[os.Path, os.Path]] = - if (os.exists(mappingFile)) { - val mappingContent = os.read(mappingFile) - if (mappingContent.isEmpty()) Some(Map.empty) - else Some( - mappingContent.split('\n').map { pair => - val sep = pair.split(',') - (os.Path(sep(0)), os.Path(sep(1))) - }.toMap - ) - } - else None - - private def writeMapping(mappingPath: os.Path, mapping: Map[os.Path, os.Path]) = { - val mappingContent = mapping.map { case (inputPath, outputPath) => - s"$inputPath,$outputPath" - }.mkString("\n") - os.write.over(mappingPath, mappingContent) - } - - protected def copyResourcesToDirWithMapping( - build: Build.Successful, - mappingFilePath: os.Path, - mappingFunction: Build.Successful => Map[os.Path, os.Path] - ): Unit = { - - def isInDirectory(output: os.Path, filePath: os.Path) = { - val outputFullPath = output.toNIO.iterator().asScala.toSeq - val fileFullPath = output.toNIO.iterator().asScala.toSeq - fileFullPath.startsWith(outputFullPath) - } - - val oldMapping = readMappingIfExists(mappingFilePath).getOrElse(Map.empty) - val newMapping = mappingFunction(build) - - val removedFiles = oldMapping.values.toSet -- newMapping.values.toSet - removedFiles.foreach { outputPath => - // Delete only if in output path, to avoid causing any harm elsewhere - if (isInDirectory(build.output, outputPath)) - os.remove(outputPath) - } - - newMapping.toList.foreach { case (inputPath, outputPath) => - os.copy( - inputPath, - outputPath, - replaceExisting = true, - createFolders = true - ) - } - - writeMapping(mappingFilePath, newMapping) - } -} - -object ResourceMapper extends BaseResourceMapper { - - private def resourceMapping(build: Build.Successful) = { - val seq = for { - resourceDirPath <- build.sources.resourceDirs.filter(os.exists(_)) - resourceFilePath <- os.walk(resourceDirPath).filter(os.isFile(_)) - relativeResourcePath = resourceFilePath.relativeTo(resourceDirPath) - // dismiss files generated by scala-cli - if !relativeResourcePath.startsWith(os.rel / Constants.workspaceDirName) - } yield { - val inputPath = resourceFilePath - val outputPath = build.output / relativeResourcePath - (resourceFilePath, outputPath) - } - - seq.toMap - } - - private def resolveResourceMappingPath(classDir: os.Path): os.Path = - classDir / os.up / os.up / ".resource_mapping" - - /* Copies and maps resources from their original path to the destination path - * in build output, also caching that mapping in a file. - * - * Remembering the mapping this way allows for the resource to be removed - * if the original file is renamed/deleted. - */ - def copyResourceToClassesDir(build: Build): Unit = build match { - case b: Build.Successful => - val fullFilePath = resolveResourceMappingPath(b.output) - copyResourcesToDirWithMapping(b, fullFilePath, resourceMapping) - - case _ => - } -} - -object NativeResourceMapper extends BaseResourceMapper { - - private def scalaNativeCFileMapping(build: Build.Successful): Map[os.Path, os.Path] = - build.inputs.flattened().collect { - case cfile: Inputs.CFile => - val inputPath = cfile.path - val destPath = build.output / "scala-native" / cfile.subPath - (inputPath, destPath) - }.toMap - - private def resolveProjectCFileMappingPath(nativeWorkDir: os.Path) = - nativeWorkDir / ".project_native_mapping" - - /* Copies and maps c file resources from their original path to the - * destination path, also caching that mapping. - * - * Remembering the mapping this way allows for the resource to be removed - * if the original file is renamed/deleted. - */ - def copyCFilesToScalaNativeDir(build: Build.Successful, nativeWorkDir: os.Path): Unit = { - val mappingFilePath = resolveProjectCFileMappingPath(nativeWorkDir) - copyResourcesToDirWithMapping(build, mappingFilePath, scalaNativeCFileMapping) - } -} diff --git a/modules/build/src/main/scala/scala/build/internal/resource/BaseResourceMapper.scala b/modules/build/src/main/scala/scala/build/internal/resource/BaseResourceMapper.scala new file mode 100644 index 0000000000..e3a1917f1e --- /dev/null +++ b/modules/build/src/main/scala/scala/build/internal/resource/BaseResourceMapper.scala @@ -0,0 +1,47 @@ +package scala.build.internal.resource + +trait BaseResourceMapper { + private def readRegistryIfExists(registryFile: os.Path): Option[List[os.Path]] = + if (os.exists(registryFile)) { + val registryContent = os.read(registryFile) + if (registryContent.isEmpty()) Some(List.empty) + else Some( + registryContent.linesIterator.filter(_.nonEmpty).map(os.Path(_)).toList + ) + } + else None + + private def writeRegistryFromMapping(registryPath: os.Path, mapping: Map[os.Path, os.Path]) = { + val registryContent = mapping.map { case (inputPath, outputPath) => + outputPath + }.mkString("\n") + os.write.over(registryPath, registryContent) + } + + protected def copyResourcesToDirWithMapping( + output: os.Path, + registryFilePath: os.Path, + newMapping: Map[os.Path, os.Path] + ): Unit = { + + val oldRegistry = readRegistryIfExists(registryFilePath).getOrElse(List.empty) + val removedFiles = oldRegistry.toSet -- newMapping.values.toSet + + removedFiles.foreach { outputPath => + // Delete only if in output path, to avoid causing any harm elsewhere + if (output.startsWith(output)) + os.remove(outputPath) + } + + newMapping.toList.foreach { case (inputPath, outputPath) => + os.copy( + inputPath, + outputPath, + replaceExisting = true, + createFolders = true + ) + } + + writeRegistryFromMapping(registryFilePath, newMapping) + } +} diff --git a/modules/build/src/main/scala/scala/build/internal/resource/NativeResourceMapper.scala b/modules/build/src/main/scala/scala/build/internal/resource/NativeResourceMapper.scala new file mode 100644 index 0000000000..5b0d8492b2 --- /dev/null +++ b/modules/build/src/main/scala/scala/build/internal/resource/NativeResourceMapper.scala @@ -0,0 +1,28 @@ +package scala.build.internal.resource + +import scala.build.{Build, Inputs} + +object NativeResourceMapper extends BaseResourceMapper { + + private def scalaNativeCFileMapping(build: Build.Successful): Map[os.Path, os.Path] = + build.inputs.flattened().collect { + case cfile: Inputs.CFile => + val inputPath = cfile.path + val destPath = build.output / "scala-native" / cfile.subPath + (inputPath, destPath) + }.toMap + + private def resolveProjectCFileRegistryPath(nativeWorkDir: os.Path) = + nativeWorkDir / ".native_registry" + + /** Copies and maps c file resources from their original path to the destination path in build + * output, also caching output paths in a file. + * + * Remembering the mapping this way allows for the resource to be removed if the original file is + * renamed/deleted. + */ + def copyCFilesToScalaNativeDir(build: Build.Successful, nativeWorkDir: os.Path): Unit = { + val mappingFilePath = resolveProjectCFileRegistryPath(nativeWorkDir) + copyResourcesToDirWithMapping(build.output, mappingFilePath, scalaNativeCFileMapping(build)) + } +} diff --git a/modules/build/src/main/scala/scala/build/internal/resource/ResourceMapper.scala b/modules/build/src/main/scala/scala/build/internal/resource/ResourceMapper.scala new file mode 100644 index 0000000000..327d8aedca --- /dev/null +++ b/modules/build/src/main/scala/scala/build/internal/resource/ResourceMapper.scala @@ -0,0 +1,40 @@ +package scala.build.internal.resource + +import scala.build.Build +import scala.build.internal.Constants + +object ResourceMapper extends BaseResourceMapper { + + private def resourceMapping(build: Build.Successful) = { + val seq = for { + resourceDirPath <- build.sources.resourceDirs.filter(os.exists(_)) + resourceFilePath <- os.walk(resourceDirPath).filter(os.isFile(_)) + relativeResourcePath = resourceFilePath.relativeTo(resourceDirPath) + // dismiss files generated by scala-cli + if !relativeResourcePath.startsWith(os.rel / Constants.workspaceDirName) + } yield { + val inputPath = resourceFilePath + val outputPath = build.output / relativeResourcePath + (resourceFilePath, outputPath) + } + + seq.toMap + } + + private def resolveResourceRegistryPath(classDir: os.Path): os.Path = + classDir / os.up / os.up / ".resource_registry" + + /** Copies and maps resources from their original path to the destination path in build output, + * also caching output paths in a file. + * + * Remembering the mapping this way allows for the resource to be removed if the original file is + * renamed/deleted. + */ + def copyResourceToClassesDir(build: Build): Unit = build match { + case b: Build.Successful => + val fullFilePath = resolveResourceRegistryPath(b.output) + copyResourcesToDirWithMapping(b.output, fullFilePath, resourceMapping(b)) + + case _ => + } +} diff --git a/modules/cli-options/src/main/scala/scala/cli/commands/ScalaNativeOptions.scala b/modules/cli-options/src/main/scala/scala/cli/commands/ScalaNativeOptions.scala index 645a36c177..8833a849e7 100644 --- a/modules/cli-options/src/main/scala/scala/cli/commands/ScalaNativeOptions.scala +++ b/modules/cli-options/src/main/scala/scala/cli/commands/ScalaNativeOptions.scala @@ -46,11 +46,11 @@ final case class ScalaNativeOptions( nativeCompileDefaults: Option[Boolean] = None, //TODO does it even work when we default it to true while handling? @Group("Scala Native") - @HelpMessage("Do not embed resources into the Scala Native binary (java style Resources will not be able to be used)") + @HelpMessage("Do not embed resources into the Scala Native binary (java resources Api will not be able to be used)") noEmbed: Option[Boolean] = None ) - // format: on +// format: on object ScalaNativeOptions { lazy val parser: Parser[ScalaNativeOptions] = Parser.derive diff --git a/modules/cli/src/main/scala/scala/cli/commands/Package.scala b/modules/cli/src/main/scala/scala/cli/commands/Package.scala index 78967ff674..5f6ce0cea9 100644 --- a/modules/cli/src/main/scala/scala/cli/commands/Package.scala +++ b/modules/cli/src/main/scala/scala/cli/commands/Package.scala @@ -28,6 +28,7 @@ import scala.build.errors.{ import scala.build.interactive.InteractiveFileOps import scala.build.internal.Util._ import scala.build.internal.{Runner, ScalaJsLinkerConfig} +import scala.build.internal.resource.NativeResourceMapper import scala.build.options.{PackageType, Platform} import scala.cli.CurrentParams import scala.cli.commands.OptionsHelper._ @@ -928,7 +929,7 @@ object Package extends ScalaCommand[PackageOptions] { NativeResourceMapper.copyCFilesToScalaNativeDir(build, nativeWorkDir) Library.withLibraryJar(build, dest.last.stripSuffix(".jar")) { mainJar => - val classpath = build.artifacts.classPath.map(_.toString) :+ mainJar.toString + val classpath = mainJar.toString +: build.artifacts.classPath.map(_.toString) val args = cliOptions ++ logger.scalaNativeCliInternalLoggerOptions ++ diff --git a/modules/cli/src/main/scala/scala/cli/internal/CachedBinary.scala b/modules/cli/src/main/scala/scala/cli/internal/CachedBinary.scala index 8770754c39..d30e6141db 100644 --- a/modules/cli/src/main/scala/scala/cli/internal/CachedBinary.scala +++ b/modules/cli/src/main/scala/scala/cli/internal/CachedBinary.scala @@ -4,7 +4,7 @@ import java.math.BigInteger import java.nio.charset.StandardCharsets import java.security.MessageDigest -import scala.build.Build +import scala.build.{Build, Inputs} import scala.build.internal.Constants object CachedBinary { @@ -23,12 +23,42 @@ object CachedBinary { String.format(s"%040x", calculatedSum) } + private def hashResources(build: Build.Successful) = { + def hashResourceDir(path: os.Path) = + os.walk(path) + .filter(os.isFile(_)) + .map { filePath => + val md = MessageDigest.getInstance("SHA-1") + md.update(os.read.bytes(filePath)) + s"$filePath:" + new BigInteger(1, md.digest()).toString() + } + + val classpathResourceDirsIt = (build.options.classPathOptions.resourcesDir.flatMap { dir => + hashResourceDir(dir) + }.iterator ++ Iterator("\n")).map(_.getBytes(StandardCharsets.UTF_8)) + + val projectResourceDirsIt = build.inputs.elements.iterator.flatMap { + case elem: Inputs.OnDisk => + val content = elem match { + case resDirInput: Inputs.ResourceDirectory => + hashResourceDir(resDirInput.path) + case _ => List.empty + } + (Iterator(elem.path.toString) ++ content.iterator ++ Iterator("\n")) + .map(_.getBytes(StandardCharsets.UTF_8)) + } + + classpathResourceDirsIt ++ projectResourceDirsIt + } + private def projectSha(build: Build.Successful, config: List[String]) = { val md = MessageDigest.getInstance("SHA-1") val charset = StandardCharsets.UTF_8 - md.update( - build.inputs.sourceHash(build.options.classPathOptions.resourcesDir).getBytes(charset) - ) + md.update(build.inputs.sourceHash().getBytes(charset)) + md.update("".getBytes()) + // Resource changes for SN require relinking, so they should also be hashed + hashResources(build).foreach(md.update) + md.update("".getBytes()) md.update(0: Byte) md.update("".getBytes(charset)) for (elem <- config) { diff --git a/modules/integration/src/test/scala/scala/cli/integration/RunTestDefinitions.scala b/modules/integration/src/test/scala/scala/cli/integration/RunTestDefinitions.scala index a5c0c29888..503480aa83 100644 --- a/modules/integration/src/test/scala/scala/cli/integration/RunTestDefinitions.scala +++ b/modules/integration/src/test/scala/scala/cli/integration/RunTestDefinitions.scala @@ -335,8 +335,11 @@ abstract class RunTestDefinitions(val scalaVersionOpt: Option[String]) .out.text().trim // LLVM throws linking errors if scalanative_print is internally repeated. - // Because of that the removed file should not be linked and this is what - // is being tested here. + // This can happen if a file containing it will be removed/renamed in src, + // but somehow those changes will not be reflected in the output directory, + // causing symbols inside linked files to be doubled. + // Because of that, the removed file should not be passed to linker, + // otherwise this test will fail. expect(output2 == interopMsg) } } diff --git a/modules/options/src/main/scala/scala/build/options/ScalaNativeOptions.scala b/modules/options/src/main/scala/scala/build/options/ScalaNativeOptions.scala index 5627b5008c..607da192f2 100644 --- a/modules/options/src/main/scala/scala/build/options/ScalaNativeOptions.scala +++ b/modules/options/src/main/scala/scala/build/options/ScalaNativeOptions.scala @@ -110,7 +110,7 @@ final case class ScalaNativeOptions( output = None ) - def configCliOptions(resourcesExist: Boolean): List[String] = // TODO consider adding a warning + def configCliOptions(resourcesExist: Boolean): List[String] = gcCliOption() ++ modeCliOption() ++ clangCliOption() ++