From 32a6d0d5d76716c7e85f05ea66bc9cd639611918 Mon Sep 17 00:00:00 2001 From: Ethan Atkins Date: Thu, 15 Aug 2019 10:29:07 -0700 Subject: [PATCH 1/2] Update ExternalHooks to look up changed binaries It was reported that in community builds, sometimes there was spurious over-compilation due to invalidation of the scala library jar (https://github.com/sbt/sbt/issues/4948). The reason for this was that the external hooks prefills the managed cache with all of the time stamps for the project dependencies but was not looking up any jars that weren't in the cache. I suspect I did this because I didn't realize that zinc also includes its own classpath in the binaries which is not a part of the dependencyClasspath. The fix is to just add the jar to the cache if it doesn't already exist by switching to getOrElseUpdate from get. I followed the steps in #4948 and published a version of sbt locally with this change and the spurious re-builds stopped. --- main/src/main/scala/sbt/internal/ExternalHooks.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/main/src/main/scala/sbt/internal/ExternalHooks.scala b/main/src/main/scala/sbt/internal/ExternalHooks.scala index 269a249f4..ca3abdbc5 100644 --- a/main/src/main/scala/sbt/internal/ExternalHooks.scala +++ b/main/src/main/scala/sbt/internal/ExternalHooks.scala @@ -101,7 +101,7 @@ private[sbt] object ExternalHooks { override def changedBinaries(previousAnalysis: CompileAnalysis): Option[Set[File]] = { Some(previousAnalysis.readStamps.getAllBinaryStamps.asScala.flatMap { case (file, stamp) => - managedCache.get(file.toPath) match { + managedCache.getOrElseUpdate(file.toPath, FileStamper.LastModified) match { case Some(cachedStamp) if equiv(cachedStamp.stamp, stamp) => None case _ => javaHome match { From 9c7acdb713247e4e17a03f8cbacda9aa783bf19e Mon Sep 17 00:00:00 2001 From: Ethan Atkins Date: Thu, 15 Aug 2019 11:25:52 -0700 Subject: [PATCH 2/2] Force invalidate dependency changes After adding the automatic lookup to external hooks for missing binary jars, the scripted test dependency-management/invalidate-internal started failing. This was because the previous analysis contained a jar dependency that still existed on disk but was no longer a part of the dependency classpath. Fundamentally the problem is that the zinc compile analysis is not tightly coupled with the sbt build state. To fix this, we can cache the dependency classpath file stamps in the same way that we cache the input file stamps in external hooks and manually diff them at the sbt level. We then force updates regardless of the difference between the zinc state and the sbt state. --- main/src/main/scala/sbt/Defaults.scala | 29 ++++++++++++++----- .../scala/sbt/internal/ExternalHooks.scala | 22 +++++++++----- main/src/main/scala/sbt/nio/Keys.scala | 2 ++ main/src/main/scala/sbt/nio/Settings.scala | 8 ++--- 4 files changed, 41 insertions(+), 20 deletions(-) diff --git a/main/src/main/scala/sbt/Defaults.scala b/main/src/main/scala/sbt/Defaults.scala index c00431caf..0c11909b6 100755 --- a/main/src/main/scala/sbt/Defaults.scala +++ b/main/src/main/scala/sbt/Defaults.scala @@ -610,19 +610,30 @@ object Defaults extends BuildCommon { }, externalHooks := { import sbt.nio.FileStamp.Formats.seqPathFileStampJsonFormatter - val current = + val currentInputs = (unmanagedSources / inputFileStamps).value ++ (managedSources / outputFileStamps).value - val previous = (externalHooks / inputFileStamps).previous - val changes = previous - .map(sbt.nio.Settings.changedFiles(_, current)) - .getOrElse(FileChanges.noPrevious(current.map(_._1))) - ExternalHooks.default.value(changes, fileTreeView.value) + val previousInputs = (externalHooks / inputFileStamps).previous + val inputChanges = previousInputs + .map(sbt.nio.Settings.changedFiles(_, currentInputs)) + .getOrElse(FileChanges.noPrevious(currentInputs.map(_._1))) + val currentOutputs = (dependencyClasspathFiles / outputFileStamps).value + val previousOutputs = (externalHooks / outputFileStamps).previous + val outputChanges = previousOutputs + .map(sbt.nio.Settings.changedFiles(_, currentOutputs)) + .getOrElse(FileChanges.noPrevious(currentOutputs.map(_._1))) + ExternalHooks.default.value(inputChanges, outputChanges, fileTreeView.value) }, 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, + externalHooks / outputFileStamps := { + compile.value // ensures the inputFileStamps previous value is only set if compile succeeds. + (dependencyClasspathFiles / outputFileStamps).value + }, + externalHooks / outputFileStamps := + (externalHooks / outputFileStamps).triggeredBy(compile).value, incOptions := { incOptions.value.withExternalHooks(externalHooks.value) }, compileIncSetup := compileIncSetupTask.value, console := consoleTask.value, @@ -2009,8 +2020,10 @@ object Classpaths { includeFilter in unmanagedJars value, excludeFilter in unmanagedJars value ) - ).map(exportClasspath) :+ - (sbt.nio.Keys.classpathFiles := data(fullClasspath.value).map(_.toPath)) + ).map(exportClasspath) ++ Seq( + sbt.nio.Keys.classpathFiles := data(fullClasspath.value).map(_.toPath), + sbt.nio.Keys.dependencyClasspathFiles := data(dependencyClasspath.value).map(_.toPath), + ) private[this] def exportClasspath(s: Setting[Task[Classpath]]): Setting[Task[Classpath]] = s.mapInitialize(init => Def.task { exportClasspath(streams.value, init.value) }) diff --git a/main/src/main/scala/sbt/internal/ExternalHooks.scala b/main/src/main/scala/sbt/internal/ExternalHooks.scala index ca3abdbc5..97c495ab5 100644 --- a/main/src/main/scala/sbt/internal/ExternalHooks.scala +++ b/main/src/main/scala/sbt/internal/ExternalHooks.scala @@ -26,7 +26,8 @@ import scala.collection.JavaConverters._ private[sbt] object ExternalHooks { private val javaHome = Option(System.getProperty("java.home")).map(Paths.get(_)) - private type Func = (FileChanges, FileTreeView[(Path, FileAttributes)]) => ExternalHooks + private type Func = + (FileChanges, FileChanges, FileTreeView[(Path, FileAttributes)]) => ExternalHooks def default: Def.Initialize[sbt.Task[Func]] = Def.task { val unmanagedCache = unmanagedFileStampCache.value val managedCache = managedFileStampCache.value @@ -37,15 +38,16 @@ private[sbt] object ExternalHooks { } val classGlob = classDirectory.value.toGlob / RecursiveGlob / "*.class" val options = (compileOptions in compile).value - (fc: FileChanges, fileTreeView: FileTreeView[(Path, FileAttributes)]) => { + ((inputFileChanges, outputFileChanges, fileTreeView) => { fileTreeView.list(classGlob).foreach { case (path, _) => managedCache.update(path, FileStamper.LastModified) } - apply(fc, options, unmanagedCache, managedCache) - } + apply(inputFileChanges, outputFileChanges, options, unmanagedCache, managedCache) + }): Func } private def apply( - changedFiles: FileChanges, + inputFileChanges: FileChanges, + outputFileChanges: FileChanges, options: CompileOptions, unmanagedCache: FileStamp.Cache, managedCache: FileStamp.Cache @@ -62,7 +64,7 @@ private[sbt] object ExternalHooks { } private def add(f: File, set: java.util.Set[File]): Unit = { set.add(f); () } val allChanges = new java.util.HashSet[File] - changedFiles match { + inputFileChanges match { case FileChanges(c, d, m, _) => c.foreach(add(_, getAdded, allChanges)) d.foreach(add(_, getRemoved, allChanges)) @@ -99,7 +101,11 @@ private[sbt] object ExternalHooks { Optional.empty[Array[FileHash]] override def changedBinaries(previousAnalysis: CompileAnalysis): Option[Set[File]] = { - Some(previousAnalysis.readStamps.getAllBinaryStamps.asScala.flatMap { + val base = + (outputFileChanges.modified ++ outputFileChanges.created ++ outputFileChanges.deleted) + .map(_.toFile) + .toSet + Some(base ++ previousAnalysis.readStamps.getAllBinaryStamps.asScala.flatMap { case (file, stamp) => managedCache.getOrElseUpdate(file.toPath, FileStamper.LastModified) match { case Some(cachedStamp) if equiv(cachedStamp.stamp, stamp) => None @@ -110,7 +116,7 @@ private[sbt] object ExternalHooks { case _ => Some(file) } } - }.toSet) + }) } override def removedProducts(previousAnalysis: CompileAnalysis): Option[Set[File]] = { diff --git a/main/src/main/scala/sbt/nio/Keys.scala b/main/src/main/scala/sbt/nio/Keys.scala index 32b61b0bd..918641c51 100644 --- a/main/src/main/scala/sbt/nio/Keys.scala +++ b/main/src/main/scala/sbt/nio/Keys.scala @@ -160,6 +160,8 @@ object Keys { 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 dependencyClasspathFiles = + taskKey[Seq[Path]]("The dependency classpath for a task.").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 1f22d406f..bc54946ba 100644 --- a/main/src/main/scala/sbt/nio/Settings.scala +++ b/main/src/main/scala/sbt/nio/Settings.scala @@ -242,12 +242,12 @@ private[sbt] object Settings { } prevMap.forEach((p, _) => deletedBuilder += p) val unmodified = unmodifiedBuilder.result() - if (unmodified.size == current.size) { + val deleted = deletedBuilder.result() + val created = createdBuilder.result() + val modified = modifiedBuilder.result() + if (created.isEmpty && deleted.isEmpty && modified.isEmpty) { FileChanges.unmodified(unmodifiedBuilder.result) } else { - val created = createdBuilder.result() - val deleted = deletedBuilder.result() - val modified = modifiedBuilder.result() FileChanges(created, deleted, modified, unmodified) } }