From f426f8ba7f57546331c4003f7b8ed64a2e5f8ea0 Mon Sep 17 00:00:00 2001 From: Grzegorz Kossakowski Date: Mon, 24 Jun 2013 14:20:13 -0700 Subject: [PATCH 1/4] Allow an empty string as a default in variable substitution. Lanucher configuration parser would previously require a default value in variable substitution pattern to be non-empty string. This is an unnecessary restriction as empty value is sometimes useful as in this example: resources: ${sbt.extraClasspath-} This commit lifts this restriction so empty default values are allowed. The change has been discussed with @harrah. --- launch/src/main/scala/xsbt/boot/ConfigurationParser.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/launch/src/main/scala/xsbt/boot/ConfigurationParser.scala b/launch/src/main/scala/xsbt/boot/ConfigurationParser.scala index 4bae3736f..4a2325dd4 100644 --- a/launch/src/main/scala/xsbt/boot/ConfigurationParser.scala +++ b/launch/src/main/scala/xsbt/boot/ConfigurationParser.scala @@ -18,7 +18,7 @@ object ConfigurationParser def trim(s: Array[String]) = s.map(_.trim).toList def ids(value: String) = trim(substituteVariables(value).split(",")).filter(isNonEmpty) - private[this] lazy val VarPattern = Pattern.compile("""\$\{([\w.]+)(\-(.+))?\}""") + private[this] lazy val VarPattern = Pattern.compile("""\$\{([\w.]+)(\-(.*))?\}""") def substituteVariables(s: String): String = if(s.indexOf('$') >= 0) substituteVariables0(s) else s // scala.util.Regex brought in 30kB, so we code it explicitly def substituteVariables0(s: String): String = From ad718587bb3686fb7492b5dbfee53e696517c38d Mon Sep 17 00:00:00 2001 From: Grzegorz Kossakowski Date: Mon, 24 Jun 2013 14:24:39 -0700 Subject: [PATCH 2/4] Configure additional classpath through `sbt.extraClasspath` Modify the default launcher configuration for sbt so extra classpath entries for sbt can be configured through `sbt.extraClasspath` system property. --- launch/src/main/input_resources/sbt/sbt.boot.properties | 1 + 1 file changed, 1 insertion(+) diff --git a/launch/src/main/input_resources/sbt/sbt.boot.properties b/launch/src/main/input_resources/sbt/sbt.boot.properties index 46d269f93..f7460164c 100644 --- a/launch/src/main/input_resources/sbt/sbt.boot.properties +++ b/launch/src/main/input_resources/sbt/sbt.boot.properties @@ -8,6 +8,7 @@ class: ${sbt.main.class-sbt.xMain} components: xsbti,extra cross-versioned: ${sbt.cross.versioned-false} + resources: ${sbt.extraClasspath-} [repositories] local From 025eae9103774a3133a06e940b9fbb027d5bc9f3 Mon Sep 17 00:00:00 2001 From: Grzegorz Kossakowski Date: Tue, 25 Jun 2013 00:04:39 -0700 Subject: [PATCH 3/4] Log API diffs using ShowAPI and java-diff-utils library. Implement displaying API changes by using textual representation of an API (ShowAPI) and good, old textual diff algorithm. We are using java-diff-utils library that is distributed under Apache 2.0 license. Notice that we have only soft dependency on java-diff-utils. It means that we'll try to lookup java-diff-utils class through reflection and fail gracefully if none is found on the classpath. This way sbt is not getting any new dependency. If user needs to debug api diffs then it's matter of starting sbt with `-Dsbt.extraClasspath=path/to/diffutils.jar` option passed to sbt launcher. --- .../inc/src/main/scala/sbt/inc/APIDiff.scala | 67 +++++++++++++++++++ .../src/main/scala/sbt/inc/Incremental.scala | 39 +++++++++-- 2 files changed, 102 insertions(+), 4 deletions(-) create mode 100644 compile/inc/src/main/scala/sbt/inc/APIDiff.scala diff --git a/compile/inc/src/main/scala/sbt/inc/APIDiff.scala b/compile/inc/src/main/scala/sbt/inc/APIDiff.scala new file mode 100644 index 000000000..3f79faa00 --- /dev/null +++ b/compile/inc/src/main/scala/sbt/inc/APIDiff.scala @@ -0,0 +1,67 @@ +package sbt.inc + +import xsbti.api.SourceAPI +import xsbt.api.ShowAPI +import xsbt.api.DefaultShowAPI._ +import java.lang.reflect.Method +import java.util.{List => JList} + +/** + * A class which computes diffs (unified diffs) between two textual representations of an API. + * + * Internally, it uses java-diff-utils library but it calls it through reflection so there's + * no hard dependency on java-diff-utils. + * + * The reflective lookup of java-diff-utils library is performed in the constructor. Exceptions + * thrown by reflection are passed as-is to the caller of the constructor. + * + * @throws ClassNotFoundException if difflib.DiffUtils class cannot be located + * @throws LinkageError + * @throws ExceptionInInitializerError + */ +class APIDiff { + + import APIDiff._ + + private val diffUtilsClass = Class.forName(diffUtilsClassName) + // method signature: diff(List, List) + private val diffMethod: Method = + diffUtilsClass.getMethod(diffMethodName, classOf[JList[_]], classOf[JList[_]]) + + private val generateUnifiedDiffMethod: Method = { + val patchClass = Class.forName(patchClassName) + // method signature: generateUnifiedDiff(String, String, List, Patch, int) + diffUtilsClass.getMethod(generateUnifiedDiffMethodName, classOf[String], + classOf[String], classOf[JList[String]], patchClass, classOf[Int]) + } + + /** + * Generates an unified diff between textual representations of `api1` and `api2`. + */ + def generateApiDiff(fileName: String, api1: SourceAPI, api2: SourceAPI, contextSize: Int): String = { + val api1Str = ShowAPI.show(api1) + val api2Str = ShowAPI.show(api2) + generateApiDiff(fileName, api1Str, api2Str, contextSize) + } + + private def generateApiDiff(fileName: String, f1: String, f2: String, contextSize: Int): String = { + assert((diffMethod != null) && (generateUnifiedDiffMethod != null), "APIDiff isn't properly initialized.") + import scala.collection.JavaConverters._ + def asJavaList[T](it: Iterator[T]): java.util.List[T] = it.toSeq.asJava + val f1Lines = asJavaList(f1.lines) + val f2Lines = asJavaList(f2.lines) + //val diff = DiffUtils.diff(f1Lines, f2Lines) + val diff /*: Patch*/ = diffMethod.invoke(null, f1Lines, f2Lines) + val unifiedPatch: JList[String] = generateUnifiedDiffMethod.invoke(null, fileName, fileName, f1Lines, diff, + (contextSize: java.lang.Integer)).asInstanceOf[JList[String]] + unifiedPatch.asScala.mkString("\n") + } + +} + +object APIDiff { + private val diffUtilsClassName = "difflib.DiffUtils" + private val patchClassName = "difflib.Patch" + private val diffMethodName = "diff" + private val generateUnifiedDiffMethodName = "generateUnifiedDiff" +} diff --git a/compile/inc/src/main/scala/sbt/inc/Incremental.scala b/compile/inc/src/main/scala/sbt/inc/Incremental.scala index 88289a426..fa39550ab 100644 --- a/compile/inc/src/main/scala/sbt/inc/Incremental.scala +++ b/compile/inc/src/main/scala/sbt/inc/Incremental.scala @@ -76,7 +76,7 @@ object Incremental 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) + val incChanges = changedIncremental(invalidated, previous.apis.internalAPI _, merged.apis.internalAPI _, log, options) debug("\nChanges:\n" + incChanges) val transitiveStep = options.transitiveStep val incInv = invalidateIncremental(merged.relations, incChanges, invalidated, cycleNum >= transitiveStep, log) @@ -101,17 +101,48 @@ object Incremental private[this] def invalidatedPackageObjects(invalidated: Set[File], relations: Relations): Set[File] = invalidated flatMap relations.publicInherited.internal.reverse filter { _.getName == "package.scala" } + /** + * Logs API changes using debug-level logging. The API are obtained using the APIDiff class. + * + * NOTE: This method creates a new APIDiff instance on every invocation. + */ + private def logApiChanges[T](changes: (collection.Set[T], Seq[Source], Seq[Source]), log: Logger, + options: IncOptions): Unit = { + val contextSize = 5 + try { + val apiDiff = new APIDiff + changes.zipped foreach { + case (src, oldApi, newApi) => + val apiUnifiedPatch = apiDiff.generateApiDiff(src.toString, oldApi.api, newApi.api, contextSize) + log.debug("Detected a change in a public API:\n" + apiUnifiedPatch) + } + } catch { + case e: ClassNotFoundException => + log.error("You have api debugging enabled but DiffUtils library cannot be found on sbt's classpath") + case e: LinkageError => + log.error("Encoutared linkage error while trying to load DiffUtils library.") + log.trace(e) + case e: Exception => + log.error("An exception has been thrown while trying to dump an api diff.") + log.trace(e) + } + } + /** * Accepts the sources that were recompiled during the last step and functions * providing the API before and after the last step. The functions should return * an empty API if the file did not/does not exist. */ - def changedIncremental[T](lastSources: collection.Set[T], oldAPI: T => Source, newAPI: T => Source, options: IncOptions): APIChanges[T] = + def changedIncremental[T](lastSources: collection.Set[T], oldAPI: T => Source, newAPI: T => Source, log: Logger, options: IncOptions): APIChanges[T] = { val oldApis = lastSources.toSeq map oldAPI val newApis = lastSources.toSeq map newAPI val changes = (lastSources, oldApis, newApis).zipped.filter { (src, oldApi, newApi) => !sameSource(oldApi, newApi) } + if (apiDebug(options) && changes.zipped.nonEmpty) { + logApiChanges(changes, log, options) + } + val changedNames = TopLevel.nameChanges(changes._3, changes._2 ) val modifiedAPIs = changes._1.toSet @@ -138,7 +169,7 @@ object Incremental val srcChanges = changes(previous.allInternalSources.toSet, sources, f => !equivS.equiv( previous.internalSource(f), current.internalSource(f) ) ) val removedProducts = previous.allProducts.filter( p => !equivS.equiv( previous.product(p), current.product(p) ) ).toSet val binaryDepChanges = previous.allBinaries.filter( externalBinaryModified(entry, forEntry, previous, current, log)).toSet - val extChanges = changedIncremental(previousAPIs.allExternals, previousAPIs.externalAPI _, currentExternalAPI(entry, forEntry), options) + val extChanges = changedIncremental(previousAPIs.allExternals, previousAPIs.externalAPI _, currentExternalAPI(entry, forEntry), log, options) InitialChanges(srcChanges, removedProducts, binaryDepChanges, extChanges ) } @@ -229,7 +260,7 @@ object Incremental /** Intermediate invalidation step: steps after the initial invalidation, but before the final transitive invalidation. */ def invalidateIntermediate(relations: Relations, modified: Set[File], log: Logger): Set[File] = { - def reverse(r: Relations.Source) = r.internal.reverse _ + def reverse(r: Relations.Source) = r.internal.reverse _ invalidateSources(reverse(relations.direct), reverse(relations.publicInherited), modified, log) } /** Invalidates inheritance dependencies, transitively. Then, invalidates direct dependencies. Finally, excludes initial dependencies not From 0d2dc7511507087a8abb15483cc4eb8c305d2b0a Mon Sep 17 00:00:00 2001 From: Grzegorz Kossakowski Date: Tue, 25 Jun 2013 00:00:50 -0700 Subject: [PATCH 4/4] Make api diff context size valuable configurable. Add `apiDiffContextSize` option to `IncOptions` which allows one to control the size (in lines) of a context used when printing diffs for textual API representation. The default value for `apiDiffContextSize` is 5 which seems to be enough for most situations. This is verified by many debugging sessions I performed when using api diffing functionality. --- .../src/main/scala/sbt/inc/IncOptions.scala | 20 +++++++++++++++++-- .../src/main/scala/sbt/inc/Incremental.scala | 2 +- 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/compile/inc/src/main/scala/sbt/inc/IncOptions.scala b/compile/inc/src/main/scala/sbt/inc/IncOptions.scala index db15b92c2..81b6910b0 100644 --- a/compile/inc/src/main/scala/sbt/inc/IncOptions.scala +++ b/compile/inc/src/main/scala/sbt/inc/IncOptions.scala @@ -23,9 +23,17 @@ final case class IncOptions( * 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 + * - diffing textual API representation which helps understanding what kind of changes + * to APIs are visible to the incremental compiler */ apiDebug: Boolean, + /** + * Controls context size (in lines) displayed when diffs are produced for textual API + * representation. + * + * This option is used only when `apiDebug == true`. + */ + apiDiffContextSize: Int, /** * 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 @@ -45,6 +53,7 @@ object IncOptions { recompileAllFraction = 0.5, relationsDebug = false, apiDebug = false, + apiDiffContextSize = 5, apiDumpDirectory = None, newClassfileManager = ClassfileManager.deleteImmediately ) @@ -57,6 +66,7 @@ object IncOptions { val relationsDebugKey = "relationsDebug" val apiDebugKey = "apiDebug" val apiDumpDirectoryKey = "apiDumpDirectory" + val apiDiffContextSize = "apiDiffContextSize" def fromStringMap(m: java.util.Map[String, String]): IncOptions = { // all the code below doesn't look like idiomatic Scala for a good reason: we are working with Java API @@ -76,6 +86,10 @@ object IncOptions { val k = apiDebugKey if (m.containsKey(k)) m.get(k).toBoolean else Default.apiDebug } + def getApiDiffContextSize: Int = { + val k = apiDiffContextSize + if (m.containsKey(k)) m.get(k).toInt else Default.apiDiffContextSize + } def getApiDumpDirectory: Option[java.io.File] = { val k = apiDumpDirectoryKey if (m.containsKey(k)) @@ -83,7 +97,8 @@ object IncOptions { else None } - IncOptions(getTransitiveStep, getRecompileAllFraction, getRelationsDebug, getApiDebug, getApiDumpDirectory, ClassfileManager.deleteImmediately) + IncOptions(getTransitiveStep, getRecompileAllFraction, getRelationsDebug, getApiDebug, getApiDiffContextSize, + getApiDumpDirectory, ClassfileManager.deleteImmediately) } def toStringMap(o: IncOptions): java.util.Map[String, String] = { @@ -93,6 +108,7 @@ object IncOptions { m.put(relationsDebugKey, o.relationsDebug.toString) m.put(apiDebugKey, o.apiDebug.toString) o.apiDumpDirectory.foreach(f => m.put(apiDumpDirectoryKey, f.toString)) + m.put(apiDiffContextSize, o.apiDiffContextSize.toString) m } } diff --git a/compile/inc/src/main/scala/sbt/inc/Incremental.scala b/compile/inc/src/main/scala/sbt/inc/Incremental.scala index fa39550ab..aaa6d3410 100644 --- a/compile/inc/src/main/scala/sbt/inc/Incremental.scala +++ b/compile/inc/src/main/scala/sbt/inc/Incremental.scala @@ -108,7 +108,7 @@ object Incremental */ private def logApiChanges[T](changes: (collection.Set[T], Seq[Source], Seq[Source]), log: Logger, options: IncOptions): Unit = { - val contextSize = 5 + val contextSize = options.apiDiffContextSize try { val apiDiff = new APIDiff changes.zipped foreach {