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.

Sign up for GitHub

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

Jump to bottom

Reconnect to BSP server upon buildTarget/didChange #3145

Merged
merged 1 commit into from Sep 30, 2021
Merged

Reconnect to BSP server upon buildTarget/didChange #3145

merged 1 commit into from Sep 30, 2021

Conversation

alexarchambault
Copy link
Contributor

No description provided.

@alexarchambault

This comment has been minimized.

Sign in to view
@alexarchambault alexarchambault force-pushed the bsp-build-target-did-change branch 2 times, most recently from dfc31c9 to a9b49e1 Compare September 20, 2021 17:09
ckipp01
ckipp01 reviewed Sep 20, 2021
View reviewed changes
Copy link
Member

@ckipp01 ckipp01 left a comment

Choose a reason for hiding this comment

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

Thanks for this @alexarchambault!

Just a couple clarifying questions right away.

metals/src/main/scala/scala/meta/internal/bsp/MetalsBspException.scala Outdated Show resolved Hide resolved
tests/unit/src/main/scala/tests/BuildServerInitializer.scala Outdated Show resolved Hide resolved
tests/unit/src/test/scala/tests/BspBuildChangedSuite.scala Outdated Show resolved Hide resolved
@alexarchambault alexarchambault force-pushed the bsp-build-target-did-change branch 2 times, most recently from 759167e to 164af92 Compare September 21, 2021 08:04
@alexarchambault

This comment has been minimized.

Sign in to view
@ckipp01
Copy link
Member

ckipp01 commented Sep 23, 2021 •
edited

Also taking a quick step back, sorry I should have asked this first. When a buildTarget/didChange even is received, why are we reconnecting to the BSP server rather than forcing a reload? I sort of assumed that in this scenario where a change was detected in a build target that the reload would do basically what we want?

The reload request is sent from the client to instruct the build server to reload the build configuration. This request should be supported by build tools that keep their state in memory. If the reload request returns with an error, it's expected that other requests respond with the previously known "good" state.

Which sort of leads me to another question. I know behind the scenes scala-cli is offloading to Bloop if I understand it correctly, so maybe reload doesn't make sense then. Is scala-cli keeping the state in memory, or just utilizing the .bloop/ files?

@alexarchambault
Copy link
Contributor Author

Both Ammonite and Scala CLI have buildTarget/didChange support, but no reload support (they always just pick the new configuration, so they always reload in some sense).

But I think it's a different problem. The changes here are about making Metals aware of the new / updated build targets, not about asking the build tool to update how it sees them.

@alexarchambault
Copy link
Contributor Author

alexarchambault commented Sep 24, 2021 •
edited

Scala CLI always makes sure the .bloop/*.json file it writes is in sync with its in-memory representation. (I mean, it does so right before asking Bloop to compile.) Maybe it should ask Bloop to reload actually 🤔 (although I never saw Bloop not reloading freshly updated JSON files on its own).

@alexarchambault alexarchambault force-pushed the bsp-build-target-did-change branch 2 times, most recently from 9332eb1 to eddfa89 Compare September 28, 2021 08:56
@alexarchambault
Copy link
Contributor Author

alexarchambault commented Sep 28, 2021 •
edited

So I scraped the Scala CLI stuff from the tests. (The test is now more "unitary", even though I prefer the former one, which was testing a more realistic scenario…)

I believe the CI job failure is unrelated to the changes of this PR.

@ckipp01
Copy link
Member

ckipp01 commented Sep 29, 2021

Maybe it should ask Bloop to reload actually 🤔 (although I never saw Bloop not reloading freshly updated JSON files on its own).

Off the top of my head, I don't think Bloop is actually a reload provider.

ckipp01
ckipp01 approved these changes Sep 29, 2021
View reviewed changes
Copy link
Member

@ckipp01 ckipp01 left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏼

@alexarchambault
Copy link
Contributor Author

(Just rebased on the latest main)

tgodzik
tgodzik approved these changes Sep 30, 2021
View reviewed changes
Copy link
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.

LGTM!

@tgodzik tgodzik merged commit dc12a17 into scalameta:main Sep 30, 2021
@alexarchambault alexarchambault deleted the bsp-build-target-did-change branch September 30, 2021 14:30
@alexarchambault alexarchambault mentioned this pull request Oct 6, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tgodzik tgodzik tgodzik approved these changes

@ckipp01 ckipp01 ckipp01 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@alexarchambault @ckipp01 @tgodzik