Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow clients to override hashing classpath. #427

Merged
merged 2 commits into from
Oct 26, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,8 @@ interface Lookup {
* @return API changes
*/
boolean shouldDoIncrementalCompilation(Set<String> changedClasses, CompileAnalysis previousAnalysis);

Optional<FileHash[]> hashClasspath(File[] classpath);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kind of curious. Does it matter if this takes File[] or just hash(File)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This way you can add e.g. paralelizm but with hash(File) you are not.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok. Sounds good to me.

}

/**
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
ProblemFilters.exclude[ReversedMissingMethodProblem]("xsbti.compile.ExternalHooks#Lookup.hashClasspath")
Original file line number Diff line number Diff line change
Expand Up @@ -90,3 +90,12 @@ trait ExternalLookup extends ExternalHooks.Lookup {
shouldDoIncrementalCompilation(changedClasses.iterator().asScala.toSet, previousAnalysis)
}
}

trait NoopExternalLookup extends ExternalLookup {
override def changedSources(previous: CompileAnalysis): Option[Changes[File]] = None
override def changedBinaries(previous: CompileAnalysis): Option[Set[File]] = None
override def removedProducts(previous: CompileAnalysis): Option[Set[File]] = None
override def shouldDoIncrementalCompilation(changedClasses: Set[String],
analysis: CompileAnalysis): Boolean = true
override def hashClasspath(classpath: Array[File]): Optional[Array[FileHash]] = Optional.empty()
}
11 changes: 6 additions & 5 deletions zinc/src/main/scala/sbt/internal/inc/LookupImpl.scala
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
package sbt.internal.inc

import java.io.File
import java.util.Optional

import xsbti.compile.{ Changes, CompileAnalysis, FileHash, MiniSetup }

Expand Down Expand Up @@ -39,11 +40,8 @@ class LookupImpl(compileConfiguration: CompileConfiguration, previousSetup: Opti

private val entry = MixedAnalyzingCompiler.classPathLookup(compileConfiguration)

override def lookupAnalysis(binaryClassName: String): Option[CompileAnalysis] = {
analyses.collectFirst {
case a if a.relations.productClassName._2s.contains(binaryClassName) => a
}
}
override def lookupAnalysis(binaryClassName: String): Option[CompileAnalysis] =
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not make this non-optional and provide a default implementation?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Empty result here has a meaning - that we have to treat this class as Library dependency instead of External one (so names will be invalidated on any change in *.class or *.jar file).

This change was just optimization since find is faster then collectFirst (and code looks also better)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought the empty meant that it falls back to the default impl of blindly using SHA-1. Couldn't that default impl just be moved to be the default?

analyses.find(_.relations.productClassName._2s.contains(binaryClassName))

override def lookupOnClasspath(binaryClassName: String): Option[File] =
entry(binaryClassName)
Expand All @@ -64,4 +62,7 @@ class LookupImpl(compileConfiguration: CompileConfiguration, previousSetup: Opti
override def shouldDoIncrementalCompilation(changedClasses: Set[String],
analysis: CompileAnalysis): Boolean =
externalLookup.forall(_.shouldDoIncrementalCompilation(changedClasses, analysis))

override def hashClasspath(classpath: Array[File]): Optional[Array[FileHash]] =
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice, this allows the user to provide a parallel implementation.

externalLookup.map(_.hashClasspath(classpath)).getOrElse(Optional.empty())
}
16 changes: 12 additions & 4 deletions zinc/src/main/scala/sbt/internal/inc/MixedAnalyzingCompiler.scala
Original file line number Diff line number Diff line change
Expand Up @@ -181,13 +181,21 @@ object MixedAnalyzingCompiler {
incrementalCompilerOptions: IncOptions,
extra: List[(String, String)]
): CompileConfiguration = {
val classpathHash = classpath map { x =>
FileHash.of(x, Stamper.forHash(x).hashCode)
}
val lookup = incrementalCompilerOptions.externalHooks().getExternalLookup

def doHash: Array[FileHash] =
classpath.map(x => FileHash.of(x, Stamper.forHash(x).hashCode))(collection.breakOut)

val classpathHash =
if (lookup.isPresent) {
val computed = lookup.get().hashClasspath(classpath.toArray)
if (computed.isPresent) computed.get() else doHash
} else doHash

val compileSetup = MiniSetup.of(
output,
MiniOptions.of(
classpathHash.toArray,
classpathHash,
options.toArray,
javacOptions.toArray
),
Expand Down
35 changes: 35 additions & 0 deletions zinc/src/test/scala/sbt/inc/ClasspathHashingHookSpec.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
package sbt.inc

import java.io.File
import java.util.Optional

import sbt.internal.inc.NoopExternalLookup
import sbt.io.IO
import xsbti.compile.DefaultExternalHooks
import xsbti.compile.FileHash
import xsbti.compile.IncOptions

class ClasspathHashingHookSpec extends BaseCompilerSpec {
it should "allow client to override classpath hashing" in {
IO.withTemporaryDirectory { tempDir =>
val setup = ProjectSetup.simple(tempDir.toPath, SourceFiles.Foo :: Nil)

def hashFile(f: File): FileHash =
FileHash.of(f, f.hashCode() * 37 + 41) // Some deterministic random changes

val lookup = new NoopExternalLookup {
override def hashClasspath(classpath: Array[File]): Optional[Array[FileHash]] =
Optional.of(classpath.map(hashFile))

}
val hooks = new DefaultExternalHooks(Optional.of(lookup), Optional.empty())
val compiler =
setup.createCompiler().copy(incOptions = IncOptions.of().withExternalHooks(hooks))
val result = compiler.doCompile()

result.setup().options().classpathHash().foreach { original =>
original shouldEqual hashFile(original.file())
}
}
}
}