-
Notifications
You must be signed in to change notification settings - Fork 121
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
Bump to sbt 1.0.0-M6 #323
Bump to sbt 1.0.0-M6 #323
Conversation
|
85070d5
to
152208f
Compare
|
||
/** | ||
* Defines a util interface to get Scala compilers and the default implementation | ||
* of the Scala incremental compiler if only {@link IncrementalCompiler} is required. | ||
*/ | ||
public interface ZincCompilerUtil { | ||
public class ZincCompilerUtil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a comment to the Scaladoc that explains the reason why this is turned into a class instead of being an interface and linking to the issue?
@@ -126,4 +126,9 @@ object ZincUtil { | |||
def compilers(javaTools: XJavaTools, scalac: ScalaCompiler): Compilers = { | |||
new Compilers(scalac, javaTools) | |||
} | |||
} | |||
|
|||
def constantBridgeProvider( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is not passing formatting.
On both my local machine and on Drone box, `ZincComponentCompilerSpec` fails to compile 2.10 compiler bridge with the following error: ``` /T/sbt_1234/xsbti/compile/CompilerBridgeProvider.java:33: error: `;' expected but `{' found. static CompilerBridgeProvider constant(File file, ScalaInstance scalaInstance) { ^ ``` I suspected that this is due to Java `interface` defining a static method, a change introduced in only in Java 8. After making this change, the spec passed the test.
dbuild has checked the following projects against Scala 2.12:
✅ The result is: SUCCESS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Just some small comments to polish this, but looks great overall!
Lemme know when you've addressed feedback and I'll merge.
P.S. I wish we had a sbt release with the logs fixes. We'll be missing stack traces... Please, as soon as RC is out, upgrade the sbt version here. Future contributors in sprees will be affected by those old bugs.
build.sbt
Outdated
|
||
def baseVersion = "1.0.0-X16-SNAPSHOT" | ||
def internalPath = file("internal") | ||
|
||
lazy val compilerBridgeScalaVersions = Seq(scala212, scala211, scala210) | ||
lazy val compilerBridgeScalaVersions = List(scala212, scala211, scala210) | ||
|
||
val scalafmtCheck = Command.command("scalafmtCheck") { state => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be removed.
@@ -271,12 +260,13 @@ lazy val zincCompileCore = (project in internalPath / "zinc-compile-core") | |||
// format from which Java sources are generated by the sbt-contraband plugin. | |||
lazy val compilerInterface = (project in internalPath / "compiler-interface") | |||
.enablePlugins(ContrabandPlugin) | |||
.disablePlugins(com.typesafe.sbt.SbtScalariform) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Context: sbt house rules has removed support for Scalariform.
.settings( | ||
minimalSettings, | ||
// javaOnlySettings, | ||
name := "Compiler Interface", | ||
crossScalaVersions := Seq(scala212), | ||
// Use the smallest Scala version in the compilerBridgeScalaVersions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be good to make this comment more descriptive and explain why this is the case so that future readers understand that the bridge has to be as much compatible as possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tbh I am not sure how much this had effect, but I did expand the comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought you did this so that the regression doesn't happen again. Forcing the latest Java version will always guarantee that our bridge compiles for all the Java versions supported, I presumed that was what you had in mind.
publishLocal := (), | ||
publish := {}, | ||
publishLocal := {}, | ||
skip in publish := true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we already have skip in publish
, why are publish
and publishLocal
definitions still required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
skip in publish AFAIK is only respected by sbt-pgp.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I thought that you had already merged the skip in publish
functionality in sbt 1.0.x.
@@ -396,11 +387,11 @@ lazy val publishBridgesAndSet = { | |||
val userScalaVersion = args.mkString("") | |||
s"${compilerInterface.id}/publishLocal" :: | |||
compilerBridgeScalaVersions.flatMap { (bridgeVersion: String) => | |||
s"wow $bridgeVersion" :: | |||
s"++ $bridgeVersion!" :: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The good thing about this is that we won't have OOM errors with ++
anymore in sbt 1.0.
@@ -1,27 +1,27 @@ | |||
import sbt._ | |||
import Keys._ | |||
// import sbt._ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you open an issue to keep track of this? We will need to enable it after RC is out and sbt-header cross-compiles.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks.
@@ -1,6 +1,8 @@ | |||
import sbt._ | |||
import Keys._ | |||
import Def.Initialize | |||
import sbt.internal.inc.ScalaInstance |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are these required? Seem they are unnecessary. I wonder if this is a slip.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should consider moving that out of internal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But why are these necessary? Why are the imports the only changes in this file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because the name has moved, from the sbt package to sbt.internal.inc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. All comments addressed.
Add support for atLeastOne method on a generator
Ref #316, #327