-
Notifications
You must be signed in to change notification settings - Fork 105
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
Add support for Scala-CLI #919
Conversation
Handling all the possible exceptions remains to be done
This also contains a beginning of a timeout implementation
* Run now directly by the runner, add cancelation * Refactor 🎉
* base of ui * quick import fix * remove println * Machin * Parsing works (please dont run this on your machine, it loops) * Commit from my laptop * Fix runner with Scala-CLI and beginning of metals support * Make MetalsDispatcher return the config * Client now understands Metals * METALS NOW WORKS!!! * Fix of convert to scala cli
* base of ui * quick import fix * remove println * Machin * Parsing works (please dont run this on your machine, it loops) * Commit from my laptop * Fix runner with Scala-CLI and beginning of metals support * Make MetalsDispatcher return the config * Client now understands Metals * METALS NOW WORKS!!! * Fix of convert to scala cli * Remove print statemtents
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 did a first part of code review. I'll do another one when you address current issues and CI will pass. There are few issues, that we must fix but other than that great job and thanks for your work !
def fromScalaVersion(version: String): Option[ScalaTarget] = { | ||
if (version.startsWith("3")) { | ||
if (version == "3") | ||
Some(ScalaTarget.Scala3(BuildInfo.latest3)) | ||
else | ||
Some(ScalaTarget.Scala3(version)) | ||
} else if (version.startsWith("2")) { | ||
if (version == "2") | ||
Some(ScalaTarget.Jvm(BuildInfo.latest213)) | ||
else if (version == "2.13") | ||
Some(ScalaTarget.Jvm(BuildInfo.latest213)) | ||
else if (version == "2.12") | ||
Some(ScalaTarget.Jvm(BuildInfo.latest212)) | ||
else if (version == "2.11") | ||
Some(ScalaTarget.Jvm(BuildInfo.latest211)) | ||
else if (version == "2.10") | ||
Some(ScalaTarget.Jvm(BuildInfo.latest210)) | ||
else | ||
Some(ScalaTarget.Jvm(version)) | ||
} else | ||
None | ||
} | ||
|
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.
From what I see this code is used to parse Scala version for metals. If that is the case, you can't fallback to latest 3 if Scala version starts with 3. Each version of Scala has a fully cross versioned dependency and has to be the proper version e.g. Let's imagine if there is some bugfix in latest3, but the real version is < latest3. The code won't compile resulting in no completions at all even tho it is valid code according to selected version. You should use the full version e.g 3.3.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 not actually how it handles the Scala version.
// case of > using scala 3…
if (version.startsWith("3")) {
if (version == "3") // if it is exactly > using scala 3
Some(ScalaTarget.Scala3(BuildInfo.latest3))
else // if it is > using scala 3.x.x
Some(ScalaTarget.Scala3(version))
But now that we actually talk about it, I thought that the format 3.x is illegal. Is it the case?
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 I didnt' notice. So is there any reason we are using the latest version for 2.13, 2.12 etc ? I think we shouldn't use latest version for example what will happen if user sets 2.13.10 and wants to show something specific to this version ?
override def sbtConfig: String = "// Non-applicable" | ||
|
||
override def sbtPluginsConfig: String = "// Non-applicable" | ||
|
||
override def sbtRunCommand(worksheetMode: Boolean): String = ??? |
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 it would be better to remove it from parent trait and make it only present in SBT compatible BuildTarget's
balancer/src/main/scala/com.olegych.scastie.balancer/DispatchActor.scala
Show resolved
Hide resolved
scli-runner/src/main/scala/com.olegych.scastie.sclirunner/BspClient.scala
Show resolved
Hide resolved
scli-runner/src/main/scala/com.olegych.scastie.sclirunner/BspClient.scala
Show resolved
Hide resolved
scli-runner/src/main/scala/com.olegych.scastie.sclirunner/BspClient.scala
Show resolved
Hide resolved
scli-runner/src/main/scala/com.olegych.scastie.sclirunner/ScliMain.scala
Show resolved
Hide resolved
scli-runner/src/main/scala/com.olegych.scastie.sclirunner/ScliRunner.scala
Show resolved
Hide resolved
I'll make all the changes and let you know when it is ready! |
Had to rebase. Because of that closing this in favor of #997 |
Description
This PR is about adding a support for Scala-CLI inside Scastie. This has been requested on issue #603.
Detailed changes…
… on the Scala-CLI runner
This PR introduces a new SBT subproject, namely
scliRunner
. It proceeds similarly to thesbtRunner
except that it communicates with Scala-CLI using the BSP protocol (with thech.epfl.bsp4j
library). Scala-CLI needs to be properly installed on the host machine for it to work.Note on abuses: to prevent any, a timeout had to be properly set-up for the compilation and running the snippets. In case of non-finishing compilations, the runner will simply
sys.exit(-1)
and Docker will restart the container (it is assumed that this will not happen oftenly). For non-finishing executions, Scala-CLI refused during testing to cancel run requests (i.e. cancel the program): to prevent this, the runner will execute Java on its own.Note on instrumentation: the code is instrumented without the directives ; directives are prepended at the top of the file with the instrumented code.
… on the cross-compiled API
Now, a new
ScalaTarget
is defined and is only for Scala-CLI. Also, ping/pongs messages between the dispatchers and the runners have been renamed to a more general name.… on the Metals runner
The Metals runner will parse the directives using the using_directives library. Client sends a
/isConfigurationSupported
with the configuration (with the code) and the Metals runner will send the configuration to use for subsequent requests. The runner will solve dependencies, find the Scala version and return the correct configuration. The client will simply pretend to be using SBT afterwards.… on the load balancer part
Now, the
DispatchActor
handles all the business stuff (saving snippets, etc.) : it will forward the SBT and Scala-CLI run requests to two distinct actors:ScliDispatcher
andSbtDispatcher
… on the client
The user can chose Scala-CLI as a target and also convert snippets from SBT to Scala-CLI!
Also, if the directives are modified, after ~5 seconds, it will show a "Reload metals" button to actually reload metals with all the directives.
… what's missing?
[ ] The deployment script actually is not made yet. But I prefer making a PR about the implementation to avoid a too huge PR.
[ ] Support for Scala.JS
[ ] Letting the user choose the JVM using the proper directives
Closes
This PR closes #603.
Note
This is my first PR to an OSS. Any suggestions about anything is kindly appreciated.