Skip to content

Commit

Permalink
Merge pull request #22786 Scala Plugin: remove ExternalBinariesLookup
Browse files Browse the repository at this point in the history 
Since Gradle 7.x, Gradle's implementation of `ExternalLookup` has become out of sync with Zinc's file stamping and always triggers full recompilation.

Since an implementation of `ExternalLookup` is optional and [Zinc's default](https://github.com/sbt/zinc/blob/develop/internal/zinc-core/src/main/scala/sbt/internal/inc/IncrementalCommon.scala#L734) already does what Gradle is trying to do here, the Gradle-customization can be completely removed.

Passes all existing tests under `./gradlew scala:build` which are all *intra-project* incremental compilation tests, so we know this change doesn't break anything there. It would probably be good to have (had) an *inter-project* incremental compilation test as well to ensure such a regression doesn't happen again, but that is beyond the scope of what I can contribute at this point. @justinb99's reproducer in #20101 would certainly be a good starting point.

Note that this does not address the interplay of the java-library plugin and sbt/zinc#1140 , i.e. only works for jar dependencies. The [workaround](https://github.com/sbt/zinc/blob/develop/internal/zinc-core/src/main/scala/sbt/internal/inc/IncrementalCommon.scala#L44) in zinc might be a short-term solution for that.

Signed-off-by: matthias_ernst <matthias_ernst@apple.com>

Fixes #20101
Fixes #22964

Co-authored-by: Alex Semin <asemin@gradle.com>
  • Loading branch information
@bot-gradle @alllex
bot-gradle and alllex committed Dec 14, 2022
2 parents 97c998c + a1664cc commit bf44f79
Show file tree
Hide file tree
Showing 2 changed files with 0 additions and 96 deletions.
8 changes: 0 additions & 8 deletions ...ala/src/integTest/groovy/org/gradle/scala/compile/ZincScalaCompilerIntegrationTest.groovy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,11 +73,7 @@ class ZincScalaCompilerIntegrationTest extends BasicZincScalaCompilerIntegration

}

@Issue("https://github.com/gradle/gradle/issues/22964")
def "compiles Scala code incrementally"() {
// TODO: remove the assumption when the linked issue fixed for Scala 3
Assume.assumeTrue(versionNumber.major == 2)

file("src/main/scala/Person.scala") << """class Person(val name: String = "foo", val age: Int = 1)"""
file("src/main/scala/House.scala") << """class House(val owner: Person = new Person())"""
file("src/main/scala/Other.scala") << """class Other"""
Expand Down Expand Up @@ -144,11 +140,7 @@ class ZincScalaCompilerIntegrationTest extends BasicZincScalaCompilerIntegration
other.lastModified() == old(other.lastModified())
}

@Issue("https://github.com/gradle/gradle/issues/22964")
def "compiles Scala incrementally across project boundaries"() {
// TODO: remove the assumption when the linked issue fixed for Scala 3
Assume.assumeTrue(versionNumber.major == 2)

file("settings.gradle") << """include 'a', 'b'"""
// overwrite the build file from setup
file("build.gradle").text = """
Expand Down
88 changes: 0 additions & 88 deletions subprojects/scala/src/main/java/org/gradle/api/internal/tasks/scala/ZincScalaCompiler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,25 +30,16 @@
import org.gradle.language.base.internal.compile.Compiler;
import org.gradle.util.internal.GFileUtils;
import sbt.internal.inc.Analysis;
import sbt.internal.inc.ExternalLookup;
import sbt.internal.inc.IncrementalCompilerImpl;
import sbt.internal.inc.Locate;
import sbt.internal.inc.LoggedReporter;
import sbt.internal.inc.PlainVirtualFileConverter;
import sbt.internal.inc.ScalaInstance;
import sbt.internal.inc.Stamper;
import scala.Option;
import scala.Some;
import scala.collection.immutable.Set;
import scala.jdk.javaapi.CollectionConverters;
import xsbti.T2;
import xsbti.VirtualFile;
import xsbti.VirtualFileRef;
import xsbti.api.AnalyzedClass;
import xsbti.compile.AnalysisContents;
import xsbti.compile.AnalysisStore;
import xsbti.compile.Changes;
import xsbti.compile.ClassFileManager;
import xsbti.compile.ClassFileManagerType;
import xsbti.compile.ClasspathOptionsUtil;
import xsbti.compile.CompileAnalysis;
Expand All @@ -57,16 +48,13 @@
import xsbti.compile.CompilerCache;
import xsbti.compile.Compilers;
import xsbti.compile.DefinesClass;
import xsbti.compile.ExternalHooks;
import xsbti.compile.FileHash;
import xsbti.compile.IncOptions;
import xsbti.compile.Inputs;
import xsbti.compile.PerClasspathEntryLookup;
import xsbti.compile.PreviousResult;
import xsbti.compile.ScalaCompiler;
import xsbti.compile.Setup;
import xsbti.compile.TransactionalManagerType;
import xsbti.compile.analysis.Stamp;

import javax.inject.Inject;
import java.io.File;
Expand Down Expand Up @@ -138,7 +126,6 @@ public WorkResult execute(final ScalaJavaJointCompileSpec spec) {
.orElse(PreviousResult.of(Optional.empty(), Optional.empty()));

IncOptions incOptions = IncOptions.of()
.withExternalHooks(new LookupOnlyExternalHooks(new ExternalBinariesLookup()))
.withRecompileOnMacroDef(Optional.of(false))
.withClassfileManagerType(classFileManagerType)
.withTransitiveStep(5);
Expand Down Expand Up @@ -211,81 +198,6 @@ public DefinesClass definesClass(VirtualFile classpathEntry) {
}
}

private static class ExternalBinariesLookup implements ExternalLookup {

@Override
public Option<AnalyzedClass> lookupAnalyzedClass(String binaryClassName, Option<VirtualFileRef> file) {
return Option.empty();
}

@Override
public Option<Changes<VirtualFileRef>> changedSources(CompileAnalysis previousAnalysis) {
return Option.empty();
}

@Override
public Option<Set<VirtualFileRef>> changedBinaries(CompileAnalysis previousAnalysis) {
List<VirtualFileRef> result = new java.util.ArrayList<>();

for (Map.Entry<VirtualFileRef, Stamp> e : previousAnalysis.readStamps().getAllLibraryStamps().entrySet()) {
File path = CONVERTER.toPath(e.getKey()).toFile();
if (!path.exists() || !e.getValue().equals(Stamper.forLastModifiedInRootPaths(CONVERTER).apply(e.getKey()))) {
result.add(e.getKey());
}
}
if (result.isEmpty()) {
return Option.empty();
} else {
return new Some<>(CollectionConverters.asScala(result).toSet());
}
}


@Override
public Option<Set<VirtualFileRef>> removedProducts(CompileAnalysis previousAnalysis) {
return Option.empty();
}

@Override
public boolean shouldDoIncrementalCompilation(Set<String> changedClasses, CompileAnalysis analysis) {
return true;
}


@Override
public Optional<FileHash[]> hashClasspath(VirtualFile[] classpath) {
return Optional.empty();
}
}

private static class LookupOnlyExternalHooks implements ExternalHooks {
private final Optional<Lookup> lookup;

public LookupOnlyExternalHooks(Lookup lookup) {
this.lookup = Optional.of(lookup);
}

@Override
public Optional<Lookup> getExternalLookup() {
return lookup;
}

@Override
public Optional<ClassFileManager> getExternalClassFileManager() {
return Optional.empty();
}

@Override
public ExternalHooks withExternalClassFileManager(ClassFileManager externalClassFileManager) {
return this;
}

@Override
public ExternalHooks withExternalLookup(Lookup externalLookup) {
return this;
}
}

private static class AnalysisBakedDefineClass implements DefinesClass {
private final Analysis analysis;

Expand Down

0 comments on commit bf44f79

Please sign in to comment.