'StartDebugAdapter' command now accepts user's parameters (launch.json) #1466

alekseiAlefirov · 2020-02-25T17:13:47Z

"configurations": [
        {
          "type": "scala",
          "request": "launch",
          "name": "Foo",

          "mainClass": "bar.Foo",
          "buildTarget": "foo",
          "args": ["42"],
          "jvmOptions": ["..."]
      }
    ]

In this PR debug-adapter-start command is modified to accept more type of parameters like configuration above.

Client side PR: scalameta/metals-vscode#223

tgodzik

Really cool, I think this will be super useful. Left some comments, but I will also check the other PR.

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

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

alekseiAlefirov · 2020-03-16T16:08:52Z

debug-resolve-params command removed, while debug-adapter-start modified to accept more type of parameters to support user-set debug configurations.
Still there are problems with newly created tests, presumably because TestingServer do not fully reflect real Metals setup.
Header description of this PR edited.

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

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

tests/unit/src/test/scala/tests/DebugProtocolSuite.scala

mzarnowski · 2020-03-18T10:35:52Z

Also, increasing the timeout to 10000 fixed the failing tests on my machine. To remove the flakiness, you could wait until the workspace indexing finishes before proceeding to start the debug session - this would probably require adding some code around indexing to know when it finishes. The other solution could be to reuse the TestingClient.refreshModelHandler in a similar fashion it is used for CodeLenses, where we facing the same issue.

The underlying cause is that the BuildTargetClasses are fetched initially as a part of the indexing and server.initialize does not wait for the indexing to finish as there is no need to in most of the cases.

alekseiAlefirov · 2020-03-18T11:14:06Z

Thank you for the review and the advice @marek1840 , will try that.
Yes, BuildTargetClasses don't get propagated with the actual data by the time, but I also have encountered following situations sometimes (it is really non-deterministic and hard to catch):

BuildTargets do not get updated as well
Seems like this indices get updated, but DebugServer.start just hangs - although it's really hard to reproduce, I think, it happens when I run metals-vscode debug (so, run debug inside debug) quite regularly. I am not sure, maybe I do something wrong, but just sharing this knowledge -- maybe you can also comment on this, as with BuildTargetClasses.

mzarnowski · 2020-03-18T12:21:55Z

BuildTargets do not get updated as well

BuildTargets also get filled during indexing, so the issue should disappear after waiting for indexing to finish (by any of the means described in the previous comment). If the issue still happens after that, we can focus on it then.

As for the server hanging - I am at a loss but can try to debug it if you share the exact sequence of steps (I can at least try, even if it would mean repeating them a few times to encounter this singularity ;) ). Also, a thread dump could help a great deal in finding the cause.

alekseiAlefirov · 2020-03-18T15:58:26Z

if you share the exact sequence of steps

basically, you open metals-vscode project in VS Code, run debug, and in newly opened code instance open a simple project with a main class with one-line main method. Put a breakpoint onto this line, and run debug via codelens. For me it stucks somewhere never reaching the breakpoint and the pause button does not do anything. When I abort it, there's an error message saying no debug adapter found.

alekseiAlefirov · 2020-03-20T17:02:14Z

@marek1840 I've made some reworks, and fixed flakiness of tests by including indexing build targets into MetalsLanguageServer#indexWorkspace and making use of indexingPromise in the tests.

alekseiAlefirov · 2020-03-23T16:04:20Z

Don't see though, why it still fails on CI, the tests are being passed now on my machine 🤔

mzarnowski

This branch was failing on my machine at first, but after second or third run it started passing and the error never appeared during 5-10 repetitions.

It is worth checking if the the query for build classes is issued before they are populated and whether they are populated at all.

tests/unit/src/test/scala/tests/DebugProtocolSuite.scala

for using with 'launch.json'

debug-resolve-class command removed, now debug-adapter-start uses additional parameters

@marek1840

+ fixed test output for 'DebugProtocolSuite', thanks to @marek1840

added rebuild recovery for resolving classes

tgodzik

Looks to be shaping up nicely! I still how some comments, but overall concept and design looks good!

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

tgodzik · 2020-03-30T15:24:01Z

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

+        } { target =>
+          buildTargetClasses
+            .classesOf(target.getId())
+            .testClasses


This looks like the only difference between the 2 functions is here and two lines below. Can w reuse the code and just add maybe a function Classes => Option[T]?

okay, I've added

private def findClassAndBuildTarget[A]( className: String, buildTarget: Option[String], findClassesByName: String => List[(A, b.BuildTargetIdentifier)], classesByBuildTarget: b.BuildTargetIdentifier => Iterable[A], getClassName: A => String ): Try[List[(A, b.BuildTarget)]]

to generalize them

tgodzik · 2020-03-30T15:30:57Z

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

+        |{
+        |   mainClass: "com.foo.App",
+        |   buildTarget: "foo",
+        |   args: ["bar"]


Are we able to also add environment variables? Also, let's add jvmOptions to the example.

the input for this Params class is based on ScalaMainClass params, and it does not take environment variables.
About jvmOpts -- can you propose one?

@marek1840 Would make sense to add them to BSP?
@alekseiAlefirov Maybe -Dfile.encoding=UTF-8?

tgodzik · 2020-03-30T15:31:24Z

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

@@ -152,7 +152,7 @@ private[debug] object DebugProxy {
      endpoint: RemoteEndpoint,
      name: String
  ): RemoteEndpoint = {
-    val trace = GlobalTrace.setup(name)
+    val trace = GlobalTrace.setupTracePrinter(name)


This should fixed the issues with missing logs.

tgodzik · 2020-03-30T15:36:24Z

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

+      buildTargetClasses: BuildTargetClasses
+  )(implicit ec: ExecutionContext): Future[A] = {
+    Future.fromTry(f()).recoverWith {
+      case _: ClassNotFoundException | _: BuildTargetNotFoundException =>


BuildTargetNotFoundException will not be needed here. User would need reimport the build to change them.

good one, fixed

tgodzik · 2020-03-30T15:39:51Z

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

@@ -105,6 +114,85 @@ object DebugServer {
    }
  }

+  def resolveMainClassParams(
+      params: DebugUnresolvedMainClassParams,
+      buildTargetClassesFinder: BuildTargetClassesFinder,


Let's maybe move these new methods to BuildTargetClassesFinder with other than params method parameters added to the constructor? Also, with client instead of showWarningMessage.

I am not sure. This method takes some debug parameters to resolve them to other parameters and reports about running debug on one class, if multiple found. So it's more about debug, then about finding, which is extracted to Finder

Then maybe this should not actually be an object but maybe DebugProvider class? Start also takes a lot of parameters, which are not specific to the actual function invocation.

DebugProvider class created, all code from DebugServer object moved to it.

tgodzik · 2020-03-30T15:49:09Z

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

+      case _: ClassNotFoundException | _: BuildTargetNotFoundException =>
+        val allTargets = buildTargets.all.toSeq.map(_.info.getId())
+        for {
+          _ <- compilations.compileTargets(allTargets)


I think even if we manage to find the build target we should still run compile on it.

So, do the same, but on single target first. If that fails, compile all targets.

sure, added narrowed recovery for ClassNotFoundInBuildTargetException

tgodzik

Just two more comments! Sorry for being so nitpicky! 😅

tgodzik · 2020-03-31T15:45:41Z

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

-            }
-
-            session.asJavaObject
+        val debugSessionParamsParser = new JsonParser.Of[b.DebugSessionParams]


Let's maybe move those parsers to an object, no need to recreate them each time.

created DebugParametersJsonParsers with these parsers

tgodzik · 2020-03-31T15:49:13Z

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

@@ -105,6 +114,85 @@ object DebugServer {
    }
  }

+  def resolveMainClassParams(
+      params: DebugUnresolvedMainClassParams,
+      buildTargetClassesFinder: BuildTargetClassesFinder,


Then maybe this should not actually be an object but maybe DebugProvider class? Start also takes a lot of parameters, which are not specific to the actual function invocation.

alekseiAlefirov · 2020-03-31T18:55:45Z

@tgodzik agreed on that DebugServer object needed to be a class. DebugProvider class created.

tgodzik · 2020-04-01T08:45:10Z

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

@@ -122,6 +123,8 @@ class MetalsLanguageServer(
  private val languageClient = new DelegatingLanguageClient(NoopLanguageClient)
  var userConfig: UserConfiguration = UserConfiguration()
  val buildTargets: BuildTargets = new BuildTargets()
+  private val buildTargetClassesFinder =


This is unused now.

thank you! fixed

tgodzik

One comment left, but otherwise I think this is ready.

tgodzik

Jus last comments, tested it all out and seems to work really good! Thanks for the awesome cotnribution!

tgodzik · 2020-04-03T08:35:28Z

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

+          params.testClass,
+          Option(params.buildTarget)
+        )
+    }).map {


This will fail on Nil, could you handle that case?

tgodzik · 2020-04-03T08:35:43Z

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

+          Option(params.buildTarget)
+        )
+    ).map {
+      case (clazz, target) :: others =>


This will also fail on Nil, could you handle that case?

as I commented on BuildTargetClassFinder#findMainClassAndItsBuildTarget (above method def), it either returns Success with non-empty list, or Failure. So it should not be the case. I wish we had a type-level solution for that.

Solution for NonEmptyList? You have scalaz.NonEmptyList, cats.data.NonEmptyList, everything is already included in dependencies

hmm, last time I checked (several months ago), cats just got eliminated from Metals, and it was said it's better to have less dependencies. Okay, thanks, @kpbochenek !

okay, seems like there's no cats used in the main code, and @tgodzik recommended not to start.
^ fixed with throw NoSuchElement(Exception) for the Nil case

tgodzik

LGTM!

tgodzik · 2020-04-03T15:00:13Z

@ckipp01 This might be helpful for coc-metals and vim-inspector -> you will able to run a simplified command without code lenses.

ckipp01 · 2020-04-03T15:03:44Z

Yes! I'll have to dive into this a bit more and then that'd be awesome. Because right now if it's reliant on code-lenses, then only nvim users will benefit.

tgodzik · 2020-04-03T15:42:06Z

Yes! I'll have to dive into this a bit more and then that'd be awesome. Because right now if it's reliant on code-lenses, then only nvim users will benefit.

Exactly, with this PR however we will no longer be reliant on code lenses.

alekseiAlefirov mentioned this pull request Feb 25, 2020

Support 'launch.json' scalameta/metals-vscode#223

Merged

tgodzik reviewed Feb 26, 2020

View reviewed changes

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

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

olafurpg changed the title ~~Added new command 'debug-resolve-class' for using with 'launch.json'~~ Add new command 'debug-resolve-class' to use with 'launch.json' Feb 26, 2020

olafurpg reviewed Feb 26, 2020

View reviewed changes

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

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

alekseiAlefirov force-pushed the resolve-debug-class branch from fb192c1 to fef5e50 Compare February 28, 2020 20:51

alekseiAlefirov changed the title ~~Add new command 'debug-resolve-class' to use with 'launch.json'~~ 'StartDebugAdapter' command now accepts user's parameters (launch.json) Feb 28, 2020

olafurpg mentioned this pull request Mar 13, 2020

Can't debug Scala code #1497

Closed

alekseiAlefirov force-pushed the resolve-debug-class branch 2 times, most recently from c751eb0 to f57dc5a Compare March 16, 2020 15:54

mzarnowski reviewed Mar 18, 2020

View reviewed changes

mzarnowski reviewed Mar 24, 2020

View reviewed changes

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

ckipp01 added the feature-request Used for feature requests label Mar 26, 2020

alekseiAlefirov force-pushed the resolve-debug-class branch from ef8c0c7 to 5772341 Compare March 27, 2020 18:10

Aleksei Alefirov added 9 commits March 29, 2020 11:27

Added new command 'debug-resolve-class'

8161946

for using with 'launch.json'

Added arguments and jvmOptions

6683590

Added support for test debug configs;

5039919

debug-resolve-class command removed, now debug-adapter-start uses additional parameters

Code polished (main functionality moved to DebugServer); tests added

e2b45ef

Test fixed

207acdc

Fixed awaiting for indexing in tests, refactor of resolve method

8a4d347

Fixed indexing done callback

3ef7882

Fixed testing awaiting for compilation and rebuildIndex cycle done

e280125

+ fixed test output for 'DebugProtocolSuite', thanks to @marek1840

didOpen added to tests

82361a8

alekseiAlefirov force-pushed the resolve-debug-class branch from 224c833 to 82361a8 Compare March 29, 2020 09:30

Reverted onLatestComplete,

da82a5e

added rebuild recovery for resolving classes

alekseiAlefirov marked this pull request as ready for review March 30, 2020 10:33

tgodzik mentioned this pull request Mar 30, 2020

Debug related suites are flaky #1569

Closed

tgodzik requested changes Mar 30, 2020

View reviewed changes

mzarnowski approved these changes Mar 30, 2020

View reviewed changes

PR reworks

857ef38

tgodzik requested changes Mar 31, 2020

View reviewed changes

DebugProvider class created to refactor DebugServer object

6088a3a

tgodzik reviewed Apr 1, 2020

View reviewed changes

tgodzik approved these changes Apr 1, 2020

View reviewed changes

fixed unused val left after refactoring

987795e

tgodzik requested changes Apr 3, 2020

View reviewed changes

Fixed not-exausting pattern match

d72fc18

tgodzik approved these changes Apr 3, 2020

View reviewed changes

tgodzik merged commit ca3def6 into scalameta:master Apr 3, 2020

SkyTrix mentioned this pull request Apr 6, 2020

Support launch.json configuration to run and debug Scala code scalameta/metals-vscode#217

Closed

'StartDebugAdapter' command now accepts user's parameters (launch.json) #1466

'StartDebugAdapter' command now accepts user's parameters (launch.json) #1466

Conversation

alekseiAlefirov commented Feb 25, 2020 • edited Loading

tgodzik left a comment

Choose a reason for hiding this comment

alekseiAlefirov commented Mar 16, 2020

mzarnowski commented Mar 18, 2020

alekseiAlefirov commented Mar 18, 2020

mzarnowski commented Mar 18, 2020

alekseiAlefirov commented Mar 18, 2020 • edited Loading

alekseiAlefirov commented Mar 20, 2020

alekseiAlefirov commented Mar 23, 2020

mzarnowski left a comment

Choose a reason for hiding this comment

tgodzik 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgodzik 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

alekseiAlefirov commented Mar 31, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgodzik left a comment

Choose a reason for hiding this comment

tgodzik 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

tgodzik left a comment

Choose a reason for hiding this comment

tgodzik commented Apr 3, 2020

ckipp01 commented Apr 3, 2020

tgodzik commented Apr 3, 2020

alekseiAlefirov commented Feb 25, 2020 •

edited

Loading

alekseiAlefirov commented Mar 18, 2020 •

edited

Loading