Make BSP 1st class citizen #969

joan38 · 2020-10-03T03:57:24Z

This is a proposal following #899.
My vision is to eventually remove IdeaGen as BSP takes over.
Let me know what you think.

lihaoyi · 2020-10-04T06:03:08Z

build.sc

+  override def compileModuleDeps = Seq(scalalib, scalajslib, scalanativelib)
+  def ivyDeps = Agg(
+    Deps.bsp,
+    Deps.ujsonCirce,


How difficult would it be to remove this Circe dependency? I'd like to avoid pulling in a heavyweight dependency tree if there's an easy way around it

I removed it and it compiled so I guess it was not used.

lihaoyi · 2020-10-04T06:04:07Z

I think this looks reasonable to me. I don't have review bandwidth right now, but if others review it and try it out and are satisfied, that's good enough to merge I think

joan38 · 2020-10-05T06:23:30Z

Does anyone understand why CI is not passing?

lefou · 2020-10-05T07:17:05Z

@joan38 Nope, but it's a typical Travis-CI failure. I'm used to restart these, but it's no guarantee. That's why I'm working on a move to GitHub Actions.

To the PR: What about the final assembly (JAR) size? Can you provide numbers in MB before and after this change?

docs/pages/1 - Intro to Mill.md

Baccata · 2020-10-05T08:19:08Z

docs/pages/1 - Intro to Mill.md


-Mill supports IntelliJ by default. Use `mill mill.scalalib.GenIdea/idea` to
+Mill supports any IDE that is compatible with [BSP](https://build-server-protocol.github.io/), sush as IntelliJ, VSCode


Suggested change

Mill supports any IDE that is compatible with [BSP](https://build-server-protocol.github.io/), sush as IntelliJ, VSCode

Mill supports any IDE that is compatible with [BSP](https://build-server-protocol.github.io/), such as IntelliJ, VSCode

joan38 · 2020-10-05T08:27:49Z

@lefou
Before:

(master)> ls -l /Users/jgoyeau/mill-release
-rwxr-xr-x  1 jgoyeau  staff  60964742 Oct  5 01:21 /Users/jgoyeau/mill-release*

After:

(bsp)> ls -l /Users/jgoyeau/mill-release
-rwxr-xr-x  1 jgoyeau  staff  61083101 Oct  5 01:26 /Users/jgoyeau/mill-release*

lefou · 2020-10-05T08:36:24Z

Is there some kind of Test Kit for BSP?

lefou · 2020-10-05T08:37:33Z

@lefou
Before:

(master)> ls -l /Users/jgoyeau/mill-release
-rwxr-xr-x  1 jgoyeau  staff  60964742 Oct  5 01:21 /Users/jgoyeau/mill-release*

After:

(bsp)> ls -l /Users/jgoyeau/mill-release
-rwxr-xr-x  1 jgoyeau  staff  61083101 Oct  5 01:26 /Users/jgoyeau/mill-release*

Thanks. Size-wise this merge is ok.

Baccata · 2020-10-05T08:39:41Z

docs/pages/1 - Intro to Mill.md


-Mill supports IntelliJ by default. Use `mill mill.scalalib.GenIdea/idea` to
+Mill supports any IDE that is compatible with [BSP](https://build-server-protocol.github.io/), such as IntelliJ, VSCode


So, I've just tried to use the latest master (0.8.0-9-6a7a11) with metals, ie removed all pre-existing bloop configuration / metals configuration, generated bsp config from the contrib bsp module, and opened vscode (waiting for it to connect to the bsp server). I get the following logs :

2020.10.05 10:34:50 INFO logging to file /private/tmp/foo/.metals/metals.log 2020.10.05 10:34:50 INFO started: Metals version 0.9.4 in workspace '/private/tmp/foo' for client vscode. 2020.10.05 10:34:51 INFO time: initialize in 0.38s Initializing Scala Debugger 2020.10.05 10:34:51 INFO tracing is enabled: /Users/oliviermelois/Library/Caches/org.scalameta.metals/bsp.trace.json 2020.10.05 10:34:51 INFO skipping build import with status 'Dismissed' 2020.10.05 10:34:53 INFO no build target: using presentation compiler with only scala-library 2020.10.05 10:34:53 WARN no build target for: /private/tmp/foo/build.sc 2020.10.05 10:34:54 INFO time: connected to build server in 2.58s 2020.10.05 10:34:54 INFO Connected to Build server v0.8.0-9-6a7a11 2020.10.05 10:34:55 INFO time: imported build in 0.77s 2020.10.05 10:34:54 INFO disconnected: build server 2020.10.05 10:34:54 ERROR Failed to connect with build server, no functionality will work. java.lang.NullPointerException at scala.math.Ordering$StringOrdering.compare(Ordering.scala:333) at scala.math.Ordering$StringOrdering.compare$(Ordering.scala:333) at scala.math.Ordering$String$.compare(Ordering.scala:335) at scala.math.Ordering$String$.compare(Ordering.scala:335) at scala.math.Ordering$$anon$1.compare(Ordering.scala:122) at java.util.TimSort.countRunAndMakeAscending(TimSort.java:355) at java.util.TimSort.sort(TimSort.java:220) at java.util.Arrays.sort(Arrays.java:1438) at scala.collection.SeqLike.sorted(SeqLike.scala:659) at scala.collection.SeqLike.sorted$(SeqLike.scala:647) at scala.collection.AbstractSeq.sorted(Seq.scala:45) at scala.collection.SeqLike.sortBy(SeqLike.scala:634) at scala.collection.SeqLike.sortBy$(SeqLike.scala:634) at scala.collection.AbstractSeq.sortBy(Seq.scala:45) at scala.meta.internal.metals.Doctor.buildTargetRows(Doctor.scala:391) at scala.meta.internal.metals.Doctor.$anonfun$buildTargetsTable$22(Doctor.scala:382) at scala.meta.internal.metals.Doctor.$anonfun$buildTargetsTable$22$adapted(Doctor.scala:382) at scala.meta.internal.metals.HtmlBuilder.element(HtmlBuilder.scala:97) at scala.meta.internal.metals.Doctor.$anonfun$buildTargetsTable$12(Doctor.scala:382) at scala.meta.internal.metals.Doctor.$anonfun$buildTargetsTable$12$adapted(Doctor.scala:382) at scala.meta.internal.metals.HtmlBuilder.element(HtmlBuilder.scala:97) at scala.meta.internal.metals.Doctor.buildTargetsTable(Doctor.scala:382) at scala.meta.internal.metals.Doctor.$anonfun$buildTargetsHtml$2(Doctor.scala:287) at scala.meta.internal.metals.Doctor.$anonfun$buildTargetsHtml$2$adapted(Doctor.scala:287) at scala.meta.internal.metals.HtmlBuilder.call(HtmlBuilder.scala:76) at scala.meta.internal.metals.Doctor.buildTargetsHtml(Doctor.scala:287) at scala.meta.internal.metals.Doctor.executeDoctor(Doctor.scala:97) at scala.meta.internal.metals.Doctor.executeRefreshDoctor(Doctor.scala:82) at scala.meta.internal.metals.Doctor.executeReloadDoctor(Doctor.scala:76) at scala.meta.internal.metals.Doctor.check(Doctor.scala:123) at scala.meta.internal.metals.MetalsLanguageServer.$anonfun$connectToNewBuildServer$8(MetalsLanguageServer.scala:1751) at scala.meta.internal.metals.MetalsLanguageServer.$anonfun$indexWorkspace$2(MetalsLanguageServer.scala:1942) at scala.meta.internal.metals.MetalsLanguageServer.timedThunk(MetalsLanguageServer.scala:1849) at scala.meta.internal.metals.MetalsLanguageServer.indexWorkspace(MetalsLanguageServer.scala:1925) at scala.meta.internal.metals.MetalsLanguageServer.$anonfun$profiledIndexWorkspace$2(MetalsLanguageServer.scala:1876) at scala.runtime.java8.JFunction0$mcV$sp.apply(JFunction0$mcV$sp.java:23) at scala.meta.internal.metals.MetalsLanguageServer.timedThunk(MetalsLanguageServer.scala:1849) at scala.meta.internal.metals.MetalsLanguageServer.$anonfun$profiledIndexWorkspace$1(MetalsLanguageServer.scala:1876) at scala.runtime.java8.JFunction0$mcV$sp.apply(JFunction0$mcV$sp.java:23) at scala.concurrent.Future$.$anonfun$apply$1(Future.scala:659) at scala.util.Success.$anonfun$map$1(Try.scala:255) at scala.util.Success.map(Try.scala:213) at scala.concurrent.Future.$anonfun$map$1(Future.scala:292) at scala.concurrent.impl.Promise.liftedTree1$1(Promise.scala:33) at scala.concurrent.impl.Promise.$anonfun$transform$1(Promise.scala:33) at scala.concurrent.impl.CallbackRunnable.run(Promise.scala:64) at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149) at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624) at java.lang.Thread.run(Thread.java:748)

I'm afraid saying it works with vscode is a bit premature :/

I should remove the mention of VSCode because they rely on Bloop instead.

I should remove the mention of VSCode because they rely on Bloop instead.

Not necessarily : if the bsp config exists, metals will try to connect to it (after the user prevents it from loading it via bloop config).

But yeah, I think the safest route for now is to just make the documentation/wording specifically mention Intellij

Btw, this issue you are seeing here was actually a Metals issue, not a Mill issue @Baccata. I've corrected it here: scalameta/metals#2164.

If you'd like to try again with Metals, the latest snapshot has this fix.

Baccata · 2020-10-05T08:40:47Z

docs/pages/1 - Intro to Mill.md

+Mill supports any IDE that is compatible with [BSP](https://build-server-protocol.github.io/), such as IntelliJ, VSCode
+or Emacs. Use `mill mill.BSP/install` to generate the BSP project config for your build.
+
+This also configures your IDE to allow easy navigate & code-completion within your build file itself.


This is interesting. Does it rely on ammonite's BSP implementation ? (I've not deep dived into your implementation, and am asking out of sheer curiosity)

No, Idea just pulls all the deps and then figures the rest out.

Right, so it does works with intellij, because of how it's got some built-in support for worksheets (and rudimentary support for ammonite scripts). Therefore, the current implementation will be insufficient for metals to support navigation/code-completion within the build file (and its potential upstreams dependencies).

I'd appreciate if this line was worded differently to reflect this. Something like :

Suggested change

This also configures your IDE to allow easy navigate & code-completion within your build file itself.

It also enables Intellij to provide navigation & code-completion features within your build file itself.

lefou

I'd rather not have the BSP object in the mill package, but instead in mill.bsp. This will result in a slightly longer command line mill mill.bsp.BSP/install but is consistent with e.g. mill mill.scalalib.GenIdea/idea and also avoids additional split packages.

joan38 · 2020-10-21T07:28:23Z

I tried to have a look at adding better tests but that looks like a non trivial investment as we need a HTTP client to emulate a IDE.
Nevertheless I moved the BSP object in the bsp package.
Should we move forward with this?

lefou · 2020-10-21T08:21:31Z

I'm going to merge this after tests have completed. Thanks, @joan38 for you work on this!

For the future, I'd like to have some kind of test setup, ideally some test suite provided by BSP project, as they obviously need to test their stuff too. Also I think we need more documentation that covers all the tricky parts regarding IDEA, metals, VS code, Bloop, and all the non-initialized or miss-initialized states one can end up in when using mill with BSP. Maybe a blog post or so.

joan38 · 2020-10-21T15:57:22Z

Agree with the tests, I'll try to find time to find something on this.
Also great idea on the blog post! I think that's a good time for me to write a blog post on this.
Thanks

ckipp01 · 2020-10-28T11:04:28Z

bsp/src/mill/bsp/BSP.scala


    write(
      BspConfigJson(
        "mill-bsp",
        Seq(
          "sh",
          "-c",
-          s"env ${sys.env.map { case (k, v) => s""""$k=$v"""" }.toSeq.mkString(" ")} $millPath -i mill.contrib.BSP/start"
+          s"env ${sys.env.map { case (k, v) => s""""$k=$v"""" }.toSeq.mkString(" ")} $millPath -i ${BSP.getClass.getCanonicalName.split("\\$").last}/start"


@joan38 So I've started to play around a bit with this in Metals, and one thing that surprised me a bit was that this captures all env variables and adds them to the mill.json. My worry is, especially in Metals if we automatically generate this for a mill build is that a user may accidentally commit this. I've fairly positive this will happen. I already see it happening a bunch with the new .bsp/sbt.json files that sbt has been producing. Unknowingly I expect users will be committing this with env variables such as passwords etc that they may obviously not want committed.

In my experience with bsp discovery and the other build servers, I haven't seen this before. Is this a limitation of Mill, is this really necessary to include to start the server?

Why are these env varibables explicitly set? Is it solving some known issue, e.g. a mill specific env varibale? In that case, we might better special handle a know case than bulk materializing a given state of env variables into this file.

We initially copied all the env vars to pick stuff such as COURSIER_REPOSITORY or JAVA_OPTS and make sure the BSP compilation is equal to a cmd compilation.
That can be reverted easily.

jastice · 2020-10-28T14:33:55Z

As to tests, note we have a bsp-testkit in the bsp repo:
https://github.com/build-server-protocol/build-server-protocol/tree/master/testkit/src/main/scala/ch/epfl/scala/bsp/testkit

It includes some utilities to create mock servers/clients, plus utility functions for building tests and scalacheck generators for the data classes.

See also usage in bazel-bsp:
https://github.com/JetBrains/bazel-bsp/blob/master/main/test/org/jetbrains/bsp/bazel/BazelBspServerTest.java

joan38 · 2020-10-28T15:21:40Z

Interesting @jastice I will have a look at that, we definitely need to invest some time in this. Thanks!

costa100 · 2020-12-20T21:41:11Z

Hello,
I am trying to use IntelliJ Idea version 2020.3 to open a project that uses mill. When I run: mill mill.bsp.BSP/install I get Cannot resolve external module mill.bsp.BSP. I use mill version 0.6.1 installed with brew on macos mojave. I ended up using mill mill.scalalib.GenIdea/idea which worked, sort of. So, if I want to have IJ work with mill, what exactly do I have to do? IJ still shows a lot of unresolved symbols when I open build.sc.

Thanks

joan38 · 2020-12-20T22:39:09Z

@costa100 0.6.x is very much EOL. Any reason you can't upgrade to 0.9.x?
I recommend using 0.9.3-17-3f8afc because is contains a fix for Intellij's BSP that is not in 0.9.3 and 0.9.4 is not yet published.

costa100 · 2020-12-21T05:37:19Z

@joan38 Hi - it was a script - mill - in the project that was pointing to version 0.6.1. I deleted it and now I am using 0.9.3 which is what I installed via homebrew. Thanks and sorry for the false alarm!

joan38 force-pushed the bsp branch 2 times, most recently from ac4e9f4 to c2c5ba1 Compare October 4, 2020 05:10

lihaoyi reviewed Oct 4, 2020

View reviewed changes

joan38 force-pushed the bsp branch 5 times, most recently from 8f61bee to 914d4cd Compare October 5, 2020 05:00

lefou reviewed Oct 5, 2020

View reviewed changes

docs/pages/1 - Intro to Mill.md Outdated Show resolved Hide resolved

joan38 mentioned this pull request Oct 5, 2020

Bump upickle #970

Closed

joan38 force-pushed the bsp branch from 914d4cd to 9ec3a4c Compare October 5, 2020 08:15

Baccata reviewed Oct 5, 2020

View reviewed changes

joan38 force-pushed the bsp branch from 9ec3a4c to 683aeec Compare October 5, 2020 08:24

Baccata reviewed Oct 5, 2020

View reviewed changes

joan38 force-pushed the bsp branch 2 times, most recently from a5fb35e to 082c985 Compare October 5, 2020 08:55

Baccata mentioned this pull request Oct 5, 2020

Piggyback on ammonite's BSP implementation for build files #971

Closed

lefou requested changes Oct 6, 2020

View reviewed changes

joan38 force-pushed the bsp branch from 082c985 to 328047a Compare October 6, 2020 08:34

joan38 mentioned this pull request Oct 6, 2020

Get the full class loader for BSP #967

Closed

joan38 force-pushed the bsp branch 2 times, most recently from faa5e99 to dfc8090 Compare October 6, 2020 16:07

ckipp01 mentioned this pull request Oct 17, 2020

Ability to start sbt session before Metals connects using sbt -bsp scalameta/metals#2152

Closed

joan38 added 2 commits October 20, 2020 23:30

Make BSP 1st class citizen

bc5c203

Improve mill build classpath

d9161ce

joan38 force-pushed the bsp branch from dfc8090 to d9161ce Compare October 21, 2020 06:30

lefou approved these changes Oct 21, 2020

View reviewed changes

lefou merged commit 2f9b633 into com-lihaoyi:master Oct 21, 2020

lefou added this to the after 0.8.0 milestone Oct 21, 2020

joan38 deleted the bsp branch October 21, 2020 15:55

ckipp01 mentioned this pull request Oct 24, 2020

Enable smoother sbt bsp integration. scalameta/metals#2154

Merged

ckipp01 reviewed Oct 28, 2020

View reviewed changes

joan38 mentioned this pull request Nov 10, 2020

Make BSP 1st class citizen #899

Closed

7 tasks

ckipp01 mentioned this pull request Nov 22, 2021

Integrated support for Mill BSP Server scalameta/metals#3307

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make BSP 1st class citizen #969

Make BSP 1st class citizen #969

joan38 commented Oct 3, 2020

lihaoyi Oct 4, 2020

joan38 Oct 4, 2020

lihaoyi commented Oct 4, 2020

joan38 commented Oct 5, 2020 •

edited

Loading

lefou commented Oct 5, 2020 •

edited

Loading

Baccata Oct 5, 2020

joan38 commented Oct 5, 2020

lefou commented Oct 5, 2020 •

edited

Loading

lefou commented Oct 5, 2020

Baccata Oct 5, 2020

joan38 Oct 5, 2020

Baccata Oct 5, 2020

ckipp01 Oct 31, 2020

Baccata Oct 5, 2020 •

edited

Loading

joan38 Oct 5, 2020

Baccata Oct 5, 2020 •

edited

Loading

lefou left a comment

joan38 commented Oct 21, 2020

lefou commented Oct 21, 2020

joan38 commented Oct 21, 2020 •

edited

Loading

ckipp01 Oct 28, 2020 •

edited

Loading

lefou Oct 28, 2020

joan38 Oct 28, 2020

joan38 Oct 28, 2020

jastice commented Oct 28, 2020 •

edited

Loading

joan38 commented Oct 28, 2020 •

edited

Loading

costa100 commented Dec 20, 2020 •

edited

Loading

joan38 commented Dec 20, 2020 •

edited

Loading

costa100 commented Dec 21, 2020


		Mill supports IntelliJ by default. Use `mill mill.scalalib.GenIdea/idea` to
		Mill supports any IDE that is compatible with [BSP](https://build-server-protocol.github.io/), sush as IntelliJ, VSCode

	This also configures your IDE to allow easy navigate & code-completion within your build file itself.
	It also enables Intellij to provide navigation & code-completion features within your build file itself.

Make BSP 1st class citizen #969

Make BSP 1st class citizen #969

Conversation

joan38 commented Oct 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lihaoyi commented Oct 4, 2020

joan38 commented Oct 5, 2020 • edited Loading

lefou commented Oct 5, 2020 • edited Loading

Choose a reason for hiding this comment

joan38 commented Oct 5, 2020

lefou commented Oct 5, 2020 • edited Loading

lefou commented Oct 5, 2020

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

Baccata Oct 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Baccata Oct 5, 2020 • edited Loading

Choose a reason for hiding this comment

lefou left a comment

Choose a reason for hiding this comment

joan38 commented Oct 21, 2020

lefou commented Oct 21, 2020

joan38 commented Oct 21, 2020 • edited Loading

ckipp01 Oct 28, 2020 • 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

jastice commented Oct 28, 2020 • edited Loading

joan38 commented Oct 28, 2020 • edited Loading

costa100 commented Dec 20, 2020 • edited Loading

joan38 commented Dec 20, 2020 • edited Loading

costa100 commented Dec 21, 2020

joan38 commented Oct 5, 2020 •

edited

Loading

lefou commented Oct 5, 2020 •

edited

Loading

lefou commented Oct 5, 2020 •

edited

Loading

Baccata Oct 5, 2020 •

edited

Loading

Baccata Oct 5, 2020 •

edited

Loading

joan38 commented Oct 21, 2020 •

edited

Loading

ckipp01 Oct 28, 2020 •

edited

Loading

jastice commented Oct 28, 2020 •

edited

Loading

joan38 commented Oct 28, 2020 •

edited

Loading

costa100 commented Dec 20, 2020 •

edited

Loading

joan38 commented Dec 20, 2020 •

edited

Loading