Skip to content
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

Unify build tool integrations with Metals #942

Merged
merged 23 commits into from Jul 31, 2019

Conversation

@tgodzik
Copy link
Contributor

tgodzik commented Jun 25, 2019

Add an additional mechanism to save information about the SemanticDB plugin version for Metals. That information is then used to add scala options to compiler instances for the SemanticDB plugin.

Fixes #895

@jvican Is this what you meant? I still need figure out some tests and a way to simulate failing fatal warnings, although the current solution works the same as the one in this PR.

@tgodzik tgodzik requested a review from jvican Jun 25, 2019
@tgodzik tgodzik force-pushed the tgodzik:automatically-add-semanticDB-plugin branch from 5528107 to 47d671c Jun 25, 2019
@jvican jvican changed the title WiP Unify build tool integrations with Metals Unify build tool integrations with Metals Jun 26, 2019
@tgodzik

This comment has been minimized.

Copy link
Contributor Author

tgodzik commented Jun 26, 2019

After talking with @jvican I think what we should do is:

  1. Create a separate module something like integration-shared
  2. Add functionality to fetch SemanticDB plugin via Coursier and save the paths to settings file - based on Scala version and SemanticDB version
  3. Depend on that shared module in all integrations and just invoke for each project's scala version - that will save all needed paths to the settings path.
  4. The mapper inside bloop would read that settings file and use the appropriate SemanticDB plugin path

We would need to add an additional parameter with SemanticDB version to all integrations (we don't have that while running bloopInstall), but that shouldn't be a problem.

Just a question to @jvican : is there a reason that we don't use Scala 2.12 in all of the integrations? At least in Gradle and Maven I don't see any reason to do that. And Metals doesn't work with 2.10 modules :P

@jvican

This comment has been minimized.

Copy link
Member

jvican commented Jul 1, 2019

I've given this a lot more thought and I think this PR is on the right track. Implementing specific semanticdb support in every build as I proposed in our call @tgodzik has the advantage that everything that requires downloading is done by the build tool but two big disadvantages:

  1. By default, we enable semanticdb and we're exporting a build that is different from that in the build tool. That should be fine in theory but it has some compiler performance implications, especially for people using Bloop with IntelliJ.
  2. It requires a lot more work. To be done safely, we need to shade ourselves the coursier dependency we pull in and we need to repeat this across all build tools.

By centralizing the download and setup in the server, we have two key advantages:

  1. Faster to implement and reason about.
  2. Build tools don't need to add support for semanticdb and apply the rest of changes in the build to play nicely with Metals.
  3. By default, if Metals is not used, we don't experience the compiler performance overhead added by Semanticdb (important for CI workflows and IntelliJ users).

Therefore, we only need to tweak how this PR is implemented internally, I'll leave few code comments after this long comment.

Copy link
Member

jvican left a comment

Thanks a lot for working on this @tgodzik, I've left comments with suggestions and improvements, please let me know what you think! 😸

@tgodzik

This comment has been minimized.

Copy link
Contributor Author

tgodzik commented Jul 3, 2019

@jvican Thanks for the comments, I just about starting to work on it, since I spent some time on Metal's issues.

@tgodzik tgodzik force-pushed the tgodzik:automatically-add-semanticDB-plugin branch 3 times, most recently from 6a08b48 to fff9e49 Jul 5, 2019
@tgodzik tgodzik force-pushed the tgodzik:automatically-add-semanticDB-plugin branch 5 times, most recently from 976c4ef to 9a30e5c Jul 5, 2019
@tgodzik tgodzik requested review from jvican and marek1840 Jul 8, 2019
Copy link
Contributor

marek1840 left a comment

Just some very-minor nitpicks

frontend/src/it/scala/bloop/CommunityBuild.scala Outdated Show resolved Hide resolved
)
val contents = new String(bytes, StandardCharsets.UTF_8)
parser.parse(contents) match {
case Left(failure) => throw failure

This comment has been minimized.

Copy link
@marek1840

marek1840 Jul 9, 2019

Contributor

why not None? or even better, chaning the return type to a Try?

@tgodzik tgodzik force-pushed the tgodzik:automatically-add-semanticDB-plugin branch 2 times, most recently from 54152c1 to 5ec42d9 Jul 16, 2019
…plugin version for Metals.

That information is then used to add scala options to compiler instances for the SemanticDB plugin.
@tgodzik tgodzik force-pushed the tgodzik:automatically-add-semanticDB-plugin branch from 5ec42d9 to 47134dd Jul 16, 2019
Copy link
Member

jvican left a comment

🎉 getting closer to the finish line, I have more questions/suggestions but the PR is looking slick for now 😄 good work!

shared/src/main/scala/bloop/io/AbsolutePath.scala Outdated Show resolved Hide resolved
frontend/src/test/scala/bloop/util/TestUtil.scala Outdated Show resolved Hide resolved
frontend/src/test/scala/bloop/util/TestUtil.scala Outdated Show resolved Hide resolved
frontend/src/main/scala/bloop/engine/Build.scala Outdated Show resolved Hide resolved
import scala.util.Failure
import scala.util.Success

case class WorkspaceSettings(semanticDBVersion: String)

This comment has been minimized.

Copy link
@jvican

jvican Jul 17, 2019

Member

I'll add docs here and in several other places when we're doing with the review

frontend/src/main/scala/bloop/engine/Build.scala Outdated Show resolved Hide resolved
@tgodzik tgodzik force-pushed the tgodzik:automatically-add-semanticDB-plugin branch 2 times, most recently from f4caeb0 to 96f6dc1 Jul 19, 2019
@@ -62,6 +63,7 @@ object DependencyResolution {
}
val fetch = ResolutionProcess.fetch(repositories, Cache.default.fetch)
val resolution = start.process.run(fetch).unsafeRun()
if (shouldReportErrors) reportErrors(resolution, logger)

This comment has been minimized.

Copy link
@tgodzik

tgodzik Jul 22, 2019

Author Contributor

Tested with Metals and now we will show a warning like:

warning

@tgodzik tgodzik requested a review from jvican Jul 23, 2019
jvican and others added 17 commits Jul 24, 2019
The intent of this piece of code was good but after thinking about it
and trying it out locally I've realized this is too verbose of a message
and this information should be shown in the Metals doctor instead.

Some of this code will be added back in a next commit in a different
place without the `displayWarningToUser`, which IMO should only be used
when the semanticdb plugin could **not** be resolved for a version we
know it's compatible for. This is truly an scenario that is unlikely to
happen and where good communication with users is key.
- Use new coursier API at the request of Alex, simplifies error handling
- Add documentation to `enableMetalsSettings` and
  `detectWorkspaceDirectory`.
- Cache `latest.release` resolution so that we only resolve the plugin
  once in the whole server session, otherwise there's too much overhead
  of resolving a `latest.release` artifact per project.
- Refine the logic transforming the project so that we unconditionally
  apply range positions and we only emit warnings to users when a
  resolution error that we didn't expect happens.
These changes are not really important. They will be refined in an
upcoming commit.
And lay the foundation for the big changes coming in the next commit.
This big commit rethinks how build load works to make it incremental. It
adds more tests and documentation to the appropiate internal APIs to
make this area easier to browse in the future.

The previous logic was working perfectly fine but could incur some
performance overhead because every time there was a change in the server
the build load process would restart and that implies also resolving
semanticdb plugins for all scala versions in a project. That is not a
big problem because most of the times they are cached but the overhead
of that operation is nevertheless too expensive, so the incremental
build load process mitigates that.
The semantics to retry adding the semanticdb plugin to a project depend
uniquely on whether the project has semanticdb disabled and whether the
`newSettings` passed by the client are not empty. Whether there is a
change in the settings or not should not affect these semantics.
Because `writeToFile` returned `Either`, there were places in our tests
where we were completely swallowing any exception that could be
happening when writing the workspace settings file. Now we will throw
the exception instead.

In the case of the internal build logic, we do swallow any exception
thrown by this logic intentionally.
The workspace already implies that it's the root directory and we
already use `Dir` as the suffix of many fields in the configuration, so
this is a more idiomatic choice.
@jvican
jvican approved these changes Jul 30, 2019
Copy link
Member

jvican left a comment

@tgodzik I've just pushed several changes that IMO make this PR already ready to be merged, it's your time now to review! I've left a few comments/explanations on the most important parts of the changes and the commit messages are also meant to be descriptive. Please let me know what you think about the changes 😄

) {
val log = new FullLogger(log0)

This comment has been minimized.

Copy link
@jvican

jvican Jul 30, 2019

Member

Happy to see this leaving 👍

*/
def resolve(
def resolveWithErrors(

This comment has been minimized.

Copy link
@jvican

jvican Jul 30, 2019

Member

I updated this logic because Alex recommended me to update to the latest coursier API.

@@ -0,0 +1,30 @@
package bloop.data

sealed trait LoadedProject {

This comment has been minimized.

Copy link
@jvican

jvican Jul 30, 2019

Member

@tgodzik This is a new key abstraction I have added to our build load infrastructure so that we can distinguish internally whether a project has been configured or it hasn't. Even if semanticdb is not supported in a project, we still consider it configured because we always add the range positions, so in checkForChange we detect configured projects that don't have semanticdb enabled and we retry the resolution when newSettings are passed.

workspaceDir: AbsolutePath,
logger: Logger
): Project = {
def enableSemanticDB(options: List[String], pluginPath: AbsolutePath): List[String] = {

This comment has been minimized.

Copy link
@jvican

jvican Jul 30, 2019

Member

Instead of adding semanticdb and range pos in one pass, we do it in two different methods so that by default we add range pos and only if we have a plugin path we enable the semanticdb.

final case object SemanticDBVersionChange extends DetectedChange

/** File name to store Metals specific settings*/
private[bloop] val settingsFileName = RelativePath("bloop.settings.json")

This comment has been minimized.

Copy link
@jvican

jvican Jul 30, 2019

Member

Nit: i relative path instead of a string is the most idiomatic way of writing file names 😄

}
}

if (version == "latest.release") {

This comment has been minimized.

Copy link
@jvican

jvican Jul 30, 2019

Member

Mostly left this as you made it @tgodzik with the exception that if the version is latest.release we only resolve it once per instance of the bloop server.

}
}

testLoad("reload if two file contents changed with same settings", Some(sameSettings)) {

This comment has been minimized.

Copy link
@jvican

jvican Jul 30, 2019

Member

I added a few more tests here.I really appreciate that you were the first one to add tons of tests here, I had almost forgotten about this test suite 😅

case Build.ReturnPreviousState =>
sys.error(s"Expected return updated state, got previous state")
case action: Build.UpdateState =>
assert(action.deleted.size == 0)

This comment has been minimized.

Copy link
@jvican

jvican Jul 30, 2019

Member

I made some tests more specific


loadBspState(workspace, projects, logger, "Metals", bloopExtraParams = extraParams) { state =>
assertNoDiff(logger.warnings.mkString(System.lineSeparator), "")
assertNoDiffInSettingsFile(

This comment has been minimized.

Copy link
@jvican

jvican Jul 30, 2019

Member

In general, I really like testing against strings and showing diffs to know what's wrong opposed to doing concrete assertions. This way, it's obvious when reading the tests what the output of workspace settings should be. Just a FYI 😄

@@ -22,11 +22,11 @@ import bloop.engine.BuildLoader

import scala.collection.JavaConverters._

class ConfigGenerationSuite481 extends ConfigGenerationSuite{
class ConfigGenerationSuite481 extends ConfigGenerationSuite {

This comment has been minimized.

Copy link
@jvican

jvican Jul 30, 2019

Member

Looks like these sources were not formatted before and my commit formatted them instead

Copy link
Contributor Author

tgodzik left a comment

Couple of early comments, I would want to go through the settings change detection again.

sys.error(
s"Resolution of module $moduleInfo failed with:${System.lineSeparator}${prettyFileErrors}"
)
try Right(fetch.run().map(f => AbsolutePath(f.toPath)).toArray)

This comment has been minimized.

Copy link
@tgodzik

tgodzik Jul 31, 2019

Author Contributor

Maybe:

Try(fetch.run().map(f => AbsolutePath(f.toPath)).toArray).toEither

This comment has been minimized.

Copy link
@jvican

jvican Jul 31, 2019

Member

Try catches everything, I now that coursier can only throw CoursierError so the try-catch only catches that

semanticDBPlugin match {
case None => projectWithRangePositions
case Some(pluginPath) =>
// Recognize 2.12.8-abdcddd as supported if 2.12.8 exists in supported versions

This comment has been minimized.

Copy link
@tgodzik

tgodzik Jul 31, 2019

Author Contributor

We don't check it here, is the comment needed?

if currentSettings.semanticDBVersion != newSettings.semanticDBVersion =>
Build.ForceReload(newSettings, List(WorkspaceSettings.SemanticDBVersionChange))
case (Some(_), Some(newSettings)) => Build.AvoidReload(Some(newSettings))
case (None, Some(newSettings)) => Build.AvoidReload(Some(newSettings))

This comment has been minimized.

Copy link
@tgodzik

tgodzik Jul 31, 2019

Author Contributor

Shouldn't we force it here?

This comment has been minimized.

Copy link
@jvican

jvican Jul 31, 2019

Member

Good catch!

)
}

val scalaVersion = instance.version

This comment has been minimized.

Copy link
@tgodzik

tgodzik Jul 31, 2019

Author Contributor

Since all paths end with metalsEnable, can't we put this into a function at least:

def enablePlugin[T](metalsEnable: Option[Path] => T): T 

a bit less of a duplciated code.

@tgodzik

This comment has been minimized.

Copy link
Contributor Author

tgodzik commented Jul 31, 2019

LGTM

@jvican

This comment has been minimized.

Copy link
Member

jvican commented Jul 31, 2019

Et voilà

@jvican jvican merged commit e307563 into scalacenter:master Jul 31, 2019
1 check failed
1 check failed
continuous-integration/drone/pr the build failed
Details
@tgodzik tgodzik deleted the tgodzik:automatically-add-semanticDB-plugin branch Jul 31, 2019
@tindzk tindzk mentioned this pull request Aug 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.