"sbt '++ 2.13.0-M2!' compile" does not work with sbt 1.0.0 by eed3si9n · Pull Request #453 · sbt/zinc

eed3si9n · 2017-11-16T04:46:34Z

This is a rework of #445
Fixes #395, sbt/sbt#3427

To verify that the new bridge compiles, it needs to compile using sbt 1.0.3, which is unable to compile 2.13 code until this fix goes in. To work around this chicken-egg, I've manually created a bridge and published it to Maven Central as "org.scala-sbt" % "compiler-bridge_2.13.0-M2" % "1.1.0-M1-bootstrap2".

I kept the original commit from #445, but it didn't compile when I hooked it up with my bridge.

This also splits up the tests into a separate subproject since the test requires more dependencies than the actual bridge, which only relies on compiler|util-interface.

/cc @cunei

* add new compiler bridge

dwijnand

Nice work.

dwijnand · 2017-11-16T10:29:14Z

build.sbt

-      case (2, y) if y >= 11 => new File(scalaSource.value.getPath + "_2.11+")
+      case (2, y) if y == 10            => new File(scalaSource.value.getPath + "_2.10")
+      case (2, y) if y == 11 || y == 12 => new File(scalaSource.value.getPath + "_2.11-12")
+      case (2, y) if y >= 13            => new File(scalaSource.value.getPath + "_2.13")


For consistency either the match should be y == 13 or the directory should end in "_2.13+".

dwijnand · 2017-11-16T10:45:58Z

build.sbt

    },
    publishLocal := publishLocal.dependsOn(cleanSbtBridge).value,
    altPublishSettings,
-    mimaSettings,


Please could you leave a comment in code explaining why this isn't MiMa-enforced?

dwijnand · 2017-11-16T10:50:43Z

internal/compiler-bridge/src/main/scala_2.11-12/xsbt/ConsoleInterface.scala

+          super.createInterpreter()
+
+        for ((id, value) <- bindNames zip bindValues)
+          intp.quietBind(NamedParam.clazz(id, value))


Due to the regressions we had I'd rather we use

NamedParamClass(id, value.asInstanceOf[AnyRef].getClass.getName, value)

rather than NamedParam.clazz.

Could you link to the regressions?
FWIW, this file was copy-pasted straight from src/main/xsbt/ConsoleInterface.scala because ConsoleInterface.scala is now split into three Scala specific pieces. For the sake of keeping this PR simple, any changes to 2.12 I think should go into another PR.

My mistake. What you have is the right implementation.

I was reading 1.x ConsoleInterface: https://github.com/sbt/zinc/blob/1.x/internal/compiler-bridge/src/main/scala/xsbt/ConsoleInterface.scala#L57
which doesn't include my fixes, that are in 1.0.x ConsoleInterface: https://github.com/sbt/zinc/blob/1.0.x/internal/compiler-bridge/src/main/scala/xsbt/ConsoleInterface.scala#L57

We should really merge 1.0.x into 1.x to avoid this confusion.

Anyways these are the relevant commits:

0e4bb2c

33d2e68

eed3si9n · 2017-11-16T16:38:59Z

build.sbt


-// Compiler-side interface to compiler that is compiled against the compiler being used either in advance or on the fly.
-//   Includes API and Analyzer phases that extract source API and relationships.
+/**


@dwijnand Added comments.

dwijnand

Excellent, thanks.

eed3si9n · 2017-11-20T16:08:29Z

@jvican or @Duhemm: I'd like to include this for 1.0.4. Could we get code review for this soon?

jvican

It generally looks good. I've added several comments and I've only looked through the build changes because the PR has to be rebased on the open PRs that change the build and have been ready for review in a while: #428, #429. I'll have a closer look when they are.

Also, can you give a high-level explanation of these changes and why you make them? I don't understand some of the design decisions taken here. The PR message doesn't contain a descriptive explanation.

Otherwise, good job!

jvican · 2017-11-20T20:44:11Z

build.sbt

+/**
+ * Tests for the compiler bridge.
+ * This is split into a separate subproject because testing introduces more dependencies,
+ * which is not always available.


jvican · 2017-11-20T20:46:00Z

internal/compiler-bridge/src/main/scala_2.10/xsbt/Compat.scala

 }

 object Compat {
+  // IR is renanmed to Results


jvican · 2017-11-20T20:47:22Z

internal/compiler-bridge/src/main/scala_2.11-12/xsbt/ConsoleInterface.scala

+  ): Array[String] =
+    MakeSettings.sync(args, bootClasspathString, classpathString, log).recreateArgs.toArray[String]
+
+  def run(


I trust you've made sure that both consoles work!

2.11-12 bridge is the same as what we had before. In any case, here's the log of console tasks on locally built sbt:

https://gist.github.com/eed3si9n/8b1b074e62093377ad4881154f29a20b

jvican · 2017-11-20T20:48:17Z

script/bridge213.sh

+
+mkdir -p target/compiler-bridge/
+
+/usr/local/Cellar/scala@2.13/2.13.0-M2/bin/scalac \


This path for sure doesn't work in my computer.

jvican · 2017-11-20T20:50:14Z

script/bridge213.sh

@@ -0,0 +1,31 @@
+#!/bin/bash


Why this script? It honestly looks a little bit ugly. Isn't is possible to do all this from within sbt?

My problems with this:

No documentation why this is required

Lots of assumptions about the underlying OS

Too fragile, lots of assumptions about the bridge (what happens if I add a new file to the bridge? I have to update this script for 2.13 to work?)

I think this is not mergeable as it is...

I added more context in the commit message 0fd57e5.

This shell script was only used as a way of confirming that the compiler bridge compiles without using sbt. This was needed because sbt 1.0.3 is unable to compile 2.13 code. To clarify, the script is not used as part of the build process to build Zinc. But in the future, if we need to bootstrap again, for 2.13.x changes etc the script might come in handy.

Why don't you add it to the bin folder and document it saying that it shouldn't be used by normal people, only by you? Also, I'd appreciate if the script is OS cross-compatible.

Duhemm

I would personally prefer if we had stable names for the source directories, and avoided using names such as scala_2.13+. When work on 2.14 starts, we'll likely need to rename that directory to scala_2.13, which will make browsing the history harder for no good reason.

Duhemm · 2017-11-21T10:55:31Z

build.sbt

 def internalPath = file("internal")

-lazy val compilerBridgeScalaVersions = List(scala212, scala211, scala210)
+lazy val compilerBridgeScalaVersions = List(scala212, scala213, scala211, scala210)


Nitpick: Could we keep that in order? 😄

Duhemm · 2017-11-21T10:55:33Z

build.sbt


-lazy val compilerBridgeScalaVersions = List(scala212, scala211, scala210)
+lazy val compilerBridgeScalaVersions = List(scala212, scala213, scala211, scala210)
+lazy val compilerBridgeTestScalaVersions = List(scala212, scala211, scala210)


Why's 2.13 not tested?

Some dependencies are not available for Scala 2.13.0-M2.

dwijnand · 2017-11-21T11:29:38Z

When work on 2.14 starts, we'll likely need to rename that directory to scala_2.13, which will make browsing the history harder for no good reason.

In retrospect, and given the recent tickets in https://github.com/scala/scala-dev/milestone/18 and the fact that 2.14 will be a compiler-focussed release, I agree with you - it's fairly likely we'd need to tweak our compiler bridge for 2.14. Good shout. 👍

Fixes sbt#395, sbt/sbt#3427 In scala/scala#5903 Scala compiler's REPL-related classes went through some changes, including move to a different package. This implements a new compiler bridge tracking the changes. To verify that the new bridge compiles under 2.13, we need to compile it using sbt 1.0.3, which in turn requires a bridge compatible with Scala 2.13.0-M2. To work around this chicken-egg, I've manually created a bridge and published it to Maven Central as "org.scala-sbt" % "compiler-bridge_2.13.0-M2" % "1.1.0-M1-bootstrap2".

Splitting compiler bridge tests to another subproject because while the bridge itself can be compiled with just compiler-interface, util-interface, and Scala Compiler as dependencies, the testing introduces more (such as IO). This creates problem for new Scala versions where IO or test libraries do not exist yet (e.g. Scala 2.13.0-M2). This also removes the Mima test due to the lack of 2.13 bridge for Zinc 1.0.0. Compiler bridge just needs to compile itself against the interface and Scala compiler, so there's no need to run Mima test.

eed3si9n · 2017-11-21T16:55:33Z

internal/compiler-bridge/src/main/scala_2.13/xsbt/Compat.scala

@@ -0,0 +1,33 @@
+/*


@Duhemm @dwijnand Back to scala_2.13 directory.

eed3si9n · 2017-11-21T16:57:08Z

bin/bridge213.sh

+
+# $ export SCALA_X_HOME=/usr/local/Cellar/scala@2.13/2.13.0-M2
+
+if [[ -z "$SCALA_X_HOME" ]]; then


@jvican Script is now moved to bin/. I added $SCALA_X_HOME so ppl can set it to whatever path they want for the development version of Scala.

jvican · 2017-11-21T17:18:24Z

bin/bridge213.sh

@@ -0,0 +1,43 @@
+#!/bin/bash
+
+# This is a hack to validate the compilation of 2.13 compiler bridge without using sbt,


Thanks for the comment too.

jvican · 2017-11-21T17:19:55Z

Let's merge this when the changes to the build I mentioned in my previous comments are, and this PR is then rebased.

eed3si9n · 2017-11-21T20:39:43Z

If a bug fix patch against 1.0.x is ready to go, I don't think pending PRs should block it. This particular bug fix has been going on for a really long time, and it's not a super high priority one, but as a general matter, we should be able to patch bugs at relatively short turnaround. That's the point of having 1.0.x branch.

As per the build improvement PRs, I haven't paid too much attention to them since there's been review activities already, but if there are unresolved issues, let's discuss in weekly meeting.

jvican · 2017-11-21T22:11:44Z

... Eugene, I disagree and I've commented on the PR why #428. There has been a lot of work into those PRs, and the fact that they haven't been reviewed in a long time and this PR now wants to override it is not nice.

As I say in the other thread, that policy has been created by you and at any point has been discussed with me. On top of it, it's not consistent with the past -- we've merged not only bug fixes into 1.0.x.

fommil · 2017-11-23T08:26:37Z

internal/compiler-bridge/src/main/scala_2.13/xsbt/Compat.scala

+
+abstract class Compat
+object Compat {
+  // IR is renanmed to Results


* 1.0.x: (28 commits) Split compiler bridge tests to another subproject Implement compiler bridge for 2.13.0-M2 Add yourkit acknoledgement in the README "sbt '++ 2.13.0-M2!' compile" does not work with sbt 1.0.0 Add header to cached hashing spec Add headers to missing files Fix sbt#332: Add sbt-header back to the build Update sbt-scalafmt to 1.12 Make classpath hashing more lightweight Fix sbt#442: Name hash of value class should include underlying type source-dependencies/value-class-underlying: fix test Ignore null in generic lambda tparams Improve and make scripted parallel Fix sbt#436: Remove annoying log4j scripted exception Fix sbt#127: Use `unexpanded` name instead of `name` Add pending test case for issue/127 source-dependencies / patMat-scope workaround Fixes undercompilation on inheritance on same source Add real reproduction case for sbt#417 Add trait-trait-212 for Scala 2.12.3 ... Conflicts: internal/zinc-apiinfo/src/main/scala/sbt/internal/inc/ClassToAPI.scala project/build.properties zinc/src/main/scala/sbt/internal/inc/MixedAnalyzingCompiler.scala The ClassToAPI conflict is due to: * sbt#393 (a 1.x PR), conflicting with * sbt#446 (a 1.0.x PR). The build.properties conflict is due to different PRs bumping sbt.version from 1.0.0 to 1.0.2 to 1.0.3. (sbt#413, sbt#418, sbt#453). The MixedAnalyzingCompiler conflict is due to: * sbt#427 (a 1.x PR), conflicting with * sbt#452 (a 1.0.x PR).

xuwei-k · 2017-11-26T10:59:55Z

internal/zinc-ivy-integration/src/main/scala/sbt/internal/inc/ZincComponentCompiler.scala

+        case sc if (sc startsWith "2.10.")     => "compiler-bridge_2.10"
+        case sc if (sc startsWith "2.11.")     => "compiler-bridge_2.11"
+        case sc if (sc startsWith "2.12.")     => "compiler-bridge_2.12"
+        case sc if (sc startsWith "2.13.0-M")  => "compiler-bridge_2.13.0-M2"


sbt/sbt#3771

Should we add follows before this line?

case sc if (sc startsWith "2.13.0-M1") => "compiler-bridge_2.12"

I'll accept a PR, but I am sort of ok if it didn't work with old M1.

"sbt '++ 2.13.0-M2!' compile" does not work with sbt 1.0.0

af6e535

* add new compiler bridge

eed3si9n force-pushed the wip/213-compiler-bridge branch from 7010eb7 to d2cdccb Compare November 16, 2017 05:31

dwijnand requested changes Nov 16, 2017

View reviewed changes

eed3si9n force-pushed the wip/213-compiler-bridge branch from d2cdccb to 0a281a3 Compare November 16, 2017 16:37

eed3si9n commented Nov 16, 2017

View reviewed changes

eed3si9n requested a review from jvican November 16, 2017 21:06

dwijnand previously approved these changes Nov 16, 2017

View reviewed changes

xuwei-k mentioned this pull request Nov 17, 2017

support Scala 2.13.0-M2 sbt/io#81

Closed

jvican requested changes Nov 20, 2017

View reviewed changes

eed3si9n dismissed dwijnand’s stale review via c9e2176 November 21, 2017 06:07

eed3si9n force-pushed the wip/213-compiler-bridge branch from 0a281a3 to c9e2176 Compare November 21, 2017 06:07

Duhemm mentioned this pull request Nov 21, 2017

Refactor the build #428

Closed

Duhemm reviewed Nov 21, 2017

View reviewed changes

eed3si9n added 2 commits November 21, 2017 11:51

eed3si9n force-pushed the wip/213-compiler-bridge branch from c9e2176 to 91cb532 Compare November 21, 2017 16:53

eed3si9n commented Nov 21, 2017

View reviewed changes

jvican reviewed Nov 21, 2017

View reviewed changes

dwijnand mentioned this pull request Nov 22, 2017

"sbt '++ 2.13.0-M2!' compile" does not work with sbt 1.0.0 #445

Closed

fommil reviewed Nov 23, 2017

View reviewed changes

internal/compiler-bridge/src/main/scala_2.13/xsbt/Compat.scala

abstract class Compat

object Compat {

// IR is renanmed to Results

Copy link

fommil Nov 23, 2017

Choose a reason for hiding this comment

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

typo

Merge branch '1.0.x' into wip/213-compiler-bridge

9d6dfd7

jvican approved these changes Nov 23, 2017

View reviewed changes

dwijnand approved these changes Nov 23, 2017

View reviewed changes

dwijnand merged commit d6d6988 into sbt:1.0.x Nov 23, 2017

dwijnand mentioned this pull request Nov 23, 2017

"sbt '++ 2.13.0-M2!' compile" does not work with sbt 1.0.0 #395

Closed

dwijnand mentioned this pull request Nov 23, 2017

Merge 1.0.x into 1.x #455

Merged

eed3si9n deleted the wip/213-compiler-bridge branch November 25, 2017 05:09

eed3si9n mentioned this pull request Nov 26, 2017

"sbt '++ 2.13.0-M2!' compile" does not work with sbt 1.0.0 sbt/sbt#3427

Closed

xuwei-k mentioned this pull request Nov 26, 2017

"sbt '++ 2.13.0-M1!' compile" does not work with sbt 1.0.4 sbt/sbt#3771

Closed

xuwei-k reviewed Nov 26, 2017

View reviewed changes

eed3si9n mentioned this pull request Oct 12, 2018

Upgrade to 2.13.0-M5 #592

Closed


		mkdir -p target/compiler-bridge/

		/usr/local/Cellar/scala@2.13/2.13.0-M2/bin/scalac \


		# $ export SCALA_X_HOME=/usr/local/Cellar/scala@2.13/2.13.0-M2

		if [[ -z "$SCALA_X_HOME" ]]; then

		@@ -0,0 +1,43 @@
		#!/bin/bash

		# This is a hack to validate the compilation of 2.13 compiler bridge without using sbt,

Conversation

eed3si9n commented Nov 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dwijnand left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dwijnand left a comment

Choose a reason for hiding this comment

Uh oh!

eed3si9n commented Nov 20, 2017

Uh oh!

jvican left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Duhemm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dwijnand commented Nov 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jvican commented Nov 21, 2017

Uh oh!

eed3si9n commented Nov 21, 2017

Uh oh!

jvican commented Nov 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

eed3si9n commented Nov 16, 2017 •

edited

Loading