-
Notifications
You must be signed in to change notification settings - Fork 205
Cross support for Scala Native 0.3 and 0.4.0-M2 #1029
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
Conversation
Seems like I can't reproduce the errors locally. Do you have any ideas? |
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 for this contribution @lolgab Great work, left some comments about the CI error and deduplicating logic. Otherwise LGTM 👍
@@ -1,7 +1,6 @@ | |||
# We publishLocal the maven plugin instead of compile to check the maven plugin integration | |||
# Only test sbt-bloop 1.x because of bug in `^` | |||
|
|||
# TODO: re-add `nativeBridge/test` |
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 CI for JDK 11 is failing because we added back these tests, which is what we should do 😄 Would you mind trying to dig on the reasons why JDK 11 is doesn't work well with our Scala Native tests? Everything seems to work on JDK 8 (ignore Windows).
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 tried something (seems something somehow related to this) but I didn't succeed. At the end I changed the ci build to test native only on jdk8 and on linux.
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 are some issues around JDK 11, e.g. scala-native/scala-native#1681
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.
Since JDK 8 is the official supported JDK by Scala Native I'm skipping the tests for that target.
When the bug is solved I can re-enable that.
Thank you for pointing out @ekrich
build.sbt
Outdated
@@ -322,7 +322,7 @@ val docs = project | |||
name := "bloop-docs", | |||
moduleName := "bloop-docs", | |||
skip in publish := true, | |||
scalaVersion := "2.12.6", | |||
scalaVersion := "2.12.9", |
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.
Maybe replace by the almost always up-to-date Dependencies.Scala212Version
? We can update the codebase to 2.12.9 in another 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.
I changed the scalaVersion
to Dependencies.Scala212Version
. I added the variables for Sbt versions too (Sbt1Version
and Sbt013Version
) to avoid duplication.
import org.junit.experimental.categories.Category | ||
|
||
@Category(Array(classOf[bloop.FastTests])) | ||
class ScalaNativeToolchainSpec { |
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.
Would it be possible to change the project build definition so that we reuse the tests from the native bridge 0.3? Given that the tests are the same, we would avoid copying the toolchain spec.
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 wrote different tests that use different features of Scala Native 0.3 and 0.4 (different imports and vprintf instead of printf on 0.4) to check that the correct version is working.
If you want I can rewrite it in pure Scala and call the same code from the two Scala Native versions.
The interop is breaking from 0.3 to 0.4 (the package names changed).
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.
Oh, in that case we're good 👍
Thank you very much for this wonderful project and for the attention put on reviewing the pull request. |
The build_windows build seems to fail because runs a build here which points to an older version of the repository. It can't find the old directory folder
|
@lolgab Yes, let me look into the windows CI issue, it sometimes goes crazy and needs some caring. |
We’re good to go! |
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, thanks again 👌
This PR splits Scala Native bridge in two bridge, one for the 0.3.9 (stable) version and one for the 0.4.0-M2 (latest) version.