Replace sbt dependency with zinc #1117

kiritsuku · 2016-08-30T16:27:24Z

Scala IDE so far did depend directly on sbt to build Scala sources. Because zinc has been extracted out of sbt into its own repo, we finally got the chance to get rid of the sbt dependency by only relying on zinc.

This greatly simplifies uber-build (see the PR: scala-ide/uber-build#87) but not so much the scala-ide build itself because packaging zinc in a way that Eclipse likes it is still a major act.

In the following a short description of the changes:

org.scala-ide.sbt.compiler.interface has been renamed to org.scala-ide.zinc.compiler.bridge. This is because the sbt dependency compiler-interface has been renamed to compiler-bridge in zinc. However, there is a newly introduced dependency also called compiler-interface in zinc (but it has a different purpose than the one in sbt).
org.scala-ide.sbt.full.library has been renamed to org.scala-ide.zinc.library
org.scala-ide.sbt.build has been renamed to org.scala-ide.zinc
The reason why org.scala-ide.zinc is still needed as an Eclipse bundle is that I didn't find another way to bundle zinc together with org.scala-ide.sdt.core. The following things had to be achieved:
- compiler-bridge depends on scalac and is therefore binary incompatible between minor versions. We need to build it from source. This is done at runtime by the IDE in CompilerBridgeStore.
- In order to allow the runtime compilation in CompilerBridgeStore we need to ship the sources of compiler-bridge and all of its dependencies as binaries (the dependencies do not depend on scalac and therefore can be shipped as binaries).
- org.scala-ide.zinc.library contains the said binaries. org.scala-ide.zinc.compiler.bridge.source contains the said sources. org.scala-ide.zinc bundles both as an Eclipse bundle.
- The bundling of org.scala-ide.zinc.library and org.scala-ide.zinc.compiler.bridge.source can't be done in org.scala-ide.sdt.core because the maven build first checks if the bundle is available before the building happens. Only with org.scala-ide.zinc we could achieve that the bundle is available at the dependency checking phase of maven. This is also the reason why build-all.sh exists, whose purpose it is to make the zinc and Scala dependencies available to scala-ide. With a single maven build this can't be achieved, because the bundles are created in a maven phase that happens after the dependency-checking phase and therefore these bundles can't be made available to other bundles of scala-ide.
The manifests need to depend on all zinc artifacts because these artifacts are not available through p2 update sites.

TODO:

Bug in zinc needs to be fixed: NoSuchElementException in TextAnalysisFormat sbt/zinc#169
Find out what to do with ClassfileManager in SbtInputs
Fix test failures
Look through the code once again to do last cleanups.

Lots of changes in this commit. It is difficult to split it up in smaller more meaningful commits. Basically, there are only these two changes: - The build had to be updated. The sbt module is now disabled and can be removed once it is verified that everything works as it should. zinc is split up into multiple modules. Therefore there are a lot of new dependencies even though they all belong to zinc. - Because of source incompatibilities, our source code had to be updated. There are a few TODOs left in the code, we need to figure out later if the new code is ok or if it needs to be improved.

These dependencies were not necessary to compile the code. Instead the code failed at runtime.

With this change, the IDE no longer depends directly on sbt but only on zinc. The most important advantage is that we no longer need to build zinc by ourselves - instead we can rely on the prebuilt zinc artifacts. We still need to build compiler-bridge (former compiler-interface) because it depends on scalac and therefore is binary incompatible between minor versions but since the infrastructure to build it already exists the only change that was needed is to add different artifacts.

The zinc artifact has been renamed from compiler-interface to compiler-bridge.

This renames everything that has to do with compiler-interface to compiler-bridge. The artifact has been renamed in zinc therefore we want to do it as well.

I don't know why these entries were on the gitignore list.

They were missed in an earlier commit. sbt.ide.version is removed because it is unused.

sbt.version is no longer used.

diffutils is no longer used and therefore removed.

Class based dependency tracking in zinc doesn't allow to disable name hashing. Since this is the only dependency tracking algorithm available we no longer need this option.

They were accidentally removed earlier.

Another artifact that is needed on the classpath.

ghprb-bot · 2016-08-30T16:41:03Z

Test FAILed.

wpopielarski · 2016-08-31T08:15:07Z

...scala-ide.sdt.core.tests/src/org/scalaide/core/sbtbuilder/NameHashingVulnerabilityTest.scala

-    val nameHashingProperty = SettingConverterUtil.convertNameToProperty(ScalaPluginSettings.nameHashing.name)
-    project.storage.setValue(nameHashingProperty, isOn)
+  private def whenNameHashingIsOn(otherwise: => Unit): Unit = {
+    project.storage.setValue("nameHashing", true)


I understand that nameHashing has got one value true

wpopielarski · 2016-08-31T08:42:04Z

looks promising 👍

Conflicts: org.scala-ide.sdt.core.tests/src/org/scalaide/TestsSuite.scala org.scala-ide.sdt.core/src/org/scalaide/core/internal/builder/zinc/JavaEclipseCompiler.scala

ghprb-bot · 2016-09-19T14:16:06Z

Test FAILed.

kiritsuku · 2016-09-19T22:26:41Z

retest this please

ghprb-bot · 2016-09-19T22:39:28Z

Test FAILed.

wpopielarski · 2016-09-20T06:39:07Z

I looked at tests and seems like javac is not called by incremental compiler

wpopielarski · 2016-09-20T08:13:58Z

I try to build it but I have problem with following p2 repo: http://download.eclipse.org/releases/luna/ btw isn't be more like http://download.eclipse.org/releases/neon/ ?

kiritsuku · 2016-09-20T08:40:03Z

The test failures are weird. On my system all of them pass.

Where do you have any problems with the luna p2 update site? Everything should point to neon.

wpopielarski · 2016-09-20T09:09:11Z

I cannot build it. Ends up with:
[ERROR] Internal error: java.lang.RuntimeException: Failed to load p2 repository with ID 'eclipse.luna' from location http://download.eclipse.org/releases/luna: Unable to read repository at http://download.eclipse.org/releases/luna. Unable to read repository at http://download.eclipse.org/releases/luna/201502271000/content.jar. Read timed out

wpopielarski · 2016-09-20T09:10:41Z

I see my mistake, forgot to run git fetch origin

wpopielarski · 2016-09-20T09:59:56Z

I have 8 failed tests in core on Win. Looking into....

ghprb-bot · 2016-09-20T12:16:06Z

Test PASSed.

wpopielarski

I think the biggest point is to make a trial of conversion from Analysis to CompileAnalysis, other things are minor one

wpopielarski · 2016-09-20T14:03:30Z

org.scala-ide.sdt.core.tests/src/org/scalaide/core/sbtbuilder/CompilerBridgeStoreTest.scala

+    store.purgeCache()
+
+    Assert.assertTrue("successful compiler bridge compilation", store.compilerBridgeFor(platformInstallation)(null).isRight)
+    Assert.assertEquals("Zero hits and one miss", (0, 1), store.getStats)


what is hit and what is miss (I suspect the cache but don't impel me to look deeper).

The first number is hits, the second one is misses. It is explained in Scaladoc of getStats. Too lazy to convert that to convert that to a case class.

wpopielarski · 2016-09-20T14:05:43Z

...scala-ide.sdt.core.tests/src/org/scalaide/core/sbtbuilder/NameHashingVulnerabilityTest.scala

@@ -61,26 +59,21 @@ object NameHashingVulnerabilityTest extends IProjectOperations {

 class NameHashingVulnerabilityTest extends IProjectOperations with IProjectHelpers {
  import NameHashingVulnerabilityTest._
-  private val On = true
-  private val Off = false
  private val errorTypes = Array(SdtConstants.ProblemMarkerId, IJavaModelMarker.JAVA_MODEL_PROBLEM_MARKER)

  @Test def shouldCorrectlyBuildProjectForAllPossibleSettingsOfNameHashingFlag(): Unit = {


do we need this test now? cannot purge it?

you are right, I removed it

wpopielarski · 2016-09-20T14:07:51Z

org.scala-ide.sdt.core/resources/META-INF/MANIFEST-2_12.MF

@@ -42,8 +42,8 @@ Require-Bundle: org.eclipse.core.expressions,
 org.scala-lang.scala-compiler;bundle-version="[2.12,2.13)",
 org.scala-refactoring.library,
 org.scala-lang.modules.scala-xml;bundle-version="[1.0.2,1.0.10]",
- org.scala-ide.sbt.full.library;bundle-version="[0.13,0.14)",
- org.scala-ide.sbt.compiler.interface;bundle-version="[0.13,0.14)",
+ org.scala-ide.zinc.library;bundle-version="[0.13,0.14)",


I'm wonder if [0.13,0.14) is relevant to zinc version... 1.0.0-X2?

wpopielarski · 2016-09-20T14:12:46Z

org.scala-ide.sdt.core/src/org/scalaide/core/internal/builder/zinc/CachingCompiler.scala

+  def compile(in: SbtInputs, comps: Compilers): Analysis = {
+    val lookup = new PerClasspathEntryLookup {
+      override def analysis(classpathEntry: File) =
+        in.analysisMap(classpathEntry).asInstanceOf[xsbti.Maybe[CompileAnalysis]]


need to change/update SbtInputs analysis functions.

wpopielarski · 2016-09-21T06:57:54Z

org.scala-ide.sdt.core/src/org/scalaide/core/internal/builder/zinc/EclipseSbtBuildManager.scala

@@ -186,7 +185,7 @@ class EclipseSbtBuildManager(val project: IScalaProject, settings: Settings, ana

  // take by-name argument because we need incOptions only when we have a cache miss
  override def latestAnalysis(incOptions: => IncOptions): Analysis =


Analysis should be turned into CompileAnalysis?

Unfortunately CompileAnalysis is just a marker interface, we can't do anything with it.

wpopielarski · 2016-09-21T07:01:01Z

org.scala-ide.sdt.core/src/org/scalaide/core/internal/builder/zinc/SbtInputs.scala

+    override def outputGroups = sourceOutputFolders.map {
+      case (src, out) => new MultipleOutput.OutputGroup {
+        override def sourceDirectory = {
+          val loc = src.getLocation


Option with map? Just proposal

We can't return an Option here

wpopielarski · 2016-09-21T07:04:40Z

org.scala-ide.sdt.core/src/org/scalaide/util/internal/SbtUtils.scala

+    })
+
+  def readAnalysis(cacheFile: File, incOptions: => IncOptions): Analysis =
+    readCache(cacheFile).map(_._1).getOrElse(Analysis.empty(nameHashing = incOptions.nameHashing()))


nameHashing is true only?

The former is a marker interface and therefore preferred to use. However, it doesn't have any functionality therefore we can't use it very often.

Name hashing is always true and we don't need to access any other options.

zinc requires that name hashing is always enabled, therefore we no longer need this test.

Not entirely sure what this thing is doing or if it is needed but TransactionalManagerType seems to be the replacement for ClassfileManager and we did use the latter so far, therefore we continue to use it.

No behavior changes introduced.

kiritsuku · 2016-09-21T15:47:29Z

I fixed all remaining TODOs and applied all review comments. Looks ready to merge now.

ghprb-bot · 2016-09-21T16:20:23Z

Test PASSed.

wpopielarski · 2016-09-21T19:09:16Z

if you are sure about CompileAnalysis and Analysis then LGTM 👍

dwijnand · 2016-09-21T21:02:55Z

Please note that anything in sbt.internal can change in any way, so it's a risk to depend on them.

It's part of the effort of exposing a smaller api so that it's less painful and limiting to sbt or zinc to evolve.

/cc @eed3si9n

kiritsuku · 2016-09-21T23:37:45Z

We always dependent on zinc internals, from this point of view nothing changed. If there are larger changes planned, we would welcome if you could drop us a message (or drop it on gitter, I'm following sbt-dev anyway)

---- On Wed, 21 Sep 2016 23:02:56 +0200 notifications@github.com wrote ----

Please note that anything in sbt.internal can change in any way, so it's a risk to depend on them.

It's part of the effort of exposing a smaller api so that it's less painful and limiting to sbt or zinc to evolve.

/cc @eed3si9n

—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub, or mute the thread.

dwijnand · 2016-09-21T23:46:42Z

Nothing in zinc wasn't internal, ie. all of sbt was public API.

Now things that are in sbt.internal are expressly internal, so caveat emptor.

Things might continue to change in the future, I'll try to remember to ping you in sbt-dev.

Note that you're depending on a milestone pre-release of zinc...

kiritsuku added 24 commits July 10, 2016 00:46

Add missing dependencies to classpath

0c2e536

These dependencies were not necessary to compile the code. Instead the code failed at runtime.

Add another sbt dependency to classpath

99db025

Cleanup pom.xml

7ae73ec

Rename compiler.interface Eclipse bundle to compiler.bridge

a8e1a40

The zinc artifact has been renamed from compiler-interface to compiler-bridge.

Rename compiler-interface to compiler-bridge

252d3d2

This renames everything that has to do with compiler-interface to compiler-bridge. The artifact has been renamed in zinc therefore we want to do it as well.

Rename sbt.full.library to zinc.library

d74e0c5

Rename sbt.compiler.bridge to zinc.compiler.bridge

b16de8d

Rename org.scala-ide.sbt.build to org.scala-ide.zinc

62cf536

Rename CompilerInterfaceStoreTest to CompilerBridgeStoreTest

532119d

Remove entries from gitignore list

13d2c8c

I don't know why these entries were on the gitignore list.

Replace remaining occurrences of sbt with zinc

19329cd

They were missed in an earlier commit. sbt.ide.version is removed because it is unused.

Get rid of further sbt references

76e7dfa

sbt.version is no longer used.

Create variables for sbt-io and sbinary artifacts

e7be6a0

Cleanup library version information

8916a0c

diffutils is no longer used and therefore removed.

Update zinc version

98b1618

Remove option to disable nameHashing

bdc6dc5

Class based dependency tracking in zinc doesn't allow to disable name hashing. Since this is the only dependency tracking algorithm available we no longer need this option.

Rename CompilerInterfaceFailed to CompilerBridgeFailed

d7e7477

Add zinc.library and zinc.compiler.bridge as dependencies to MANIFEST

b483bfb

They were accidentally removed earlier.

Add CompilerBridgeStoreTest to TestsSuite

f665598

Fix formatting for pom.xml

5bbdced

Add zinc-classfile to dependency list

15e0c07

Another artifact that is needed on the classpath.

Add zinc library to manifest files of debug bundle

c1e9780

wpopielarski reviewed Aug 31, 2016
View reviewed changes

Merge branch 'master' into zinc

53aa3fc

Conflicts: org.scala-ide.sdt.core.tests/src/org/scalaide/TestsSuite.scala org.scala-ide.sdt.core/src/org/scalaide/core/internal/builder/zinc/JavaEclipseCompiler.scala

Update zinc to X3

57abb4e

wpopielarski reviewed Sep 21, 2016

View reviewed changes

kiritsuku added 6 commits September 21, 2016 16:57

Use CompileAnalysis instead of Analysis where applicable

6b909d4

The former is a marker interface and therefore preferred to use. However, it doesn't have any functionality therefore we can't use it very often.

Remove outdated version ranges from 2.12 config

fc1c293

Remove unused incOptions parameter from readAnalysis method

291703c

Name hashing is always true and we don't need to access any other options.

Remove NameHashingVulnerabilityTest

3d546f1

zinc requires that name hashing is always enabled, therefore we no longer need this test.

Replace ClassfileManager with TransactionalManagerType

ca2f457

Not entirely sure what this thing is doing or if it is needed but TransactionalManagerType seems to be the replacement for ClassfileManager and we did use the latter so far, therefore we continue to use it.

Cleanup of source code

855ba78

No behavior changes introduced.

kiritsuku merged commit 3adba0e into scala-ide:master Sep 21, 2016

kiritsuku deleted the zinc branch September 21, 2016 20:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace sbt dependency with zinc #1117

Replace sbt dependency with zinc #1117

kiritsuku commented Aug 30, 2016 •

edited

Loading

ghprb-bot commented Aug 30, 2016

wpopielarski Aug 31, 2016

wpopielarski commented Aug 31, 2016

ghprb-bot commented Sep 19, 2016

kiritsuku commented Sep 19, 2016

ghprb-bot commented Sep 19, 2016

wpopielarski commented Sep 20, 2016

wpopielarski commented Sep 20, 2016

kiritsuku commented Sep 20, 2016

wpopielarski commented Sep 20, 2016

wpopielarski commented Sep 20, 2016

wpopielarski commented Sep 20, 2016

ghprb-bot commented Sep 20, 2016

wpopielarski left a comment

wpopielarski Sep 20, 2016 •

edited

Loading

kiritsuku Sep 21, 2016

wpopielarski Sep 20, 2016

kiritsuku Sep 21, 2016

wpopielarski Sep 20, 2016

kiritsuku Sep 21, 2016

wpopielarski Sep 20, 2016

kiritsuku Sep 21, 2016

wpopielarski Sep 21, 2016

kiritsuku Sep 21, 2016

wpopielarski Sep 21, 2016

kiritsuku Sep 21, 2016

wpopielarski Sep 21, 2016

kiritsuku Sep 21, 2016

kiritsuku commented Sep 21, 2016

ghprb-bot commented Sep 21, 2016

wpopielarski commented Sep 21, 2016

dwijnand commented Sep 21, 2016

kiritsuku commented Sep 21, 2016

dwijnand commented Sep 21, 2016

		@@ -186,7 +185,7 @@ class EclipseSbtBuildManager(val project: IScalaProject, settings: Settings, ana

		// take by-name argument because we need incOptions only when we have a cache miss
		override def latestAnalysis(incOptions: => IncOptions): Analysis =

Replace sbt dependency with zinc #1117

Replace sbt dependency with zinc #1117

Conversation

kiritsuku commented Aug 30, 2016 • edited Loading

ghprb-bot commented Aug 30, 2016

Choose a reason for hiding this comment

wpopielarski commented Aug 31, 2016

ghprb-bot commented Sep 19, 2016

kiritsuku commented Sep 19, 2016

ghprb-bot commented Sep 19, 2016

wpopielarski commented Sep 20, 2016

wpopielarski commented Sep 20, 2016

kiritsuku commented Sep 20, 2016

wpopielarski commented Sep 20, 2016

wpopielarski commented Sep 20, 2016

wpopielarski commented Sep 20, 2016

ghprb-bot commented Sep 20, 2016

wpopielarski left a comment

Choose a reason for hiding this comment

wpopielarski Sep 20, 2016 • edited Loading

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

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

kiritsuku commented Sep 21, 2016

ghprb-bot commented Sep 21, 2016

wpopielarski commented Sep 21, 2016

dwijnand commented Sep 21, 2016

kiritsuku commented Sep 21, 2016

dwijnand commented Sep 21, 2016

kiritsuku commented Aug 30, 2016 •

edited

Loading

wpopielarski Sep 20, 2016 •

edited

Loading