New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 4 commits into from Nov 23, 2017

Conversation

Projects
None yet
7 participants
@eed3si9n
Member

eed3si9n commented Nov 16, 2017

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

@dwijnand

Nice work.

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")

This comment has been minimized.

@dwijnand

dwijnand Nov 16, 2017

Member

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

@dwijnand

dwijnand Nov 16, 2017

Member

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

This comment has been minimized.

@eed3si9n

eed3si9n Nov 16, 2017

Member

sure.

@eed3si9n

eed3si9n Nov 16, 2017

Member

sure.

@@ -394,7 +388,25 @@ lazy val compilerBridge: Project = (project in internalPath / "compiler-bridge")
},
publishLocal := publishLocal.dependsOn(cleanSbtBridge).value,
altPublishSettings,
mimaSettings,

This comment has been minimized.

@dwijnand

dwijnand Nov 16, 2017

Member

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

@dwijnand

dwijnand Nov 16, 2017

Member

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

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

This comment has been minimized.

@dwijnand

dwijnand Nov 16, 2017

Member

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

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

rather than NamedParam.clazz.

@dwijnand

dwijnand Nov 16, 2017

Member

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

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

rather than NamedParam.clazz.

This comment has been minimized.

@eed3si9n

eed3si9n Nov 16, 2017

Member

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.

@eed3si9n

eed3si9n Nov 16, 2017

Member

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.

This comment has been minimized.

@dwijnand

dwijnand Nov 16, 2017

Member

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:

@dwijnand

dwijnand Nov 16, 2017

Member

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:

@@ -330,10 +331,17 @@ def wrapIn(color: String, content: String): String = {
else color + content + scala.Console.RESET
}
// 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.
/**

This comment has been minimized.

@eed3si9n

eed3si9n Nov 16, 2017

Member

@dwijnand Added comments.

@eed3si9n

eed3si9n Nov 16, 2017

Member

@dwijnand Added comments.

@eed3si9n eed3si9n requested a review from jvican Nov 16, 2017

@dwijnand

Excellent, thanks.

@xuwei-k xuwei-k referenced this pull request Nov 17, 2017

Closed

support Scala 2.13.0-M2 #81

@eed3si9n

This comment has been minimized.

Show comment
Hide comment
@eed3si9n

eed3si9n Nov 20, 2017

Member

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

Member

eed3si9n commented Nov 20, 2017

@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!

Show outdated Hide outdated build.sbt Outdated
Show outdated Hide outdated internal/compiler-bridge/src/main/scala_2.10/xsbt/Compat.scala Outdated
): Array[String] =
MakeSettings.sync(args, bootClasspathString, classpathString, log).recreateArgs.toArray[String]
def run(

This comment has been minimized.

@jvican

jvican Nov 20, 2017

Member

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

@jvican

jvican Nov 20, 2017

Member

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

This comment has been minimized.

@eed3si9n

eed3si9n Nov 21, 2017

Member

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

@eed3si9n

eed3si9n Nov 21, 2017

Member

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

Show outdated Hide outdated script/bridge213.sh Outdated
Show outdated Hide outdated script/bridge213.sh Outdated
@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.

@@ -5,7 +5,8 @@ import Scripted._
def baseVersion = "1.1.0-SNAPSHOT"
def internalPath = file("internal")
lazy val compilerBridgeScalaVersions = List(scala212, scala211, scala210)
lazy val compilerBridgeScalaVersions = List(scala212, scala213, scala211, scala210)

This comment has been minimized.

@Duhemm

Duhemm Nov 21, 2017

Contributor

Nitpick: Could we keep that in order? 😄

@Duhemm

Duhemm Nov 21, 2017

Contributor

Nitpick: Could we keep that in order? 😄

@@ -5,7 +5,8 @@ import Scripted._
def baseVersion = "1.1.0-SNAPSHOT"
def internalPath = file("internal")
lazy val compilerBridgeScalaVersions = List(scala212, scala211, scala210)
lazy val compilerBridgeScalaVersions = List(scala212, scala213, scala211, scala210)
lazy val compilerBridgeTestScalaVersions = List(scala212, scala211, scala210)

This comment has been minimized.

@Duhemm

Duhemm Nov 21, 2017

Contributor

Why's 2.13 not tested?

@Duhemm

Duhemm Nov 21, 2017

Contributor

Why's 2.13 not tested?

This comment has been minimized.

@eed3si9n

eed3si9n Nov 21, 2017

Member

Some dependencies are not available for Scala 2.13.0-M2.

@eed3si9n

eed3si9n Nov 21, 2017

Member

Some dependencies are not available for Scala 2.13.0-M2.

@dwijnand

This comment has been minimized.

Show comment
Hide comment
@dwijnand

dwijnand Nov 21, 2017

Member

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. 👍

Member

dwijnand commented Nov 21, 2017

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. 👍

eed3si9n added some commits Nov 16, 2017

Implement compiler bridge for 2.13.0-M2
Fixes #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".
Split compiler bridge tests to another subproject
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.
@@ -0,0 +1,33 @@
/*

This comment has been minimized.

@eed3si9n

eed3si9n Nov 21, 2017

Member

@Duhemm @dwijnand Back to scala_2.13 directory.

@eed3si9n

eed3si9n Nov 21, 2017

Member

@Duhemm @dwijnand Back to scala_2.13 directory.

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

This comment has been minimized.

@eed3si9n

eed3si9n Nov 21, 2017

Member

@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.

@eed3si9n

eed3si9n Nov 21, 2017

Member

@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.

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

This comment has been minimized.

@jvican

jvican Nov 21, 2017

Member

Thanks for the comment too.

@jvican

jvican Nov 21, 2017

Member

Thanks for the comment too.

@jvican

This comment has been minimized.

Show comment
Hide comment
@jvican

jvican Nov 21, 2017

Member

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

Member

jvican commented Nov 21, 2017

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

@eed3si9n

This comment has been minimized.

Show comment
Hide comment
@eed3si9n

eed3si9n Nov 21, 2017

Member

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.

Member

eed3si9n commented Nov 21, 2017

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

This comment has been minimized.

Show comment
Hide comment
@jvican

jvican Nov 21, 2017

Member

... 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.

Member

jvican commented Nov 21, 2017

... 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.

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

This comment has been minimized.

@fommil
@fommil

fommil Nov 23, 2017

typo

@jvican

jvican approved these changes Nov 23, 2017

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

1 check passed

continuous-integration/drone/pr the build was successful
Details

dwijnand added a commit to dwijnand/sbt-zinc that referenced this pull request Nov 23, 2017

Merge branch '1.0.x' into merge-1.0.x-into-1.x
* 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).

@dwijnand dwijnand referenced this pull request Nov 23, 2017

Merged

Merge 1.0.x into 1.x #455

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"

This comment has been minimized.

@xuwei-k

xuwei-k Nov 26, 2017

Member

sbt/sbt#3771

Should we add follows before this line?

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

xuwei-k Nov 26, 2017

Member

sbt/sbt#3771

Should we add follows before this line?

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

This comment has been minimized.

@eed3si9n

eed3si9n Nov 26, 2017

Member

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

@eed3si9n

eed3si9n Nov 26, 2017

Member

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

@eed3si9n eed3si9n referenced this pull request Oct 12, 2018

Open

Upgrade to 2.13.0-M5 #592

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment