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

Internal refactorings to prepare for Pants support #1145

merged 40 commits into from Dec 5, 2019


Copy link

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.

olafurpg added 7 commits Dec 4, 2019
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 Dec 4, 2019
olafurpg added 22 commits Dec 4, 2019
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 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!
olafurpg added 7 commits Dec 4, 2019
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.
@olafurpg olafurpg force-pushed the olafurpg:pre-pants branch from da5fa9a to d7770f1 Dec 4, 2019
tgodzik approved these changes Dec 5, 2019
Copy link

tgodzik left a comment

Just three small comments, but overall looks good!

@@ -99,12 +99,17 @@ trait MtagsEnrichments {
case None =>
def isBuild: Boolean =

This comment has been minimized.

Copy link

tgodzik Dec 5, 2019


== BUILD ?

This comment has been minimized.

Copy link

tgodzik Dec 5, 2019


Nevermind, found Marek's comment 😅

@olafurpg olafurpg merged commit 5b5d1aa into scalameta:master Dec 5, 2019
10 checks passed
10 checks passed
ubuntu-latest tests
windows-latest tests
macOS-latest tests
Sbt integration
Maven integration
Gradle integration
Mill integration
Slow tests
Scala cross tests
@olafurpg olafurpg deleted the olafurpg:pre-pants branch Dec 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
None yet
4 participants
You can’t perform that action at this time.