From 4061dabf4d3a999099dc9e93238a9d81e6bc7cfe Mon Sep 17 00:00:00 2001 From: Ethan Atkins Date: Sat, 27 Jul 2019 12:57:43 -0700 Subject: [PATCH] Override zinc compile analysis for source changes Zinc records all of the compile source file hashes when compilation completes. This is problematic because its possible that a source file was changed during compilation. From the user perspective, this may mean that their source change will not be recompiled even if a build is triggered by the change. To overcome this, I add logic in the sbt provided external hooks to override the zinc analysis stamps. This is done by writing the source file stamps to the previous cache after compilation completes. This allows us to see the source differences from sbt's perspective, rather than zinc's perspective. We then merge the combined differences in the actual implementation of ExternalHooks. In some cases this may result in over-compilation but generally over-compilation is preferred to under compilation. Most of the time, the results should be the same. The scripted test that I added modifies a file during compilation by invoking a macro. It then effectively asserts that the file is recompiled during the next test run by validating the compilation result in the test. The test fails on the latest develop hash. --- main/src/main/scala/sbt/Defaults.scala | 14 ++++- main/src/main/scala/sbt/Keys.scala | 1 + .../scala/sbt/internal/ExternalHooks.scala | 37 ++++++++---- main/src/main/scala/sbt/nio/Settings.scala | 60 ++++++++++--------- sbt/src/sbt-test/nio/external-hooks/build.sbt | 23 +++++++ .../external-hooks/src/test/scala/Foo.scala | 3 + .../src/test/scala/FooTest.scala | 8 +++ sbt/src/sbt-test/nio/external-hooks/test | 5 ++ .../restore-classes/build.sbt | 1 + 9 files changed, 110 insertions(+), 42 deletions(-) create mode 100644 sbt/src/sbt-test/nio/external-hooks/build.sbt create mode 100644 sbt/src/sbt-test/nio/external-hooks/src/test/scala/Foo.scala create mode 100644 sbt/src/sbt-test/nio/external-hooks/src/test/scala/FooTest.scala create mode 100644 sbt/src/sbt-test/nio/external-hooks/test diff --git a/main/src/main/scala/sbt/Defaults.scala b/main/src/main/scala/sbt/Defaults.scala index 5412ef666..0e9299b79 100755 --- a/main/src/main/scala/sbt/Defaults.scala +++ b/main/src/main/scala/sbt/Defaults.scala @@ -410,6 +410,7 @@ object Defaults extends BuildCommon { }, unmanagedSources := (unmanagedSources / inputFileStamps).value.map(_._1.toFile), managedSourceDirectories := Seq(sourceManaged.value), + managedSources / outputFileStamper := sbt.nio.FileStamper.Hash, managedSources := { val stamper = inputFileStamper.value val cache = managedFileStampCache.value @@ -603,7 +604,18 @@ object Defaults extends BuildCommon { else "" s"inc_compile$extra.zip" }, - incOptions := { incOptions.value.withExternalHooks(ExternalHooks.default.value) }, + externalHooks := { + val current = + (unmanagedSources / inputFileStamps).value ++ (managedSources / outputFileStamps).value + val previous = (externalHooks / inputFileStamps).previous + ExternalHooks.default.value(previous.flatMap(sbt.nio.Settings.changedFiles(_, current))) + }, + externalHooks / inputFileStamps := { + compile.value // ensures the inputFileStamps previous value is only set if compile succeeds. + (unmanagedSources / inputFileStamps).value ++ (managedSources / outputFileStamps).value + }, + externalHooks / inputFileStamps := (externalHooks / inputFileStamps).triggeredBy(compile).value, + incOptions := { incOptions.value.withExternalHooks(externalHooks.value) }, compileIncSetup := compileIncSetupTask.value, console := consoleTask.value, collectAnalyses := Definition.collectAnalysesTask.map(_ => ()).value, diff --git a/main/src/main/scala/sbt/Keys.scala b/main/src/main/scala/sbt/Keys.scala index 24f6c9c63..b1a132a4e 100644 --- a/main/src/main/scala/sbt/Keys.scala +++ b/main/src/main/scala/sbt/Keys.scala @@ -212,6 +212,7 @@ object Keys { val copyResources = taskKey[Seq[(File, File)]]("Copies resources to the output directory.").withRank(AMinusTask) val aggregate = settingKey[Boolean]("Configures task aggregation.").withRank(BMinusSetting) val sourcePositionMappers = taskKey[Seq[xsbti.Position => Option[xsbti.Position]]]("Maps positions in generated source files to the original source it was generated from").withRank(DTask) + private[sbt] val externalHooks = taskKey[ExternalHooks]("The external hooks used by zinc.") // package keys val packageBin = taskKey[File]("Produces a main artifact, such as a binary jar.").withRank(ATask) diff --git a/main/src/main/scala/sbt/internal/ExternalHooks.scala b/main/src/main/scala/sbt/internal/ExternalHooks.scala index 46aa3e29e..6c36d7279 100644 --- a/main/src/main/scala/sbt/internal/ExternalHooks.scala +++ b/main/src/main/scala/sbt/internal/ExternalHooks.scala @@ -7,7 +7,7 @@ package sbt.internal -import java.nio.file.Paths +import java.nio.file.{ Path, Paths } import java.util.Optional import sbt.Def @@ -16,18 +16,17 @@ import sbt.internal.inc.ExternalLookup import sbt.internal.inc.Stamp.equivStamp.equiv import sbt.io.syntax._ import sbt.nio.Keys._ -import sbt.nio.file.RecursiveGlob import sbt.nio.file.syntax._ +import sbt.nio.file.{ ChangedFiles, RecursiveGlob } import sbt.nio.{ FileStamp, FileStamper } import xsbti.compile._ import xsbti.compile.analysis.Stamp import scala.collection.JavaConverters._ -import scala.collection.mutable private[sbt] object ExternalHooks { private val javaHome = Option(System.getProperty("java.home")).map(Paths.get(_)) - def default: Def.Initialize[sbt.Task[ExternalHooks]] = Def.task { + def default: Def.Initialize[sbt.Task[Option[ChangedFiles] => ExternalHooks]] = Def.task { val unmanagedCache = unmanagedFileStampCache.value val managedCache = managedFileStampCache.value val cp = dependencyClasspath.value.map(_.data) @@ -39,9 +38,11 @@ private[sbt] object ExternalHooks { fileTreeView.value.list(classGlob).foreach { case (path, _) => managedCache.update(path, FileStamper.LastModified) } - apply((compileOptions in compile).value, unmanagedCache, managedCache) + val options = (compileOptions in compile).value + apply(_, options, unmanagedCache, managedCache) } private def apply( + changedFiles: Option[ChangedFiles], options: CompileOptions, unmanagedCache: FileStamp.Cache, managedCache: FileStamp.Cache @@ -53,22 +54,34 @@ private[sbt] object ExternalHooks { val getRemoved: java.util.Set[File] = new java.util.HashSet[File] val getChanged: java.util.Set[File] = new java.util.HashSet[File] val getUnmodified: java.util.Set[File] = new java.util.HashSet[File] + private def add(p: Path, sets: java.util.Set[File]*): Unit = { + sets.foreach(add(p.toFile, _)) + } + private def add(f: File, set: java.util.Set[File]): Unit = { set.add(f); () } + val allChanges = new java.util.HashSet[File] + changedFiles foreach { + case ChangedFiles(c, d, u) => + c.foreach(add(_, getAdded, allChanges)) + d.foreach(add(_, getRemoved, allChanges)) + u.foreach(add(_, getChanged, allChanges)) + } override def isEmpty: java.lang.Boolean = getAdded.isEmpty && getRemoved.isEmpty && getChanged.isEmpty - val prevSources: mutable.Map[File, Stamp] = - previousAnalysis.readStamps().getAllSourceStamps.asScala - prevSources.foreach { - case (file: File, s: Stamp) => + private val prevSources = previousAnalysis.readStamps().getAllSourceStamps + prevSources.forEach { (file: File, s: Stamp) => + if (!allChanges.contains(file)) { val path = file.toPath unmanagedCache .get(path) .orElse(managedCache.getOrElseUpdate(file.toPath, FileStamper.Hash)) match { - case None => getRemoved.add(file) + case None => add(file, getRemoved) case Some(stamp) => - if (equiv(stamp.stamp, s)) getUnmodified.add(file) else getChanged.add(file) + if (equiv(stamp.stamp, s)) add(file, getUnmodified) + else add(file, getChanged) } + } } - options.sources.foreach(file => if (!prevSources.contains(file)) getAdded.add(file)) + options.sources.foreach(file => if (!prevSources.containsKey(file)) getAdded.add(file)) } } diff --git a/main/src/main/scala/sbt/nio/Settings.scala b/main/src/main/scala/sbt/nio/Settings.scala index 8600a50e8..1f80467a9 100644 --- a/main/src/main/scala/sbt/nio/Settings.scala +++ b/main/src/main/scala/sbt/nio/Settings.scala @@ -251,36 +251,38 @@ private[sbt] object Settings { ): Def.Setting[_] = addTaskDefinition(changeKey in scopedKey.scope := { val current = (stampKey in scopedKey.scope).value - (stampKey in scopedKey.scope).previous match { - case Some(previous) => - val createdBuilder = new VectorBuilder[Path] - val deletedBuilder = new VectorBuilder[Path] - val updatedBuilder = new VectorBuilder[Path] - val currentMap = current.toMap - val prevMap = previous.toMap - current.foreach { - case (path, currentStamp) => - prevMap.get(path) match { - case Some(oldStamp) => if (oldStamp != currentStamp) updatedBuilder += path - case None => createdBuilder += path - } - } - previous.foreach { - case (path, _) => - if (currentMap.get(path).isEmpty) deletedBuilder += path - } - val created = createdBuilder.result() - val deleted = deletedBuilder.result() - val updated = updatedBuilder.result() - if (created.isEmpty && deleted.isEmpty && updated.isEmpty) { - None - } else { - val cf = ChangedFiles(created = created, deleted = deleted, updated = updated) - Some(cf) - } - case None => None - } + (stampKey in scopedKey.scope).previous.flatMap(changedFiles(_, current)) }) + private[sbt] def changedFiles( + previous: Seq[(Path, FileStamp)], + current: Seq[(Path, FileStamp)] + ): Option[ChangedFiles] = { + val createdBuilder = new VectorBuilder[Path] + val deletedBuilder = new VectorBuilder[Path] + val updatedBuilder = new VectorBuilder[Path] + val currentMap = current.toMap + val prevMap = previous.toMap + current.foreach { + case (path, currentStamp) => + prevMap.get(path) match { + case Some(oldStamp) => if (oldStamp != currentStamp) updatedBuilder += path + case None => createdBuilder += path + } + } + previous.foreach { + case (path, _) => + if (currentMap.get(path).isEmpty) deletedBuilder += path + } + val created = createdBuilder.result() + val deleted = deletedBuilder.result() + val updated = updatedBuilder.result() + if (created.isEmpty && deleted.isEmpty && updated.isEmpty) { + None + } else { + val cf = ChangedFiles(created = created, deleted = deleted, updated = updated) + Some(cf) + } + } /** * Provides an automatically generated clean method for a task that provides fileOutputs. diff --git a/sbt/src/sbt-test/nio/external-hooks/build.sbt b/sbt/src/sbt-test/nio/external-hooks/build.sbt new file mode 100644 index 000000000..c01dc8321 --- /dev/null +++ b/sbt/src/sbt-test/nio/external-hooks/build.sbt @@ -0,0 +1,23 @@ +val generateSourceFile = taskKey[Unit]("generate source file") +generateSourceFile := { + val testDir = ((Test / scalaSource).value.toPath / "Foo.scala").toString + val content = s"object Foo { val x = 2 }" + val src = + s""" + |import scala.language.experimental.macros + |import scala.reflect.macros.blackbox + |import java.nio.file.{ Files, Paths } + | + |object Generate { + | def gen: Unit = macro genImpl + | def genImpl(c: blackbox.Context): c.Expr[Unit] = { + | Files.write(Paths.get("${testDir.replace("\\", "\\\\")}"), "$content".getBytes) + | c.universe.reify(()) + | } + |} + |""".stripMargin + IO.write((Compile / scalaSource).value / "Generate.scala", src) +} + +libraryDependencies += "org.scala-lang" % "scala-reflect" % scalaVersion.value +libraryDependencies += "org.scalatest" %% "scalatest" % "3.0.5" % "test" diff --git a/sbt/src/sbt-test/nio/external-hooks/src/test/scala/Foo.scala b/sbt/src/sbt-test/nio/external-hooks/src/test/scala/Foo.scala new file mode 100644 index 000000000..b4736784b --- /dev/null +++ b/sbt/src/sbt-test/nio/external-hooks/src/test/scala/Foo.scala @@ -0,0 +1,3 @@ +object Foo { + def x = 1 +} diff --git a/sbt/src/sbt-test/nio/external-hooks/src/test/scala/FooTest.scala b/sbt/src/sbt-test/nio/external-hooks/src/test/scala/FooTest.scala new file mode 100644 index 000000000..3be9ef70b --- /dev/null +++ b/sbt/src/sbt-test/nio/external-hooks/src/test/scala/FooTest.scala @@ -0,0 +1,8 @@ +import org.scalatest.FlatSpec + +class FooTest extends FlatSpec { + Generate.gen + it should "work" in { + assert(Foo.x == 2) + } +} \ No newline at end of file diff --git a/sbt/src/sbt-test/nio/external-hooks/test b/sbt/src/sbt-test/nio/external-hooks/test new file mode 100644 index 000000000..6c8ad46f4 --- /dev/null +++ b/sbt/src/sbt-test/nio/external-hooks/test @@ -0,0 +1,5 @@ +> generateSourceFile + +-> test + +> test \ No newline at end of file diff --git a/sbt/src/sbt-test/source-dependencies/restore-classes/build.sbt b/sbt/src/sbt-test/source-dependencies/restore-classes/build.sbt index 3fe5758ad..e0254391f 100644 --- a/sbt/src/sbt-test/source-dependencies/restore-classes/build.sbt +++ b/sbt/src/sbt-test/source-dependencies/restore-classes/build.sbt @@ -14,6 +14,7 @@ recordPreviousIterations := { log.info("No previous analysis detected") 0 case Some(a: Analysis) => a.compilations.allCompilations.size + case Some(_) => -1 // should be unreachable but causes warnings } } }