Provide ocamlformat-rpc through the ocamlformat package

[gpetiot]

Reverts some bits of #2022

cc @rgrinberg
cc @tmattio since you opened #2022

[rgrinberg]

The dune file still lists ocamlformat as a library dependency. Where would that dependency come from if this package is removed?

[gpetiot]

That's because #2022 renamed ocamlformat_lib to ocamlformat, but this dependency was always here.

[Julow]

I think this was necessary. Both the library and the binary are part of the ocamlformat package, Dune won't build if it isn't installed first:

$ dune build -p ocamlformat-rpc-lib,ocamlformat-rpc
File "lib-rpc-server/dune", line 4, characters 12-23:
4 |  (libraries ocamlformat ocamlformat-rpc-lib))
                ^^^^^^^^^^^
Error: Library "ocamlformat" not found.

For this to work, the library should be moved to its own package.
What about building the RPC server as part of the main package ? This way people can forget about whether they want the RPC or the binary and just have both.

[tmattio]

What about building the RPC server as part of the main package ? This way people can forget about whether they want the RPC or the binary and just have both.

This would also improve the UX with ocaml-lsp. The fact that you need to install both ocamlformat and ocamlformat-rpc as a dependency is a little awkward.

[gpetiot]

Is this what you had in mind? I'm just worried about the .mld file I had to rename to avoid having 2 index.mld in a single package, I hope it will properly show up in the v3 online documentation.

[Julow]

It doesn't build because now the ocamlformat packages requires ocamlformat-rpc-lib:

$ dune build -p ocamlformat
File "lib-rpc-server/dune", line 4, characters 24-43:
4 |  (libraries ocamlformat ocamlformat-rpc-lib))
                            ^^^^^^^^^^^^^^^^^^^
Error: Library "ocamlformat-rpc-lib" not found.

This dependency was useful to be sure the encoder/decoder functions are in sync but is not fundamentally required and brings a lot of dangerous code into the server impl. It should be possible to copy the few needed functions into the server instead.

The documentation doesn't work because there's no link to it from the index.mld. You could move all the doc into a single place to make things easier.

[gpetiot]

Would it be such a bad thing to add a dependency to ocamlformat-rpc-lib in ocamlformat instead of duplicating code?

[Julow]

Would it be such a bad thing to add a dependency to ocamlformat-rpc-lib in ocamlformat instead of duplicating code?

I think it's not that much duplication, given that this dependency doesn't make much sense in the first place. The rpc-lib API doesn't make sense for a server and is here just to share a few decode functions (that don't need to be shared anyway).

[gpetiot]

The rpc-lib API doesn't make sense for a server and is here just to share a few decode functions (that don't need to be shared anyway).

I think if makes sense to avoid duplicating the whole ocamlformat_rpc_lib file, since the whole is functorized over the IO module now we can't just the the bits we need. Or maybe we need to redesign the whole thing again.

[Julow]

Here's a way to remove the dependency but still share the serialization/deserialization code: gpetiot#12
This approach requires changing the API of lib-rpc but has the advantage of not linking unrelated code into the server. The new API should be slightly cleaner and still allow users to implement a server or a client.

[gpetiot]

@Julow thanks, I agree with your changes!

@rgrinberg @ulugbekna @tmattio @panglesd let us know if everything is alright for you then we can merge.

[panglesd]

Looks good to me!

[rgrinberg]

I won't be able to review, sorry. But from my perspective, if it's possible to use the rpc lib without depending on anything else, it's enough.

[Julow]

If the API changes to lib-rpc are not an issue, this can be merged.