CompletionException in BSP server for buildTarget/jvmRunEnvironment request
#2643
Comments
|
Reformated stack trace |
|
I'm debugging some stuck errors in Metals while using mill-bsp and added some logs and a capture to scalameta/metals#4652. I also see this CompletionException error in it. |
|
There is another stack trace reported by Damian, https://matrix.to/#/!zXvttAKLPnvdnmiYsY:matrix.am/$124RiHjSmCeBGxv7U4GCApafH7L5-Q9FGA91kV92ib0?via=matrix.am&via=gitter.im&via=matrix.org As I spotted this line: I think it might be related to this comment: #2629 (comment)
|
|
Has anyone else made any progress on this issue. This has become a huge problem for my team and I'm at the point of thinking of switching our rather large Mill build back to sbt so teammates can get working IDEs/ VSCode.. |
|
@lihaoyi Is there an easy API to find to which project (main or meta-build) a module belongs, when I have the module at hand? I think there is not, but I also think we need it to make better choices in the evaluator about the From looking at the stacktraces, it looks like we try to evaluate a task that belongs to the meta-build in the context of the main build, resulting in using a cache location that does not exists. This is the path I think it should use instead:
-.../out/generateScriptSources.super/mill/runner/MillBuildRootModule/generateScriptSources.dest
+.../out/mill-build/generateScriptSources.super/mill/runner/MillBuildRootModule/generateScriptSources.dest |
|
I don't think there's a way to check what project a module depends on. In theory, each set of modules should be in a totally different classloader and one depth in MillBuildBootstrap resulting in an evaluator with one root module folder Yeah it definitely sounds like some of the meta build modules are being evaluated using the wrong evaluator. And this seems to happen only when using mill-bsp. IIRC we cache the evaluator in some parts of the mill-bsp codebase; could it be that cache is being kept around too long when it should instead be invalidated? |
@DamianReeves I understand your frustration. I already posted my idea of the issue. The envisioned fix isn't trivial though. Of course, anybody is free to work on whatever they want and extra pressure isn't much rewarding. Before switching to a completely different build tool, which probably comes with it's own issues, you should consider just downgrading to Mill 0.10.12. BSP support might not be as good as it could be in Mill 0.11, but if the latter is broken for you, it might do. You could also give IntelliJ IDEA a try, which works very well for me and also others, also in the free Community Edition. Donating some developer time towards a fix might be another option. |
I think it's rather the fact that we collect all modules, including the ones of the meta-build, when building up the BSP state (we do this to represent all modules, also the meta-build in the IDE). While doing so, we drop the extra information of the project root folder, which would be needed to calculate the correct destination folder ( |
I think Intellij doesn't use some specific endpoints that we do, which is why it's working better in this particular case. |
Oh, I just realized that it was ambiguous, as you can use IDEA as BSP client, or by generating the config with |
|
@lihaoyi It would be nice if we could encode the meta-build-ness in something like the |
|
It's definitely possible. But if the problem is mill-bsp mixing the modules together and losing the distinction between stages, maybe we should just keep the information in mill-bsp instead in some dictionary or something? Since the vast majority of Mill code doesn't need to know about this as far as I can tell, putting it on the Module object itself may be exposing it unnecessarily broadly |
Now, we actually need those info to have a proper unique module name. We also need it in |
Sorry I wasn't meaning to add pressure to you, and more communicating the pressure I am experiencing. Sorry again. I was intending to raise urgency (i.e. this is important not a nice to have), not pressure. |
We've seen some issues here as well, or my teammate has. I am able to work, but others seem to have issues. |
|
Given this discussion, is a workaround for me to stop using meta-build? |
This is definitely worth a try. The situation with BSP is currently, that we more or less fly blindfolded, as we don't have proper integration testing for it in Mill. Other BSP servers have it, Metals also runs some, and there is also a bsp-testkit(2), but I still haven't found the time to adapt it to Mill. Once we have integration testing for BSP, we could make and keep it more robust, faster and easier. Currently, it's manual testing and luck to find issues, which is not very rewarding, quite the opposite. Adding some integration testing for our BSP server to assert expected BSP responses for given projects would be a great contribution. I think there is almost no knowledge about Mill internals needed, as it's mostly protocol testing. |
|
Where can I find this |
…in BSP to avoid using the wrong evaluator (#2692) Should fix #2643. The basic problem is previously BSP always used the `frame(0)` evaluator passed in to `updateEvaluator`, which is meant specifically for evaluating user tasks in the root `./build.sc`. However, BSP also querying of modules and tasks both in `build.sc` as well as meta-builds in `mill-build/build.sc` and nested meta-builds. This ends up evaluating the meta-builds in the same `out/` folder as the main build, rather than in the `out/mill-build/` folders they should be evaluated in. This PR avoids using the `Evaluator` given in `updateEvaluator`, and instead preserves the `Evaluator`s we already construct as part of `MillBuildBootstrap` in `mill.bsp.worker.State`, and passes them as a `Seq[Evaluator]` from the enclosing `MillBuildBootstrap` code all the way to the Mill-BSP code so we can associate each `Module` with its respective `Ealuator`. We then use those associated `Evaluator`s to perform the actual evaluation throughout the `Mill{Scala,Java,Jvm,}BuildServer` implementations. There's some additional re-shaping, so that in case a single BSP API call wants to query modules/tasks from multiple evaluators at once we properly group the tasks/modules together according to their respective evaluators for evaluation. By passing the `Seq[Evaluator]` from the enclosing `MillBuildBootstrap`, this also lets us remove the now-redundant call to `MillBuildBootstrap` buried within `State.scala`. We already have all the necessary data structures from the enclosing `MillBuildBootstrap` and its evaluators, so we don't need to re-run the bootstrap process and re-construct all of it from scratch just for BSP I haven't managed to reproduce the original error, so confirming the fix will have to wait until this lands in master and people can try it out. But I have manually tested using VSCode to load and jump around `example/basic/1-simple-scala/` and `example/scalabuilds/10-scala-realistic/`, and it seems all the references and modules work and are set up correctly. So at least we have some confidence that this doesn't cause any regressions. This should also supersede #2648, since we now no longer need to re-run the `MillBuildBootstrap`/`RootModuleFinder` logic at all since we just take the existing list of `Evaluator`s from the enclosing bootstrap process
|
We have merged #2692, which should fix this issue. It would be nice if you could confirm the fix. I started CI job https://github.com/com-lihaoyi/mill/actions/runs/5864992453 to publish a snapshot release. |
Looks to be working! I need to do some more work to make run work in metals, but that should be ready soon. The only issue is that it will still throw an exception when the code doesn't compile, which will show up in the logs and might be annoying for users. Ideally, it would just return last jvmEnvironment especially that this shouldn't be influenced by regular compilation. |
|
Ok, the PR is ready scalameta/metals#5566 though I need to refactor things at some point since we get a lot of different options when things can be run (via Dap or via build tool itself or via client) |
@tgodzik wrote:
Originally posted by @tgodzik in #2499 (comment)
The text was updated successfully, but these errors were encountered: