Skip to content

Commit

Permalink
Address review comments
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
jchyb committed Aug 18, 2022
1 parent 503b2d2 commit d7d1da0
Show file tree
Hide file tree
Showing 11 changed files with 161 additions and 145 deletions.
1 change: 1 addition & 0 deletions modules/build/src/main/scala/scala/build/Build.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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.*
Expand Down
14 changes: 1 addition & 13 deletions modules/build/src/main/scala/scala/build/Inputs.scala
Original file line number Diff line number Diff line change
Expand Up @@ -103,34 +103,22 @@ 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 =>
val content = elem match {
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)
Expand Down
122 changes: 0 additions & 122 deletions modules/build/src/main/scala/scala/build/ResourceMapper.scala

This file was deleted.

Original file line number Diff line number Diff line change
@@ -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)
}
}
Original file line number Diff line number Diff line change
@@ -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))
}
}
Original file line number Diff line number Diff line change
@@ -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 _ =>
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
3 changes: 2 additions & 1 deletion modules/cli/src/main/scala/scala/cli/commands/Package.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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._
Expand Down Expand Up @@ -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 ++
Expand Down
38 changes: 34 additions & 4 deletions modules/cli/src/main/scala/scala/cli/internal/CachedBinary.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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("<resources>".getBytes())
// Resource changes for SN require relinking, so they should also be hashed
hashResources(build).foreach(md.update)
md.update("</resources>".getBytes())
md.update(0: Byte)
md.update("<config>".getBytes(charset))
for (elem <- config) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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() ++
Expand Down

0 comments on commit d7d1da0

Please sign in to comment.