Allow running tests concurrently with watch server

[gridbugs]

No description provided.

[Leonidas-from-XIV]

Maybe this loop should have another abort condition? We probably don't want it to loop forever as this might block other software waiting on dune runtest forever if the connection can't be made.

[gridbugs]

I agree that would be nice. I had a go implementing this (specifically having this function timeout after some period) and it's quite straightforward however for such a feature I'd want to add a cram test, and doing so would require adding an argument to various commands to allow the user to set the timeout, or hardcoding a specific non-infinite timeout into the code. The former would be too big a change for this PR I think, and the latter would change the behaviour of dune which I'm hesitant to do.

I'll note that the code in this file was mostly just moved from bin/rpc/build.ml and not written specifically for this PR.

[ElectreAAS]

Reading your work in dune_rpc_impl/server.ml, I get the impression that this PR doesn't use the fact that @runtest is a build target, and that dune build @runtest will send a build request to the rpc server with that particular target, is that correct?

cc @shonfeder you were interested in the answer as well

For context, I am asking this question to better implement #11958

[gridbugs]

Reading your work in dune_rpc_impl/server.ml, I get the impression that this PR doesn't use the fact that @runtest is a build target, and that dune build @runtest will send a build request to the rpc server with that particular target, is that correct?

cc @shonfeder you were interested in the answer as well

For context, I am asking this question to better implement #11958

That's correct. dune runtest and dune build @runtest are not completely equivalent, as evidenced by the fact that there's additional logic inside the implementation of dune runtest command, mostly for handling paths.

That said, there is an oversight which I made that I'll address before merging, which is that I believed that the only way to evaluate a memo is by taking the global lock and running the build system, but I've recently learnt that this is not the case as Memo.run returns a fiber that can be scheduled without invoking the build system. I'm not sure if all memos can be safely evaluated in this way since some memos have side effects within _build that mean they should only be evaluated while holding the global lock.

I'll see if I can take advantage of this fact to simplify this PR.

[gridbugs]

I spent some time today trying to take advantage of the @runtest build target alias, but some of the path processing done by dune runtest requires information only available when the build system runs (the list of build contexts). Therefore I think we need to keep this in its current state where runtest is a separate rpc method from build.

[gridbugs]

@ElectreAAS I've added you as a reviewer as I think the other reviewers may be short on time at the moment.

[gridbugs]

That failing test is a problem installing deps with apt, so it's unrelated to this change.

[Alizter]

@gridbugs I don't think we need a dedicated runtest method for this. dune runtest may do some path processing, but at the end of all of that, it only ever builds an alias. At the moment, cram tests are processed to their corresponding aliases, and directories are passed as runtest in that directory. This method of searching for and executing tests will be extended in #11980.

I think what we need is a specific RPC call for getting cram tests in a given source directory. We don't need to know about contexts on our side. If a user wishes to build the alias _build/mycontext/runtest we can pass that path directly. If a user wants to build the alias ./runtest, then the build server will already build this in all contexts.

This cram test finder could be made a bit smarter since we don't even need to return all the tests. We should instead have something like

val find_cram_test
  : dir:Path.Source.t ->
    [ `Filename of string | `Name of string ] ->
    (Cram_test.t * [ `Enabled | `Disabled ]) option Fiber.t

That way all dune runtest would have to do is process some paths, query tests using find_cram_test and then send specific alias requests to build.

I'm a little hesitant to add a dedicated runtest method since it is just a build with some searching before, and it will make it trickier to extend in the future. Whereas searching for cram tests is very specific and unlikely to change.

[gridbugs]

Thanks @Alizter! Can you clarify why it's necessary to enumerate the cram tests in a directory using an RPC method. Is there a possibility that it will race with the build system in some way? Ideally we could just make it a fiber or memo that resolves entirely within the client, but transforming the current logic doesn't seem straightforward (but it's not immediately clear to me why).

[shonfeder]

Let's discuss with @ElectreAAS and @gridbugs how best to move this forward. Ali has communicated that his comments here are not blocking.

[ElectreAAS]

Superseded by #12473