Re: Add glfw 3.1.2 to GNU Guix

[Ricardo Wurmus]

Hi Dennis,

> This patch adds glfw 3.1.2 to GNU Guix.

Thank you for your patch!

> From 6d93bf068efe283940952cb31da1f0c1ba82a0cd Mon Sep 17 00:00:00 2001
> From: brainiarc7 <address@hidden>
> Date: Thu, 26 May 2016 22:49:25 +0300
> Subject: [PATCH] Add glfw to GNU Guix

> ---
>  gnu/packages/gl.scm | 43 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 43 insertions(+)

The commits should have a summar in a conventional format.  In this case
it should be something like this:

~~~~~~~~~~~~~~~~
gnu: Add glfw.

* gnu/packages/gl.scm (glfw): New variable.
~~~~~~~~~~~~~~~~


> +(define-public glfw
> +  (package
> +    (name "glfw")
> +    (version "3.1.2")
> +    (source (origin
> +             (method url-fetch)
> +             (uri (string-append "https://github.com/glfw/glfw/archive/";
> +                                 version ".tar.gz"))

Since the tarball doesn’t contain the name of the package, we usually
override its name by adding this:

    (file-name (string-append name "-" version ".tar.gz"))

> +             (sha256
> +              (base32
> +               "08pixv8hd5xsccf7l8cqcijjqaq4k4da8qbp77wggal2fq445ika"))))
> +    (build-system cmake-build-system)
> +    (arguments `(#:configure-flags '("-DBUILD_SHARED_LIBS=ON") 
> +                 #:tests? #f))

Could you please add a comment explaining why the tests are disabled?
If there are no tests it is enough to add “; no tests” on the same line.

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

The indentation here is off.  When we have input lists with more than
one item we usually start them on the next line.

    (native-inputs
     `(("autoconf" ,autoconf)
       ("automake" ,automake)
       ...))

> +        ("cmake" ,cmake)

As you are using the “cmake-build-system” you don’t need to add “cmake”
explicitly.

> +        ("git" ,git)

Why is git needed?  This is not a git checkout.  Does the build fail
without git?

> +        ("libtool" ,libtool)
> +        ("libpthread-stubs" ,libpthread-stubs)
> +        ("pkg-config" ,pkg-config)))
> +    (inputs `(("curl" ,curl)

The above comment on indenting lists applies here too.

> +       ("dbus" ,dbus)
> +       ("enca" ,enca)
> +       ("eudev" ,eudev)
> +       ("glew" ,glew)
> +       ("libcap" ,libcap)
> +       ("libjpeg" ,libjpeg)
> +       ("libltdl" ,libltdl)
> +       ("libtiff" ,libtiff)
> +       ("mesa-utils" ,mesa-utils)
> +       ("randrproto" ,randrproto)
> +       ("libxrandr" ,libxrandr)
> +       ("xineramaproto" ,xineramaproto)
> +       ("libxinerama" ,libxinerama)
> +       ("libxcursor" ,libxcursor)
> +       ("python" ,python-2)))

That’s a long list of inputs and I haven’t checked if all of them are
necessary.  Python is a pretty big input, though.  Is it really
necessary?  Or is this for additional Python bindings?  If so, could we
move the Python bindings to a separate output?

> +    (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.")

The synopsis is too long.  How about:

   “Library for windows with OpenGL contexts and event handling”

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

We don’t use the term “Open Source”.  All packages in Guix are free
software, so we don’t explicitly mention this.

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

Is it GPLv2 only or is it GPLv2+?  What part of this package is under
the zlib license?  Whenever lists are used it is good to add a comment
explaining what it means.

Could you please send an updated patch?

Thanks!

~~ Ricardo