feature: Add support for running Native and JS #5197

tgodzik · 2023-05-02T17:08:52Z

This works by doing the bare minimum that DAP requires and forwarding the logs from the build server.

We can later use this minimal DAP server to also run quicker test instead of setting up the entire debugging session

Resolves scalameta/metals-feature-requests#325

metals/src/main/scala/scala/meta/internal/metals/BuildServerConnection.scala

kpodsiad · 2023-05-04T08:18:42Z

metals/src/main/scala/scala/meta/internal/metals/codelenses/RunTestCodeLens.scala

+        scalaCliCodeLenses(
+          textDocument,
+          buildTargetId,
+          classes,
+          distance,
+          isJVM,
+        )


so this only works for scala cli right now because it can run native and js, right?

It works also for Bloop, but for sbt BSP it seems to only detect the JVM classes.

Mill gets a bunch of exceptions, but I don't want to dig into that yet.

Newest Mill doesn't get the exceptions, though it hangs for JS, not sure why. But that's not really related to this PR. We can turn it off for mill, but then we might forget about it.

metals/src/main/scala/scala/meta/internal/metals/ForwardingMetalsBuildClient.scala

ckipp01

Very cool. Just tested it out with nvim-metals as well and it all worked 🥳

metals/src/main/scala/scala/meta/internal/metals/debug/DebugProtocol.scala

tgodzik · 2023-05-05T12:10:57Z

Important drawback from this approach is that some logs might get mixed between the output, we could filter known things like compiling etc. but I don't think this will be a large issue.

tgodzik · 2023-05-29T20:28:22Z

Anyone willing to take a look at it to review? 😓

filipwiech

I have looked at the changes, as I've been interested in this in the context of a future speedup to test execution (on the JVM platform). However, I don't know the project well, so my remarks might make no sense, in which case please just ignore them, sorry. 😅

tests/unit/src/test/scala/tests/ScalaCliSuite.scala

metals/src/main/scala/scala/meta/internal/metals/codelenses/RunTestCodeLens.scala

metals/src/main/scala/scala/meta/internal/metals/debug/DebugProvider.scala

metals/src/main/scala/scala/meta/internal/metals/ForwardingMetalsBuildClient.scala

metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala

metals/src/main/scala/scala/meta/internal/metals/debug/DebugProvider.scala

tgodzik · 2023-05-30T10:58:29Z

I have looked at the changes, as I've been interested in this in the context of a future speedup to test execution (on the JVM platform). However, I don't know the project well, so my remarks might make no sense, in which case please just ignore them, sorry. sweat_smile

Thanks for the review! I will take a look at the remarks as soon as I can!

metals/src/main/scala/scala/meta/internal/metals/StacktraceAnalyzer.scala

tests/unit/src/test/scala/tests/ScalaCliSuite.scala

Works by doing the bare minimum DAP requires and forwars the logs from the build server

tgodzik · 2023-12-28T08:36:15Z

This is finally ready to be reviewed if anyone has the time.

kpodsiad · 2024-01-02T07:15:24Z

@tgodzik do you have an idea what might be wrong here? Is shaded coursier sth from bloop? 🤔

unit/testOnly tests.CodeLensLspSuite
tests.CodeLensLspSuite:
==> X tests.CodeLensLspSuite.initializationError  0.001s coursierapi.shaded.coursier.jvm.JvmCache$JvmNotFoundInIndex: JVM 8 not found in index: No adoptium version matching '1.8+' found
[error] Failed: Total 1, Failed 1, Errors 0, Passed 0
[error] Failed tests:
[error]         tests.CodeLensLspSuite
[error] (unit / Test / testOnly) sbt.TestsFailedException: Tests unsuccessful
[error] Total time: 1 s, completed Jan 1, 2024, 9:18:43 PM

tgodzik · 2024-01-02T10:02:46Z

@tgodzik do you have an idea what might be wrong here? Is shaded coursier sth from bloop? 🤔

unit/testOnly tests.CodeLensLspSuite
tests.CodeLensLspSuite:
==> X tests.CodeLensLspSuite.initializationError  0.001s coursierapi.shaded.coursier.jvm.JvmCache$JvmNotFoundInIndex: JVM 8 not found in index: No adoptium version matching '1.8+' found
[error] Failed: Total 1, Failed 1, Errors 0, Passed 0
[error] Failed tests:
[error]         tests.CodeLensLspSuite
[error] (unit / Test / testOnly) sbt.TestsFailedException: Tests unsuccessful
[error] Total time: 1 s, completed Jan 1, 2024, 9:18:43 PM

It probably is from Bloop, where did you find that error?

kpodsiad · 2024-01-02T10:12:13Z

@tgodzik it's the result of running unit/testOnly tests.CodeLensLspSuite in sbt shell. However, when I run bloop test unit -o tests.CodeLensLspSuite I'm getting the same error. Bloop is running already (version 1.5.13). I wonder what is trying to get JVM8 and why. Also, other test suites seems to work, I can run RunProviderLensLspSuite for instance

tgodzik · 2024-01-02T10:50:08Z

I can reproduce, you could run: unit/testOnly tests.CodeLensLspSuite -- -F to see where exactly is the exception coming from 🤔

kpodsiad · 2024-01-02T11:33:29Z

Ok, here's the cause:

metals/tests/unit/src/test/scala/tests/CodeLensLspSuite.scala

Line 312 in 614f290

Some(JvmManager.create().get("8").toString()),

There is no temurin:8 for apple silicon. Replacing with zulu:8 fixes issue.
Thanks @tgodzik for your help!

kpodsiad · 2024-01-02T11:35:07Z

metals/src/main/scala/scala/meta/internal/metals/BuildServerConnection.scala

+    cancelPromise.future.foreach { _ =>
+      completableFuture.cancel(true)
+    }


this has no effect. This methods is called from DebugProvider, but promise is never completed so cancelation logic will never be called which results in dead code.

Actually, the promise was being cancelled in the DebugRunner, but it was under a wrong message. Should have been under DisconnectRequest, but was under TerminateRequest.

Should work correctly and I added a test for that.

I also realised that running a main method if multiple are present was not possible, that's fixed too.

- make sure cancelation works correctly on disconnect request - send main class info in case of multiple classes - return error status if run fails

kpodsiad

I left few nits and two questions. I might have some improvements in mind, but I still need to run this and see all moving pieces in action though.

kpodsiad · 2024-01-02T19:57:53Z

metals/src/main/scala/scala/meta/internal/metals/debug/DebugProvider.scala

+          if (
+            parameters
+              .getDataKind() == b.DebugSessionParamsDataKind.SCALA_MAIN_CLASS
+          ) {
+            runParams.setDataKind(parameters.getDataKind())
+            runParams.setData(parameters.getData())
+          }


this sets main class info in case of multiple classes, right? It's not obvious from the code though. It'll be great if you add some comment.

Added a comment about it.

kpodsiad · 2024-01-02T20:05:22Z

metals/src/main/scala/scala/meta/internal/metals/debug/DebugProtocol.scala

+  object ConfigurationDone {
+    def unapply(
+        request: DebugRequestMessage
+    ): Option[Option[dap.ConfigurationDoneArguments]] = {


Is nested option intentional? I see that you're discarding unnaply result anyway

Fixed, that was not intentional, probably a lot of things changed on the way.

kpodsiad · 2024-01-03T21:16:18Z

metals/src/main/scala/scala/meta/internal/metals/debug/DebugProvider.scala

        cancelPromise,
      )
-    } yield debugServer
+      .flatMap { runner =>
+        currentRunner.set(runner)


what will happen if I run 2 different programs? Let assume that first one is long running computation and in the meantime I'll run another one. Will it mess logging or is it negligible corner case? I checked vscode now and I can debug two different programs 🤔

negligible corner case

I guess number of people trying to run on js and native platforms is already negligible 😅

I don't have a way currently to differentiate between the two programs, so this will not be possible. We can make it explicit.

There was a discussion in BSP about it, but I can't find it currently. We could identify each log message by the origin id maybe, let me check it.

We could identify each log message by the origin id maybe, let me check it.

just have in mind that this is probably minor issue and might not be worth spending too much time on it ;)

I added an atomic boolean that should take care of it. It will be set to false only if run ends or the user disconnects, but that should work fine.

I also realized that we don't show errors proeprly if code lense failed to run, so I added that in the PR here: scalameta/metals-vscode#1449

atomic flag might not be the best looking solution, but it's a working one. Let's get this thing merged

metals/src/main/scala/scala/meta/internal/metals/debug/DebugProvider.scala

kpodsiad · 2024-01-03T21:27:55Z

metals/src/main/scala/scala/meta/internal/metals/ForwardingMetalsBuildClient.scala

+ * Used to forward messages from the build server. Messages might
+ * be mixed if the server is sending messages as well as output from
+ * running. This hasn't been a problem yet, not perfect solution,
+ * but seems to work ok.


It'll forward all messages from the build server, e.g. if during a running I'll trigger a compilation or some different action, those logs might end up in "program output", right?

yes, unfortunately. No way around it for now, we could do filtering I guess but some messages could end up anyway.

+ some small review fixes

kpodsiad

looks good enough to me ;)

tgodzik requested review from ckipp01, kpodsiad and jkciesluk May 2, 2023 17:08

tgodzik changed the title ~~feature: Add support for running Native~~ feature: Add support for running Native and JS May 2, 2023

kpodsiad reviewed May 4, 2023

View reviewed changes

ckipp01 reviewed May 4, 2023

View reviewed changes

metals/src/main/scala/scala/meta/internal/metals/debug/DebugProtocol.scala Outdated Show resolved Hide resolved

tgodzik requested review from ckipp01 and kpodsiad May 5, 2023 12:09

tgodzik requested a review from kasiaMarek May 29, 2023 20:30

filipwiech reviewed May 30, 2023

View reviewed changes

kpodsiad force-pushed the run-native branch from ea0ae4d to 10034c7 Compare June 14, 2023 18:15

kpodsiad reviewed Jun 15, 2023

View reviewed changes

metals/src/main/scala/scala/meta/internal/metals/StacktraceAnalyzer.scala Outdated Show resolved Hide resolved

tests/unit/src/test/scala/tests/ScalaCliSuite.scala Outdated Show resolved Hide resolved

tests/unit/src/test/scala/tests/ScalaCliSuite.scala Outdated Show resolved Hide resolved

tgodzik force-pushed the run-native branch from 10034c7 to 9f82072 Compare December 22, 2023 14:35

tgodzik mentioned this pull request Dec 27, 2023

bugfix: Fix StacktraceAnalyser in case of empty package #5981

Merged

tgodzik force-pushed the run-native branch 3 times, most recently from 85cd09c to 7bc75c5 Compare December 27, 2023 17:07

feature: Add support for running Native

8d5961f

Works by doing the bare minimum DAP requires and forwars the logs from the build server

tgodzik force-pushed the run-native branch from 7bc75c5 to 8d5961f Compare December 27, 2023 19:17

tgodzik requested review from kpodsiad and filipwiech December 28, 2023 08:32

kpodsiad reviewed Jan 2, 2024

View reviewed changes

tgodzik requested a review from kpodsiad January 2, 2024 18:47

tgodzik force-pushed the run-native branch from c1672b2 to af8a790 Compare January 3, 2024 10:39

Fix possible issues:

279e29e

- make sure cancelation works correctly on disconnect request - send main class info in case of multiple classes - return error status if run fails

tgodzik force-pushed the run-native branch from af8a790 to 279e29e Compare January 3, 2024 11:26

kpodsiad reviewed Jan 3, 2024

View reviewed changes

tgodzik requested a review from kpodsiad January 4, 2024 11:32

bugfix: Don't allow multiple run sessions since they would interleave

b912d01

+ some small review fixes

tgodzik force-pushed the run-native branch from 1070232 to b912d01 Compare January 4, 2024 13:23

kpodsiad approved these changes Jan 4, 2024

View reviewed changes

tgodzik merged commit d71009e into scalameta:main Jan 5, 2024
25 checks passed

tgodzik deleted the run-native branch January 5, 2024 12:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: Add support for running Native and JS #5197

feature: Add support for running Native and JS #5197

tgodzik commented May 2, 2023

kpodsiad May 4, 2023

tgodzik May 4, 2023

tgodzik May 4, 2023

tgodzik Dec 28, 2023

ckipp01 left a comment

tgodzik commented May 5, 2023

tgodzik commented May 29, 2023

filipwiech left a comment

tgodzik commented May 30, 2023

tgodzik commented Dec 28, 2023

kpodsiad commented Jan 2, 2024

tgodzik commented Jan 2, 2024

kpodsiad commented Jan 2, 2024

tgodzik commented Jan 2, 2024

kpodsiad commented Jan 2, 2024

kpodsiad Jan 2, 2024

tgodzik Jan 2, 2024

kpodsiad left a comment

kpodsiad Jan 2, 2024

tgodzik Jan 4, 2024

kpodsiad Jan 2, 2024

tgodzik Jan 4, 2024

kpodsiad Jan 3, 2024

kpodsiad Jan 3, 2024

tgodzik Jan 4, 2024

kpodsiad Jan 4, 2024

tgodzik Jan 4, 2024

kpodsiad Jan 4, 2024

kpodsiad Jan 3, 2024

tgodzik Jan 4, 2024

kpodsiad left a comment

feature: Add support for running Native and JS #5197

feature: Add support for running Native and JS #5197

Conversation

tgodzik commented May 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ckipp01 left a comment

Choose a reason for hiding this comment

tgodzik commented May 5, 2023

tgodzik commented May 29, 2023

filipwiech left a comment

Choose a reason for hiding this comment

tgodzik commented May 30, 2023

tgodzik commented Dec 28, 2023

kpodsiad commented Jan 2, 2024

tgodzik commented Jan 2, 2024

kpodsiad commented Jan 2, 2024

tgodzik commented Jan 2, 2024

kpodsiad commented Jan 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kpodsiad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kpodsiad left a comment

Choose a reason for hiding this comment