Re: [PATCH] gnu: Add p7zip.

[Kei Kebreau <kei@openmailbox.org>]

[-- Attachment #1.1: Type: text/plain, Size: 5286 bytes --]

Ricardo Wurmus <rekado@elephly.net> writes:

> Kei Kebreau <kei@openmailbox.org> 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.
Attached is a patch with your adjustments taken into consideration.

>
> Thanks for your patience so far :)
>
> ~~ Ricardo
You're welcome! I expect that you're busy and getting to things as time
and energy allows. :)


[-- Attachment #1.2: 0001-gnu-Add-p7zip.patch --]
[-- Type: text/plain, Size: 4171 bytes --]

From aad5d8a92d95f831ead90ec213f2b28c56dfeb79 Mon Sep 17 00:00:00 2001
From: Kei Kebreau <kei@openmailbox.org>
Date: Mon, 22 Aug 2016 19:54:07 -0400
Subject: [PATCH] gnu: Add p7zip.

* gnu/packages/compression.scm (p7zip): New variable.
* gnu/packages/patches/remove-unused-p7zip-code.patch: New patch.
* gnu/local.mk (dist_patch_DATA): Add it.
---
 gnu/packages/compression.scm | 47 +++++++++++++++++++++-----------------------
 1 file changed, 22 insertions(+), 25 deletions(-)

diff --git a/gnu/packages/compression.scm b/gnu/packages/compression.scm
index c6820c1..b8df41c 100644
--- a/gnu/packages/compression.scm
+++ b/gnu/packages/compression.scm
@@ -915,10 +915,11 @@ for most inputs, but the resulting compressed files are anywhere from 20% to
                '(begin
                   ;; Remove non-free source files
                   (for-each delete-file
-                            (find-files "CPP/7zip/Compress/Rar*")
-                            (find-files "CPP/7zip/Crypto/Rar*")
-                            (find-files "DOC/unRarLicense.txt")
-                            (find-files "Utils/file_Codecs_Rar_so.py"))
+                            (append
+                             (find-files "CPP/7zip/Compress" "Rar.*")
+                             (find-files "CPP/7zip/Crypto" "Rar.*")         
+                             (find-files "DOC/unRarLicense.txt")       
+                             (find-files  "Utils/file_Codecs_Rar_so.py")))
                   (delete-file-recursively "CPP/7zip/Archive/Rar")
                   (delete-file-recursively "CPP/7zip/Compress/Rar")
                   #t))
@@ -927,33 +928,29 @@ for most inputs, but the resulting compressed files are anywhere from 20% to
     (arguments
      `(#:make-flags
        (list
-        ("all3")
-        (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")))))
+        (string-append "DEST_HOME=" (assoc-ref %outputs "out"))
+        "all3")
        #:phases
        (modify-phases %standard-phases
+         (replace 'configure
+           (lambda* (#:key system outputs #:allow-other-keys)
+             (zero? (system* "cp"
+                             (let ((system ,(or (%current-target-system)
+                                                (%current-system))))
+                               
+                               (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")))
+                             "makefile.machine"))))
          (replace 'check
            (lambda _
              (and (zero? (system* "make" "test"))
                   (zero? (system* "make" "test_7z"))
-                  (zero? (system* "make" "test_7zr")))))
-         ;; without calling install.sh manually, install.sh is passed arguments
-         ;; containing the wrong installation directories (see line 116 of
-         ;; makefile.common)
-         (replace 'install
-           (lambda* (#:key system outputs #:allow-other-keys) 
-             ;; fix install directory
-             (substitute* "install.sh"
-               (("/usr/local") (assoc-ref outputs "out")))
-             #t)))))
+                  (zero? (system* "make" "test_7zr"))))))))
     (inputs
      `(,@(cond ((string-prefix? "x86_64" (or (%current-target-system)
                                              (%current-system)))
-- 
2.9.3


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]