Skip to content

Internal refactorings to prepare for Pants support#1145

Merged
olafurpg merged 40 commits intoscalameta:masterfrom
olafurpg:pre-pants
Dec 5, 2019
Merged

Internal refactorings to prepare for Pants support#1145
olafurpg merged 40 commits intoscalameta:masterfrom
olafurpg:pre-pants

Conversation

@olafurpg
Copy link
Copy Markdown
Member

@olafurpg olafurpg commented Dec 4, 2019

To make it easier to code review the changes in #935 adding Pants support, this PR adds all the necessary non-Pants related changes to unblock that PR. See individual commits for explanations.

Olafur Pall Geirsson added 7 commits December 4, 2019 15:56
Previously, the super shell had a glitch when running `slow/test` because we ran two parallel sbt states at the same time.
This module is necessary to convert between Scala/Java futures.
Even if file watchers seems a bit more reliable on Linux, they're unreliable and possible sources of flaky test failures.
@olafurpg olafurpg requested a review from tgodzik December 4, 2019 16:48
Olafur Pall Geirsson added 22 commits December 4, 2019 16:54
Previously, the mtags symbol index only supported indexing jars. Now, it also supports source directories.

This feature will be used by Pants where we don't only have `*-sources.jar` but also local directories containing source code of dependencies.
Previously, Metals would crash when reading an invalid `*-sources.jar` file (for example because
it's actually an HTML redirect file). It's not possible to catch these crashes with `NonFatal` because
`ZipError` is fatal, so this exception needs special handling.
Previously, it was only possible to run `bloopInstall` steps via a system process.
Now, with this refactoring it's possible to run a `bloopInstall` command as a normal function call.
The system process logic for build tools like sbt, Maven, Gradle has been abstracted over in a new `BloopPluginBuildTool` helper.
This class is useful outside of tests.
The file hashing feature in the directory watcher makes file watching very expensive to start in large directories.
See http://scalameta.org/metals/blog/2019/01/22/bloom-filters.html#indexing-time for more details on how expensive this step is.
We can safely disable the file hashing feature because it's fine if we get duplicate file watching notifications.
Our mtags indexer is fast enough that it's not a big deal if we end up tokenizing the same file twice.
Previously, Metals assumed that the source directories returned by BSP were good approximations for what files we want to file watch.
This model works great for build tools like sbt, Maven where the build are structured so that most of the source code lives under a "source directory".
This model doesn't work great in build tools like Pants where we have lots of small targets that have individual source files and no directories.

With "source roots", build tools like Pants can declare what directories should for example be file watched without us trying to infer from a long list of individual source files.
Previously, the file watcher supported file watching individual files but it did so in a very expensive way creating a new Thread for each watched file.
In one large workspace, the file watcher spent 7 minutes getting started.
Now, the file watcher only watches directories and build tools that use individual files can use "source roots"
if they want to declare what directories to watch.
Previously, this class only supported completing a `cancel=true` value.
Now, it's possible to complete with `cancel=false`
Previously, build tools didn't have a way for example to manually register "source root" directories.
Previously, build tools were not able to tell Metals how to handle "no build target" errors.
Now, build tools like Pants can register a handler for this situation and gracefully recover from it by
calling out to Pants to for  example re-expand source globs.
This commit should have been included with the Pants PR but it ended up coming a bit earlier.
Experiments show that this data structure can grow up to 700mb for large workspaces.
In comparison, all the other indexes use <50mb combined!
Olafur Pall Geirsson added 7 commits December 4, 2019 16:54
This method allows build tools to for example register "source roots"
For large workspaces with large classpaths this results in faster indexing.
Previously, the indexing pipeline would crash if a `*-sources.jar` did not have valid jar contents.
Copy link
Copy Markdown
Contributor

@tgodzik tgodzik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just three small comments, but overall looks good!

file.toURI.toString
}
def isBuild: Boolean =
file.filename.startsWith("BUILD")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

== BUILD ?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nevermind, found Marek's comment 😅

@olafurpg olafurpg merged commit 5b5d1aa into scalameta:master Dec 5, 2019
@olafurpg olafurpg deleted the pre-pants branch December 5, 2019 11:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants