Re: [PATCH] Add the clBLAS (OpenCL accelerated BLAS library) to GNU Guix

[Ricardo Wurmus]

Hi Dennis,

> This patch adds the clBLAS package to GNU Guix.

thank you for your patches!

Here a couple of comments:

* We usually require one patch per package.  Your patch adds three
  packages.

* The module is named “(gn packages clBLAS)”.  Packages should go to
  “(gnu packages ...)”.  Also, all module names are to be lowercase,
  i.e. “clblas” rather than “clBLAS”.  That said, it may be better to
  not create a new module but add the packages to existing modules such
  as “maths.scm”.

* Please keep the length of lines below 80 characters.

* Please run “guix lint” on your new packages for helpful hints.


> +(define-public clBLAS

Variable names should be lowercase.

> +  (package
> +    (name "clBLAS")
> +    (version "v2.10")

The “v” should not be part of the version number.

> +    (source (origin
> +             (method url-fetch)
> +             (uri (string-append 
> "https://github.com/clMathLibraries/clBLAS/archive/";
> +                                 version ".tar.gz"))

Since the tarball does not contain the name of the package, please add a
“(file-name ...)” expression.  You can grep the “gnu/packages/”
directory for examples on how to use it.

> +             (sha256
> +              (base32
> +               "0adlb02lqzrklfybhnv4n0p37mvkvdi3vqiwa05x2mv05ywnr93j"))))
> +    (build-system cmake-build-system)    
> +    (arguments `(#:tests? #f

Why are tests disabled?  If there are no tests we usually just add a
comment saying so.

> +                 #:configure-flags '("../clBLAS-2.10/src"
> "-DBUILD_SHARED_LIBS=ON" "-DCMAKE_BUILD_TYPE=Release"
> "-DBUILD_TEST=OFF")))

“CMAKE_BUILD_TYPE=Release” does not need to be set.  Why are the other
arguments needed?  In particular, why do you pass “../clBLAS-2.10/src”?

> +    (native-inputs `(("autoconf" ,autoconf)

This should be on a new line.

> +        ("automake" ,automake)

Why do you need autoconf and automake when this package uses cmake?

> +        ("cmake" ,cmake)
> +        ("gfortran" ,gfortran)
> +        ("libtool" ,libtool)
> +        ("pkg-config" ,pkg-config)))
> +    (inputs `(("curl" ,curl)

This should be on a new line.

> +       ("dbus" ,dbus)
> +       ("boost" ,boost)
> +       ("enca" ,enca)
> +       ("eudev" ,eudev)
> +       ("fftw-openmpi" ,fftw-openmpi)
> +       ("glew" ,glew)       
> +       ("libcap" ,libcap)
> +       ("libjpeg" ,libjpeg)
> +       ("libltdl" ,libltdl)
> +       ("libtiff" ,libtiff)
> +       ("mesa-utils" ,mesa-utils)
> +       ("openmpi" ,openmpi)
> +       ("ocl-icd" ,ocl-icd)
> +       ("opencl-headers" ,opencl-headers)
> +       ("randrproto" ,randrproto)
> +       ("libxrandr" ,libxrandr)
> +       ("xineramaproto" ,xineramaproto)
> +       ("libxinerama" ,libxinerama)
> +       ("libxcursor" ,libxcursor)
> +       ("python" ,python-2)))       
> +    (home-page "http://www.glfw.org/";)
> +    (synopsis "glfw is an Open Source, multi-platform library for creating 
> windows with OpenGL contexts and receiving input and events.")

We usually don’t say “Open Source” (or “free”) because all software in
Guix is Free Software.  A synopsis should not be a full sentence.  It
should be short enough not to require line breaks.

> +    (description "glfw is an Open Source, multi-platform library for 
> creating windows with OpenGL contexts and receiving input and events.")

Please break lines.  In Emacs you can format the paragraph with M-q.

> +    (license (list license:gpl2))))

No list needed when it’s just one license.  Are you sure it’s GPLv2 only?

> +(define-public ocl-icd
> +  (package
> +   (name "ocl-icd")
> +   (version "2.2.9")
> +   (source (origin
> +             (method url-fetch)
> +             (uri (string-append 
> "https://forge.imag.fr/frs/download.php/716/ocl-icd-";
> +                                 version ".tar.gz"))
> +             (file-name (string-append name "-" version ".tar.gz"))

This is not required here because the tarball already contains package
name and version.

> +             (sha256
> +              (base32
> +               "1rgaixwnxmrq2aq4kcdvs0yx7i6krakarya9vqs7qwsv5hzc32hc"))))
> +    (inputs `(("zip" ,zip)

It looks nicer to have the zip pair on the next line.
The list of inputs is usually placed below the build-system and
arguments fields.

> +             ("autoconf" ,autoconf)
> +             ("automake" ,automake)

These are probably native inputs only.

> +             ("ruby" ,ruby)
> +             ("libtool" ,libtool)
> +             ("opencl-headers" ,opencl-headers)
> +             ("libgcrypt" ,libgcrypt)))                                      
>         
> +    (build-system gnu-build-system)
> +     (arguments

The indentation here looks wrong.

> +     '(#:phases (modify-phases %standard-phases
> +                    (add-after 'unpack `bootstrap
> +                      (lambda _
> +                        (zero? (system* "autoreconf" "-vfi")))))))

Is bootstrapping really required?  Is there a release that doesn’t
require it?

> +    (home-page "https://forge.imag.fr/projects/ocl-icd/";)
> +    (synopsis "OpenCL implementations are provided as ICD (Installable 
> Client Driver).")

I don’t understand this synopsis.

> +    (description "OpenCL implementations are provided as ICD (Installable 
> Client Driver).
> +    An OpenCL program can use several ICD thanks to the use of an ICD Loader 
> as provided by this project.
> +    This free ICD Loader can load any (free or non free) ICD")

I would not mention “free” and “(free or non free)”.  ICD should be defined in 
the
description.  The description should probably start with the last sentence.

> +    (license (list license:gpl2 license:ruby))))

Is the license really GPLv2 only?  What parts are under the Ruby license?

> +(define-public opencl-headers
> +(let ((commit "c1770dc"))
> +  (package
> +    (name "opencl-headers")
> +    (version (string-append "2.1-" commit ))
> +    (source (origin
> +              (method git-fetch)
> +              (uri (git-reference
> +              (url "https://github.com/KhronosGroup/OpenCL-Headers.git";)
> +              (commit commit)))

The indentation is not correct here.

> +              (file-name (string-append name "-" commit))
> +              (sha256
> +               (base32
> +                "0m9fkblqja0686i2jjqiszvq3df95gp01a2674xknlmkd6525rck"))))
> +    (propagated-inputs '())
> +    (inputs '())
> +    (native-inputs '())

These fields are not needed when they are just empty.

> +    (build-system gnu-build-system)
> +    (arguments
> +     '(#:phases
> +       (modify-phases %standard-phases
> +         (delete 'configure)
> +         (delete 'build)
> +         (delete 'check)
> +         (replace 'install
> +                  (lambda* (#:key outputs #:allow-other-keys)
> +                    (copy-recursively "." (string-append
> +                                                 (assoc-ref outputs "out")
> +                                                 "/include/CL")))))))

I think the trivial-build-system would be more appropriate here.

> +    (synopsis "The Khronos OpenCL headers")
> +    (description "This package provides the Khronos OpenCL headers")

The description should be a full sentence with period.

> +    (home-page "https://www.khronos.org/registry/cl/";)
> +    (license (list license:gpl2)))))

Again, no “list” needed.  Also please make sure the license is in fact
GPLv2 only.

Could you please resend updated patches?

Thanks again!

~~ Ricardo