Le Thu, 21 Sep 2017 23:09:28 -0500,
Eric Bavier <ericbavier@centurylink.net> a écrit :

> Hi Julien,
> 
> Thanks for the patches.  Just a few comments below:

Thank you for this very detailed review :)

I think I addressed all your comments in the attached new patches, and
I will answer your other questions below:

> 
> On Thu, 21 Sep 2017 22:46:51 +0200
> Julien Lepiller <julien@lepiller.eu> wrote:
> 
> 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?

It seems this is not the way it is done elsewhere, and I was asked to
stick with this style.

> > "1whqr2bb3gds2zmrzqnv8vqka9928w4lx6mi6g244kmbwb2h8d8l"))
> > +              (file-name (string-append name "-" version
> > ".tar.gz"))
> > +              (patches (search-patches
> > "ocaml-piqilib-fix-makefile.patch"))))  
>                                            ^
> This patch is missing.

Indeed, I use GUIX_PACKAGE_PATH, so it was actually fetching the
package and the patch from that other directory... I should remember to
unset this variable before testing a patch.

> 
> > +    (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?

Yes it does, thank you for spotting this. It also works for most of the
other packages.

> 
> > +               (zero? (system* "./configure" "--prefix" out
> > "--ocaml-libdir"
> > +                               (string-append out
> > "/lib/ocaml/site-lib"))))))  
> 
> Is passing '#:configure-flags' not enough?

The configure script of OCaml packages is usually not an autotools one.
The all have a different set of options they require. Our
ocaml-build-system passes "-prefix" out and then configure-flags.
Setting configure-flags only would therefore fail, because the
"-prefix" option would not be recognized (one dash, when it expects
two). The ocaml-build-system is made that way because it seems most
configure scripts for ocaml packages require only one dash.

> 
> > +       (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?

Unfortunately, I didn't find any.

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

This package doesn't come from the same source as piqi-ocaml. I don't
know exactly how it works, though, because I added it only as a
dependency.

> > +   (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?

Again, --prefix vs -prefix

> 
> 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?

Since most configure scripts are not autotools one, they don't
recognize variables passed as arguments.

> 
> 
> Otherwise looks good to me.
> 
> Sorry for long reply and all the questions,
> `~Eric