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 <dmng...@gmail.com> > 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
