From dcccd17fd2a324093c392dcabc53d723a95cad64 Mon Sep 17 00:00:00 2001 From: Ethan Atkins Date: Wed, 29 May 2019 16:52:33 -0700 Subject: [PATCH] Improve managed file management in watch @olegych reported in https://github.com/sbt/sbt/issues/4722 that sometimes even when a build was triggered during watch that no recompilation would occur. The cause of this was that we never invalidated the file stamp cache for managed sources or output files. The optimization of persisting the source file stamps between task evaluations in a continuous build only really makes sense for unmanaged sources. We make the implicit assumption that unmanaged sources are infrequently updated and generally one at a time. That assumption does not hold for unmanaged source or output files. To fix this, I split the fileStampCache into two caches: one for unmanaged sources and one for everything else. We only persist the unmanagedFileStampCache during continuous builds. The managedFileStampCache gets invalidated every time. I added a scripted test that simulates changing a generated source file. Prior to this change, the test would fail because the file stamp was not invalidated for the new source file content. Fixes #4722 --- main/src/main/scala/sbt/Defaults.scala | 18 +++++++------- .../scala/sbt/internal/ExternalHooks.scala | 21 +++++++++------- main/src/main/scala/sbt/nio/FileStamp.scala | 11 ++++++++- main/src/main/scala/sbt/nio/Keys.scala | 8 +++---- main/src/main/scala/sbt/nio/Settings.scala | 24 +++++-------------- sbt/src/sbt-test/watch/managed/build.sbt | 23 ++++++++++++++++++ .../watch/managed/changes/Write.scala | 7 ++++++ .../watch/managed/sources/Write.scala | 12 ++++++++++ sbt/src/sbt-test/watch/managed/success.txt | 0 sbt/src/sbt-test/watch/managed/test | 1 + .../sbt/scriptedtest/ScriptedTests.scala | 1 + 11 files changed, 86 insertions(+), 40 deletions(-) create mode 100644 sbt/src/sbt-test/watch/managed/build.sbt create mode 100644 sbt/src/sbt-test/watch/managed/changes/Write.scala create mode 100644 sbt/src/sbt-test/watch/managed/sources/Write.scala create mode 100644 sbt/src/sbt-test/watch/managed/success.txt create mode 100644 sbt/src/sbt-test/watch/managed/test diff --git a/main/src/main/scala/sbt/Defaults.scala b/main/src/main/scala/sbt/Defaults.scala index c3744be56..03bc8c39d 100755 --- a/main/src/main/scala/sbt/Defaults.scala +++ b/main/src/main/scala/sbt/Defaults.scala @@ -148,7 +148,6 @@ object Defaults extends BuildCommon { private[sbt] lazy val globalCore: Seq[Setting[_]] = globalDefaults( defaultTestTasks(test) ++ defaultTestTasks(testOnly) ++ defaultTestTasks(testQuick) ++ Seq( excludeFilter :== HiddenFileFilter, - pathToFileStamp :== sbt.nio.FileStamp.hash, fileInputs :== Nil, inputFileStamper :== sbt.nio.FileStamper.Hash, outputFileStamper :== sbt.nio.FileStamper.LastModified, @@ -157,11 +156,9 @@ object Defaults extends BuildCommon { watchPersistFileStamps :== true, watchTriggers :== Nil, clean := { () }, - sbt.nio.Keys.fileStampCache := { - state.value - .get(sbt.nio.Keys.persistentFileStampCache) - .getOrElse(new sbt.nio.FileStamp.Cache) - }, + unmanagedFileStampCache := + state.value.get(persistentFileStampCache).getOrElse(new sbt.nio.FileStamp.Cache), + managedFileStampCache := new sbt.nio.FileStamp.Cache, ) ++ globalIvyCore ++ globalJvmCore ) ++ globalSbtCore @@ -420,9 +417,12 @@ object Defaults extends BuildCommon { unmanagedSources := (unmanagedSources / inputFileStamps).value.map(_._1.toFile), managedSourceDirectories := Seq(sourceManaged.value), managedSources := { - val stamper = sbt.nio.Keys.pathToFileStamp.value + val stamper = inputFileStamper.value + val cache = managedFileStampCache.value val res = generate(sourceGenerators).value - res.foreach(f => stamper(f.toPath)) + res.foreach { f => + cache.putIfAbsent(f.toPath, stamper) + } res }, sourceGenerators :== Nil, @@ -1647,7 +1647,7 @@ object Defaults extends BuildCommon { val contents = AnalysisContents.create(analysisResult.analysis(), analysisResult.setup()) store.set(contents) } - val map = sbt.nio.Keys.fileStampCache.value + val map = managedFileStampCache.value val analysis = analysisResult.analysis import scala.collection.JavaConverters._ analysis.readStamps.getAllProductStamps.asScala.foreach { diff --git a/main/src/main/scala/sbt/internal/ExternalHooks.scala b/main/src/main/scala/sbt/internal/ExternalHooks.scala index d993f691b..46aa3e29e 100644 --- a/main/src/main/scala/sbt/internal/ExternalHooks.scala +++ b/main/src/main/scala/sbt/internal/ExternalHooks.scala @@ -28,21 +28,23 @@ 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 { - val cache = fileStampCache.value + val unmanagedCache = unmanagedFileStampCache.value + val managedCache = managedFileStampCache.value val cp = dependencyClasspath.value.map(_.data) cp.foreach { file => val path = file.toPath - cache.getOrElseUpdate(path, FileStamper.LastModified) + managedCache.getOrElseUpdate(path, FileStamper.LastModified) } val classGlob = classDirectory.value.toGlob / RecursiveGlob / "*.class" fileTreeView.value.list(classGlob).foreach { - case (path, _) => cache.update(path, FileStamper.LastModified) + case (path, _) => managedCache.update(path, FileStamper.LastModified) } - apply((compileOptions in compile).value, cache) + apply((compileOptions in compile).value, unmanagedCache, managedCache) } private def apply( options: CompileOptions, - fileStampCache: FileStamp.Cache + unmanagedCache: FileStamp.Cache, + managedCache: FileStamp.Cache ): DefaultExternalHooks = { val lookup = new ExternalLookup { override def changedSources(previousAnalysis: CompileAnalysis): Option[Changes[File]] = Some { @@ -57,7 +59,10 @@ private[sbt] object ExternalHooks { previousAnalysis.readStamps().getAllSourceStamps.asScala prevSources.foreach { case (file: File, s: Stamp) => - fileStampCache.getOrElseUpdate(file.toPath, FileStamper.Hash) match { + val path = file.toPath + unmanagedCache + .get(path) + .orElse(managedCache.getOrElseUpdate(file.toPath, FileStamper.Hash)) match { case None => getRemoved.add(file) case Some(stamp) => if (equiv(stamp.stamp, s)) getUnmodified.add(file) else getChanged.add(file) @@ -79,7 +84,7 @@ private[sbt] object ExternalHooks { override def changedBinaries(previousAnalysis: CompileAnalysis): Option[Set[File]] = { Some(previousAnalysis.readStamps.getAllBinaryStamps.asScala.flatMap { case (file, stamp) => - fileStampCache.get(file.toPath) match { + managedCache.get(file.toPath) match { case Some(cachedStamp) if equiv(cachedStamp.stamp, stamp) => None case _ => javaHome match { @@ -94,7 +99,7 @@ private[sbt] object ExternalHooks { override def removedProducts(previousAnalysis: CompileAnalysis): Option[Set[File]] = { Some(previousAnalysis.readStamps.getAllProductStamps.asScala.flatMap { case (file, stamp) => - fileStampCache.get(file.toPath) match { + managedCache.get(file.toPath) match { case Some(s) if equiv(s.stamp, stamp) => None case _ => Some(file) } diff --git a/main/src/main/scala/sbt/nio/FileStamp.scala b/main/src/main/scala/sbt/nio/FileStamp.scala index dafdca57f..32f88c425 100644 --- a/main/src/main/scala/sbt/nio/FileStamp.scala +++ b/main/src/main/scala/sbt/nio/FileStamp.scala @@ -9,6 +9,7 @@ package sbt.nio import java.io.{ File, IOException } import java.nio.file.{ Path, Paths } +import java.util.concurrent.ConcurrentHashMap import sbt.internal.inc.{ EmptyStamp, Stamper, LastModified => IncLastModified } import sbt.io.IO @@ -217,7 +218,7 @@ private[sbt] object FileStamp { } private[sbt] class Cache { - private[this] val underlying = new java.util.HashMap[Path, Either[FileStamp, FileStamp]] + private[this] val underlying = new ConcurrentHashMap[Path, Either[FileStamp, FileStamp]] /** * Invalidate the cache entry, but don't re-stamp the file until it's actually used @@ -251,6 +252,14 @@ private[sbt] object FileStamp { case null => None case e => e.value } + + def putIfAbsent(key: Path, stamper: FileStamper): Unit = { + underlying.get(key) match { + case null => updateImpl(key, stamper) + case _ => + } + () + } def update(key: Path, stamper: FileStamper): (Option[FileStamp], Option[FileStamp]) = { underlying.get(key) match { case null => (None, updateImpl(key, stamper)) diff --git a/main/src/main/scala/sbt/nio/Keys.scala b/main/src/main/scala/sbt/nio/Keys.scala index 1d10d0dbe..9d2702a96 100644 --- a/main/src/main/scala/sbt/nio/Keys.scala +++ b/main/src/main/scala/sbt/nio/Keys.scala @@ -142,11 +142,11 @@ object Keys { private[sbt] val allInputPathsAndAttributes = taskKey[Seq[(Path, FileAttributes)]]("Get all of the file inputs for a task") .withRank(Invisible) - private[sbt] val fileStampCache = taskKey[FileStamp.Cache]( - "Map of file stamps that may be cleared between task evaluation runs." + private[sbt] val unmanagedFileStampCache = taskKey[FileStamp.Cache]( + "Map of managed file stamps that may be cleared between task evaluation runs." ).withRank(Invisible) - private[sbt] val pathToFileStamp = taskKey[Path => Option[FileStamp]]( - "A function that computes a file stamp for a path. It may have the side effect of updating a cache." + private[sbt] val managedFileStampCache = taskKey[FileStamp.Cache]( + "Map of managed file stamps that may be cleared between task evaluation runs." ).withRank(Invisible) private[sbt] val classpathFiles = taskKey[Seq[Path]]("The classpath for a task.").withRank(Invisible) diff --git a/main/src/main/scala/sbt/nio/Settings.scala b/main/src/main/scala/sbt/nio/Settings.scala index 0a52741f8..532ed90ae 100644 --- a/main/src/main/scala/sbt/nio/Settings.scala +++ b/main/src/main/scala/sbt/nio/Settings.scala @@ -144,8 +144,7 @@ private[sbt] object Settings { (transitiveClasspathDependency in scopedKey.scope := { () }) :: Nil case changedOutputFiles.key => changedFilesImpl(scopedKey, changedOutputFiles, outputFileStamps) - case pathToFileStamp.key => stamper(scopedKey) :: Nil - case _ => Nil + case _ => Nil } /** @@ -318,10 +317,12 @@ private[sbt] object Settings { */ private[sbt] def fileStamps(scopedKey: Def.ScopedKey[_]): Def.Setting[_] = addTaskDefinition(Keys.inputFileStamps in scopedKey.scope := { - val stamper = (Keys.pathToFileStamp in scopedKey.scope).value + val cache = (unmanagedFileStampCache in scopedKey.scope).value + val stamper = (Keys.inputFileStamper in scopedKey.scope).value (Keys.allInputPathsAndAttributes in scopedKey.scope).value.flatMap { - case (p, a) if a.isRegularFile && !Files.isHidden(p) => stamper(p).map(p -> _) - case _ => None + case (p, a) if a.isRegularFile && !Files.isHidden(p) => + cache.getOrElseUpdate(p, stamper).map(p -> _) + case _ => None } }) private[this] def outputsAndStamps[T: JsonFormat: ToSeqPath]( @@ -348,17 +349,4 @@ private[sbt] object Settings { (allOutputFiles in scope).value.flatMap(p => stamper(p).map(p -> _)) }) - /** - * Returns a function from `Path` to [[FileStamp]] that can be used by tasks to retrieve - * the stamp for a file. It has the side effect of stamping the file if it has not already - * been stamped during the task evaluation. - * - * @return a task definition for a function from `Path` to [[FileStamp]]. - */ - private[this] def stamper(scopedKey: Def.ScopedKey[_]): Def.Setting[_] = - addTaskDefinition((Keys.pathToFileStamp in scopedKey.scope) := { - val attributeMap = Keys.fileStampCache.value - val stamper = (Keys.inputFileStamper in scopedKey.scope).value - path: Path => attributeMap.getOrElseUpdate(path, stamper) - }) } diff --git a/sbt/src/sbt-test/watch/managed/build.sbt b/sbt/src/sbt-test/watch/managed/build.sbt new file mode 100644 index 000000000..7384c265e --- /dev/null +++ b/sbt/src/sbt-test/watch/managed/build.sbt @@ -0,0 +1,23 @@ +import java.nio.file.Files + +import sbt.nio.Watch + +import scala.concurrent.duration._ + +Compile / sourceGenerators += Def.task { + baseDirectory.value / "sources" / "Write.scala" :: Nil +}.taskValue + +val runTest = taskKey[Unit]("run the test") +runTest := Def.taskDyn { + val args = s" ${baseDirectory.value}" + (Runtime / run).toTask(args) +}.value + +runTest / watchTriggers += baseDirectory.value.toGlob / "*.txt" +watchAntiEntropy := 0.milliseconds +watchOnFileInputEvent := { (count, e) => + if (new String(Files.readAllBytes(e.path)) == "ok") Watch.CancelWatch + else if (count < 2) Watch.Trigger + else new Watch.HandleError(new IllegalStateException(s"Wrong event triggered the build: $e")) +} \ No newline at end of file diff --git a/sbt/src/sbt-test/watch/managed/changes/Write.scala b/sbt/src/sbt-test/watch/managed/changes/Write.scala new file mode 100644 index 000000000..2cbfcf767 --- /dev/null +++ b/sbt/src/sbt-test/watch/managed/changes/Write.scala @@ -0,0 +1,7 @@ +import java.nio.file._ + +object Write { + def main(args: Array[String]): Unit = { + Files.write(Paths.get(args(0)).resolve("output.txt"), "ok".getBytes) + } +} \ No newline at end of file diff --git a/sbt/src/sbt-test/watch/managed/sources/Write.scala b/sbt/src/sbt-test/watch/managed/sources/Write.scala new file mode 100644 index 000000000..f899d0a5e --- /dev/null +++ b/sbt/src/sbt-test/watch/managed/sources/Write.scala @@ -0,0 +1,12 @@ +import java.nio.file._ +object Write { + def main(args: Array[String]): Unit = { + val dir = Paths.get(args(0)) + Files.write( + dir.resolve("sources").resolve("Write.scala"), + Files.readAllBytes(dir.resolve("changes").resolve("Write.scala")), + ) + val output = dir.resolve("output.txt") + Files.write(output, "failure".getBytes) + } +} \ No newline at end of file diff --git a/sbt/src/sbt-test/watch/managed/success.txt b/sbt/src/sbt-test/watch/managed/success.txt new file mode 100644 index 000000000..e69de29bb diff --git a/sbt/src/sbt-test/watch/managed/test b/sbt/src/sbt-test/watch/managed/test new file mode 100644 index 000000000..b513556cb --- /dev/null +++ b/sbt/src/sbt-test/watch/managed/test @@ -0,0 +1 @@ +> ~runTest \ No newline at end of file diff --git a/scripted-sbt-redux/src/main/scala/sbt/scriptedtest/ScriptedTests.scala b/scripted-sbt-redux/src/main/scala/sbt/scriptedtest/ScriptedTests.scala index bd28fa6d3..5fef5fad7 100644 --- a/scripted-sbt-redux/src/main/scala/sbt/scriptedtest/ScriptedTests.scala +++ b/scripted-sbt-redux/src/main/scala/sbt/scriptedtest/ScriptedTests.scala @@ -232,6 +232,7 @@ final class ScriptedTests( case "source-dependencies/linearization" => LauncherBased // sbt/Package$ case "source-dependencies/named" => LauncherBased // sbt/Package$ case "source-dependencies/specialized" => LauncherBased // sbt/Package$ + case "watch/managed" => LauncherBased // sbt/Package$ case "tests/test-cross" => LauncherBased // the sbt metabuild classpath leaks into the test interface classloader in older versions of sbt case _ => RunFromSourceBased