-
Notifications
You must be signed in to change notification settings - Fork 91
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
Cross-compile the plugin against 0.13.15 and 1.0.0-M6 #117
Conversation
Note that there are still one or two deprecation warnings that can be addressed in a subsequent PR. |
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.
Looks good, thanks a ton :)
compile in Jmh <<= (compile in Jmh) dependsOn (compile in Test) | ||
run in Jmh <<= (run in Jmh) dependsOn (Keys.compile in Jmh) | ||
compile in Jmh := (compile in Jmh).dependsOn(compile in Test).value | ||
run in Jmh := (run in Jmh).dependsOn(Keys.compile in Jmh).value |
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.
cool thanks
version.sbt
Outdated
@@ -1 +1 @@ | |||
version in ThisBuild := "0.2.25" | |||
version in ThisBuild := "0.3.0-SNAPSHOT" |
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.
yeah, I guess hm... timing should work out fine I hope
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 you prefer 0.2.26-SNAPSHOT
that's fine too. No real reason to bump to 0.3 tbh.
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.
Yeah, let's not bump the minor for now.
It likely deserves a 1.0 soon anyway.
build.sbt
Outdated
@@ -13,12 +13,12 @@ val jmhVersion = { | |||
val commonSettings = Seq( | |||
organization := "pl.project13.scala", | |||
|
|||
scalaVersion := "2.10.6", | |||
crossSbtVersions := Vector("0.13.15", "1.0.0-M5"), |
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.
👍
@@ -113,7 +108,7 @@ lazy val extras = project | |||
.settings(sonatypeSettings: _*) | |||
.settings( | |||
name := "sbt-jmh-extras", | |||
scalaVersion := "2.12.2", |
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.
That's the only thing I am not sure about, since it's pure Java we may want to force it to target 1.6 to be able to use the plugin with a 1.6 JVM (using sbt 0.13.15)?
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.
@retronym do you guys still care about benchmarking against JDK6 with JMH?
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're only benchmarking on JDK8+ now. But sbt-jmh-extras
doesn't actually depend on Scala (see setting below this line), so I'm not sure if this is needed at all.
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 do benchmark on Scala 2.13.x, which is a reason to avoid a Scala dependency for this module.
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.
Yeah, extras is java only so the question is more about what jdk target should be set here.
We're also fine in akka with 8+ nowadays I think
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 should still be possible to use a custom JDK for the forks with jmh:run -jvm /jdk/1.6
, right?
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.
Hm, should work but perhaps best to sanity check - I can look at it tomorrow :)
Bumped to M6 to see if it builds |
scipted breaks:
is this expected @eed3si9n plugin upgrade or sth else? |
That's sbt/sbt#3245. Can you update with the following snippet and try again? It should work: ScriptedPlugin.scripted := {
val args = ScriptedPlugin.asInstanceOf[{
def scriptedParser(f: File): complete.Parser[Seq[String]]
}].scriptedParser(sbtTestDirectory.value).parsed
val prereq: Unit = scriptedDependencies.value
try {
if((sbtVersion in pluginCrossBuild).value == "1.0.0-M6") {
ScriptedPlugin.scriptedTests.value.asInstanceOf[{
def run(
x1: File,
x2: Boolean,
x3: Array[String],
x4: File,
x5: Array[String],
x6: java.util.List[File]
): Unit
}].run(
sbtTestDirectory.value,
scriptedBufferLog.value,
args.toArray,
sbtLauncher.value,
scriptedLaunchOpts.value.toArray,
new java.util.ArrayList()
)
} else {
ScriptedPlugin.scriptedTests.value.asInstanceOf[{
def run(
x1: File,
x2: Boolean,
x3: Array[String],
x4: File,
x5: Array[String]
): Unit
}].run(
sbtTestDirectory.value,
scriptedBufferLog.value,
args.toArray,
sbtLauncher.value,
scriptedLaunchOpts.value.toArray
)
}
} catch { case e: java.lang.reflect.InvocationTargetException => throw e.getCause }
} I'll try to fix this at the core and you can ditch that code in the next RC. |
@ktoso I will try to add this fix tonight, or you prefer to wait for a proper fix in the upstream project? |
I would appreciate if you use that fix. We don't know if this is going to be released before the release candidate yet, and this is blocking my migration to 1.0 in Zinc: sbt/zinc#316. |
@lomigmegard Thanks. Tests are passing @ktoso. |
Thanks a ton @lomigmegard @jvican |
Yes please :) |
Ok, managed to get it working. ^ was failing before it got to the right version. |
Yay!! |
I'm still seeing the |
This is a Scala compiler bug. You need to abstain from using App in
projects that are classpath scanned by java annotation processors (like
JMH) as a workaround
scala/bug#10487
|
Thanks for pointing that out. |
This PR fixes #115.
I had to update the two
CustomRunnerApp
in the scripted tests two use themain()
method. I ran into theIncompatibleClassChangeError
issue, changes fix #66.Now you have to use
^ scripted
and^ publishSigned
to cross-compile with the correct sbt version (see the 0.13.16-M1 release notes).