Update Zinc to 1.6.0 and drop the fork #1662

tgodzik · 2022-01-21T18:24:52Z

No description provided.

tgodzik · 2022-01-25T14:07:59Z

backend/src/main/scala/bloop/BloopClassFileManager.scala

@@ -142,12 +149,26 @@ final class BloopClassFileManager(
      }
    }

+    // Idea taken from the default TransactionalClassFileManager in zinc
+    // https://github.com/sbt/zinc/blob/c18637c1b30f8ab7d1f702bb98301689ec75854b/internal/zinc-core/src/main/scala/sbt/internal/inc/ClassFileManager.scala#L183
+    val toBeBackedUp = (classes ++ invalidatedExtraCompileProducts).filter(c =>


Previously, Bloop was using forked Zinc, that allowed to invalidate all symbols from invalidated files from the compiler. That API is not available for the mainline Zinc though, so we need to move them explicitely.

bloopoid · 2022-01-26T15:14:06Z

Performance test finished successfully.

Benchmarks is based on merging with master

bloopoid · 2022-01-26T15:14:59Z

Performance test finished successfully.

Benchmarks is based on merging with master

bloopoid · 2022-01-26T15:19:17Z

Performance test finished successfully.

Benchmarks is based on merging with master

bloopoid · 2022-01-26T17:51:03Z

Performance test finished successfully.

Benchmarks is based on merging with master

tgodzik · 2022-01-31T16:52:52Z

backend/src/main/scala/bloop/ScalaInstance.scala

@@ -20,17 +20,25 @@ final class ScalaInstance private (
    override val version: String,
    override val allJars: Array[File]
 ) extends xsbti.compile.ScalaInstance {
-  override val compilerJar: File = {
+
+  override lazy val loaderCompilerOnly: ClassLoader =


This is based on sbt itself.

tgodzik · 2022-01-31T17:48:45Z

backend/src/main/scala/sbt/internal/inc/bloop/internal/BloopIncremental.scala

+      val modifiedLibraries = initialChanges.libraryDeps.toArray
+
+      val modifiedBinaries: Array[File] = modifiedLibraries.map(converter.toPath(_)).collect {
+        case path if !isJrt(path) =>


Not sure about this one. Class files from the JDK are represented as jrt: (so I think it corresponds) and can't be cast to java.io.File, but on the other hand modifiedBinaries is deprecated so this should not be an issue.

tgodzik · 2022-01-31T17:56:25Z

backend/src/main/scala/sbt/internal/inc/bloop/internal/BloopNameHashing.scala

      previousAnalysis: Analysis,
      stamps: ReadStamps,
      lookup: Lookup,
+      converter: FileConverter,
      output: Output
  )(implicit equivS: Equiv[XStamp]): InitialChanges = {
    tracer.traceVerbose("detecting initial changes") { tracer =>
      // Copy pasting from IncrementalCommon to optimize/remove IO work


I checked the recent changes and updated.

tgodzik · 2022-01-31T17:59:34Z

backend/src/main/scala/sbt/internal/inc/bloop/internal/BloopStamps.scala

      // The hash is for the sources
      BloopStamps.forHash,
-      Stamper.forLastModified
+      Stamper.forHashInRootPaths(converter)


Hash seems enought for libraries, it doesn't look like we need last modified. This also started failing with jrt path issues.

tgodzik · 2022-01-31T18:03:41Z

backend/src/main/scala/bloop/CompilerCache.scala

+          case _ =>
+        }
+
+        val outputOption = CompilerArguments.outputOption(output)


It's needed to specify the output directory now, otherwise it will get compiled next to the source file. The change is based on changes to zinc/sbt

romanowski

I am a bit worried about tons of allocations we introduce with all this Path / FIle >=> VirtualFile conversions. I guess we would need to profile the bloop to see how much of the effect it will have.

backend/src/main/scala/bloop/BloopClassFileManager.scala

backend/src/main/scala/bloop/CompilerCache.scala

romanowski · 2022-01-31T19:32:12Z

backend/src/main/scala/bloop/ScalaInstance.scala

@@ -60,7 +68,25 @@ final class ScalaInstance private (
    filename.startsWith(ScalacCompilerName) ||
      (isDotty && (filename.startsWith("dotty-compiler") || filename.startsWith("scala3-compiler")))
  private def hasScalaLibraryName(filename: String): Boolean =
-    filename.startsWith("scala-library")
+    filename.startsWith("scala-library") || filename.startsWith("scala3-library")


I really do not like how it works... Maybe we can change bloop options at some point to pass compiler jars more directly?

It's not perfect, but we would need to split the Scala configuration to have two lists and we would most likely move this part to the integrations (which might also just have a single Scala classpath). Overall I feel it's better to keep it here and besides allJars are only Scala jars, so no user defined libraries will pop up here.

The only risk is if someone forks the compiler, but then a lot more things could break.

romanowski · 2022-01-31T19:34:48Z

backend/src/main/scala/sbt/internal/inc/BloopComponentCompiler.scala

@@ -386,8 +380,8 @@ private[inc] class BloopComponentCompiler(
          val allSourceContents =
            (hydraSourceContents ++ regularSourceContents).map(s => s -> relativize(tempDir, s).get)

-          zip(allSourceContents.toSeq, mergedJar)
-          Right(Vector(mergedJar))
+          zip(allSourceContents.toSeq, mergedJar, time = None)


👍 Do we have other palces in bloop where we zip some files?

Not from what I can see, this is for the compiler bridge compiled sources.

romanowski · 2022-01-31T19:36:46Z

backend/src/main/scala/sbt/internal/inc/bloop/BloopZincCompiler.scala

      val previous = previous0 match { case a: Analysis => a }
-      classfileManager.delete(invalidatedSrcs.flatMap(previous.relations.products).toArray)
+      val toDelete = invalidatedSrcs.flatMap(previous.relations.products).toArray.collect {
+        case vf: VirtualFile => vf


Can we get anything else then VirtualFile here (like a entry in the jar)? If so, shouldn't we somehow invalidate

We should not really have anything else, classFileManager only works with VirtualFile, so I don't imagine anything else is normally handled.

romanowski · 2022-01-31T19:42:54Z

backend/src/main/scala/sbt/internal/inc/bloop/internal/BloopIncremental.scala

+        .map(converter.toPath(_))
+        .collect {
+          // jrt path is neither a jar nor a normal file
+          case path if !isJrt(path) =>


What is jrt is modified? We are dropping that information here and this can lead to undercompilations

I asked that on discord and Eugene is not even sure if jrt is handled correctly by zinc. Anyways, this a deprecated API and I have no idea if we can get file from it (I tried)

backend/src/main/scala/sbt/internal/inc/bloop/internal/ConcurrentAnalysisCallback.scala

tgodzik · 2022-01-31T20:00:05Z

I am a bit worried about tons of allocations we introduce with all this Path / FIle >=> VirtualFile conversions. I guess we would need to profile the bloop to see how much of the effect it will have.

I run the benchmarks and I haven't seen any impact, but I can try and minimize the conversions even further ( tried to do it as much as possible)

romanowski · 2022-02-01T07:34:44Z

if there is no regression then it can stay at it is. Let's merge it and optimize it when needed.

In order to make upgrading versions easier we decided to switch back to sbt/zinc instead of a custom fork. The original reason for using the fork was to enable build pipelining, but that itself has been implemented in the orginal fork. Unfortunately, to make the migration easier we needed to remove build pipelining for now to later add it with the default mechanism. Related to scalacenter#1383 The benchmarks are being run to make sure we are not regressing in performance.

Biggest changes required here involved the new VirtualFile interface as we needed to convert them to java.io.File in a number of places. I tried to keep the conversions to a minimum, so that we don't bump into unexpected things such as issues with `jrt` filesystem and tried using Path wherever possible. I am not 100% if everything is correct, but I double checked and everything seems in place. We also needed to adjust ScalaInstance to make it work with the new interface and add an additional setting for output in the Java compilers, since otherwise everything gets produced to source directories (this mirrors the change in zinc itself.)

tgodzik · 2022-02-21T08:33:29Z

The failures are unrelated, seems after windows being bumped the tests started failing.

tgodzik mentioned this pull request Jan 21, 2022

[WiP] Switch zinc from fork to original one. #1477

Closed

tgodzik force-pushed the update-newest-zinc branch 4 times, most recently from dfd8e35 to f29bf6d Compare January 24, 2022 18:47

tgodzik commented Jan 25, 2022

View reviewed changes

tgodzik force-pushed the update-newest-zinc branch from f29bf6d to e6decfe Compare January 25, 2022 14:19

tgodzik force-pushed the update-newest-zinc branch from e6decfe to 243306e Compare January 26, 2022 15:14

tgodzik force-pushed the update-newest-zinc branch from 243306e to fc2d635 Compare January 31, 2022 18:10

tgodzik commented Jan 31, 2022

View reviewed changes

romanowski approved these changes Jan 31, 2022

View reviewed changes

lukaszwawrzyk approved these changes Feb 1, 2022

View reviewed changes

tgodzik force-pushed the update-newest-zinc branch from f0a70df to 611e39f Compare February 2, 2022 19:56

This was referenced Feb 15, 2022

chore: bump sbt version #1679

Merged

[EPIC] Bloop Next Steps #1680

Closed

tgodzik added 3 commits February 18, 2022 17:03

Apply refactorings after review

f69f1a3

tgodzik force-pushed the update-newest-zinc branch from 611e39f to f69f1a3 Compare February 18, 2022 16:03

tgodzik merged commit 7f377e9 into scalacenter:master Feb 21, 2022

tgodzik deleted the update-newest-zinc branch February 21, 2022 08:33

TheElectronWill mentioned this pull request Feb 28, 2022

Follow-up on Zinc upgrade scala/scala3#10816

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Zinc to 1.6.0 and drop the fork #1662

Update Zinc to 1.6.0 and drop the fork #1662

tgodzik commented Jan 21, 2022

tgodzik Jan 25, 2022

bloopoid commented Jan 26, 2022

bloopoid commented Jan 26, 2022

bloopoid commented Jan 26, 2022

bloopoid commented Jan 26, 2022

tgodzik Jan 31, 2022

tgodzik Jan 31, 2022

tgodzik Jan 31, 2022

tgodzik Jan 31, 2022

tgodzik Jan 31, 2022

romanowski left a comment

romanowski Jan 31, 2022

tgodzik Feb 2, 2022

romanowski Jan 31, 2022

tgodzik Feb 2, 2022

romanowski Jan 31, 2022

tgodzik Feb 2, 2022

romanowski Jan 31, 2022

tgodzik Jan 31, 2022

tgodzik commented Jan 31, 2022

romanowski commented Feb 1, 2022

tgodzik commented Feb 21, 2022

Update Zinc to 1.6.0 and drop the fork #1662

Update Zinc to 1.6.0 and drop the fork #1662

Conversation

tgodzik commented Jan 21, 2022

Choose a reason for hiding this comment

bloopoid commented Jan 26, 2022

bloopoid commented Jan 26, 2022

bloopoid commented Jan 26, 2022

bloopoid commented Jan 26, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romanowski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgodzik commented Jan 31, 2022

romanowski commented Feb 1, 2022

tgodzik commented Feb 21, 2022