diff --git a/notes/2.0.0/dirzip-restore-on-hit.md b/notes/2.0.0/dirzip-restore-on-hit.md new file mode 100644 index 000000000..afa242a3c --- /dev/null +++ b/notes/2.0.0/dirzip-restore-on-hit.md @@ -0,0 +1,15 @@ +### Directories declared with `Def.declareOutputDirectory` are restored on cache hits + +A directory declared as a cached task's output is packaged as a `.sbtdir.zip` +sibling of the directory. Deleting the directory leaves the sibling zip behind +(an `rm -rf` of the directory or of `classes/` does exactly this), which left +the cache convinced everything was in sync: on the next cache hit the task did not re-run, but the +directory was never re-extracted either. For sbt's own `compile`, whose classes +directory is declared this way, a deleted output directory plus a warm cache +meant `run` failed with `ClassNotFoundException` and no recompile. The cache now +re-extracts a declared directory when any of its files are missing, checking +cheaply (without unzipping) on the warm path. + +This addresses the directory-restoration half of [#9462][i9462]. + +[i9462]: https://github.com/sbt/sbt/issues/9462 diff --git a/sbt-app/src/sbt-test/cache/compile-classes-restore/Main.scala b/sbt-app/src/sbt-test/cache/compile-classes-restore/Main.scala new file mode 100644 index 000000000..368364e2b --- /dev/null +++ b/sbt-app/src/sbt-test/cache/compile-classes-restore/Main.scala @@ -0,0 +1,2 @@ +@main def main(args: String*) = + println("hello from probe") diff --git a/sbt-app/src/sbt-test/cache/compile-classes-restore/build.sbt b/sbt-app/src/sbt-test/cache/compile-classes-restore/build.sbt new file mode 100644 index 000000000..3335d1b29 --- /dev/null +++ b/sbt-app/src/sbt-test/cache/compile-classes-restore/build.sbt @@ -0,0 +1,53 @@ +import sbt.internal.util.CacheEventSummary + +val checkCompileHit = taskKey[Unit]("asserts the previous command took cache hits") +val delClasses = taskKey[Unit]("deletes the classes directory") +val delClassesZip = taskKey[Unit]("deletes the sibling classes.sbtdir.zip") +val checkClasses = taskKey[Unit]("asserts .class files exist") +val checkNoClasses = taskKey[Unit]("asserts no .class files exist") + +Global / localCacheDirectory := baseDirectory.value / "diskcache" + +// A distinct project id keeps this fixture's output paths from colliding with same-named +// sibling fixtures in scripted's shared batch directory. +lazy val compileClassesRestore = project + .in(file(".")) + .settings( + scalaVersion := "3.8.4" + ) + +delClasses := Def.uncached { + val dir = (Compile / classDirectory).value + IO.delete(dir) + streams.value.log.info(s"deleted $dir") +} + +delClassesZip := Def.uncached { + val dir = (Compile / classDirectory).value + val zip = new java.io.File(dir.getParentFile, dir.getName + ".sbtdir.zip") + streams.value.log.info(s"deleting $zip (exists=${zip.exists})") + IO.delete(zip) +} + +checkClasses := Def.uncached { + val dir = (Compile / classDirectory).value + val classes = (dir ** "*.class").get() + streams.value.log.info(s"classes under $dir: ${classes.mkString(", ")}") + assert(classes.nonEmpty, s"no class files under $dir") +} + +checkNoClasses := Def.uncached { + val dir = (Compile / classDirectory).value + val classes = (dir ** "*.class").get() + streams.value.log.info(s"classes under $dir: ${classes.mkString(", ")}") + assert(classes.isEmpty, s"unexpected class files under $dir: ${classes.mkString(", ")}") +} + +checkCompileHit := Def.uncached { + val config = Def.cacheConfiguration.value + val prev = config.cacheEventLog.previous match + case s: CacheEventSummary.Data => s + case _ => sys.error("empty event log") + streams.value.log.info(s"prev hitCount=${prev.hitCount} missCount=${prev.missCount}") + assert(prev.hitCount >= 1, s"expected cache hits but hitCount=${prev.hitCount}") +} diff --git a/sbt-app/src/sbt-test/cache/compile-classes-restore/test b/sbt-app/src/sbt-test/cache/compile-classes-restore/test new file mode 100644 index 000000000..1ddc3bc71 --- /dev/null +++ b/sbt-app/src/sbt-test/cache/compile-classes-restore/test @@ -0,0 +1,18 @@ +# Regression for #9462: compile's classes dir (declared via Def.declareOutputDirectory) +# must be restored on a cache hit after the classes directory is deleted. +> compile +> checkClasses +> run +> delClasses +> checkNoClasses +> compile +> checkCompileHit +> checkClasses +> run + +# restoration also works when the sibling classes.sbtdir.zip is gone too +> delClasses +> delClassesZip +> compile +> checkClasses +> run diff --git a/sbt-app/src/sbt-test/cache/declare-output-dir-restore/build.sbt b/sbt-app/src/sbt-test/cache/declare-output-dir-restore/build.sbt new file mode 100644 index 000000000..2b3fa4f6c --- /dev/null +++ b/sbt-app/src/sbt-test/cache/declare-output-dir-restore/build.sbt @@ -0,0 +1,60 @@ +import sbt.internal.util.CacheEventSummary +import xsbti.HashedVirtualFileRef + +val declareDir = taskKey[HashedVirtualFileRef]("writes 2 files into a dir and declares the dir") +val checkFiles = taskKey[Unit]("asserts both files exist") +val checkGone = taskKey[Unit]("asserts the dir has no files") +val delDir = taskKey[Unit]("deletes the generated dir") +val delZip = taskKey[Unit]("deletes the sibling .sbtdir.zip") +val checkHit = taskKey[Unit]("asserts previous command was a pure cache hit") + +Global / localCacheDirectory := baseDirectory.value / "diskcache" + +lazy val declareOutputDirRestore = project.in(file(".")) + +declareDir := { + val log = streams.value.log + val dir = target.value / "gen-dir" + IO.createDirectory(dir) + IO.write(dir / "a.txt", "contents A") + IO.write(dir / "b.txt", "contents B") + log.info(s"COMPUTED declareDir (cache miss)") + val vf = fileConverter.value.toVirtualFile(dir.toPath) + Def.declareOutputDirectory(vf) +} + +checkFiles := Def.uncached { + val log = streams.value.log + val dir = target.value / "gen-dir" + val listing = if (dir.exists) (dir ** "*").get().mkString(", ") else "" + log.info(s"gen-dir listing: $listing") + assert((dir / "a.txt").exists, s"a.txt missing under $dir") + assert((dir / "b.txt").exists, s"b.txt missing under $dir") +} + +checkGone := Def.uncached { + val dir = target.value / "gen-dir" + assert(!(dir / "a.txt").exists && !(dir / "b.txt").exists, s"files still present under $dir") +} + +delDir := Def.uncached { + val dir = target.value / "gen-dir" + IO.delete(dir) + streams.value.log.info(s"deleted $dir") +} + +delZip := Def.uncached { + val zip = new java.io.File(target.value, "gen-dir.sbtdir.zip") + streams.value.log.info(s"deleting $zip (exists=${zip.exists})") + IO.delete(zip) +} + +checkHit := Def.uncached { + val config = Def.cacheConfiguration.value + val prev = config.cacheEventLog.previous match + case s: CacheEventSummary.Data => s + case _ => sys.error("empty event log") + streams.value.log.info(s"prev hitCount=${prev.hitCount} missCount=${prev.missCount}") + assert(prev.missCount == 0, s"expected pure hit but missCount=${prev.missCount}") + assert(prev.hitCount >= 1, s"expected a hit but hitCount=${prev.hitCount}") +} diff --git a/sbt-app/src/sbt-test/cache/declare-output-dir-restore/test b/sbt-app/src/sbt-test/cache/declare-output-dir-restore/test new file mode 100644 index 000000000..79b8154d3 --- /dev/null +++ b/sbt-app/src/sbt-test/cache/declare-output-dir-restore/test @@ -0,0 +1,16 @@ +# Regression for #9462: a Def.declareOutputDirectory dir must be restored on a cache hit +# after the extracted directory is deleted (the sibling .sbtdir.zip survives an rm -rf). +> declareDir +> checkFiles +> delDir +> checkGone +> declareDir +> checkHit +> checkFiles + +# restoration also works when the sibling zip itself is gone +> delDir +> delZip +> declareDir +> checkHit +> checkFiles diff --git a/util-cache/src/main/scala/sbt/util/ActionCache.scala b/util-cache/src/main/scala/sbt/util/ActionCache.scala index 79f9c3cd2..99c0d4be3 100644 --- a/util-cache/src/main/scala/sbt/util/ActionCache.scala +++ b/util-cache/src/main/scala/sbt/util/ActionCache.scala @@ -342,6 +342,11 @@ object ActionCache: val json = Parser.parseFromFile(manifest.toFile()).get Converter.fromJsonUnsafe[Manifest](json) + private[sbt] def manifestFromString(manifest: String): Manifest = + import sbt.internal.util.codec.ManifestCodec.given + val json = Parser.parseUnsafe(manifest) + Converter.fromJsonUnsafe[Manifest](json) + private val default2010Timestamp: Long = 1262304000000L /** diff --git a/util-cache/src/main/scala/sbt/util/ActionCacheStore.scala b/util-cache/src/main/scala/sbt/util/ActionCacheStore.scala index 4eaf41961..6b8d2f750 100644 --- a/util-cache/src/main/scala/sbt/util/ActionCacheStore.scala +++ b/util-cache/src/main/scala/sbt/util/ActionCacheStore.scala @@ -1,6 +1,7 @@ package sbt.util import java.io.{ ByteArrayInputStream, IOException } +import java.nio.charset.StandardCharsets import java.nio.ByteBuffer import java.nio.file.{ Files, @@ -11,6 +12,7 @@ import java.nio.file.{ StandardCopyOption } import java.util.concurrent.atomic.AtomicBoolean +import java.util.zip.ZipFile import sjsonnew.support.scalajson.unsafe.{ CompactPrinter, Converter, Parser } import sjsonnew.shaded.scalajson.ast.unsafe.JValue @@ -339,7 +341,9 @@ case class DiskActionCacheStore(base: Path, converter: FileConverter) try // `!symlinkSupported` prevents unnecessary deletion of files and then copying them again // in #writeFileAndNotify on machines that don't support symlinks. - if Digest.sameDigest(p, d) && (!symlinkSupported.get() || Files.isSymbolicLink(p)) then p + if Digest.sameDigest(p, d) && (!symlinkSupported.get() || Files.isSymbolicLink(p)) then + afterFileUpToDate(ref, p, outputDirectory) + p else // println(s"- syncFile: $p has different digest") IO.delete(p.toFile()) @@ -357,6 +361,31 @@ case class DiskActionCacheStore(base: Path, converter: FileConverter) if path.toString().endsWith(ActionCache.dirZipExt) then unpackageDirZip(path, outputDirectory) else () + /** + * Re-extract a dirzip whose directory or manifest-listed files are missing; presence is + * checked without unzipping. + */ + private def afterFileUpToDate( + ref: HashedVirtualFileRef, + path: Path, + outputDirectory: Path + ): Unit = + if path.toString().endsWith(ActionCache.dirZipExt) && !dirZipContentsPresent(path) then + val _ = unpackageDirZip(path, outputDirectory) + + private def dirZipContentsPresent(dirzip: Path): Boolean = + val dirPath = Paths.get(dirzip.toString.dropRight(ActionCache.dirZipExt.size)) + try + Files.isDirectory(dirPath) && + Using.resource(new ZipFile(dirzip.toFile())): zf => + Option(zf.getEntry(ActionCache.manifestFileName)) match + case None => false + case Some(entry) => + val str = new String(zf.getInputStream(entry).readAllBytes(), StandardCharsets.UTF_8) + val m = ActionCache.manifestFromString(str) + m.outputFiles.forall(ref => Files.exists(converter.toPath(ref))) + catch case NonFatal(_) => false + /** * Given a dirzip, unzip it in a temp directory, and sync each items to the outputDirectory. */ diff --git a/util-cache/src/test/scala/sbt/util/ActionCacheTest.scala b/util-cache/src/test/scala/sbt/util/ActionCacheTest.scala index 03c858829..624cd5642 100644 --- a/util-cache/src/test/scala/sbt/util/ActionCacheTest.scala +++ b/util-cache/src/test/scala/sbt/util/ActionCacheTest.scala @@ -1,6 +1,6 @@ package sbt.util -import java.io.{ IOException, InputStream } +import java.io.{ ByteArrayInputStream, IOException, InputStream } import java.nio.charset.StandardCharsets import java.nio.file.{ Files, NoSuchFileException, Path, Paths } import java.util.Optional @@ -88,6 +88,30 @@ object ActionCacheTest extends BasicTestSuite: index += 1 b + test("Disk cache re-extracts a dirzip whose directory was deleted"): + withDiskCache: cache => + IO.withTemporaryDirectory: tempDir => + val outputDirectory = tempDir.toPath() + val dir = tempDir / "gen-dir" + IO.write(dir / "a.txt", "contents A") + IO.write(dir / "b.txt", "contents B") + val zipVf = ActionCache.packageDirectory( + binaryFileConverter.toVirtualFile(dir.toPath()), + binaryFileConverter, + outputDirectory, + ) + val refs = cache.putBlobs(Seq(zipVf)) + assert(refs.size == 1) + + cache.syncBlobs(refs, outputDirectory) + assert((dir / "a.txt").exists && (dir / "b.txt").exists) + + IO.delete(dir) + assert(!dir.exists) + cache.syncBlobs(refs, outputDirectory) + assert((dir / "a.txt").exists, "a.txt not re-extracted after the directory was deleted") + assert((dir / "b.txt").exists, "b.txt not re-extracted after the directory was deleted") + test("In-memory cache can hold action value"): withInMemoryCache(testActionCacheBasic) @@ -507,6 +531,18 @@ object ActionCacheTest extends BasicTestSuite: cacheVersion, ) + // The String-based fileConverter mangles binary blobs (zips). + def binaryFileConverter = new FileConverter: + override def toPath(ref: VirtualFileRef): Path = Paths.get(ref.id) + override def toVirtualFile(path: Path): VirtualFile = + val bytes = + if Files.isRegularFile(path) then Files.readAllBytes(path) else Array.empty[Byte] + new xsbti.BasicVirtualFileRef(path.toString) with VirtualFile: + override def contentHash: Long = sbt.util.HashUtil.xxhash64(bytes) + override def sizeBytes: Long = bytes.length.toLong + override def contentHashStr: String = Digest.sha256Hash(bytes).contentHashStr + override def input: InputStream = new ByteArrayInputStream(bytes) + def fileConverter = new FileConverter: override def toPath(ref: VirtualFileRef): Path = Paths.get(ref.id) override def toVirtualFile(path: Path): VirtualFile =