-
Notifications
You must be signed in to change notification settings - Fork 202
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
feature: Switch to using Bloop Rifle and backport all improvements #2355
Conversation
@@ -345,13 +310,14 @@ lazy val buildpressConfig = (project in file("buildpress-config")) | |||
) | |||
|
|||
lazy val buildpress = project | |||
.dependsOn(bloopgun, bloopShared, buildpressConfig) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was the sole reason that bloopgun was depended by from frontend and it was only due to usage of one method in one class (Shell) 😅
Now we don't have to release bloop rifle for 2.12 at all. (we could, but that's not needed now)
Btw. @oyvindberg after merging the changes from bloop-core I will stop backporting things there, so will be best to move bleep back to mainline Bloop. This should mostly be just changing org name. |
@@ -37,7 +37,7 @@ jobs: | |||
- uses: graalvm/setup-graalvm@v1 | |||
with: | |||
version: '22.3.0' | |||
java-version: '11' | |||
java-version: '17' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it not compatible with Java 11 anymore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need at least Java 16 for UnixDomainSocket to work, which was the biggest blocker here, but Metals already dropped JDK 8 and uses JDK 17 as default with correct release flags being set automatically. And Bloop also does add release flags to make sure the compilation is done with the correct version of Java. Plus we also download JDK automatically using coursier both in Metals and Bloop new (moved) cli project, so this should be smooth for most people especially since Scala CLI already does it.
What are the main differences between |
I think the main one are the sockets and all the work around them (making sure there is only one server running) and additional configuration that you can pass instead of having the |
The biggest difference is that bloop rifle already works in Scala Cli and we have a draft ready for metals, while bloopgun would require work on scala CLI side and fixing the launcher module to work with new sockets. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am probably not the best person to review this but LGTM
Thanks @tgodzik for taking care of this
There is no one that understands this fully aside from @alexarchambault most likely :D I tried my best to make sure everything works and looked through the code to see exactly what is going on. I will probably wait a bit to merge it, maybe do https://github.com/scalacenter/bloop/pull/2049/files first, release and then do Bloop 2.0.0 😨 |
Anyway, thanks for looking at this @adpi2 ! Very much appreciated! |
wonderful @tgodzik - I've been hoping to avoid going through the fork for a while |
82889e4
to
48f9ab6
Compare
4c4e974
to
e139218
Compare
Scala CLI uses a fork of Bloop by default and that fork has added a bunch of improvements especially connected to using domain sockets, which should work much nicer in our case. IT also solves the issue of having multiple Bloop servers on one machine, but with different users. This might sometimes lead to using 2 bloop servers on one machine, which take up memory uneccesarily. To fix that I decided to backport most of the fixes from the fork, add bloop rifle and deprecate launcher.
afcbaa2
to
6853e63
Compare
Scala CLI uses a fork of Bloop by default and that fork has added a bunch of improvements especially connected to using domain sockets, which should work much nicer in our case. IT also solves the issue of having multiple Bloop servers on one machine, but with different users.
This might sometimes lead to using 2 bloop servers on one machine, which take up memory uneccesarily. To fix that I decided to backport most of the fixes from the fork, add bloop rifle and deprecate launcher.
TODO:
There is a huge amount of changes, but in reality this stems from the fact that
cli
andbloopRifle
modules were moved from bloop-core, and the cli module actually has much more utility than previous bloopgun. On the other hand, I remove bloopgun, sockets and launcher projects. I tested it all on Metals and it seems to work correctly.In reality any large changes are copied over, I only had to do minor fixes.
I could try to separate some things out, but bloopgun stops making sense once we add bloop-rifle as I think it would start a second server.