From mboxrd@z Thu Jan 1 00:00:00 1970 From: Kei Kebreau Subject: Re: 01/01: gnu: Add niftilib. Date: Sun, 19 Mar 2017 17:03:56 -0400 Message-ID: <87d1dd5683.fsf@openmailbox.org> References: <20170318142616.19421.55332@vcs0.savannah.gnu.org> <20170318142617.B58A320DF8@vcs0.savannah.gnu.org> <87d1dd5775.fsf@netris.org> Mime-Version: 1.0 Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha256; protocol="application/pgp-signature" Return-path: Received: from eggs.gnu.org ([2001:4830:134:3::10]:52012) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1cphzv-0001S1-LW for guix-devel@gnu.org; Sun, 19 Mar 2017 17:04:20 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1cphzs-0001Qr-G4 for guix-devel@gnu.org; Sun, 19 Mar 2017 17:04:19 -0400 In-Reply-To: <87d1dd5775.fsf@netris.org> (Mark H. Weaver's message of "Sun, 19 Mar 2017 16:42:54 -0400") List-Id: "Development of GNU Guix and the GNU System distribution." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guix-devel-bounces+gcggd-guix-devel=m.gmane.org@gnu.org Sender: "Guix-devel" To: Mark H Weaver Cc: guix-devel@gnu.org, John Darrington --=-=-= Content-Type: text/plain Content-Transfer-Encoding: quoted-printable Mark H Weaver writes: > john@darrington.wattle.id.au (John Darrington) writes: > >> jmd pushed a commit to branch master >> in repository guix. >> >> commit 21122bd79e7f9b0b5349ffffe2c146bace7205dc >> Author: John Darrington >> Date: Tue Mar 7 07:59:21 2017 +0100 >> >> gnu: Add niftilib. >>=20=20=20=20=20 >> * gnu/packages/image.scm (niftilib): New variable. > > Did you post this for review? Please see below for comments. > It was in fact posted for review. I reviewed it. >> +(define-public niftilib >> + (package >> + (name "niftilib") >> + (version "2.0.0") >> + (source (origin >> + (method url-fetch) >> + (uri (list (string-append "mirror://sourceforge/niftilib/" >> + "nifticlib/nifticlib_" >> + (string-join (string-split version #\.) "_") >> + "/nifticlib-" version ".tar.gz"))) > > Omit the superfluous 'list' call above. > I missed this somehow. >> + (sha256 >> + (base32 "123z9bwzgin5y8gi5ni8j217k7n683whjsvg0lrpii9flgk8isd3")))) >> + (build-system gnu-build-system) >> + (arguments >> + '(#:tests? #f >> + #:parallel-build? #f >> + #:phases >> + (modify-phases %standard-phases >> + (replace 'install > > Is there a reason not to use the included "make install" target? It > looks like it should work, if you pass appropriate settings for > INSTALL_{BIN,LIB,INC}_DIR in #:make-flags. > >> + (lambda _ >> + (for-each >> + (lambda (dir) >> + (let ((directory (assoc-ref %outputs "out"))) > > If you were going to keep the custom 'install' phase, then instead of > using %outputs, please accept the 'outputs' keyword argument and use > that. > >> + (mkdir-p (string-append directory "/" dir)) >> + (zero? (system* "cp" "-a" dir directory)))) >> + '("bin" "lib" "include")))) > > We have a 'copy-recursively' procedure that you could use here. If you > were going to use "cp", then you should pay attention to its result code > so that failures are not silently ignored (by using 'every' instead of > 'for-each'). > This is interesting. Which module contains the definition for the 'every' procedure? >> + (replace 'configure >> + (lambda _ >> + (substitute* "Makefile" >> + (("^SHELL[ \t]*=3D[ \t]*csh") >> + (string-append "SHELL =3D " >> + (assoc-ref %build-inputs "bash") >> + "/bin/sh")) >> + >> + (("^CFLAGS[ \t]*=3D[ \t]\\$\\(ANSI_FLAGS\\)") >> + "CFLAGS =3D $(ANSI_FLAGS) -fPIC") >> + >> + (("^ZLIB_INC[ \t]*=3D[ \t]*-I/usr/include") >> + (string-append "ZLIB_INC =3D -I" >> + (assoc-ref %build-inputs "zlib") >> + "/include")) >> + >> + (("^CP[ \t]*=3D[ \t]*cp") >> + (string-append "CP =3D " >> + (assoc-ref %build-inputs "coreutil= s") >> + "/bin/cp"))) > > Instead of patching the Makefile, it's preferable to simply pass these > settings in #:make-flags. Also, within phase procedures, please accept > the 'inputs' and 'outputs' keyword arguments instead of using > %build-inputs and %outputs. Finally, for purposes of Makefile settings, > SHELL can simply be set to "bash" or "sh", since it's in the PATH. I'm > not sure why you changed the setting for CP. > > Mark I should keep a closer eye on details like these. Thank you for the second review. --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEg7ZwOtzKO2lLzi2m5qXuPBlGeg0FAljO8j0ACgkQ5qXuPBlG eg0saBAAosxYUhx8t+kv7xkMRAvRb6JEaup5XONaA95Gmpxsq5lRkpPGpE55klTI kcEX6bZh9yhulKa/n+KH27V9WaN/ZBhM7839WuFIHQvYSV2ebqxdYzVw8ovtAPQz t5XweivQUnCljnATb3sixHQge2zWabAaNvi8ZKeTw5apJJJYOxfm4hjKl5dMT9o8 22+lUAtibfH22fnrEvDXJYpoiTpHeraI0pCm8JOFXxVHQXjBr0kgt08RTj7hWlh6 eWsTZf9yEbSWaqQ56ujrZIK0LU8azzF1M9uH4sV3E+Hr6V8BTAJ5fs4WSS02mXK3 DiarTla4aW4VRh+LZ4/2ip3rifdIy4xKGXc8+3HWI5XLm9Dx/tL2cm04f/j+YTrp yjkPEuzixyUI7LxFm9b+2qaYQG8a63eG1k0o9uS/AyiSQEQhKWbdDYGdiSkKSQjZ aDkenKNOK94PcudZLl21EQZgQFj9wEVH63G+GP57OfI//5+ih2cfRaWYLLNZ3Tm1 ktaAgGqk/yTanvEk9WnxyHsMiBx+anB+YQReJVUdimax4ApBMW1CcIi+elmMcYlk iXH9DMW403whLkDtrwgwvc6/u9iOUeKdmaKC06oGNoAd8BouYyqg0HnhXkmD9MOI 8lhDNCueK+5ZFbUbV+vulbgky6vWveYG6MQRhsXaw9X0bmOo8Eg= =5KCn -----END PGP SIGNATURE----- --=-=-=--