Allow dev tools to be installed while watch server runs #11808
Conversation
voodoos
left a comment
voodoos
left a comment
There was a problem hiding this comment.
Exposing Build_cmd.build_via_rpc_server seems very useful !
However I think we should be a bit more ambitious than using it only for ocamllsp server: we also need it for ocamlformat and probably other tools. Why not make it generic for all calls to dune tools exec ?
In the future we probably also want dune build ... commands to be able to piggy-back on an existing rpc server. This would allow editors to request up-to-date indexes for example. But that's for another PR.
Yeh I definitely agree that there should be a more general mechanism for building via rpc rather than duplicating code between all the commands that build targets. I've intentionally not generalized the mechanism for the first few commands I've changed so I can get a better understanding of how to generalize it. This will come in a later PR, along with adding rpc support to the remaining commands that build targets. |
gridbugs
force-pushed
the
dev-tool-install-concurrent-with-watch-mode
branch
from
4a9335e to
0299d4e
Compare
Actually I've changed my mind about this. I'll add a general mechanism for installing dev tools via rpc to this PR since it's quite simple. As I add support for other commands ( |
gridbugs
force-pushed
the
dev-tool-install-concurrent-with-watch-mode
branch
2 times, most recently
from
faabe2f to
e6ce36a
Compare
This is already somewhat the case! Simple usage of |
gridbugs
force-pushed
the
dev-tool-install-concurrent-with-watch-mode
branch
2 times, most recently
from
fde0fac to
de5aa34
Compare
|
Applied feedback and added a test that I forgot to check in last week. |
gridbugs
force-pushed
the
dev-tool-install-concurrent-with-watch-mode
branch
2 times, most recently
from
a092404 to
d7e90c4
Compare
gridbugs
changed the title
gridbugs
force-pushed
the
dev-tool-install-concurrent-with-watch-mode
branch
from
d7e90c4 to
53d9f6c
Compare
|
@voodoos let me know if you have any more feedback before I merge this |
Leonidas-from-XIV
left a comment
Leonidas-from-XIV
left a comment
There was a problem hiding this comment.
Some minor comments only.
| val lock_ocamlformat : unit -> unit Memo.t | ||
| val lock_odoc : unit -> unit Memo.t | ||
| val lock_ocamllsp : unit -> unit Memo.t | ||
| val lock_dev_tool : Dune_pkg.Dev_tool.t -> unit Memo.t |
There was a problem hiding this comment.
Maybe it would make sense to replace all the other lock_* functions at this point, to avoid having two different ways to do the same thing.
bin/tools/tools_common.ml
Outdated
|
|
||
| let build_dev_tool_directly common dev_tool = | ||
| let exe_path = dev_tool_exe_path dev_tool in | ||
| if Path.exists exe_path |
There was a problem hiding this comment.
I wonder whether it would make sense to make sure that this is the right version, otherwise updating dev tools might not be possible?
(That said, I don't have a good idea how to record a dependency from the version to the built binary and it's possibly good enough for now)
There was a problem hiding this comment.
Oh good catch. The logic for building ocamllsp originally didn't have this check, and that's possibly why. I'll remove the check for now to prevent accidentally introducing a problem where ocamllsp isn't rebuilt when it should be. This will add a small amount of latency when invoking other dev tools I doubt it will be a big deal.
If `dune tools exec <tool>` is executed while the watch server is running, dune will now build the tool in the instance of dune where the watch server is running rather than failing due to the build directory being locked. This change also unifies some of the logic for building dev tools to reduce duplication. Signed-off-by: Stephen Sherratt <stephen@sherra.tt>
gridbugs
force-pushed
the
dev-tool-install-concurrent-with-watch-mode
branch
from
53d9f6c to
9df61d9
Compare
If `dune tools exec <tool>` is executed while the watch server is running, dune will now build the tool in the instance of dune where the watch server is running rather than failing due to the build directory being locked. This change also unifies some of the logic for building dev tools to reduce duplication. Signed-off-by: Stephen Sherratt <stephen@sherra.tt>
3 participants
If
dune tools exec ocamllspis executed while the watch server is running, dune will now build ocamllsp in the instance of dune where the watch server is running rather than failing due to the build directory being locked.