Re: [PATCH] gnu: Add p7zip.

[Ricardo Wurmus <rekado@elephly.net>]

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.

Thanks for your patience so far :)

~~ Ricardo