From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ricardo Wurmus Subject: Re: [PATCH] gnu: Add p7zip. Date: Mon, 22 Aug 2016 13:06:53 +0200 Message-ID: <87r39hysdu.fsf@elephly.net> References: <87a8gwxa9c.fsf@openmailbox.org> <87y44f4plm.fsf@mdc-berlin.de> <20ae08af155acdc7edd02b68afb9e66d@openmailbox.org> <20160808195835.GF18650@jasmine> <871t1y74fe.fsf@mdc-berlin.de> <87d1lheore.fsf@mdc-berlin.de> <87oa51puzr.fsf@openmailbox.org> <878tw5el71.fsf@elephly.net> <87k2floj6v.fsf@openmailbox.org> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Return-path: Received: from eggs.gnu.org ([2001:4830:134:3::10]:54137) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bbn4L-00022X-Ta for guix-devel@gnu.org; Mon, 22 Aug 2016 07:07:07 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bbn4G-0001UY-LY for guix-devel@gnu.org; Mon, 22 Aug 2016 07:07:04 -0400 Received: from sender163-mail.zoho.com ([74.201.84.163]:24771) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bbn4G-0001UG-BL for guix-devel@gnu.org; Mon, 22 Aug 2016 07:07:00 -0400 In-reply-to: <87k2floj6v.fsf@openmailbox.org> 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: Kei Kebreau Cc: guix-devel@gnu.org Kei Kebreau writes: > Here's the corresponding patch. Maybe you or someone else can double (or triple?) > check and make sure there are no proprietary files the source after running > "./pre-inst-env guix build -S p7zip." The patch looks good. It’s fine to delete the non-free files in a snippet. I have a couple of changes that I’d to see in the package expression, though. > + > +(define-public p7zip > + (package > + (name "p7zip") > + (version "16.02") > + (source (origin > + (method url-fetch) > + (uri (string-append "mirror://sourceforge/" name "/" name "/" > + version "/" name "_" version > + "_src_all.tar.bz2")) > + (sha256 > + (base32 > + "07rlwbbgszq8i7m8jh3x6j2w2hc9a72dc7fmqawnqkwlwb00mcjy")) > + (modules '((guix build utils))) > + (snippet > + '(begin > + ;; Remove non-free source files > + (for-each delete-file > + (find-files "CPP/7zip/Compress/" > + (string-append "Rar*"))) You don’t need “string-append” here. > + (delete-file-recursively "CPP/7zip/Compress/Rar"))) I think we may want to delete even more Rar stuff to be on the safe side, so the total list of deletions would be: CPP/7zip/Archive/Rar/ CPP/7zip/Archive/Rar* CPP/7zip/Crypto/Rar* CPP/7zip/Compress/Rar* DOC/unRarLicense.txt Utils/file_Codecs_Rar_so.py What do you think? Also note that the snippet should end on “#t”. > + (patches (search-patches "remove-unused-p7zip-code.patch")))) > + (build-system gnu-build-system) > + (arguments > + `(#:make-flags > + (list (let ((system ,(or (%current-target-system) > + (%current-system)))) > + (string-append "-f " > + (cond > + ((string-prefix? "x86_64" system) > + "makefile.linux_amd64_asm") > + ((string-prefix? "i686" system) > + "makefile.linux_x86_asm_gcc_4.X") > + (else > + "makefile.linux_any_cpu_gcc_4.X"))))) > + #:phases > + (modify-phases %standard-phases > + (replace 'configure > + (lambda* (#:key system outputs #:allow-other-keys) > + ;; fix install directory > + (substitute* "install.sh" > + (("/usr/local") (assoc-ref outputs "out"))))) We should end phases with “#t”. However, in this case I think we should better move this to the replaced install phase to keep related things in one place. > + (replace 'build > + (lambda _ > + (zero? (system* "make" "all3")))) I think we can avoid replacing the build phase by adding #:make-flags '("all3") instead. What do you think? > + (replace 'check > + (lambda _ > + (and (zero? (system* "make" "test")) > + (zero? (system* "make" "test_7z")) > + (zero? (system* "make" "test_7zr"))))) > + ;; without replacing the install phase, install.sh would be passed > + ;; arguments containing the wrong installation directories. > + (replace 'install > + (lambda _ > + (zero? (system* "sh" "install.sh"))))))) I don’t understand the comment. The makefiles don’t seem to have an “install” target at all. > + (inputs > + `(,@(cond ((string-prefix? "x86_64" (or (%current-target-system) > + (%current-system))) > + `(("yasm" ,yasm))) > + ((string-prefix? "i686" (or (%current-target-system) > + (%current-system))) > + `(("nasm" ,nasm))) > + (else '())))) > + (home-page "http://p7zip.sourceforge.net/") > + (synopsis "Command-line file archiver with high compression ratio") > + (description "p7zip is the Unix command-line port of 7-Zip, a file archiver > +that handles the 7z format which features very high compression > ratios.") How about replacing “Unix” with “POSIX”? > + (license (list license:lgpl2.1+ license:gpl2+ > license:public-domain)))) Here I’d like to see a comment that explains what this list means. Do you think you could provide us with an updated patch? If you’re already tired of this patch I could perform the changes on your behalf some time later. Thanks for your patience so far :) ~~ Ricardo