BSP: Provide mill source files #1029

joan38 · 2020-12-03T23:10:59Z

Not sure why the following does not bring in the dependencies anymore since 0.9.x:

val classpath = Try(evaluator.rootModule.getClass.getClassLoader.asInstanceOf[SpecialClassLoader]).fold(
  _ => Seq.empty,
  _.allJars
)

So this is resolving the source files directly. Let me know if you have a better idea.

joan38 · 2020-12-04T03:54:51Z

@lefou this is ready for review

lefou

I suspect the SpecialClassLoader never carried the mill API. But, maybe it's a good thing to be more verbose in case we don't get a SpecialClassLoader.

I made some comments below.

lefou · 2020-12-04T06:41:45Z

bsp/src/mill/bsp/ModuleUtils.scala

+    val repos = Evaluator
+      .evalOrElse(evaluator, T.task(T.traverse(modules)(_.repositoriesTask)()), Seq.empty)


The T.task is probably redundant. T.traverse already produces a task.

lefou · 2020-12-04T06:45:49Z

scalalib/src/Lib.scala

@@ -36,7 +36,7 @@ object Lib{
                                  depToDependency: Dep => coursier.Dependency,
                                  deps: TraversableOnce[Dep],
                                  mapDependencies: Option[Dependency => Dependency] = None,
-                                  ctx: Option[mill.util.Ctx.Log] = None) = {
+                                  ctx: Option[mill.util.Ctx.Log] = None): (Seq[Dependency], Resolution) = {


👍 for explicit return type.

lefou · 2020-12-04T06:50:53Z

bsp/src/mill/bsp/ModuleUtils.scala

+      Agg(
+        ivy"com.lihaoyi::mill-main:$millVersion",
+        ivy"com.lihaoyi::mill-scalalib:$millVersion",
+        ivy"com.lihaoyi::mill-scalajslib:$millVersion",
+        ivy"com.lihaoyi::mill-scalanativelib:$millVersion"
+      ),
+      sources = sources


I think these deps were never there before, at least all my experiments with mill-bsp resulted in missing mill API when editing build.sc. But, we should not bury such a hardcoded list here. Instead, this is knowledge best located in the buildfile (build.sc). We could generate a Scala file there (generateSources or BuildInfo plugin) and use it here. We have this exactly hard-coded list also in GenIdeaImpl, and I think this is an opportunity to fix both places the right way.

Excellent idea! Check out the new commit.

lefou · 2020-12-04T08:49:57Z

bsp/src/mill/bsp/ModuleUtils.scala

+    val millJars = resolveDependencies(
+      Resolve.defaultRepositories,
+      depToDependency(_, BuildInfo.scalaVersion),
+      BuildInfo.millEmbeddedModules.map(module => ivy"com.lihaoyi::mill-$module:${BuildInfo.millVersion}"),


I think using full ivy artifact is the way to go (using module.publishSelfDependency() when generating the BuildInfo). Less assumptions about the deps.

Check out the new commit.

lefou

@joan38 I'm sorry when it appears as nitpicking. I think keeping the generated file dependency free is a good thing, so I suggest to use a plain string to transport the dependency. I added suggestions to show what I have in mind. Don't know if it compiles though.

If tests work, this looks good to me! Looking forward to try that out.

lefou · 2020-12-04T09:39:15Z

build.sc

@@ -260,6 +261,8 @@ object scalalib extends MillModule {
        |  val ammonite = "${Deps.ammonite.dep.version}"
        |  /** Version of Zinc. */
        |  val zinc = "${Deps.zinc.dep.version}"
+        |  /** Dependency artifacts embedded in mill by default. */
+        |  val millEmbeddedDeps = ${artifacts.map(artifact => s"""ivy"${artifact.group}::${artifact.id}:${artifact.version}"""")}


Suggested change

| val millEmbeddedDeps = ${artifacts.map(artifact => s"""ivy"${artifact.group}::${artifact.id}:${artifact.version}"""")}

| val millEmbeddedDeps = ${artifacts.map(artifact => s""""${artifact.group}:${artifact.id}:${artifact.version}"""")}

Artitfact ID is already platform-resolved. Also I'd rather just use the string.

lefou · 2020-12-04T09:45:00Z

scalalib/src/GenIdeaImpl.scala

-            for(name <- artifactNames)
-            yield ivy"com.lihaoyi::mill-$name:${sys.props("MILL_VERSION")}",
-            false,
+            Versions.millEmbeddedDeps,


Suggested change

Versions.millEmbeddedDeps,

Versions.millEmbeddedDeps.map(d => ivy"$d"),

lefou · 2020-12-04T09:46:17Z

bsp/src/mill/bsp/ModuleUtils.scala

+    val millJars = resolveDependencies(
+      Resolve.defaultRepositories,
+      depToDependency(_, BuildInfo.scalaVersion),
+      Versions.millEmbeddedDeps,


Suggested change

Versions.millEmbeddedDeps,

Versions.millEmbeddedDeps.map(d => ivy"$d"),

lefou · 2020-12-04T09:52:54Z

@joan38, since our move to GitHub Actions, there is no longer the need to rebase or force-pushg all PRs to latest master to let tests pass. We can squash when merging.

joan38 · 2020-12-04T10:05:25Z

Your suggestions are good @lefou, keep on nitpicking that's how we achieve perfection :)

Do you recommend moving this back to BuildInfo in core since we don't need access to Dep in scalalib?

I'm just used to amend force pushing but would incremental commits better help incremental reviewing?

lefou · 2020-12-04T10:08:55Z

Your suggestions are good @lefou, keep on nitpicking that's how we achieve perfection :)

You asked for it. ;-)

Do you recommend moving this back to BuildInfo in core since we don't need access to Dep in scalalib?

Yes.

I'm just used to amend force pushing but would incremental commits better help incremental reviewing?

Yeah, avoiding forced pushes would greatly help, esp. in the Github UI.

joan38 requested a review from lefou December 3, 2020 23:11

joan38 force-pushed the bsp branch from 8f0d734 to f872fd5 Compare December 4, 2020 01:26

lefou reviewed Dec 4, 2020

View reviewed changes

joan38 force-pushed the bsp branch from f872fd5 to 5bbc244 Compare December 4, 2020 07:59

lefou reviewed Dec 4, 2020

View reviewed changes

BSP: Provide mill source files

38a70a6

joan38 force-pushed the bsp branch from 5bbc244 to 38a70a6 Compare December 4, 2020 09:35

lefou reviewed Dec 4, 2020

View reviewed changes

Address suggestions

0371017

joan38 merged commit 3f8afc2 into com-lihaoyi:master Dec 4, 2020

joan38 deleted the bsp branch December 4, 2020 19:00

lefou added this to the after 0.9.3 milestone Dec 5, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BSP: Provide mill source files #1029

BSP: Provide mill source files #1029

joan38 commented Dec 3, 2020 •

edited

Loading

joan38 commented Dec 4, 2020

lefou left a comment

lefou Dec 4, 2020

lefou Dec 4, 2020

lefou Dec 4, 2020

joan38 Dec 4, 2020

lefou Dec 4, 2020

joan38 Dec 4, 2020

lefou left a comment •

edited

Loading

lefou Dec 4, 2020

lefou Dec 4, 2020

lefou Dec 4, 2020

lefou commented Dec 4, 2020

joan38 commented Dec 4, 2020 •

edited

Loading

lefou commented Dec 4, 2020

		val repos = Evaluator
		.evalOrElse(evaluator, T.task(T.traverse(modules)(_.repositoriesTask)()), Seq.empty)

	\| val millEmbeddedDeps = ${artifacts.map(artifact => s"""ivy"${artifact.group}::${artifact.id}:${artifact.version}"""")}
	\| val millEmbeddedDeps = ${artifacts.map(artifact => s""""${artifact.group}:${artifact.id}:${artifact.version}"""")}

	Versions.millEmbeddedDeps,
	Versions.millEmbeddedDeps.map(d => ivy"$d"),

BSP: Provide mill source files #1029

BSP: Provide mill source files #1029

Conversation

joan38 commented Dec 3, 2020 • edited Loading

joan38 commented Dec 4, 2020

lefou 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

lefou left a comment • 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 Dec 4, 2020

joan38 commented Dec 4, 2020 • edited Loading

lefou commented Dec 4, 2020

joan38 commented Dec 3, 2020 •

edited

Loading

lefou left a comment •

edited

Loading

joan38 commented Dec 4, 2020 •

edited

Loading