From 925ec98900cb7d8af4a6d4dc5656f035c2c9bff3 Mon Sep 17 00:00:00 2001 From: Mark Harrah Date: Sun, 3 Mar 2013 19:43:37 -0500 Subject: [PATCH] Restore class files after an unsuccessful compilation. This is useful when an error occurs in a later incremental step that requires a fix in the originally changed files. CC @gkossakowski --- .../main/scala/sbt/inc/ClassfileManager.scala | 65 +++++++++++++++++ .../src/main/scala/sbt/inc/IncOptions.scala | 70 ++++++++++--------- .../src/main/scala/sbt/inc/Incremental.scala | 46 ++++++++---- main/src/main/scala/sbt/Defaults.scala | 2 +- .../restore-classes/build.sbt | 12 ++++ .../restore-classes/changes/A1.scala | 3 + .../restore-classes/changes/A2.scala | 5 ++ .../restore-classes/changes/B.scala | 4 ++ .../source-dependencies/restore-classes/test | 23 ++++++ 9 files changed, 184 insertions(+), 46 deletions(-) create mode 100644 compile/inc/src/main/scala/sbt/inc/ClassfileManager.scala create mode 100644 sbt/src/sbt-test/source-dependencies/restore-classes/build.sbt create mode 100644 sbt/src/sbt-test/source-dependencies/restore-classes/changes/A1.scala create mode 100644 sbt/src/sbt-test/source-dependencies/restore-classes/changes/A2.scala create mode 100644 sbt/src/sbt-test/source-dependencies/restore-classes/changes/B.scala create mode 100644 sbt/src/sbt-test/source-dependencies/restore-classes/test diff --git a/compile/inc/src/main/scala/sbt/inc/ClassfileManager.scala b/compile/inc/src/main/scala/sbt/inc/ClassfileManager.scala new file mode 100644 index 000000000..636c0aec3 --- /dev/null +++ b/compile/inc/src/main/scala/sbt/inc/ClassfileManager.scala @@ -0,0 +1,65 @@ +package sbt.inc + + import sbt.IO + import java.io.File + import collection.mutable + +/** During an incremental compilation run, a ClassfileManager deletes class files and is notified of generated class files. +* A ClassfileManager can be used only once.*/ +trait ClassfileManager +{ + /** Called once per compilation step with the class files to delete prior to that step's compilation. + * The files in `classes` must not exist if this method returns normally. + * Any empty ancestor directories of deleted files must not exist either.*/ + def delete(classes: Iterable[File]): Unit + + /** Called once per compilation step with the class files generated during that step.*/ + def generated(classes: Iterable[File]): Unit + + /** Called once at the end of the whole compilation run, with `success` indicating whether compilation succeeded (true) or not (false).*/ + def complete(success: Boolean): Unit +} + +object ClassfileManager +{ + /** Constructs a minimal ClassfileManager implementation that immediately deletes class files when requested. */ + val deleteImmediately: () => ClassfileManager = () => new ClassfileManager + { + def delete(classes: Iterable[File]): Unit = IO.deleteFilesEmptyDirs(classes) + def generated(classes: Iterable[File]) {} + def complete(success: Boolean) {} + } + /** When compilation fails, this ClassfileManager restores class files to the way they were before compilation.*/ + def transactional(tempDir0: File): () => ClassfileManager = () => new ClassfileManager + { + val tempDir = tempDir0.getCanonicalFile + IO.delete(tempDir) + IO.createDirectory(tempDir) + + private[this] val generatedClasses = new mutable.HashSet[File] + private[this] val movedClasses = new mutable.HashMap[File, File] + + def delete(classes: Iterable[File]) + { + for(c <- classes) if(c.exists && !movedClasses.contains(c) && !generatedClasses(c)) + movedClasses.put(c, move(c)) + IO.deleteFilesEmptyDirs(classes) + } + def generated(classes: Iterable[File]): Unit = generatedClasses ++= classes + def complete(success: Boolean) + { + if(!success) { + IO.deleteFilesEmptyDirs(generatedClasses) + for( (orig, tmp) <- movedClasses ) IO.move(tmp, orig) + } + IO.delete(tempDir) + } + + def move(c: File): File = + { + val target = File.createTempFile("sbt", ".class", tempDir) + IO.move(c, target) + target + } + } +} \ No newline at end of file diff --git a/compile/inc/src/main/scala/sbt/inc/IncOptions.scala b/compile/inc/src/main/scala/sbt/inc/IncOptions.scala index 50f37b48e..5be46d85f 100644 --- a/compile/inc/src/main/scala/sbt/inc/IncOptions.scala +++ b/compile/inc/src/main/scala/sbt/inc/IncOptions.scala @@ -1,37 +1,40 @@ package sbt.inc + import java.io.File + /** - * Case class that represents all configuration options for incremental compiler. - * - * Those are options that configure incremental compiler itself and not underlying - * Java/Scala compiler. + * Represents all configuration options for the incremental compiler itself and + * not the underlying Java/Scala compiler. */ -case class IncOptions( - /** After which step include whole transitive closure of invalidated source files. */ - val transitiveStep: Int, - /** - * What's the fraction of invalidated source files when we switch to recompiling - * all files and giving up incremental compilation altogether. That's useful in - * cases when probability that we end up recompiling most of source files but - * in multiple steps is high. Multi-step incremental recompilation is slower - * than recompiling everything in one step. - */ - val recompileAllFraction: Double, - /** Print very detail information about relations (like dependencies between source files). */ - val relationsDebug: Boolean, - /** - * Enable tools for debugging API changes. At the moment that option is unused but in the - * future it will enable for example: - * - disabling API hashing and API minimization (potentially very memory consuming) - * - dumping textual API representation into files - */ - val apiDebug: Boolean, - /** - * The directory where we dump textual representation of APIs. This method might be called - * only if apiDebug returns true. This is unused option at the moment as the needed functionality - * is not implemented yet. - */ - val apiDumpDirectory: Option[java.io.File]) +final case class IncOptions( + /** After which step include whole transitive closure of invalidated source files. */ + transitiveStep: Int, + /** + * What's the fraction of invalidated source files when we switch to recompiling + * all files and giving up incremental compilation altogether. That's useful in + * cases when probability that we end up recompiling most of source files but + * in multiple steps is high. Multi-step incremental recompilation is slower + * than recompiling everything in one step. + */ + recompileAllFraction: Double, + /** Print very detailed information about relations, such as dependencies between source files. */ + relationsDebug: Boolean, + /** + * Enable tools for debugging API changes. At the moment this option is unused but in the + * future it will enable for example: + * - disabling API hashing and API minimization (potentially very memory consuming) + * - dumping textual API representation into files + */ + apiDebug: Boolean, + /** + * The directory where we dump textual representation of APIs. This method might be called + * only if apiDebug returns true. This is unused option at the moment as the needed functionality + * is not implemented yet. + */ + apiDumpDirectory: Option[java.io.File], + /** Creates a new ClassfileManager that will handle class file deletion and addition during a single incremental compilation run. */ + newClassfileManager: () => ClassfileManager +) object IncOptions { val Default = IncOptions( @@ -39,7 +42,10 @@ object IncOptions { recompileAllFraction = 0.5, relationsDebug = false, apiDebug = false, - apiDumpDirectory = None) + apiDumpDirectory = None, + newClassfileManager = ClassfileManager.deleteImmediately + ) + def defaultTransactional(tempDir: File): IncOptions = Default.copy(newClassfileManager = ClassfileManager.transactional(tempDir)) val transitiveStepKey = "transitiveStep" val recompileAllFractionKey = "recompileAllFraction" @@ -72,7 +78,7 @@ object IncOptions { else None } - IncOptions(getTransitiveStep, getRecompileAllFraction, getRelationsDebug, getApiDebug, getApiDumpDirectory) + IncOptions(getTransitiveStep, getRecompileAllFraction, getRelationsDebug, getApiDebug, getApiDumpDirectory, ClassfileManager.deleteImmediately) } def toStringMap(o: IncOptions): java.util.Map[String, String] = { diff --git a/compile/inc/src/main/scala/sbt/inc/Incremental.scala b/compile/inc/src/main/scala/sbt/inc/Incremental.scala index 5979feba1..e9722f294 100644 --- a/compile/inc/src/main/scala/sbt/inc/Incremental.scala +++ b/compile/inc/src/main/scala/sbt/inc/Incremental.scala @@ -13,13 +13,13 @@ import java.io.File object Incremental { def compile(sources: Set[File], - entry: String => Option[File], - previous: Analysis, - current: ReadStamps, - forEntry: File => Option[Analysis], - doCompile: (Set[File], DependencyChanges) => Analysis, - log: Logger, - options: IncOptions)(implicit equivS: Equiv[Stamp]): (Boolean, Analysis) = + entry: String => Option[File], + previous: Analysis, + current: ReadStamps, + forEntry: File => Option[Analysis], + doCompile: (Set[File], DependencyChanges) => Analysis, + log: Logger, + options: IncOptions)(implicit equivS: Equiv[Stamp]): (Boolean, Analysis) = { val initialChanges = changedInitial(entry, sources, previous, current, forEntry, options) val binaryChanges = new DependencyChanges { @@ -29,18 +29,32 @@ object Incremental } val initialInv = invalidateInitial(previous.relations, initialChanges, log) log.debug("Initially invalidated: " + initialInv) - val analysis = cycle(initialInv, sources, binaryChanges, previous, doCompile, 1, log, options) + val analysis = manageClassfiles(options) { classfileManager => + cycle(initialInv, sources, binaryChanges, previous, doCompile, classfileManager, 1, log, options) + } (!initialInv.isEmpty, analysis) } + private[this] def manageClassfiles[T](options: IncOptions)(run: ClassfileManager => T): T = + { + val classfileManager = options.newClassfileManager() + val result = try run(classfileManager) catch { case e: Exception => + classfileManager.complete(success = false) + throw e + } + classfileManager.complete(success = true) + result + } + val incDebugProp = "xsbt.inc.debug" private def incDebug(options: IncOptions): Boolean = options.relationsDebug || java.lang.Boolean.getBoolean(incDebugProp) val apiDebugProp = "xsbt.api.debug" def apiDebug(options: IncOptions): Boolean = options.apiDebug || java.lang.Boolean.getBoolean(apiDebugProp) + // TODO: the Analysis for the last successful compilation should get returned + Boolean indicating success // TODO: full external name changes, scopeInvalidations - def cycle(invalidatedRaw: Set[File], allSources: Set[File], binaryChanges: DependencyChanges, previous: Analysis, - doCompile: (Set[File], DependencyChanges) => Analysis, cycleNum: Int, log: Logger, options: IncOptions): Analysis = + @tailrec def cycle(invalidatedRaw: Set[File], allSources: Set[File], binaryChanges: DependencyChanges, previous: Analysis, + doCompile: (Set[File], DependencyChanges) => Analysis, classfileManager: ClassfileManager, cycleNum: Int, log: Logger, options: IncOptions): Analysis = if(invalidatedRaw.isEmpty) previous else @@ -48,17 +62,20 @@ object Incremental def debug(s: => String) = if (incDebug(options)) log.debug(s) else () val withPackageObjects = invalidatedRaw ++ invalidatedPackageObjects(invalidatedRaw, previous.relations) val invalidated = expand(withPackageObjects, allSources, log, options) - val pruned = prune(invalidated, previous) + val pruned = prune(invalidated, previous, classfileManager) debug("********* Pruned: \n" + pruned.relations + "\n*********") + val fresh = doCompile(invalidated, binaryChanges) + classfileManager.generated(fresh.relations.allProducts) debug("********* Fresh: \n" + fresh.relations + "\n*********") val merged = pruned ++ fresh//.copy(relations = pruned.relations ++ fresh.relations, apis = pruned.apis ++ fresh.apis) debug("********* Merged: \n" + merged.relations + "\n*********") + val incChanges = changedIncremental(invalidated, previous.apis.internalAPI _, merged.apis.internalAPI _, options) debug("Changes:\n" + incChanges) val transitiveStep = options.transitiveStep val incInv = invalidateIncremental(merged.relations, incChanges, invalidated, cycleNum >= transitiveStep, log) - cycle(incInv, allSources, emptyChanges, merged, doCompile, cycleNum+1, log, options) + cycle(incInv, allSources, emptyChanges, merged, doCompile, classfileManager, cycleNum+1, log, options) } private[this] def emptyChanges: DependencyChanges = new DependencyChanges { val modifiedBinaries = new Array[File](0) @@ -227,8 +244,11 @@ object Incremental } def prune(invalidatedSrcs: Set[File], previous: Analysis): Analysis = + prune(invalidatedSrcs, previous, ClassfileManager.deleteImmediately()) + + def prune(invalidatedSrcs: Set[File], previous: Analysis, classfileManager: ClassfileManager): Analysis = { - IO.deleteFilesEmptyDirs( invalidatedSrcs.flatMap(previous.relations.products) ) + classfileManager.delete( invalidatedSrcs.flatMap(previous.relations.products) ) previous -- invalidatedSrcs } diff --git a/main/src/main/scala/sbt/Defaults.scala b/main/src/main/scala/sbt/Defaults.scala index bc9693436..c3678c280 100755 --- a/main/src/main/scala/sbt/Defaults.scala +++ b/main/src/main/scala/sbt/Defaults.scala @@ -200,7 +200,7 @@ object Defaults extends BuildCommon compilersSetting, javacOptions in GlobalScope :== Nil, scalacOptions in GlobalScope :== Nil, - incOptions in GlobalScope :== sbt.inc.IncOptions.Default, + incOptions in GlobalScope := sbt.inc.IncOptions.defaultTransactional(crossTarget.value.getParentFile / "classes.bak"), scalaInstance <<= scalaInstanceTask, scalaVersion in GlobalScope := appConfiguration.value.provider.scalaProvider.version, scalaBinaryVersion in GlobalScope := binaryScalaVersion(scalaVersion.value), diff --git a/sbt/src/sbt-test/source-dependencies/restore-classes/build.sbt b/sbt/src/sbt-test/source-dependencies/restore-classes/build.sbt new file mode 100644 index 000000000..2231204ea --- /dev/null +++ b/sbt/src/sbt-test/source-dependencies/restore-classes/build.sbt @@ -0,0 +1,12 @@ +import complete.DefaultParsers._ + +crossTarget in Compile := target.value + +val checkIterations = inputKey[Unit]("Verifies the accumlated number of iterations of incremental compilation.") + +checkIterations := { + val expected: Int = (Space ~> NatBasic).parsed + val actual: Int = (compile in Compile).value.compilations.allCompilations.size + assert(expected == actual, s"Expected $expected compilations, got $actual") +} + diff --git a/sbt/src/sbt-test/source-dependencies/restore-classes/changes/A1.scala b/sbt/src/sbt-test/source-dependencies/restore-classes/changes/A1.scala new file mode 100644 index 000000000..2a499fa7b --- /dev/null +++ b/sbt/src/sbt-test/source-dependencies/restore-classes/changes/A1.scala @@ -0,0 +1,3 @@ +object A { + val x = 3 +} diff --git a/sbt/src/sbt-test/source-dependencies/restore-classes/changes/A2.scala b/sbt/src/sbt-test/source-dependencies/restore-classes/changes/A2.scala new file mode 100644 index 000000000..10d738255 --- /dev/null +++ b/sbt/src/sbt-test/source-dependencies/restore-classes/changes/A2.scala @@ -0,0 +1,5 @@ +object A { + val x = "a" +} + +class C diff --git a/sbt/src/sbt-test/source-dependencies/restore-classes/changes/B.scala b/sbt/src/sbt-test/source-dependencies/restore-classes/changes/B.scala new file mode 100644 index 000000000..945e97bb3 --- /dev/null +++ b/sbt/src/sbt-test/source-dependencies/restore-classes/changes/B.scala @@ -0,0 +1,4 @@ +object B { + val y: Int = A.x +} + diff --git a/sbt/src/sbt-test/source-dependencies/restore-classes/test b/sbt/src/sbt-test/source-dependencies/restore-classes/test new file mode 100644 index 000000000..028d6226c --- /dev/null +++ b/sbt/src/sbt-test/source-dependencies/restore-classes/test @@ -0,0 +1,23 @@ +$ copy-file changes/A1.scala A.scala +$ copy-file changes/B.scala B.scala +# B depends on A +# 1 iteration +> compile + +$ copy-file changes/A2.scala A.scala + +# will successfully compile A.scala in the first step but fail to compile B.scala in the second +# because type of A.x changed. The original classes should be restored after this failure. +# 2 iterations, but none are recorded in the Analysis +-> compile + +# the class file for C should be deleted: +# it was only added by A2, but compilation hasn't succeeded yet +$ absent target/classes/C.class + + +$ copy-file changes/A1.scala A.scala +# if the classes were correctly restored, another compilation shouldn't be necessary +> compile +# so, there should only be the original 1 iteration recorded in the Analysis +> checkIterations 1