Metals support

[megri]

I've been playing around with this little build tool and I really like the feel of it so far. One problem though is metals integration.

It seems like seed doesn't generate everything metals need to be happy. For instance, the resolution.modules path is completely empty. By adding modules manually I was able to get "go to definition" working for the Scala library, but obviously this is not feasible to do for every resolved artifact in the build, especially as things get larger and definitions need to be updated.

It is also a bit tricky—but manageable—to add the required semanticdb compiler plugin to the build: resolving the plugin itself is dead easy with compilerDeps, but the user also needs to manually add full plugin path to the scalaOptions, along with a few plugin-specific settings. At least the path could be added automatically.

[tindzk]

Thanks for giving Seed a try!

Metals support is still incomplete as you already noticed. Here are the things we will need to change to make the experience smoother:

• [x] Presently, compilerDeps can be defined only on modules. It should be possible to define compiler dependencies for the entire project. Compiler dependencies should be inherited.
• [ ] Create a global Seed setting semanticDb (in analogy to the tmpfs setting). It will add the SemanticDB plug-in to all builds and also add the necessary scalac options. We will need to make sure that the SemanticDB plug-in is compatible with the project's Scala version and report it to the user otherwise.
• [x] Populate resolution.modules in the generated Bloop configuration regardless of whether the SemanticDB setting is enabled or not.

[megri]

I'm having a try at the first of those checkboxes and have some questions:

• Should I "inherit" the compilerDeps at the parse step (processBuild), at the resolution step (compilerDeps), or elsewhere?
• ScalaCompiler#compilerPlugins seem to only output -XPlugin:-strings for base modules, excluding target platform modules. Is this intended?

[tindzk]

Thanks for taking a stab at it.

1. What I had in mind was to add compilerDeps: List[ScalaDep] = List() to the case class seed.model.Project. Then, in processBuild you could append all compilerDeps as we already do with testFrameworks: https://github.com/tindzk/seed/blob/7fa092f606180f8d03abcde5c52b8ec2f3de2e69/src/main/scala/seed/config/BuildConfig.scala#L72-L76

Next, add the project-level compilerDeps here:

https://github.com/tindzk/seed/blob/7fa092f606180f8d03abcde5c52b8ec2f3de2e69/src/main/scala/seed/artefact/ArtefactResolution.scala#L126-L127

2. Good catch! This is a bug. We can add a test case derived from example-paradise to reproduce the faulty behaviour:

--- a/test/example-paradise/build.toml
+++ b/test/example-paradise/build.toml
@@ -9,6 +9,13 @@ scalaOptions   = ["-encoding", "UTF-8", "-unchecked", "-deprecation", "-Xfuture"
 root    = "macros"
 sources = ["macros"]
 targets = ["jvm", "js"]
+
+[module.macros.jvm]
+compilerDeps = [
+  ["org.scalamacros", "paradise", "2.1.1", "full"]
+]
+
+[module.macros.js]
 compilerDeps = [
   ["org.scalamacros", "paradise", "2.1.1", "full"]
 ]

[megri]

That's pretty much what I've done now. I'll see if I can wrap it up tonight :)

[megri]

Actually now that I think about it, shouldn't an imported project's compilerDeps only be applied to the imported project's modules?

This would mean that the combining of project-level deps ++ module-level deps have to happen in processBuild. If modules are amended like so…:

val imported = parsed.`import`.flatMap(parse(_))

def combineCompilerDeps(project: Project, module: Module): Module =
  module.copy(compilerDeps = (project.compilerDeps ++ module.compilerDeps).distinct)

val parsedModules = parsed.module.mapValues(combineCompilerDeps(parsed.project, _))

val importedModules = imported.foldLeft(Map.empty[String, Module]) { case (acc, importedBuild) =>
  importedBuild.module.mapValues(combineCompilerDeps(importedBuild.project, _))
}

parsed.copy(
  project = parsed.project.copy(
    testFrameworks = (parsed.project.testFrameworks ++ imported.flatMap(_.project.testFrameworks)).distinct,
  ),
  module = parsedModules ++ importedModules)

… it then follows that the modules will already have their compilerDeps set once artifact resolution kicks in. The drawback, perhaps, is that the processing of the build now updates module definitions up-front. That is, it's impossible to later see if the compiler dep came from the project or was assigned directly to the module. Maybe this isn't a big deal?

[megri]

One more question: README.md states that

External builds can be imported into the scope by using the import setting at the root level. It can point to a build file, or its parent directory in which case it will attempt to load /build.toml. From the imported file, Seed will only make its modules accessible. Other project-level settings are being ignored. Multiple projects can be imported as import is a list.

Does this mean that, in the scenario of some project "foo" importing project "bar" with imports = ["bar"], the modules imported from bar should not get the compilerDeps from foo, or vice versa?

[tindzk]

Actually now that I think about it, shouldn't an imported project's compilerDeps only be applied to the imported project's modules?

Agreed. The importing of projects is quite simplistic at the moment. All we do is pull in the modules into the scope. The compiler flags and the Scala/Scala.js/Scala Native versions of the imported projects are ignored. The assumption was that the versions and settings from the root project are indicative. Also, one more limitation is that the module names have to be unique across all imported builds.

Despite its simplicity, this approach worked rather well even with multiple levels of inheritance. Still, we should strive for a more correct solution which is to compile the modules exactly with the settings specified in their build files (i.e. versions, plug-ins, flags etc.) Then, the only thing we would have to ensure is that imported modules have a compatible Scala version (importing a Scala 2.13 build into a Scala 2.12 build would not be possible anymore).

The drawback, perhaps, is that the processing of the build now updates module definitions up-front.

If we want to retain the original build definitions, we could represent them as a tree structure rather than flattening them (as we do right now). This would be particularly useful when visualising builds, but for the time being your proposed solution is fairly sufficient.

Does this mean that, in the scenario of some project "foo" importing project "bar" with imports = ["bar"], the modules imported from bar should not get the compilerDeps from foo, or vice versa?

Yes. This was the approach up until now, but I think we'd better change it.

[megri]

I'm looking at checkbox #2 now, "Create a global Seed setting semanticDb".

From the context I'm assuming that the setting should be a boolean flag on Build just like tmpfs, but I want to think this through a few extra steps. Notably

• Is semantic-db in the same "class" as tmpfs? tmpfs to me signifies a non-essential property of how intermediate build artefacts should be stored, whereas semanticdb is a hard dependency requirement for metals (and some other tools like scalafix) to work. Perhaps it should always be added when a .bloop configuration is generated?
• If we decide to use a build property to enable semantic-db, should we merge it with other build settings on import? Which "way" should the property merge? (import overrides base build vs. base build propagates semanticdb settings to imported modules.)

[tindzk]

• Is semantic-db in the same "class" as tmpfs? tmpfs to me signifies a non-essential property of how intermediate build artefacts should be stored, whereas semanticdb is a hard dependency requirement for metals (and some other tools like scalafix) to work. Perhaps it should always be added when a .bloop configuration is generated?

Good objections.

The problem with adding SemanticDB to every Bloop configuration is that it will slow down compilation and can introduce indeterminism (I saw it crash the compiler in some cases). In CI settings, this would be undesired.

One more limitation to account for is that SemanticDB is not available for all Scala versions. Mill hard-codes the SemanticDB version as well as the Scala versions the plug-in is compatible with. In Seed, we could define a default version which we treat as the recommended minimum version (similar to how we already do it with Bloop and Coursier). We should let users override this version, such that they do not have to wait for a new Seed release to come out.

I am thinking of a separate section that looks as follows:

[semantic-db]
enable = true

# The following two settings are optional
version = "4.1.12"
scalaVersions = [
  "2.13.0",
  "2.12.8",
  "2.12.7",
  "2.11.12"
]

If we decide to use a build property to enable semantic-db, should we merge it with other build settings on import? Which "way" should the property merge? (import overrides base build vs. base build propagates semanticdb settings to imported modules.)

The setting will be added to the global Seed configuration file (~/.config/seed.toml). At the moment, we cannot define project-specific Seed configurations yet, so we do not have to worry about inheritance. In the future, we could allow users to create a .seed.toml file in the project folder to disable/enable tmpfs or SemanticDB only for the current project.

[megri]

I am thinking of a separate section that looks as follows:

[semantic-db]
enable = true

The following two settings are optional

version = "4.1.12" scalaVersions = [ "2.13.0", "2.12.8", "2.12.7", "2.11.12" ]

Is the scalaVersions necessary? The plugin needs to be resolved with full Scala versioning and if that fails the build will fail.

This also makes me wonder if it's good to have the definition in the global configuration, since it will cause unrelated projects (those with incorrect scalaVersion) to either fail or silently not install semanticdb. This seems like a source of confusion.

[tindzk]

We should only resolve the plug-in if the build file's Scala version is compatible, otherwise a warning will be triggered. Seed already does something similar when generating IDEA projects. If a module does not specify a root path, it will be skipped with a warning.

[megri]

This is relevant: https://github.com/scalameta/metals/pull/852

It seems we can drop the last checkbox with this change :) (when/if it ships)

[tindzk]

Great. For reference, the corresponding pull request in Bloop is https://github.com/scalacenter/bloop/pull/942.

@tgodzik Do I see correctly that we will only have to set workspaceDir to make Bloop builds compatible with Metals?

[tgodzik]

@tgodzik Do I see correctly that we will only have to set workspaceDir to make Bloop builds compatible with Metals?

Yes, that's needed for one of the parameters of SemanticDB and this path itself might be useful in a number of other things.

We default it to the parent of the configDir:

project.workspaceDirectory.getOrElse(configDir.getParent)

[jvican]

Really excited that the next bloop release will support seed out of the box.

[megri]

Bloop 1.3.3 is released!

And metals has this merged: https://github.com/scalameta/metals/pull/852 although not yet published it seems.

[tgodzik]

Bloop 1.3.3 is released!

And metals has this merged: scalameta/metals#852 although not yet published it seems.

Should published under a snapshot - will try to release a new version next week or so.