From f032a9a5ff975492534e9a468f577f1626cdbf2d Mon Sep 17 00:00:00 2001 From: Andrew Brett Date: Wed, 4 Dec 2024 17:53:52 +0400 Subject: [PATCH] Ensure we register a cycle if we've got scala sources but pipelining is disabled --- .../scala/sbt/internal/inc/Incremental.scala | 22 ++++++++++++++----- .../sbt/internal/inc/IncrementalCommon.scala | 12 +++++----- .../scala/sbt/internal/inc/IncHandler.scala | 19 ++++++++++++++++ .../check-cycles-no-pipelining/A.scala | 13 +++++++++++ .../changes/A.scala | 16 ++++++++++++++ .../incOptions.properties | 1 + .../profiler/check-cycles-no-pipelining/test | 5 +++++ .../sbt-test/profiler/check-cycles/A.scala | 13 +++++++++++ .../profiler/check-cycles/changes/A.scala | 16 ++++++++++++++ zinc/src/sbt-test/profiler/check-cycles/test | 5 +++++ 10 files changed, 109 insertions(+), 13 deletions(-) create mode 100644 zinc/src/sbt-test/profiler/check-cycles-no-pipelining/A.scala create mode 100644 zinc/src/sbt-test/profiler/check-cycles-no-pipelining/changes/A.scala create mode 100644 zinc/src/sbt-test/profiler/check-cycles-no-pipelining/incOptions.properties create mode 100644 zinc/src/sbt-test/profiler/check-cycles-no-pipelining/test create mode 100644 zinc/src/sbt-test/profiler/check-cycles/A.scala create mode 100644 zinc/src/sbt-test/profiler/check-cycles/changes/A.scala create mode 100644 zinc/src/sbt-test/profiler/check-cycles/test diff --git a/internal/zinc-core/src/main/scala/sbt/internal/inc/Incremental.scala b/internal/zinc-core/src/main/scala/sbt/internal/inc/Incremental.scala index 7a3fa13c1..5d90c4df4 100644 --- a/internal/zinc-core/src/main/scala/sbt/internal/inc/Incremental.scala +++ b/internal/zinc-core/src/main/scala/sbt/internal/inc/Incremental.scala @@ -70,14 +70,18 @@ object Incremental { * Merge latest analysis as of pickling into pruned previous analysis, compute invalidations * and decide whether we need another cycle. */ - def mergeAndInvalidate(partialAnalysis: Analysis, completingCycle: Boolean): CompileCycleResult + def mergeAndInvalidate( + partialAnalysis: Analysis, + shouldRegisterCycle: Boolean + ): CompileCycleResult /** * Merge latest analysis as of analyzer into pruned previous analysis and inform file manager. */ def completeCycle( prev: Option[CompileCycleResult], - partialAnalysis: Analysis + partialAnalysis: Analysis, + shouldRegisterCycle: Boolean ): CompileCycleResult def previousAnalysisPruned: Analysis @@ -938,7 +942,13 @@ private final class AnalysisCallback( if (!writtenEarlyArtifacts) // writing implies the updates merge has happened mergeUpdates() // must merge updates each cycle or else scalac will clobber it } - incHandler.completeCycle(invalidationResults, getAnalysis) + + val partialAnalysis = getAnalysis + val hasScala = Analysis.sources(partialAnalysis).scala.nonEmpty + // If we had early output and scala sources, then the cycle has already been registered + val shouldRegisterCycle = earlyOutput.isEmpty || !hasScala + + incHandler.completeCycle(invalidationResults, partialAnalysis, shouldRegisterCycle) } else { throw new IllegalStateException( "can't call AnalysisCallback#getCycleResultOnce more than once" @@ -1090,12 +1100,12 @@ private final class AnalysisCallback( override def apiPhaseCompleted(): Unit = { // If we know we're done with cycles (presumably because all sources were invalidated) we can store early analysis - // and picke data now. Otherwise, we need to wait for dependency information to decide if there are more cycles. + // and pickle data now. Otherwise, we need to wait for dependency information to decide if there are more cycles. incHandlerOpt foreach { incHandler => if (earlyOutput.isDefined && incHandler.isFullCompilation) { val a = getAnalysis val CompileCycleResult(continue, invalidations, merged) = - incHandler.mergeAndInvalidate(a, false) + incHandler.mergeAndInvalidate(a, shouldRegisterCycle = true) if (lookup.shouldDoEarlyOutput(merged)) { assert( !continue && invalidations.isEmpty, @@ -1113,7 +1123,7 @@ private final class AnalysisCallback( if (earlyOutput.isDefined && invalidationResults.isEmpty) { val a = getAnalysis val CompileCycleResult(continue, invalidations, merged) = - incHandler.mergeAndInvalidate(a, false) + incHandler.mergeAndInvalidate(a, shouldRegisterCycle = true) // Store invalidations and continuation decision; the analysis will be computed again after Analyze phase. invalidationResults = Some(CompileCycleResult(continue, invalidations, Analysis.empty)) // If there will be no more compilation cycles, store the early analysis file and update the pickle jar diff --git a/internal/zinc-core/src/main/scala/sbt/internal/inc/IncrementalCommon.scala b/internal/zinc-core/src/main/scala/sbt/internal/inc/IncrementalCommon.scala index 19cfff70e..2326ae9dd 100644 --- a/internal/zinc-core/src/main/scala/sbt/internal/inc/IncrementalCommon.scala +++ b/internal/zinc-core/src/main/scala/sbt/internal/inc/IncrementalCommon.scala @@ -158,7 +158,7 @@ private[inc] abstract class IncrementalCommon( override def mergeAndInvalidate( partialAnalysis: Analysis, - completingCycle: Boolean + shouldRegisterCycle: Boolean, ): CompileCycleResult = { val analysis = if (isFullCompilation) @@ -190,10 +190,7 @@ private[inc] abstract class IncrementalCommon( val continue = nextInvalidations.nonEmpty && lookup.shouldDoIncrementalCompilation(nextInvalidations, analysis) - val hasScala = Analysis.sources(partialAnalysis).scala.nonEmpty - - // If we're completing the cycle and we had scala sources, then mergeAndInvalidate has already been called - if (!completingCycle || !hasScala) { + if (shouldRegisterCycle) { registerCycle(recompiledClasses, newApiChanges, nextInvalidations, continue) } CompileCycleResult(continue, nextInvalidations, analysis) @@ -201,12 +198,13 @@ private[inc] abstract class IncrementalCommon( override def completeCycle( prev: Option[CompileCycleResult], - partialAnalysis: Analysis + partialAnalysis: Analysis, + shouldRegisterCycle: Boolean ): CompileCycleResult = { classFileManager.generated(partialAnalysis.relations.allProducts.map(toVf).toArray) prev match { case Some(prev) => prev.copy(analysis = pruned ++ partialAnalysis) - case _ => mergeAndInvalidate(partialAnalysis, true) + case _ => mergeAndInvalidate(partialAnalysis, shouldRegisterCycle) } } } diff --git a/internal/zinc-scripted/src/test/scala/sbt/internal/inc/IncHandler.scala b/internal/zinc-scripted/src/test/scala/sbt/internal/inc/IncHandler.scala index d282f4338..8b8d19305 100644 --- a/internal/zinc-scripted/src/test/scala/sbt/internal/inc/IncHandler.scala +++ b/internal/zinc-scripted/src/test/scala/sbt/internal/inc/IncHandler.scala @@ -26,11 +26,14 @@ import xsbti.{ FileConverter, Problem, Severity, VirtualFileRef, VirtualFile } import xsbti.compile.{ AnalysisContents, AnalysisStore, + ClassFileManager => XClassFileManager, ClasspathOptionsUtil, CompileAnalysis, CompileOrder, CompileProgress, + DefaultExternalHooks, DefinesClass, + ExternalHooks, IncOptions, IncOptionsUtil, PerClasspathEntryLookup, @@ -222,6 +225,9 @@ class IncHandler(directory: Path, cacheDir: Path, scriptedLog: ManagedLogger, co onArgs("checkIterations") { case (p, x :: Nil, i) => p.checkNumberOfCompilerIterations(i, x.toInt) }, + onArgs("checkCycles") { + case (p, x :: Nil, i) => p.checkNumberOfCycles(i, x.toInt) + }, // note that this can only tell us the *last* round a class got compiled in. // it can't tell us *every* round something got compiled in, since only // still-extant classfiles are available for inspection @@ -328,6 +334,7 @@ case class ProjectStructure( val earlyCacheFile = baseDirectory / "target" / "early" / "inc_compile.zip" val earlyAnalysisStore = FileAnalysisStore.binary(earlyCacheFile.toFile) // val earlyCachedStore = AnalysisStore.cached(fileStore) + val profiler = new ZincInvalidationProfiler // We specify the class file manager explicitly even though it's noew possible // to specify it in the incremental option property file (this is the default for sbt) @@ -424,6 +431,15 @@ case class ProjectStructure( () } + def checkNumberOfCycles(i: IncState, expected: Int): Future[Unit] = + compile(i).map { _ => + import scala.collection.JavaConverters._ + val count = profiler.toProfile.getRunsList.asScala.map(_.getCyclesList.size).sum + val msg = s"Expected $expected cycles, got $count" + assert(count == expected, msg) + () + } + def checkClasses(i: IncState, src: String, expected: List[String]): Future[Unit] = compile(i).map { analysis => def classes(src: String): Set[String] = @@ -783,10 +799,13 @@ case class ProjectStructure( import scala.collection.JavaConverters._ val map = new java.util.HashMap[String, String] properties.asScala foreach { case (k: String, v: String) => map.put(k, v) } + val externalHooks = new DefaultExternalHooks(Optional.empty[ExternalHooks.Lookup], Optional.empty[XClassFileManager]) + .withInvalidationProfiler(profiler) val base = IncOptions .of() .withPipelining(defaultPipelining) .withApiDebug(true) + .withExternalHooks(externalHooks) // .withRelationsDebug(true) val incOptions = { val opts = IncOptionsUtil.fromStringMap(base, map, scriptedLog) diff --git a/zinc/src/sbt-test/profiler/check-cycles-no-pipelining/A.scala b/zinc/src/sbt-test/profiler/check-cycles-no-pipelining/A.scala new file mode 100644 index 000000000..fb24dcd51 --- /dev/null +++ b/zinc/src/sbt-test/profiler/check-cycles-no-pipelining/A.scala @@ -0,0 +1,13 @@ +/* + * Zinc - The incremental compiler for Scala. + * Copyright Scala Center, Lightbend, and Mark Harrah + * + * Licensed under Apache License 2.0 + * SPDX-License-Identifier: Apache-2.0 + * + * See the NOTICE file distributed with this work for + * additional information regarding copyright ownership. + */ + +class A() + diff --git a/zinc/src/sbt-test/profiler/check-cycles-no-pipelining/changes/A.scala b/zinc/src/sbt-test/profiler/check-cycles-no-pipelining/changes/A.scala new file mode 100644 index 000000000..017538128 --- /dev/null +++ b/zinc/src/sbt-test/profiler/check-cycles-no-pipelining/changes/A.scala @@ -0,0 +1,16 @@ +/* + * Zinc - The incremental compiler for Scala. + * Copyright Scala Center, Lightbend, and Mark Harrah + * + * Licensed under Apache License 2.0 + * SPDX-License-Identifier: Apache-2.0 + * + * See the NOTICE file distributed with this work for + * additional information regarding copyright ownership. + */ + +class A() + + + +// random placeholder change \ No newline at end of file diff --git a/zinc/src/sbt-test/profiler/check-cycles-no-pipelining/incOptions.properties b/zinc/src/sbt-test/profiler/check-cycles-no-pipelining/incOptions.properties new file mode 100644 index 000000000..e7b9a57ec --- /dev/null +++ b/zinc/src/sbt-test/profiler/check-cycles-no-pipelining/incOptions.properties @@ -0,0 +1 @@ +pipelining = false \ No newline at end of file diff --git a/zinc/src/sbt-test/profiler/check-cycles-no-pipelining/test b/zinc/src/sbt-test/profiler/check-cycles-no-pipelining/test new file mode 100644 index 000000000..3c91240b7 --- /dev/null +++ b/zinc/src/sbt-test/profiler/check-cycles-no-pipelining/test @@ -0,0 +1,5 @@ +> compile + +$ copy-file changes/A.scala A.scala + +> checkCycles 2 \ No newline at end of file diff --git a/zinc/src/sbt-test/profiler/check-cycles/A.scala b/zinc/src/sbt-test/profiler/check-cycles/A.scala new file mode 100644 index 000000000..fb24dcd51 --- /dev/null +++ b/zinc/src/sbt-test/profiler/check-cycles/A.scala @@ -0,0 +1,13 @@ +/* + * Zinc - The incremental compiler for Scala. + * Copyright Scala Center, Lightbend, and Mark Harrah + * + * Licensed under Apache License 2.0 + * SPDX-License-Identifier: Apache-2.0 + * + * See the NOTICE file distributed with this work for + * additional information regarding copyright ownership. + */ + +class A() + diff --git a/zinc/src/sbt-test/profiler/check-cycles/changes/A.scala b/zinc/src/sbt-test/profiler/check-cycles/changes/A.scala new file mode 100644 index 000000000..017538128 --- /dev/null +++ b/zinc/src/sbt-test/profiler/check-cycles/changes/A.scala @@ -0,0 +1,16 @@ +/* + * Zinc - The incremental compiler for Scala. + * Copyright Scala Center, Lightbend, and Mark Harrah + * + * Licensed under Apache License 2.0 + * SPDX-License-Identifier: Apache-2.0 + * + * See the NOTICE file distributed with this work for + * additional information regarding copyright ownership. + */ + +class A() + + + +// random placeholder change \ No newline at end of file diff --git a/zinc/src/sbt-test/profiler/check-cycles/test b/zinc/src/sbt-test/profiler/check-cycles/test new file mode 100644 index 000000000..3c91240b7 --- /dev/null +++ b/zinc/src/sbt-test/profiler/check-cycles/test @@ -0,0 +1,5 @@ +> compile + +$ copy-file changes/A.scala A.scala + +> checkCycles 2 \ No newline at end of file