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

Switch to to using Bloop Launcher #1004

Merged
merged 6 commits into from Oct 21, 2019

Conversation

@tgodzik
Copy link
Collaborator

tgodzik commented Oct 19, 2019

Previously, we were connecting via running bloop with the bsp command, which required Python to start Bloop if it isn't exist.

Now, we connect using the Bloop Launcher, which uses bloopgun underneath, which doesn't require Python and is implemented inside Bloop project itself.

tgodzik added 2 commits Oct 18, 2019
…, which required Python to start Bloop if it isn't exist.

Now, we connect using the Bloop Launcher, which uses bloopgun underneath, which doesn't require python.
@tgodzik

This comment has been minimized.

Copy link
Collaborator Author

tgodzik commented Oct 19, 2019

Fixed the test broken by merging the two yesterday's PRs

Copy link
Member

olafurpg left a comment

This is very exciting! Lovely to see so much code deleted and I believe this is a good first step towards improving the build server disconnections people have reporters about.

I wonder if the test.sh script is still needed for CI? If I understand correctly, the launcher will do it for us.

LGTM 👍

build.sbt Outdated Show resolved Hide resolved
build.sbt Outdated
@@ -248,6 +248,8 @@ lazy val metals = project
// for BSP
"org.scala-sbt.ipcsocket" % "ipcsocket" % "1.0.0",
"ch.epfl.scala" % "bsp4j" % V.bsp,
"ch.epfl.scala" % "bloop-launcher_2.12" % V.bloop,
"ch.epfl.scala" % "bloop-launcher-core_2.12" % V.bloop,

This comment has been minimized.

Copy link
@olafurpg

olafurpg Oct 19, 2019

Member

Do we need both the launcher and launcher-core?

This comment has been minimized.

Copy link
@tgodzik

tgodzik Oct 19, 2019

Author Collaborator

I think so, I had ClassMissingException when I didn't include core.

This comment has been minimized.

Copy link
@jvican

jvican Oct 20, 2019

Collaborator

You should only use bloop-launcher, bloop-launcher-core contains the unshaded classes. What ClassNotFoundException did you get?

This comment has been minimized.

Copy link
@tgodzik

tgodzik Oct 20, 2019

Author Collaborator

I removed it and currently it just freezes:

INFO  time: initialize in 0.2s
Starting the bsp launcher for bloop...

but previously it was I think UnixDomainSocket - for sure a class from sockets

This comment has been minimized.

Copy link
@olafurpg

olafurpg Oct 20, 2019

Member

What happens if you only use bloop-launcher-core? I don't think we need the shaded classes, it would be good to share the coursier dependency for example

This comment has been minimized.

Copy link
@olafurpg

olafurpg Oct 20, 2019

Member

My vote goes to use bloop-launcher-core and deal with binary conflicts like we do with any other 3rd party dep

This comment has been minimized.

Copy link
@tgodzik

tgodzik Oct 21, 2019

Author Collaborator

I agree, I think it's fine from Metals perspective.

This comment has been minimized.

Copy link
@jvican

jvican Oct 21, 2019

Collaborator

Sounds good but keep in mind that you will then need to manage any dependency breakage that happens upstream. Not a big deal but worth thinking about

This comment has been minimized.

Copy link
@olafurpg

olafurpg Oct 21, 2019

Member

Maybe we should start using https://github.com/scalacenter/sbt-missinglink 🤔

This comment has been minimized.

Copy link
@gabro
@tgodzik

This comment has been minimized.

Copy link
Collaborator Author

tgodzik commented Oct 19, 2019

I wonder if the test.sh script is still needed for CI? If I understand correctly, the launcher will do it for us.

It should work, I will remove it and test.

@tgodzik tgodzik requested a review from jvican Oct 20, 2019
@tgodzik tgodzik force-pushed the tgodzik:switch-to-bloop-launcher branch from 3fe7080 to 1accc0a Oct 20, 2019
Copy link
Member

gabro left a comment

This looks 😍

@tgodzik I've noticed that BloopServers does not take MetalsServerConfig as input anymore. Two questions:

  • was there any bloop-specific parameter in there that we can remove (e.g. bloopProtocol)?
  • was there any parameter that we should respect instead (e.g. verbose)?
Copy link
Collaborator

jvican left a comment

Yay, excited to see this happening @tgodzik! 👍 Left a comment regarding the bloopgun artefact that should be used, otherwise LGTM 💯

@tgodzik

This comment has been minimized.

Copy link
Collaborator Author

tgodzik commented Oct 20, 2019

This looks

@tgodzik I've noticed that BloopServers does not take MetalsServerConfig as input anymore. Two questions:

  • was there any bloop-specific parameter in there that we can remove (e.g. bloopProtocol)?
  • was there any parameter that we should respect instead (e.g. verbose)?

Removed bloopProtocol and some more no longer needed classes. However, not exactly sure how to add verbose via launcher - there is no room for CLI options there.

@jvican
jvican approved these changes Oct 21, 2019
@gabro
gabro approved these changes Oct 21, 2019
Copy link
Member

gabro left a comment

👍

…in the code"

This reverts commit 1accc0a.
@tgodzik

This comment has been minimized.

Copy link
Collaborator Author

tgodzik commented Oct 21, 2019

Looks like the tests started timing out again - I reverted the commit removing the script.

@tgodzik tgodzik merged commit 0881fff into scalameta:master Oct 21, 2019
3 checks passed
3 checks passed
build
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
scalameta.metals Build #20191021.2 succeeded
Details
@tgodzik tgodzik deleted the tgodzik:switch-to-bloop-launcher branch Oct 21, 2019
@olafurpg

This comment has been minimized.

Copy link
Member

olafurpg commented Oct 21, 2019

🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.