[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: Add glfw 3.1.2 to GNU Guix
From: |
Ricardo Wurmus |
Subject: |
Re: Add glfw 3.1.2 to GNU Guix |
Date: |
Fri, 27 May 2016 10:26:17 +0200 |
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