Subject: Re: [bug#28546] Add some ocaml packages

From: Eric Bavier <[email-masked]>
Date: Thu, 21 Sep 2017 23:09:28 -0500

Hi Julien,

Thanks for the patches.  Just a few comments below:

On Thu, 21 Sep 2017 22:46:51 +0200
Julien Lepiller <[email-masked]> wrote:

> From d177cda39184fd1a1a879feb93f133eecd3bbcc4 Mon Sep 17 00:00:00 2001
> From: Julien Lepiller <[email-masked]>
> Date: Thu, 21 Sep 2017 20:32:27 +0200
> Subject: [PATCH 01/10] gnu: Add ocaml-ezjsonm.
> 
> * gnu/packages/ocaml.scm (ocaml-ezjsonm): New variable.
> ---
>  gnu/packages/ocaml.scm | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
> 
> diff --git a/gnu/packages/ocaml.scm b/gnu/packages/ocaml.scm
> index 700c5698e..d765a085a 100644
> --- a/gnu/packages/ocaml.scm
> +++ b/gnu/packages/ocaml.scm
> @@ -3197,6 +3197,36 @@ writing to these structures, and they are accessed via the Bigarray module.")
>      (description "Hex is a minimal library providing hexadecimal converters.")
>      (license license:isc)))
>  
> +(define-public ocaml-ezjsonm
> +  (package
> +    (name "ocaml-ezjsonm")
> +    (version "0.4.3")
> +    (source (origin
> +              (method url-fetch)
> +              (uri (string-append "https://github.com/mirage/ezjsonm/archive/"
> +                                  version ".tar.gz"))

In a few of the later patches, you declared 'home-page' before 'source'
so that it could be used in the uri.  That seams reasonable to me.  Did
you want to do that in all these patches?

> +    (home-page "https://github.com/mirage/ezjsonm/")
> +    (synopsis "Easy interface on top of the Jsonm library")

How about some more widely meaningful, like: "Read and write JSON
data"?  IDK, see section 6.7.5 in the manual.

> From 395199079f38b96299dbc02b14a945236ef7d6fe Mon Sep 17 00:00:00 2001
> From: Julien Lepiller <[email-masked]>
> Date: Thu, 21 Sep 2017 20:33:33 +0200
> Subject: [PATCH 02/10] gnu: Add ocaml-uri.
> 
> * gnu/packages/ocaml.scm (ocaml-uri): New variable.
> ---
> +    (synopsis "RFC3986 URI/URL parsing library")

Similarly, I think we can leave out reference to the RFC in the
synopsis, since its in the description already.  "URI/URL parsing
library" seems enough.

> From 8a8f1bedad93fe403df7d374515340f7f2462b2a Mon Sep 17 00:00:00 2001
> From: Julien Lepiller <[email-masked]>
> Date: Thu, 21 Sep 2017 20:35:17 +0200
> Subject: [PATCH 04/10] gnu: Add ocaml-optcomp.
> 
> * gnu/packages/ocaml.scm (ocaml-optcomp): New variable.
> ---
> +(define-public ocaml-optcomp
> +  (package
> +    (name "ocaml-optcomp")

Since this is a tool/application in its own right, rather than a
library, I think this package could be named "optcomp".  This would be
in line with other packages like "starman", "scons", "behave",
"snakemake", etc.

> +    (version "1.6")
> +    (source (origin
> +              (method url-fetch)
> +              (uri (string-append "https://github.com/diml/optcomp/archive/"
> +                                  version ".tar.gz"))
> +              (sha256
> +               (base32
> +                "0hhhb2gisah1h22zlg5iszbgqxdd7x85cwd57bd4mfkx9l7dh8jh"))
> +              (file-name (string-append name "-" version ".tar.gz"))))
> +    (build-system ocaml-build-system)
> +    (arguments
> +     `(#:use-make? #t
> +	   #:make-flags
           ^
Remove the tab and align with spaces to '#:' on previous line.

> +    (native-inputs `(("camlp4" ,camlp4)))
> +    (propagated-inputs `(("camlp4" ,camlp4)))
> +    (home-page "https://github.com/diml/optcomp")
> +    (synopsis "Optional compilation with cpp-like directives")

Maybe: "Optional compilation for Ocaml"?

> From 3887a8aa6b8610f4ccfece291d7c8bc12a3966bf Mon Sep 17 00:00:00 2001
> From: Julien Lepiller <[email-masked]>
> Date: Thu, 21 Sep 2017 20:44:57 +0200
> Subject: [PATCH 05/10] gnu: Add ocaml-piqilib.
> 
> * gnu/packages/ocaml.scm (ocaml-piqilib): New variable.
> ---
>  gnu/packages/ocaml.scm | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 53 insertions(+)
> 
> diff --git a/gnu/packages/ocaml.scm b/gnu/packages/ocaml.scm
> index 910c892d6..86fe1a9e4 100644
> --- a/gnu/packages/ocaml.scm
> +++ b/gnu/packages/ocaml.scm
> @@ -3303,6 +3303,59 @@ Format module of the OCaml standard library.")
>  cpp-like directives.")
>      (license license:bsd-3)))
>  
> +(define-public ocaml-piqilib
> +  (package
> +    (name "ocaml-piqilib")
> +    (version "0.6.13")
> +    (source (origin
> +              (method url-fetch)
> +              (uri (string-append "https://github.com/alavrik/piqi/archive/v"
> +                                  version ".tar.gz"))
> +              (sha256
> +               (base32
> +                "1whqr2bb3gds2zmrzqnv8vqka9928w4lx6mi6g244kmbwb2h8d8l"))
> +              (file-name (string-append name "-" version ".tar.gz"))
> +              (patches (search-patches "ocaml-piqilib-fix-makefile.patch"))))
                                           ^
This patch is missing.

> +    (build-system ocaml-build-system)
> +    (arguments
> +     `(#:phases
> +       (modify-phases %standard-phases
> +         (replace 'configure
> +           (lambda* (#:key outputs #:allow-other-keys)
> +             (let ((out (assoc-ref outputs "out")))
> +               (substitute* "make/OCamlMakefile"
> +                 (("/bin/sh") (which "bash")))

Does setting the "SHELL" environment variable work instead?

> +               (zero? (system* "./configure" "--prefix" out "--ocaml-libdir"
> +                               (string-append out "/lib/ocaml/site-lib"))))))

Is passing '#:configure-flags' not enough?

> +       (add-after 'build 'build-ocaml
> +         (lambda* (#:key outputs #:allow-other-keys)
> +           (zero? (system* "make" "ocaml")))) 
> +       (add-after 'install 'install-ocaml
> +         (lambda* (#:key outputs #:allow-other-keys)
> +           (zero? (system* "make" "ocaml-install"))))
> +       (add-after 'install-ocaml 'link-stubs
> +         (lambda* (#:key outputs #:allow-other-keys)
> +           (let* ((out (assoc-ref outputs "out"))
> +                  (stubs (string-append out "/lib/ocaml/site-lib/stubslibs"))
> +                  (lib (string-append out "/lib/ocaml/site-lib/piqilib")))
> +             (mkdir-p stubs)
> +             (symlink (string-append lib "/dllpiqilib_stubs.so")
> +                      (string-append stubs "/dllpiqilib_stubs.so"))))))))

Is there some sort of configuration flag that can be used to install
these library into the right place?

Is this package to avoid having to build the entire piqi tool?

> From 4afbe64878f1b047ba936015e005f7d48a847afa Mon Sep 17 00:00:00 2001
> From: Julien Lepiller <[email-masked]>
> Date: Thu, 21 Sep 2017 20:45:47 +0200
> Subject: [PATCH 06/10] gnu: Add ocaml-uuidm.
> 
> * gnu/packages/ocaml.scm (ocaml-uuidm): New variable.
> ---
> +(define-public ocaml-uuidm
> +  (package
...
> +    (synopsis "Universally unique identifiers (UUIDs) for OCaml")

I think we'd rather leave the acronym out of the synopsis, and place it
in the description instead.

> +    (description "Uuidm is an OCaml module implementing 128 bits universally
> +unique identifiers version 3, 5 (named based with MD5, SHA-1 hashing) and 4
> +(random based) according to RFC 4122.")

> From 3756da5e60dae4a109c18e552c6d07233ddfb5b9 Mon Sep 17 00:00:00 2001
> From: Julien Lepiller <[email-masked]>
> Date: Thu, 21 Sep 2017 20:46:37 +0200
> Subject: [PATCH 07/10] gnu: Add ocamlgraph.
> 
> * gnu/packages/ocaml.scm (ocamlgraph): New variable.
> ---
> +(define-public ocamlgraph
> +  (package
> +    (name "ocamlgraph")

I think we can name this packages "ocaml-graph".  Similar to python
modules whose names begins with "python".

> +    (version "1.8.7")
> +    (source (origin
> +              (method url-fetch)
> +              (uri (string-append "http://ocamlgraph.lri.fr/download/"
> +                                  "ocamlgraph-" version ".tar.gz"))
> +              (sha256
> +               (base32
> +                "1845r537swjil2fcj7lgbibc2zybfwqqasrd2s7bncajs83cl1nz"))
> +              (patches (search-patches "ocamlgraph-honor-source-date-epoch.patch"))))
                                           ^
This patch is missing.

> +    (build-system ocaml-build-system)
> +    (arguments
> +     `(#:install-target "install-findlib"
> +       #:phases
> +       (modify-phases %standard-phases
> +         (add-before 'configure 'fix-/bin/sh
> +           (lambda* (#:key inputs #:allow-other-keys)
> +             (substitute* "configure"
> +               (("-/bin/sh") (string-append "-" (assoc-ref inputs "bash")
> +                                            "/bin/sh"))))))))
> +    (inputs `(("lablgtk" ,lablgtk)))
> +    (home-page "http://ocamlgraph.lri.fr/")
> +    (synopsis "A generic graph library for OCaml")

Synopses should not start with 'A'.  'guix lint' will warn about this.

> +    (description "A generic graph library for OCaml.")

Rather: "OCamlgraph is a generic graph library for OCaml."

> From aa994f7c4ca830f6b3834fd100d7395fcb83dbf5 Mon Sep 17 00:00:00 2001
> From: Julien Lepiller <[email-masked]>
> Date: Thu, 21 Sep 2017 20:47:40 +0200
> Subject: [PATCH 08/10] gnu: Add ocaml-piqi.
> 
> * gnu/packages/ocaml.scm (ocaml-piqi): New variable.
> ---
> +(define-public ocaml-piqi
> +  (package
> +    (name "ocaml-piqi")
> +    (version "0.7.5")
> +    (source (origin
> +              (method url-fetch)
> +              (uri (string-append "https://github.com/alavrik/piqi-ocaml/"
> +                                  "archive/v" version ".tar.gz"))
> +              (sha256
> +               (base32
> +                "0ngz6y8i98i5v2ma8nk6mc83pdsmf2z0ks7m3xi6clfg3zqbddrv"))))
> +    (build-system ocaml-build-system)
> +    (arguments
> +     `(#:make-flags
> +       (list (string-append "DESTDIR=" (assoc-ref %outputs "out")))

Too bad this is so different from GNU's DESTDIR semantics...

> +       #:phases
> +       (modify-phases %standard-phases
> +         (delete 'configure)
> +         (add-before 'build 'patch-/bin/sh
> +           (lambda _
> +             (substitute* "make/OCamlMakefile"
> +               (("/bin/sh") (which "sh")))

Again, I wonder if setting "SHELL" in the environment would suffice.
It would be less heavy-handed.

> From 2e4a7549a148e56d5d4fe7a869b56e1524dafcb6 Mon Sep 17 00:00:00 2001
> From: Julien Lepiller <[email-masked]>
> Date: Thu, 21 Sep 2017 20:49:29 +0200
> Subject: [PATCH 09/10] gnu: Add ocaml-bap.
> 
> * gnu/packages/ocaml.scm (ocaml-bap): New variable.
> ---
>  gnu/packages/ocaml.scm | 78 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 78 insertions(+)
> 
> diff --git a/gnu/packages/ocaml.scm b/gnu/packages/ocaml.scm
> index 7f65ce859..2e7001c4c 100644
> --- a/gnu/packages/ocaml.scm
> +++ b/gnu/packages/ocaml.scm
> @@ -3449,6 +3449,84 @@ It provides a uniform interface for serializing OCaml data structures to JSON,
>  XML and Protocol Buffers formats.")
>      (license license:asl2.0)))
>  
> +(define-public ocaml-bap
> +  (package
> +    (name "ocaml-bap")

This package should be simply named "bap".

> +    (version "1.1.0")
> +    (home-page "https://github.com/BinaryAnalysisPlatform/bap")
> +    (source (origin
> +              (method url-fetch)
> +              (uri (string-append home-page "/archive/v" version ".tar.gz"))
> +              (sha256
> +               (base32
> +                "1ms95m4j1qrmy7zqmsn2izh7gq68lnmssl7chyhk977kd3sxj66m"))
> +              (file-name (string-append name "-" version ".tar.gz"))))
> +   (build-system ocaml-build-system)
> +   (native-inputs
> +    `(("oasis" ,ocaml-oasis)
> +      ("clang" ,clang)
> +      ("ounit" ,ocaml-ounit)))
> +   (propagated-inputs
> +    `(("core-kernel" ,ocaml-core-kernel)
> +      ("ppx-driver" ,ocaml-ppx-driver)
> +      ("uri" ,ocaml-uri)
> +      ("llvm" ,llvm)
> +      ("gmp" ,gmp)
> +      ("clang-runtime" ,clang-runtime)
> +      ("fileutils" ,ocaml-fileutils)
> +      ("cmdliner" ,ocaml-cmdliner)
> +      ("zarith" ,ocaml-zarith)
> +      ("uuidm" ,ocaml-uuidm)
> +      ("camlzip" ,camlzip)
> +      ("frontc" ,ocaml-frontc)
> +      ("ezjsonm" ,ocaml-ezjsonm)
> +      ("ocurl" ,ocaml-ocurl)
> +      ("piqi" ,ocaml-piqi)
> +      ("ocamlgraph" ,ocamlgraph)
> +      ("bitstring" ,ocaml-bitstring)
> +      ("ppx-jane" ,ocaml-ppx-jane)
> +      ("re" ,ocaml-re)))
> +   (inputs `(("llvm" ,llvm)))
> +   (arguments
> +    `(#:use-make? #t
> +      #:phases
> +      (modify-phases %standard-phases
> +        (replace 'configure
> +          (lambda* (#:key outputs #:allow-other-keys)
> +            (zero? (system* "./configure" "--prefix"
> +                            (assoc-ref outputs "out")
> +                            "--libdir"
> +                            (string-append
> +                              (assoc-ref outputs "out")
> +                              "/lib/ocaml/site-lib")
> +                            "--with-llvm-version=3.8"
> +                            "--with-llvm-config=llvm-config"
> +                            "--enable-everything"))))

Could you put these flags in '#:configure-flags' instead?

> +        (add-before 'install 'fix-ocamlpath

A short comment on why this phase is needed might be useful.

> +          (lambda* (#:key outputs #:allow-other-keys)
> +            (setenv "OCAMLPATH"
> +                    (string-append
> +                      (getenv "OCAMLPATH") ":"
> +                      (assoc-ref outputs "out")
> +                      "/lib/ocaml/site-lib"))
> +            (setenv "PATH"
> +                    (string-append (getenv "PATH") ":"
> +                                   (assoc-ref outputs "out") "/bin"))))

> From 553a30b6fd292a7b69a271e61e7a64a1ad9bcbf5 Mon Sep 17 00:00:00 2001
> From: Julien Lepiller <[email-masked]>
> Date: Thu, 21 Sep 2017 20:51:14 +0200
> Subject: [PATCH 10/10] gnu: Add ocaml-camomile.
> 
> * gnu/packages/ocaml.scm (ocaml-camomile): New variable.
> ---
>  gnu/packages/ocaml.scm | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
> 
> diff --git a/gnu/packages/ocaml.scm b/gnu/packages/ocaml.scm
> index 2e7001c4c..31dde1952 100644
> --- a/gnu/packages/ocaml.scm
> +++ b/gnu/packages/ocaml.scm
> @@ -3527,6 +3527,36 @@ of libraries, plugins, and frontends.  The libraries provide code reusability,
>  the plugins facilitate extensibility, and the frontends serve as entry points.")
>     (license license:expat)))
>  
> +(define-public ocaml-camomile
> +  (package
> +    (name "ocaml-camomile")
> +    (version "0.8.5")
> +    (home-page "https://github.com/yoriyuki/Camomile")
> +    (source (origin
> +              (method url-fetch)
> +              (uri (string-append home-page "/releases/download/rel-" version
> +                                  "/camomile-" version ".tar.bz2"))
> +              (sha256
> +               (base32
> +                "003ikpvpaliy5hblhckfmln34zqz0mk3y2m1fqvbjngh3h2np045"))))
> +    (build-system ocaml-build-system)
> +    (native-inputs `(("camlp4" ,camlp4)))
> +    (arguments
> +     `(#:phases
> +       (modify-phases %standard-phases
> +         (add-before 'configure 'fix-bin/sh
> +           (lambda* (#:key #:allow-other-keys)
> +             (substitute* "configure"
> +               (("CONFIG_SHELL-/bin/sh")
> +                (string-append "CONFIG_SHELL-" (which "bash")))))))))

Maybe our ocaml-build-system should be defining SHELL and CONFIG_SHELL
in the flags passed to configure, like gnu-build-system does.  Or does
that not work for some ocaml projects?

Otherwise looks good to me.

Sorry for long reply and all the questions,
`~Eric