Re-use MillBuildRootModule for BSP #2415

lihaoyi · 2023-04-04T10:50:02Z

Tested it on example/basic/1-simple-scala and example/scala-builds/10-scala-realistic by running installLocalCache and setting a .mill-version to point at it. Seems to work about as well as 0.11.0-M7 before all the ammonite removal stuff.

One thing I bumped into was https://github.com/scalameta/metals/blob/fae222573a82d28654cdedb6dc841976b1d5f8e2/metals/src/main/scala/scala/meta/internal/builds/MillBuildTool.scala#L91-L93C22, where Metals was picking the wrong bloop package path. I suspect it's just an issue due to me testing using an unstable installLocalCache version, making SemVer.isCompatibleVersion("0.9.3", millVersion) fail. Not 100% sure though. Didn't manage to reproduce reliably

Tested manually using VSCode. Navigation within a file, across modules, between Scala and Java modules, to generated code, to external Scala libraries (scalatags) and Java std lib (System.out.println) all seem to work. Navigation within build.sc doesn't work, but that seems to be the case with 0.11.0-M7 too. Both give a no build target for: /Users/lihaoyi/Github/mill/example/scalabuilds/10-scala-realistic/build.sc warning

The version of IntelliJ I'm using fails to load using BSP even when using 0.11.0-M7, so I couldn't verify if it works using this branch

lihaoyi · 2023-04-04T10:52:29Z

@lefou here's my cursory attempt at #2414. Do you think this is the right approach to take? It seems like MillBuildServer lets us pretty easily swap out mill-build for another kind of root module. I'm not familiar with the BSP relate code, or how to best test it.

lihaoyi · 2023-04-04T13:59:59Z

@lefou seems CI is mostly green, but AFAICT there isn't much coverage for these code paths, so not sure how much confidence that gives us

lefou · 2023-04-05T19:17:17Z

bsp/worker/src/mill/bsp/worker/MillBuildServer.scala

@@ -105,7 +54,7 @@ class MillBuildServer(
            val modules: Seq[Module] =
              ModuleUtils.transitiveModules(evaluator.rootModule) ++ Seq(`mill-build`)
            val map = modules.collect {
-              case m: MillBuildModule =>
+              case m: MillBuildRootModule =>


I wonder whether this is still a singleton. Do we currently project meta-builds to individual synthetic modules? If not, should we?

I think, we should, if possible. To provide a nicer editing experience. On each level we might have different dependencies and settings, so separating them as (synthetic) modules make sense. But if we plan to do that, we should not match simply match on the type here, as we assume hard-coded module segments in the code below.

@lefou no, it should no longer be a singleton. However, this PR assumes it is, basically ignoring meta-builds

It shouldn't be too hard to make it a non-singleton, we'd just need to move MIllBuildBootstrap over to make it available, and then fish out the MillBuildRootModule from each level.

I sketched out what it would look like handling all the meta-module's MillBuildRootModule, rather than just hardcoding one. But I'll probably need your help to test this and poke and it to get it working, not familiar with the BSP logic at all

I currently just run vscode (which I'm not using myself regularily) on a recent Mill project and test a bit manually, but that's unsatisfying and way too ineffective. There are some BSP test suites out there which we could use, also a recently renewed one (https://github.com/build-server-protocol/bsp-testkit2), but they all come with no documentation and I simply lack the time to dig deeper how to use/integrate them.

lihaoyi · 2023-04-06T00:16:18Z

bsp/worker/src/mill/bsp/worker/MillBuildServer.scala

                (id, m)
              case m: BspModule =>
-                val uri = sanitizeUri(`mill-build`.millSourcePath / m.millModuleSegments.parts)
+                val uri = sanitizeUri(??? : os.Path)


@lefou I'm not sure what to do here. Other places it seemed straightforward to turn the singleton mill-build into a list of millBuildRootModules, even though I don't know what the code is trying to do, but over here it's unclear to me what the code doing so I'm not sure how to proceed

The point is to have a unique buildTarget ID as an URI. We currently use module segments and prefix them with the full path of the project, that's why mill-build.millSourcePath is used here. We can replace that with some other way to access the project base directory to get rid of the use of the sigleton mill-build here.

lefou · 2023-04-06T11:50:19Z

I guess, one issue with the current MillBuildRootModule is, that all instances have empty millModuleSegments. This breaks assumptions in various places, e.g. in BSP, but also in other unforseen places. E.g. I tried to rebase my recursive module deps detection branch and wondered, why I don't see any module when rendering them.

lihaoyi · 2023-04-06T12:18:13Z

@lefou yeah that's true. It's definitely confused me when I println() the module and nothing gets printed. OTOH, it does make sense that they have empty millModuleSegments, since they do inherit from RootModule and are treated as the root module for all intents and purposes, with the wrapping "outside" root module being discarded very early in the process.

I think the correct thing to do is to provided a nice .toString that shows something even when empty, and provide a .render method when you want the segments. I tried to do that as part of my remove-ammonite PR, but decided to defer that work since that PR had grown large enough as is

lefou · 2023-04-06T12:38:07Z

Maybe, reusing the foreign segments is appropriate?

lihaoyi · 2023-05-11T03:38:52Z

@lefou I rebased this on latest main and tested it on example/basic/1-simple-scala and example/scala-builds/10-scala-realistic by running installLocalCache and setting a .mill-version to point at it. Seems to work about as well as 0.11.0-M7 before all the ammonite removal stuff.

One thing I bumped into was https://github.com/scalameta/metals/blob/fae222573a82d28654cdedb6dc841976b1d5f8e2/metals/src/main/scala/scala/meta/internal/builds/MillBuildTool.scala#L91-L93C22, where Metals was picking the wrong bloop package path. I suspect it's just an issue due to me testing using an unstable installLocalCache version, making SemVer.isCompatibleVersion("0.9.3", millVersion) fail. Not 100% sure though

Tested manually using VSCode. Navigation within a file, across modules, between Scala and Java modules, to generated code, to external Scala libraries (scalatags) and Java std lib (System.out.println) all seem to work. Navigation within build.sc doesn't work, but that seems to be the case with 0.11.0-M7 too. Both give a no build target for: /Users/lihaoyi/Github/mill/example/scalabuilds/10-scala-realistic/build.sc warning

The version of IntelliJ I'm using fails to load using BSP even when using 0.11.0-M7, so I couldn't verify if it works using this branch

lefou · 2023-05-11T06:33:34Z

scalalib/src/mill/scalalib/bsp/MillBuildModule.scala

- * Just for testing purposes.
- */
-@internal
-object MillBuildModule extends ExternalModule with MillBuildModule {


@lihaoyi Is there any way after this change, to inspect a root module, e.g. to show it's sources etc.?

For RootModule, you can just mill show sources. For MillBuildRootModule, there wasn't a way before this PR and this PR doesn't change things AFAIK. We'd need to add it in another PR.

Ah thanks. I thought, I tested it, but probably with the wrong mill dev version. 🤦‍♂️

I see src included, but not the build.sc. This is probably the reason, you don't get language-specific edit features for build.sc.

lefou · 2023-05-11T09:47:26Z

My guess is, that we need to override the sources target and return all Mill source files, so that Metals and Idea know that the build.sc and potential imports (and meta-build files?) are source files. Then, they also should enable proper language features in their editors. Latest Idea (dev releases?) does support single files instead of directories, so we also should no longer use the dummySources. We also need to be careful, to not include the root project dir in sources.

lihaoyi · 2023-05-11T11:20:54Z

It seems to me that the Mill build.sc module isn't being picked up by Bloop/BSP/Metals at all. e.g. I don't see any .json file generated for it in.bloop/ and .metals/ doesn't seem to contain any hints.

Should we merge this first? It seems to behave more or less identically to the status quo, which achieves the goal of re-using MillBuildRootModule. Fixing BSP navigation in build.sc can be a follow up

lefou · 2023-05-11T11:22:15Z

Sure. We can merge and then improve BSP experience for Mill and it's meta builds.

lihaoyi · 2023-05-12T03:37:48Z

On some further testing, it appears that this whole time I was only exercising the Bloop code path, which seems to be entirely separate from the MillBuildServer code path. Furthermore, even with further effort I have not managed to wrangle VSCode into going through the MillBuildServer code path.

@lefou how do you manually test MillBuildServer? VSCode seems to keep using Bloop when i try to import stuff, and I can't figure out how to make it not.

lihaoyi · 2023-05-12T04:41:47Z

nm I managed to find the switch to use BSP, I'll put up another PR to get that code path working

lefou · 2023-05-12T05:33:52Z

FTR: If you start Metals, it uses Bloop as default BSP server. You need to run the command "Metals: Switch Build Server" manually and select "mill-bsp".

#2415 only tested the Bloop codepath, and left the BSP code path broken. This fixes it Tested manually by setting `switch build server` in VSCode to BSP and then clicking around in the following examples: 1. `example/basic/1-simple-scala` 2. `example/misc/3-import-file-ivy` 3. `example/misc/4-mill-build-folder` The following navigations work: 1. between `foo` and `foo.test` 2. between `Foo` and the Java and Scala std libs 3. between `Foo` and Scalatags 4. between `build.sc` and Mill library code 5. between `build.sc` and Scalatags 6. between `build.sc` and `mill-build/src/` 7. between `mill-build/build.sc` and Mill library code 8. between `mill-build/build.sc` and Java and Scala std libs I re-used some code from `GenIdeaImpl` to gather the source jars of mill bundled modules in `MillBuildServer`. TBH the code in both `MillBuildServer` and `GenIdeaImpl` is a mess and could use some housekeeping, and `MillBuildServer` could also use some unit/e2e tests to verify basic the basic shape of that `workspaceBuildTargets`, `buildTargetSources`, `buildTargetDependencySources` and others return in a few simple example cases. But that can probably come in a follow up

lefou linked an issue Apr 4, 2023 that may be closed by this pull request

Unify MillBuildModule with MillBuildRootModule #2414

Closed

lefou reviewed Apr 5, 2023

View reviewed changes

lihaoyi force-pushed the share-root-module branch from 3c6c2ed to 39e0069 Compare April 5, 2023 23:57

lihaoyi commented Apr 6, 2023

View reviewed changes

lihaoyi added 4 commits May 10, 2023 10:33

compiles

08ce006

fix child root module resolution for bootstrap

51a996b

.

a86289f

.

7089b02

lihaoyi force-pushed the share-root-module branch from 39e0069 to 7089b02 Compare May 11, 2023 03:32

.

db571ef

lihaoyi changed the title ~~[WIP] Re-use MillBuildBootstrapModule for BSP~~ Re-use MillBuildBootstrapModule for BSP May 11, 2023

.

60556b9

lihaoyi requested a review from lefou May 11, 2023 03:48

fix

a923eea

lefou reviewed May 11, 2023

View reviewed changes

lihaoyi changed the title ~~Re-use MillBuildBootstrapModule for BSP~~ Re-use MillBuildRootModule for BSP May 11, 2023

lihaoyi merged commit 6a15da4 into com-lihaoyi:main May 11, 2023

lefou added this to the 0.11.0-M9 milestone May 11, 2023

lihaoyi mentioned this pull request May 12, 2023

Improve VSCode/BSP integration #2514

Merged

lihaoyi mentioned this pull request May 13, 2023

Bloop doesn't work with RootModule #2486

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Re-use MillBuildRootModule for BSP #2415

Re-use MillBuildRootModule for BSP #2415

lihaoyi commented Apr 4, 2023 •

edited

Loading

lihaoyi commented Apr 4, 2023

lihaoyi commented Apr 4, 2023

lefou Apr 5, 2023

lihaoyi Apr 5, 2023

lihaoyi Apr 6, 2023 •

edited

Loading

lefou Apr 6, 2023

lihaoyi Apr 6, 2023

lefou Apr 6, 2023

lefou commented Apr 6, 2023

lihaoyi commented Apr 6, 2023

lefou commented Apr 6, 2023

lihaoyi commented May 11, 2023 •

edited

Loading

lefou May 11, 2023

lihaoyi May 11, 2023

lefou May 11, 2023

lefou commented May 11, 2023

lihaoyi commented May 11, 2023

lefou commented May 11, 2023

lihaoyi commented May 12, 2023

lihaoyi commented May 12, 2023

lefou commented May 12, 2023 •

edited

Loading

Re-use MillBuildRootModule for BSP #2415

Re-use MillBuildRootModule for BSP #2415

Conversation

lihaoyi commented Apr 4, 2023 • edited Loading

lihaoyi commented Apr 4, 2023

lihaoyi commented Apr 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lihaoyi Apr 6, 2023 • edited Loading

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

lefou commented Apr 6, 2023

lihaoyi commented Apr 6, 2023

lefou commented Apr 6, 2023

lihaoyi commented May 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lefou commented May 11, 2023

lihaoyi commented May 11, 2023

lefou commented May 11, 2023

lihaoyi commented May 12, 2023

lihaoyi commented May 12, 2023

lefou commented May 12, 2023 • edited Loading

lihaoyi commented Apr 4, 2023 •

edited

Loading

lihaoyi Apr 6, 2023 •

edited

Loading

lihaoyi commented May 11, 2023 •

edited

Loading

lefou commented May 12, 2023 •

edited

Loading