Re: [PATCH] Port Arrayfire to GNU Guix

[Ludovic Courtès]

Hi,

Dennis Mungai <address@hidden> skribis:

> The patch attached adds Arrayfire, a software library for GPU computing.
> For compliance with the GPL licenses, I've configured the package not
> to build with CUDA support, and the OpenCL backend builds with the
> open source OpenCL ICD and the standard Khronos OpenCL headers for
> vendor neutrality.

Sorry for the late reply, and thanks for your work!

> From c25732b9beb99a0788349c086f460d45f228dd74 Mon Sep 17 00:00:00 2001
> From: Dennis Mungai <address@hidden>
> Date: Sun, 20 Mar 2016 04:51:15 +0300
> Subject: [PATCH] Ported ArrayFire to GNU Guix
>
> ---
>  arrayfire.scm | 362 
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++

Could you:

  1. Send a patch against Guix ‘master’, where this file should be
     called gnu/packages/opencl.scm?  (I think ‘opencl’ is more
     appropriate than ‘arrayfire’.)

  2. Send one patch per package, to simply the review workflow?

  3. Run ‘guix lint’ for each package and address any problems it raises
     (hopefully simple to do, but feel free to ask for help on #guix).

  4. Provide ChangeLog-style commit logs, as noted at
     
<https://www.gnu.org/software/guix/manual/html_node/Submitting-Patches.html>?

That’s a bit of extra work, but it will really help us!

Some more specific comments:

> +(define-module (gn packages arrayfire)

s/gn/gnu/

> +(define-public arrayfire
> +(package
> +    (name "arrayfire")

Please try to indent as in the other files.

> +       ("cmake" ,cmake)))
> +    (build-system cmake-build-system)

CMake is not needed in ‘inputs’ when using ‘cmake-build-system’.

> +    (arguments 
> +     `(#:configure-flags '("-DBUILD_OPENCL=ON" "-DBUILD_CUDA=OFF" 
> "-DBUILD_GRAPHICS=OFF" "-DUSE_SYSTEM_BOOST_COMPUTE=ON" 
> "-DUSE_SYSTEM_CLBLAS=ON" "-DUSE_SYSTEM_CLFFT=ON") 
> +       #:tests? #f))     

Please add a comment above #:tests? #f to justify why tests are skipped.

> +    (synopsis "ArrayFire: a general purpose GPU library. 
> https://arrayfire.com";)
> +    (description "ArrayFire is a high performance software library for 
> parallel computing with an easy-to-use API. Its array based function set 
> makes parallel programming simple.Now on Guix")

‘guix lint’ will suggest some changes here.  :-)

> +    (home-page "http://arrayfire.com/";)
> +    (license (list license:gpl2 
> +                   license:gpl2+ 
> +                   license:gpl3 
> +                   license:gpl3+))))

What does it mean?  Please add a comment above explaining what the
license list is about.

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

s/Open Source//g

The GNU project focuses on free software, not “open source” (see
<https://www.gnu.org/philosophy/open-source-misses-the-point.html>), and
since every package in Guix is free, we don’t mention it in synopses.

Also, please see
<https://www.gnu.org/software/guix/manual/html_node/Synopses-and-Descriptions.html>
on the kind of synopses/descriptions we try to provide to our users.

> +(define-public clBLAS
> +  (package
> +    (name "clBLAS")

Variable and package names should be lower-case.

> +    (version "v2.10")

No “v”.

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

Indentation.

> +(define-public opencl-headers
> +(let ((commit "c1770dc"))
> +  (package

Indentation.

Could you send updated patches, preferably using ‘git send-email’?

Thank you!

Ludo’.