mbox series

[bug#64209,00/14] Add ocaml-lsp-server and its dependencies

Message ID cover.1687361650.git.benjamin@uvy.fr
Headers show
Series Add ocaml-lsp-server and its dependencies | expand

Message

Benjamin June 21, 2023, 3:45 p.m. UTC 
Hello,

These patches add ocaml-lsp-server package.

A lot of dependencies were required, but for most of them, this is just 
a dune subpackage.
For ocaml-dune-build-info, note that I included the patch from [0]
and I used the same technique for other dune packages.

The latest version of ocaml-lsp-server depends on ocaml-merlin-lib.
I added a compatible version as a new package instead of upgrading
current ocaml-merlin-lib as I was not sure if it would be safe to do so.

Finally, the version 1.6.1 is supporting ocaml 5.0 and 5.1 [1] but I only
tested for 5.0.
Also, as 4.14 is not compatible with it, I only made public version 5.0
of it. I do not no either if was the good thing to be done.

I tested lsp server localy and it has been working fine with neovim for
my usage.

[0] https://issues.guix.gnu.org/63947
[1] https://github.com/ocaml/ocaml-lsp/blob/master/CHANGES.md#1161


Benjamin (14):
  gnu: Fix ocaml-dune-build-info
  gnu: Add ocaml-dune-rpc.
  gnu: Add ocaml-chrome-trace.
  gnu: Add ocaml-dyn.
  gnu: Add ocaml-stdune.
  gnu: Add ocaml-fiber.
  gnu: Add ocaml-xdg.
  gnu: Add ocaml-ordering.
  gnu: Add ocamlc-loc.
  gnu: Add ocamlformat-rpc-lib.
  gnu: Add ocaml-pp.
  gnu: Add ocaml-ppx-yojson-conv-lib.
  gnu: Add ocaml-merlin-lib-4.9.
  gnu: Add ocaml-lsp-server.

 gnu/packages/ocaml.scm | 290 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 289 insertions(+), 1 deletion(-)


base-commit: dcca13ed7340f17a33a4c0907c13b64d5da66a8e

Comments

Ludovic Courtès July 18, 2023, 11:52 a.m. UTC | #1 
Hi,

Benjamin <benjamin@uvy.fr> skribis:

> A lot of dependencies were required, but for most of them, this is just 
> a dune subpackage.
> For ocaml-dune-build-info, note that I included the patch from [0]
> and I used the same technique for other dune packages.
>
> The latest version of ocaml-lsp-server depends on ocaml-merlin-lib.
> I added a compatible version as a new package instead of upgrading
> current ocaml-merlin-lib as I was not sure if it would be safe to do so.
>
> Finally, the version 1.6.1 is supporting ocaml 5.0 and 5.1 [1] but I only
> tested for 5.0.
> Also, as 4.14 is not compatible with it, I only made public version 5.0
> of it. I do not no either if was the good thing to be done.
>
> I tested lsp server localy and it has been working fine with neovim for
> my usage.
>
> [0] https://issues.guix.gnu.org/63947
> [1] https://github.com/ocaml/ocaml-lsp/blob/master/CHANGES.md#1161

I’m not on the OCaml team, so I’ll let Julien and pukkamustard comment.

One issue: could you work on improving synopses and descriptions a bit
and submit a v2, as per
<https://guix.gnu.org/manual/en/html_node/Synopses-and-Descriptions.html>?
Don’t spend hours on this, but at least try to have sentences with a
bit of context in ‘description’.

Thanks in advance!

Ludo’.
pukkamustard Aug. 6, 2023, 8:25 p.m. UTC | #2 
Hi,

Thanks for the patches!

Couldn't look at everything yet, just some initial remarks.

Benjamin <benjamin@uvy.fr> writes:

> Hello,
>
> These patches add ocaml-lsp-server package.
>
> ...
>
> Benjamin (14):
>   gnu: Fix ocaml-dune-build-info

Lgtm, this patch would fix #63947.

>   gnu: Add ocaml-dune-rpc.

There seems to be an issue with the ordering of the patches. I can not
build ocaml-dune-rpc, as it requires ocaml-ordering which is only
available a couple of commits later.

In general, try to make things build at every commit. In this case it
requires re-ordering the commits.

Could you explain why the 'remove-vendor phase is needed? It seems like
if code is vendored it should maybe also be removed in the `dune`
package?

As Ludo mentioned, the description and synopses could use a little bit
of improvement. For example descriptions should be full sentences.

>   gnu: Add ocaml-chrome-trace.

We need a more descriptive description.

I guess the descriptions come from OPAM. Unfortunately Guix has stricter
requirements than OPAM...:)

-pukkamustard