-
-
Notifications
You must be signed in to change notification settings - Fork 323
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
Re-use MillBuildRootModule for BSP #2415
Conversation
@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 |
@@ -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 => |
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 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.
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.
@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.
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 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
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 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.
3c6c2ed
to
39e0069
Compare
(id, m) | ||
case m: BspModule => | ||
val uri = sanitizeUri(`mill-build`.millSourcePath / m.millModuleSegments.parts) | ||
val uri = sanitizeUri(??? : os.Path) |
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.
@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
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.
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.
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. |
@lefou yeah that's true. It's definitely confused me when I I think the correct thing to do is to provided a nice |
Maybe, reusing the foreign segments is appropriate? |
39e0069
to
7089b02
Compare
@lefou I rebased this on latest main and tested it on 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 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 ( The version of IntelliJ I'm using fails to load using BSP even when using |
* Just for testing purposes. | ||
*/ | ||
@internal | ||
object MillBuildModule extends ExternalModule with MillBuildModule { |
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.
@lihaoyi Is there any way after this change, to inspect a root module, e.g. to show it's sources etc.?
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.
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.
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.
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
.
My guess is, that we need to override the |
It seems to me that the Mill Should we merge this first? It seems to behave more or less identically to the status quo, which achieves the goal of re-using |
Sure. We can merge and then improve BSP experience for Mill and it's meta builds. |
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 @lefou how do you manually test |
nm I managed to find the switch to use BSP, I'll put up another PR to get that code path working |
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
Tested it on
example/basic/1-simple-scala
andexample/scala-builds/10-scala-realistic
by runninginstallLocalCache
and setting a.mill-version
to point at it. Seems to work about as well as0.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, makingSemVer.isCompatibleVersion("0.9.3", millVersion)
fail. Not 100% sure though. Didn't manage to reproduce reliablyTested 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 withinbuild.sc
doesn't work, but that seems to be the case with0.11.0-M7
too. Both give ano build target for: /Users/lihaoyi/Github/mill/example/scalabuilds/10-scala-realistic/build.sc
warningThe 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