BSP fixes and workaround for cross projects by joan38 · Pull Request #885 · com-lihaoyi/mill

joan38 · 2020-05-14T00:07:09Z

This is fixing:

Some source file considered as directories.
A workaround SCL-17551 that doesn't manage well 2 modules with the same base directory. This is mitigating BSP: Not really working when used with Cross and CrossScalaModule #864 just like how SBT does 1 version only.
I'm also trying to fix it on the client side BSP: Fix cross modules JetBrains/intellij-scala#534
Partially fixing BSP: Import build related deps for IDE support in build.sc files #863. It's resolving $ivy imports but missing the $file resolutions.
Removes the weird /build root project so that in the future we will be able to support actual root project.

Thanks

lefou

This look ok to me, but I'm not really familiar with the BSP contrib module. What does @samvel1024 or @adadima think?

Also it seems there is a unrelated change in GenIdeaImpl.

joan38 · 2020-05-15T01:40:26Z

Please ignore this PR for now, it needs more work.

joan38 · 2020-05-15T06:09:28Z

@samvel1024 @adadima Please have a look.
I'm still testing this locally and also trying to get the $file to work somehow.

joan38 · 2020-05-15T08:30:05Z

This is working like a charm on my projects.

@samvel1024 @adadima Do you guys know how to specify "Excluded Folders"? The red folders in this screenshot:

joan38 · 2020-05-15T17:50:08Z

On my previous question it seems not supported yet https://gitter.im/scalacenter/bsp?at=5ebe65b2ecc55a312d0446fc but this is not really important anyway.

scalalib/src/GenIdeaImpl.scala

joan38 · 2020-05-15T19:26:34Z

main/src/modules/Util.scala

+  def millProperty(key: String): Option[String] =
+    Option(sys.props(key)) // System property has priority
+      .orElse(Option(LongMillProps.getProperty(key)))


I changed this to an Option for type safety. It didn't make sense to get nulls and try to wrap in an Option at multiple call sites.

Makes sense.

lefou

Is there any BSP test suite or compliance kit where we can check our support against?

Still can't say much about your changes. I haven't tested them, but if they solve some issue and the original authors don't complain, I tend to merge. I added some questions.

lefou · 2020-05-16T08:00:01Z

contrib/bsp/src/mill/contrib/BSP.scala

+          System.err.println(
+            s"""An exception occured while connecting to the client.
+              |Cause: ${e.getCause}
+              |Message: ${e.getMessage}
+              |Exception class: ${e.getClass}
+              |Stack Trace: ${e.getStackTrace}""".stripMargin
+          )
+      } finally {
+        System.err.println("Shutting down executor")


Shouldn't we use T.log.error here?

Good point. I will do the change in a bit.

lefou · 2020-05-16T08:08:40Z

scalalib/src/JavaModule.scala

-      path <- (if (os.isDir(root.path)) os.walk(root.path) else Seq(root.path))
-      if os.isFile(path) && ((path.ext == "java") && !isHiddenFile(path))
+      path <- if (os.isDir(root.path)) os.walk(root.path) else Seq(root.path)
+      if os.isFile(path) && ((path.ext == "java") && !path.toIO.isHidden)


Why is that change?

The isHidden is already defined in Java IO and is probably more robust wrt cross platform (i.e on windows a hidden file don't start with a .)

Yeah, but I think a dot file should always be excluded and treated not as sourcefile. If you make that logic OS-specific, your build end up to be non-reproducible.

Ah, I see. We want a file starting with a . to always be excluded no matter what OS it it.
Reverting this change.

lefou · 2020-05-16T08:09:01Z

scalalib/src/ScalaModule.scala

-      path <- (if (os.isDir(root.path)) os.walk(root.path) else Seq(root.path))
-      if os.isFile(path) && ((path.ext == "scala" || path.ext == "java") && !isHiddenFile(path))
+      path <- if (os.isDir(root.path)) os.walk(root.path) else Seq(root.path)
+      if os.isFile(path) && ((path.ext == "scala" || path.ext == "java") && !path.toIO.isHidden)


The isHidden is already defined in Java IO and is probably more robust wrt cross platform (i.e on windows a hidden file don't start with a .)

scalalib/src/GenIdeaImpl.scala

lefou · 2020-05-16T08:11:12Z

main/src/modules/Util.scala

+  def millProperty(key: String): Option[String] =
+    Option(sys.props(key)) // System property has priority
+      .orElse(Option(LongMillProps.getProperty(key)))


Makes sense.

joan38 · 2020-05-16T18:17:06Z

contrib/bsp/src/mill/contrib/BSP.scala

+          .setInput(stdin)
+          .setLocalService(millServer)
+          .setRemoteInterface(classOf[BuildClient])
+          .traceMessages(new PrintWriter((os.pwd / ".bsp" / "mill.log").toIO))


I moved the log file in the .bsp folder.
I think it was too much in the face of the user.

joan38 · 2020-05-18T09:12:40Z

contrib/bsp/src/mill/contrib/bsp/ModuleUtils.scala

    buildTarget.setData(dataBuildTarget)
-    buildTarget.setDisplayName(moduleName(module.millModuleSegments))
-    buildTarget.setBaseDirectory(module.intellijModulePath.toIO.toURI.toString)
+    buildTarget.setDisplayName(module.millModuleSegments.render)


I removed moduleName for .render. That makes display of modules in Idea (and other BSP clients) look exactly the same as mill cli.

joan38 · 2020-05-18T09:24:13Z

This PR is ready to go @lefou @lihaoyi . We are looking forward to use it in my team since the current version of BSP is a great start but contained important bugs and missing support for Cross projects.

FYI I also finished JetBrains/intellij-scala#534 which fixes SCL-17551.
And when this is merged and I have more time I will also fix #893.

joan38 · 2020-05-19T08:04:20Z

Thanks

joan38 force-pushed the bsp branch 2 times, most recently from 46c7e36 to 50e0062 Compare May 14, 2020 10:49

lefou reviewed May 14, 2020

View reviewed changes

joan38 marked this pull request as draft May 15, 2020 01:37

joan38 force-pushed the bsp branch 2 times, most recently from 1b76901 to 944f0b4 Compare May 15, 2020 06:04

joan38 marked this pull request as ready for review May 15, 2020 06:08

joan38 force-pushed the bsp branch 2 times, most recently from c178f91 to a1de4bd Compare May 15, 2020 08:26

joan38 force-pushed the bsp branch 4 times, most recently from d309131 to 8f6a234 Compare May 15, 2020 19:19

joan38 commented May 15, 2020

View reviewed changes

scalalib/src/GenIdeaImpl.scala Show resolved Hide resolved

joan38 commented May 15, 2020

View reviewed changes

joan38 force-pushed the bsp branch 2 times, most recently from 76af078 to c93f7ab Compare May 15, 2020 22:12

lefou reviewed May 16, 2020

View reviewed changes

joan38 force-pushed the bsp branch 2 times, most recently from d3bb1fd to 7bf15f7 Compare May 16, 2020 18:15

joan38 commented May 16, 2020

View reviewed changes

joan38 mentioned this pull request May 18, 2020

BSP: Fix cross modules JetBrains/intellij-scala#534

Merged

BSP fixes and workaround for cross projects

13d0ee0

joan38 force-pushed the bsp branch from 7bf15f7 to 13d0ee0 Compare May 18, 2020 09:08

joan38 commented May 18, 2020

View reviewed changes

lefou merged commit 08be5a7 into com-lihaoyi:master May 18, 2020

lefou added this to the after 0.7.1 milestone May 18, 2020

joan38 deleted the bsp branch May 19, 2020 08:04

Uh oh!

Conversation

joan38 commented May 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lefou left a comment

Choose a reason for hiding this comment

Uh oh!

joan38 commented May 15, 2020

Uh oh!

joan38 commented May 15, 2020

Uh oh!

joan38 commented May 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joan38 commented May 15, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lefou left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joan38 commented May 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joan38 commented May 19, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

joan38 commented May 14, 2020 •

edited

Loading

joan38 commented May 15, 2020 •

edited

Loading

lefou left a comment •

edited

Loading

joan38 commented May 18, 2020 •

edited

Loading