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

Add bloopgun CLI library and binary #1032

Merged
merged 36 commits into from
Sep 27, 2019
Merged

Add bloopgun CLI library and binary #1032

merged 36 commits into from
Sep 27, 2019

Conversation

jvican
Copy link
Contributor

@jvican jvican commented Sep 12, 2019

Bloopgun is a Scala library to talk to bloop via Nailgun. It can be compiled to a binary via GraalVM and be used as a (shaded) library. The first use case is essential for users that need a fast CLI experience, the second use case is essential for build clients that want to connect to Bloop quickly and robustly.

This pull request adds a lot of infrastructure necessary to make bloopgun possible as well as bloop4j, a bsp4j wrapper to talk to bloop via BSP which will be necessary for the sbt offloading.

On the bloopgun side, there is a new innovation compared to the previous Python bloop nailgun client: bloopgun will start the server if there is no server running. This is a game changer for users of the library because it means neither users nor library authors will need to manager a server running in the background.

Given that some of this was done previously by the launcher, that logic has been removed and the launcher instead relies on bloopgun. The launcher has become less of a launcher --now it only takes care of establishing a bap connection--, so I'm considering changing its name to something else.

@jvican
Copy link
Contributor Author

jvican commented Sep 12, 2019

I'm not asking for review yet given that I'm experimenting/finishing some stuff on the bloop4j front. I'm probably going to leave this out of the minor release this week.

@jvican
Copy link
Contributor Author

jvican commented Sep 12, 2019

Missing items:

  • Reuse resolution field from a previous configuration file in sbt-bloop. Blocked by the lack of a shaded sbt-bloop. A little bit unrelated but will add it to this pull request.
  • Publish the shaded sbt-bloop instead of the normal sbt-bloop.
  • Publish the shaded bloopgun and figure out infrastructure to publish native images and generate them in other operating systems.
  • Publish the shaded launcher instead of the normal one, especially important now that we depend on coursier. Unfortunately, we cannot compile the launcher to native because we use JNA for sockets and GraalVM doesn't have support for JNA code yet.
  • Add built-in support for JVM flags to speed up compilation: -XX:UseParallelGC and -XX:MaxInlineLevel=20.

@jvican jvican force-pushed the topic/bloopgun branch 2 times, most recently from 0612abf to 0861dc4 Compare September 19, 2019 08:37
@jvican
Copy link
Contributor Author

jvican commented Sep 24, 2019

Everything is more or less in place except for the GraalVM-native infrastructure in the CI. I'm going to hold off doing that for now given that it's quite difficult. I'll focus on modifying sbt-bloop to keep the resolution modules in the configuration file and this is good to go.

The shaded plugin helps us solve several problems that existed when
loading circe dependencies in the sbt bloop plugin. These errors don't
happen again.
When `bloop server` is currently run, the current pynailgun script will
block for the duration of the server execution. This commit refactors
the codebase to imitate the same behavior, which is important for
Systemd and Brew services running `bloop server` in the background. If
we were returning immediately, these services would try to run the start
routines several times per second which would create an unnecessary
overhead on the server.
Maven Central had problems releasing snailgun-core for 0.3.0 so we
released a new version and now we upgrade to it.
According to our performance guide, running bloop with FullParallelGC
and MaxInlineLevel as well as other options is beneficial for
performance. This commit allows bloop to try to use those options and if
they fail backtracks to the initial server launch.
@jvican jvican force-pushed the topic/bloopgun branch 2 times, most recently from 54c2245 to ae7198f Compare September 26, 2019 06:42
@alexarchambault
Copy link
Contributor

alexarchambault commented Sep 26, 2019

@jvican It seems sbt-scalafix and sbt-scalafmt are around in some projects here. They depend on coursier-small, itself depending on old versions of coursier, which don't play well concurrently when shaded, hence the errors you're seeing. sbt-scalafmt and sbt-scalafix should switch to coursier-interface.

@olafurpg
Copy link
Contributor

I opened scalacenter/sbt-scalafix#43 to migrate sbt-scalafix from coursier-small. I'll do the same for sbt-scalafmt.

@jvican
Copy link
Contributor Author

jvican commented Sep 26, 2019

@alexarchambault Thanks for the pointers. We don't use sbt-scalafix but we do use sbt-scalafmt. Unfortunately, disabling sbt-scalafmt doesn't remove the error. Disabling the CoursierPlugin on the offending project did work in a previous attempt, though, for a part of the build. Any ideas what could be wrong here? I tried to see where another coursier version gets pulled in but I couldn't find it.

I should say that /root/.coursier is a symlink and it has been working just fine previously. After the upgrade to 2.0.0-RC3-3, things have started to go wrong, so I really wonder what's happening here 🤔

@olafurpg
Copy link
Contributor

olafurpg commented Sep 26, 2019

PR migrating scalafmt to coursier/interface scalameta/scalafmt#1511

JNA checks fail on Windows CI. Let's just assume all clients are
non-interactive, it should be OK since Bloop doesn't do anything fancy
based on this information.
When downgrading, `toShadeJars` didn't work as expected. Therefore, we
reimplement our own `toShadeJars`, which gives us a lot of power to
filter out whatever jars we don't want to shade.
@jvican
Copy link
Contributor Author

jvican commented Sep 26, 2019

Alright, I've run out of hacks. I downgraded coursier and the issue persists, this looks like a bug inside sbt-shading. I'm going to reimplement the functionality I care about in my build and remove the dependency of sbt-shading on coursier by just populating toShadeJars with files from dependencyClasspath and then filtering which jars should be shaded.

@alexarchambault
Copy link
Contributor

@jvican Did you check sbt-declarative-packaging (via this)?

@alexarchambault
Copy link
Contributor

but yeah, doing that manually might actually be more straightforward

Depending on coursier's sbt-shading is causing us lots of headaches in
the CI where we get cryptic errors we haven't found a solution for.

This patch implements the important bits of sbt-shading inside the build
for our very particular use cases.
@olafurpg
Copy link
Contributor

sbt-scalafmt upgrade to a no-coursier-small dependency scalameta/sbt-scalafmt#50

In preparation for the offloading compilation feature in sbt, we need to
make sure we never lose the information in the resolutions field as it
is important for the good behavior of Metals.

Given a build that was exported with source information from the Metals
interface, we want sbt-bloop to continue to preserve that information if
it runs `bloopGenerate` on its own before offloading compilation to sbt.
@jvican
Copy link
Contributor Author

jvican commented Sep 26, 2019

The error here seems to be caused by a coursier version higher than 1.1.0-M3-2. My intuition is that coursier doesn't like the fact that the coursier cache is a symlink in those versions, while in the current version seems to work OK.

I ended up anyway implementing sbt-shading inside bloop, it makes it easier to modify what jars should be shaded and which should be not. This is just a temporary workaround, in the future I'd like to have an independent sbt-plugin built on top of coursier's sbt-shading. For now, we're taking this shortcut.

CI seems happy after the changes I did. This is good to go, so I'm merging as soon as the CI is green.

Hopefully this will address some race conditions I've been seeing in bsp
diagnostics reports where the output of some diagnostics is
intermingled.
@jvican jvican merged commit 71ebe9a into master Sep 27, 2019
@olafurpg
Copy link
Contributor

🥳👏👏👏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants