all messages for Guix-related lists mirrored at yhetil.org
 help / color / mirror / code / Atom feed
From: Ricardo Wurmus <ricardo.wurmus@mdc-berlin.de>
To: Dennis Mungai <dmngaie@gmail.com>
Cc: guix-devel@gnu.org
Subject: Re: [PATCH] Add the clBLAS (OpenCL accelerated BLAS library) to GNU Guix
Date: Mon, 21 Mar 2016 11:28:14 +0100	[thread overview]
Message-ID: <idj7fgwt9a9.fsf@bimsb-sys02.mdc-berlin.net> (raw)
In-Reply-To: <CAKKYfmEwhq5AX7CgKzH6Va1F-7unTpeiBhZQufScnagZO2mjKg@mail.gmail.com>


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

      reply	other threads:[~2016-03-21 10:28 UTC|newest]

Thread overview: 2+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-03-20  2:24 [PATCH] Add the clBLAS (OpenCL accelerated BLAS library) to GNU Guix Dennis Mungai
2016-03-21 10:28 ` Ricardo Wurmus [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=idj7fgwt9a9.fsf@bimsb-sys02.mdc-berlin.net \
    --to=ricardo.wurmus@mdc-berlin.de \
    --cc=dmngaie@gmail.com \
    --cc=guix-devel@gnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this external index

	https://git.savannah.gnu.org/cgit/guix.git

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.