Re: [PATCH 09/10] gnu: Add ocaml-js-build-tools.

[Ludovic Courtès]

Hey!

Julien Lepiller <address@hidden> skribis:

> * gnu/packages/ocaml.scm (ocaml-js-build-tools): New variable.
> * gnu/packages/patches/ocaml-janestreet-fix-libdir.patch: New file.
> * gnu/local.mk (dist_patch_DATA): Add it.

[...]

> +;; Janestreet packages are found in a similar way and all need the same patch

Period at the end of a sentence please.

> +(define (janestreet-origin name version hash)
> +  (origin (method url-fetch)
> +          (uri (string-append "https://ocaml.janestreet.com/ocaml-core/";
> +                              (version-major+minor version) "/files/"
> +                              name "-" version ".tar.gz"))
> +          (sha256 (base32 hash))
> +          (patches (search-patches "ocaml-janestreet-fix-libdir.patch"))
> +          (modules '((guix build utils)))
> +          (snippet `(substitute* "install.ml"
> +                                 (((string-append "lib/" ,name))
> +                                  (string-append "lib/ocaml/site-lib/" 
> ,name))))))

I prefer not to rely on the ability to use non-literal patterns in
‘substitute*’, so I would write it this way:

  (let ((pattern (string-append "lib/" name)))
    `(substitute* "install.ml"
       ((,pattern)
        …)))

Also, could you add a comment about what the snippet does (I suppose
Jane Street’s default install.ml files specify a wrong location or
something?).

Lastly, please pass this through indent-code.el.

> +(define-public ocaml-js-build-tools
> +  (package
> +    (name "ocaml-js-build-tools")
> +    (version "113.33.06")
> +    (source (janestreet-origin "js-build-tools" version
> +              "0r8z4fz8iy5y6hkdlkpwf6rk4qigcr3dzyv35585xgg2ahf12zy6"))
> +    (native-inputs
> +     `(("oasis" ,ocaml-oasis)
> +       ("opam" ,opam)))
> +    (build-system ocaml-build-system)
> +    (arguments janestreet-arguments)
> +    (home-page "https://github.com/janestreet/js-build-tools";)
> +    (synopsis "Collection of tools to help building Jane Street Packages")
> +    (description "This packages contains tools to help building Jane Street
> +Packages, but can be used for other purposes.  It contains:

s/Packages/packages/

> address@hidden
> address@hidden an oasis2opam-install tool to produce a .install file from the 
> oasis build log

Nitpick: @command{oasis2opam-install}, @file{.install}, OASIS.

> address@hidden an js_build_tools ocamlbuild plugin with various goodies

s/an js_build_tools/a @code{js_build_tools}/
Period at the end.

> --- /dev/null
> +++ b/gnu/packages/patches/ocaml-janestreet-fix-libdir.patch
> @@ -0,0 +1,39 @@
> +This patch adds a --libdir option to opam-installer so it installs the plugin
> +in the specified directory rather than in the default one (ocaml's directory 
> in
> +the store, which is forbidden).

s/forbidden/read-only/  :-)

Also please mention that this patch is meant to apply to all the
packages published by Jane Street (IIUC).

Thanks,
Ludo’.