Skip to content

Introduce workspace/reload request#135

Merged
jastice merged 2 commits intomasterfrom
build-reload-request
Sep 14, 2020
Merged

Introduce workspace/reload request#135
jastice merged 2 commits intomasterfrom
build-reload-request

Conversation

@jastice
Copy link
Copy Markdown
Member

@jastice jastice commented Sep 10, 2020 •
edited
Loading

The reload request instructs 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.

This request is motivated by tools such as sbt, which maintain an in-memory state of the build, where reloading the build on-demand is not considered practical.

Prompted by discussion in sbt/sbt#5783
Solves #134

cc @adpi2

@jastice jastice requested review from jvican and olafurpg September 10, 2020 15:26
@jastice
Copy link
Copy Markdown
Member Author

jastice commented Sep 10, 2020

Discussion point: should this be build/reload or workspace/reload?

build/reload may be better than workspace/reload because it is part of the server lifetime category with:

  • build/initialize
  • build/shutdown
  • builld/exit

@jastice
Introduce build/reload request
efd58b3 
The `reload` request instructs 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.

This request is motivated by tools such as sbt, which maintain an in-memory state of the build, where reloading the build on-demand is not considered practical.
@jastice jastice force-pushed the build-reload-request branch from 487b1f7 to efd58b3 Compare September 10, 2020 15:32
@jvican
Copy link
Copy Markdown
Contributor

jvican commented Sep 10, 2020

I'll make a closer review at this soon, but I think it should be workspace/reload. Build servers can support N build workspaces, whereas build/reload assumes a build server supports only one workspace.

@adpi2
Copy link
Copy Markdown
Member

adpi2 commented Sep 10, 2020

You are right @jvican. It makes sense since we have the uri field in workspace/buildTargets.

So I guess we should rename it workspace/reload and have an uri field in the params.

Sorry @jastice for misleading you on this.

@jastice
Copy link
Copy Markdown
Member Author

jastice commented Sep 10, 2020

So I guess we should rename it workspace/reload and have an uri field in the params.

True, though the workspace is implicitly provided via the working directory of the build server, so we probably don't need a uri field

@adpi2
Copy link
Copy Markdown
Member

adpi2 commented Sep 10, 2020 •
edited
Loading

I think we do because some build server does not "live" in the same working directory. Bloop, for instance, can manage many workspaces and he wants to know which one to reload.

@jastice
Copy link
Copy Markdown
Member Author

jastice commented Sep 10, 2020

I think we do because some build server does not "live" in the same working directory. Bloop, for instance, can manage many workspaces and he wants to know which one to reload.

Yes, but the server that the clients speaks to is always started in the workspace root directory. Note we don't have any root uri parameter for workspace/buildTargets either.

@adpi2
Copy link
Copy Markdown
Member

adpi2 commented Sep 10, 2020

Oh yes you are right. Thank you for the clarification.

@adpi2 adpi2 mentioned this pull request Sep 11, 2020
Merged
@adpi2
Copy link
Copy Markdown
Member

adpi2 commented Sep 11, 2020

I am ready to submit the implementation of workspace/reload in sbt as soon as this one is merged!

See sbt/sbt#5838

olafurpg
olafurpg approved these changes Sep 13, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@olafurpg olafurpg left a comment

Choose a reason for hiding this comment

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

LGTM 👍 I'm open for both build/reload and workspace/reload, the comment is just a nit, feel free to ignore

@jastice
Introduce workspace/reload request
347db41 
The `reload` request instructs 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.

This request is motivated by tools such as sbt, which maintain an in-memory state of the build, where reloading the build on-demand is not considered practical.
@jastice
Copy link
Copy Markdown
Member Author

jastice commented Sep 14, 2020

updated to workspace/reload

jvican
jvican approved these changes Sep 14, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@jvican jvican left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! 👍

@jastice jastice merged commit a185071 into master Sep 14, 2020
@jastice jastice changed the title Introduce build/reload request Introduce workspace/reload request Sep 14, 2020
@jastice jastice linked an issue Oct 7, 2020 that may be closed by this pull request
Add build/reload request #134
Closed
@ckipp01 ckipp01 mentioned this pull request Oct 26, 2020
Closed
@agluszak agluszak deleted the build-reload-request branch July 17, 2023 18:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@olafurpg olafurpg olafurpg approved these changes

@jvican jvican jvican 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.

Add build/reload request

4 participants

@jastice @jvican @adpi2 @olafurpg