Configurable Unix system command

[3Rafal]

As described in ocaml/ocaml-lsp#1052

There was already a discussion about introducing Spawn dependency to replace Sys.command here: #1420

[3Rafal]

@rgrinberg , can you specify the ocaml-lsp cases in which this patch is necessary?
We're changing Sys.command which uses system() call, for a Spawn.spawn.
As I understand it, both solutions rely on fork/exec.

[rgrinberg]

Note that it's not about avoiding fork/exec (although i'll take it when it's possible). It's for:

1. Having access to the spawned pid for cancellation
2. Avoiding the shell and not relying on it for stdin/stdout/stderr

[rgrinberg]

Btw, this fix isn't bringing us to an ideal long term situation. We don't want merlin imposing on lsp on how to lunch processes, nor do we want lsp imposing on merlin in the same way. Something closer to what we want would be an API for running preprocessors. E.g:

val pp : cwd:string -> prog:string -> args:string list -> (string, int) result

[voodoos]

Btw, this fix isn't bringing us to an ideal long term situation. We don't want merlin imposing on lsp on how to lunch processes, nor do we want lsp imposing on merlin in the same way. Something closer to what we want would be an API for running preprocessors. E.g:

val pp : cwd:string -> prog:string -> args:string list -> (string, int) result

We could make it customizable in https://github.com/ocaml/merlin/blob/master/src/utils/lib_config.mli

It's not the most elegant solution but it might provide the flexibility we need until a more involved refactor of "merlin-as-a-lib" is done.

[3Rafal]

@rgrinberg , I'm not sure that I follow.

If we create the API with following function:

val pp : cwd:string -> prog:string -> args:string list -> (string, int) result

then how will we handle cancelation? I assume that returned int stands for exit code and PID is not exposed.

[rgrinberg]

The implementation of that function on the lsp side would register the spawned pid somewhere and then kill it once cancellation is called. By the way, the error code should be a bit more accurate:

val pp : cwd:string -> prog:string -> args:string list -> (string, [`Finish of int | `Cancelled ]) result

[3Rafal]

@voodoos , @rgrinberg
I have created an extension point that fits the "standard" Merlin implementation and the one proposed by Rudi.
The interface type is simplified and mimicked after Sys.command. I think it provides enough flexibility for ocaml-lsp to implement a cancelation mechanism, and does not require significant changes on Merlin part.

[voodoos]

Looks good to me !

[3Rafal]

Ok, I'll just rebase it on current main branch and provide a changelog :-)

[voodoos]

Ok, I'll just rebase it on current main branch and provide a changelog :-)

👍 also we will wait for @rgrinberg double-check before merging :-)

[rgrinberg]

This interface is problematic because it relies on the shell to run the pp/ppx commands. We want the drop the dependency on the shell whenever possible.

Personally, I would prefer an API that is even higher level. For example:

val pp : ast:string -> [`Impl | `Intf] -> source_path:string -> prog:string -> args:string list -> [`Pp of string | `Marshal of string]

Also I find the reliance on error code to signal errors to be a poor choice as well.

I suppose it's good enough to solve the immediate problem, but I wouldn't want to be stuck with this interface.

Is there a reason why we're in a rush to fix this issue?

[3Rafal]

@rgrinberg

Is there a reason why we're in a rush to fix this issue?

The primary reason for prioritizing this issue is the potential for reducing maintenance costs on both projects. By upstreaming the merlin fork, we can simplify the overall development process and minimize duplicated efforts.

Moreover, this issue has been identified as a significant factor blocking adoption for some users in the OCaml community.

The proposed Pull Request offers a quick and efficient way to upstream this patch. This solution is not the final version. We intend to further refine the integration between ocaml-lsp and merlin once the initial version has been upstreamed.

[voodoos]

@rgrinberg, about the return type: (string, [`Finish of int | `Cancelled ]) result:

Adding a `Canceled case without properly handling it on Merlin's side wouldn't make much sense to me.
Right now, if a PPX fails Merlin simply keeps going down the pipeline with the non-preprocessed source.

We should at some point (and probably sooner than later) add cancellation support to the Mpipeline itself, but I think this is out-of-scope for this PR that really simply aims to provide enough flexibility to support current ocaml-lsp usage without additional patches.

[rgrinberg]

Sure, I was talking about the ideal API that we should be looking towards. Of course merlin should handle cancellation in a smarter way than just continuing with the unpreprocessed ast.

[voodoos]

@3Rafal could you add a changelog entry ? I will try to review by the end of the week.

[3Rafal]

@voodoos , done. :)

[voodoos]

I had a go at the latest changes we discussed with Rafal. This lead me to (re)-discover the source of our difficulties: in configuration, a ppx and its arguments are passed in one block to Merlin which makes it hard to distinguished the program from it's arguments. Additionally, when the .merlin file has been hand-crafted it can rely on various different quoting styles and even shell expansions / embedded commands. (free visit to the zoo: https://github.com/search?q=path%3A%22.merlin%22+FLG+-ppx&type=code&p=1).

That means that unless we decide to parse these and unquote them (which is something we really don't want to do) there is no way for us to use sane quotation functions like Filename.quote_command, or drop the call to the shell, if we want to ensure Merlin compatibility with the whole Zoo.

I documented the hook exposed in Lib_config with all that should be taken care of by implementers:

merlin/src/utils/lib_config.mli

Lines 22 to 54 in 5d09af9

	(** [set_run_in_directory] sets an implementation for spawning external
	programs. This is used by Merlin to spawn preprocessors and ppxes. For
	compatibility reasons, there are currently some limitations to how this
	should be implemented:
	- Implementation should expect [prog] to be already quoted and contain
	arguments. This is due to how ppx configuration is passed to Merlin. In
	order to prepare a future transition to more sane argument passing, the
	implementation can look at the [prog_is_quoted] argument to know if it
	is actually safe to quote the command normally (using
	[Filename.quote_command] for example).
	- [prog] might contain shell expansions, command substitutions etc. It
	should therefore be ran under a shell for maximum compatibility. However
	this should never happen when the configuration is generated by Dune.
	- Programs runned by this function should never output on stdout since it
	is the channel used by Merlin to communicate with the editor. One way to
	enforce that is to redirect stdout to stderr.
	- As of today Merlin handles the [`Cancelled] return case identically as
	other error codes. *)
	val set_run_in_directory
	: (prog:string
	-> prog_is_quoted:bool
	-> args:string list
	-> cwd:string
	-> ?stdin:string
	-> ?stdout:string
	-> ?stderr:string
	-> unit
	-> [ `Finished of int | `Cancelled ])
	-> unit

@rgrinberg We should at some point add a new directive to the configuration protocol to finally pass ppx configuration in a clean way when using Dune.