GNU bug report logs - #26730
[PATCH] Fix bzip2 utilities

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 26730 in the body.
You can then email your comments to 26730 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Christopher Baines <mail <at> cbaines.net>
To: guix-patches <at> gnu.org
Subject: [PATCH] Fix bzip2 utilities
Date: Mon, 1 May 2017 10:49:50 +0100

[Message part 1 (text/plain, inline)]

The bzip2 package includes wrappers around diff, grep and less/more.
These shell scripts currently include /usr/bin (and sometimes /bin) on
the PATH, and therefore fail if any of the commands that they rely on
cannot be found.

By substituting /usr/bin (and /bin) for the appropriate package paths,
these scripts work much more reliably.

I had some issues with bzmore which uses more from util-linux, I think
there might be a circular dependency in the Guix package tree, as I get
a VM stack overflow when including util-linux as a input to bzip2.

Note that as the bzip2 package is used by so many other packages, any
non-superficial change to it probably means rebuilding the world.

Below is a script, and two invocations of it, showing first what happens
with the unaltered package, and then the package with the modifications
I describe above.


#!/bin/sh
set -x
bzip2 < /etc/shells > shells.bz2

bzcmp shells.bz2 shells.bz2
bzdiff shells.bz2 shells.bz2
bzegrep bash shells.bz2
bzfgrep bash shells.bz2
bzgrep bash shells.bz2
bzless shells.bz2
bzmore shells.bz2


→ PATH=/gnu/store/s3c442d075fc8a0q0nspc9jjsgjq613p-bzip2-1.0.6/bin
./test.sh
+ bzip2
+ bzcmp shells.bz2 shells.bz2
/gnu/store/s3c442d075fc8a0q0nspc9jjsgjq613p-bzip2-1.0.6/bin/bzcmp: line
16: sed: command not found
/gnu/store/s3c442d075fc8a0q0nspc9jjsgjq613p-bzip2-1.0.6/bin/bzcmp: line
40: mktemp: command not found
cannot create a temporary file
+ bzdiff shells.bz2 shells.bz2
/gnu/store/s3c442d075fc8a0q0nspc9jjsgjq613p-bzip2-1.0.6/bin/bzdiff: line
16: sed: command not found
/gnu/store/s3c442d075fc8a0q0nspc9jjsgjq613p-bzip2-1.0.6/bin/bzdiff: line
40: mktemp: command not found
cannot create a temporary file
+ bzegrep bash shells.bz2
/gnu/store/s3c442d075fc8a0q0nspc9jjsgjq613p-bzip2-1.0.6/bin/bzegrep:
line 11: sed: command not found
/gnu/store/s3c442d075fc8a0q0nspc9jjsgjq613p-bzip2-1.0.6/bin/bzegrep:
line 43: sed: command not found
/gnu/store/s3c442d075fc8a0q0nspc9jjsgjq613p-bzip2-1.0.6/bin/bzegrep:
line 63: grep: command not found
+ bzfgrep bash shells.bz2
/gnu/store/s3c442d075fc8a0q0nspc9jjsgjq613p-bzip2-1.0.6/bin/bzfgrep:
line 11: sed: command not found
/gnu/store/s3c442d075fc8a0q0nspc9jjsgjq613p-bzip2-1.0.6/bin/bzfgrep:
line 43: sed: command not found
/gnu/store/s3c442d075fc8a0q0nspc9jjsgjq613p-bzip2-1.0.6/bin/bzfgrep:
line 63: grep: command not found
+ bzgrep bash shells.bz2
/gnu/store/s3c442d075fc8a0q0nspc9jjsgjq613p-bzip2-1.0.6/bin/bzgrep: line
11: sed: command not found
/gnu/store/s3c442d075fc8a0q0nspc9jjsgjq613p-bzip2-1.0.6/bin/bzgrep: line
43: sed: command not found
/gnu/store/s3c442d075fc8a0q0nspc9jjsgjq613p-bzip2-1.0.6/bin/bzgrep: line
63: grep: command not found
+ bzless shells.bz2
/gnu/store/s3c442d075fc8a0q0nspc9jjsgjq613p-bzip2-1.0.6/bin/bzless: line
8: sed: command not found
------> shells.bz2 <------
/gnu/store/s3c442d075fc8a0q0nspc9jjsgjq613p-bzip2-1.0.6/bin/bzless: line
55: more: command not found
+ bzmore shells.bz2
/gnu/store/s3c442d075fc8a0q0nspc9jjsgjq613p-bzip2-1.0.6/bin/bzmore: line
8: sed: command not found
------> shells.bz2 <------
/gnu/store/s3c442d075fc8a0q0nspc9jjsgjq613p-bzip2-1.0.6/bin/bzmore: line
55: more: command not found


→ PATH=/gnu/store/i3zkljbz6ryqdlfc9gnpcjg964inp9l3-bzip2-1.0.6/bin
./test.sh
+ bzip2
+ bzcmp shells.bz2 shells.bz2
+ bzdiff shells.bz2 shells.bz2
+ bzegrep bash shells.bz2
/run/current-system/profile/bin/bash
/gnu/store/nylrl843mkfdwzz8cd5iabsib37vqc1j-bash-4.4.A/bin/bash
+ bzfgrep bash shells.bz2
/run/current-system/profile/bin/bash
/gnu/store/nylrl843mkfdwzz8cd5iabsib37vqc1j-bash-4.4.A/bin/bash
+ bzgrep bash shells.bz2
/run/current-system/profile/bin/bash
/gnu/store/nylrl843mkfdwzz8cd5iabsib37vqc1j-bash-4.4.A/bin/bash
+ bzless shells.bz2
------> shells.bz2 <------
+ bzmore shells.bz2
------> shells.bz2 <------
/gnu/store/i3zkljbz6ryqdlfc9gnpcjg964inp9l3-bzip2-1.0.6/bin/bzmore: line
55: more: command not found

[signature.asc (application/pgp-signature, attachment)]

Message #8 received at 26730 <at> debbugs.gnu.org (full text, mbox):

From: Christopher Baines <mail <at> cbaines.net>
To: 26730 <at> debbugs.gnu.org
Subject: [PATCH 2/2] gnu: bzip2: Patch bzip2 utilities.
Date: Mon,  1 May 2017 11:10:15 +0100

* gnu/packages/compression.scm (bzip2)[inputs]: Add grep, less, diffutils, sed
  and coreutils.
  [arguments]: Add patch-script phase.
---
 gnu/packages/compression.scm | 44 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 43 insertions(+), 1 deletion(-)

diff --git a/gnu/packages/compression.scm b/gnu/packages/compression.scm
index 031ecaad4..fe32b95bd 100644
--- a/gnu/packages/compression.scm
+++ b/gnu/packages/compression.scm
@@ -46,6 +46,7 @@
   #:use-module (gnu packages backup)
   #:use-module (gnu packages base)
   #:use-module (gnu packages check)
+  #:use-module (gnu packages less)
   #:use-module (gnu packages perl)
   #:use-module (gnu packages pkg-config)
   #:use-module (gnu packages python)
@@ -218,6 +219,12 @@ file; as a result, it is often used in conjunction with \"tar\", resulting in
                (base32
                 "1kfrc7f0ja9fdn6j1y6yir6li818npy6217hvr3wzmnmzhs8z152"))))
     (build-system gnu-build-system)
+    (inputs
+     `(("grep" ,grep)
+       ("less" ,less)
+       ("diff" ,diffutils)
+       ("sed" ,sed)
+       ("coreutils" ,coreutils)))
     (arguments
      `(#:modules ((guix build gnu-build-system)
                   (guix build utils)
@@ -252,7 +259,42 @@ file; as a result, it is often used in conjunction with \"tar\", resulting in
                                      base libdir)
                              (copy-file file
                                         (string-append libdir "/" base))))
-                         (find-files "." "^libbz2\\.so"))))))
+                         (find-files "." "^libbz2\\.so")))))
+         (add-after 'install-shared-lib 'patch-scripts
+           (lambda* (#:key outputs inputs #:allow-other-keys)
+             (let* ((out       (assoc-ref outputs "out"))
+                    (grep      (assoc-ref inputs "grep"))
+                    (less      (assoc-ref inputs "less"))
+                    (diff      (assoc-ref inputs "diff"))
+                    (sed       (assoc-ref inputs "sed"))
+                    (coreutils (assoc-ref inputs "coreutils")))
+               (substitute* (string-append out "/bin/bzgrep")
+                 (("/usr/bin:\\$PATH")
+                  (string-join
+                   (list (string-append grep "/bin")
+                         (string-append out "/bin")
+                         (string-append sed "/bin"))
+                   ":")))
+               (substitute* (string-append out "/bin/bzmore")
+                 (("/usr/bin") ;; Don't remove $PATH, as if bzmore is to work,
+                               ;; more must be on the PATH in the
+                               ;; environment. util-linux, which contains more
+                               ;; is not included here as there is a potential
+                               ;; issues with circular dependencies.
+                  (string-join
+                   (list (string-append less "/bin")
+                         (string-append sed "/bin")
+                         (string-append out "/bin"))
+                   ":")))
+               (substitute* (string-append out "/bin/bzdiff")
+                 (("/usr/bin:/bin:\\$PATH")
+                  (string-join
+                   (list (string-append diff "/bin")
+                         (string-append coreutils "/bin")
+                         (string-append out "/bin")
+                         (string-append sed "/bin"))
+                   ":"))
+                 (("/bin/rm") "rm"))))))
 
        #:make-flags (list (string-append "PREFIX="
                                          (assoc-ref %outputs "out")))
-- 
2.12.2

Message #11 received at 26730 <at> debbugs.gnu.org (full text, mbox):

From: Christopher Baines <mail <at> cbaines.net>
To: 26730 <at> debbugs.gnu.org
Subject: [PATCH 1/2] gnu: bzip2: Use 'modify-phases' syntax.
Date: Mon,  1 May 2017 11:10:14 +0100

* gnu/packages/compression.scm (bzip2)[arguments]: Use 'modify-phases' syntax.
---
 gnu/packages/compression.scm | 130 +++++++++++++++++++------------------------
 1 file changed, 58 insertions(+), 72 deletions(-)

diff --git a/gnu/packages/compression.scm b/gnu/packages/compression.scm
index 4793755c2..031ecaad4 100644
--- a/gnu/packages/compression.scm
+++ b/gnu/packages/compression.scm
@@ -207,84 +207,70 @@ file; as a result, it is often used in conjunction with \"tar\", resulting in
    (home-page "https://www.gnu.org/software/gzip/")))
 
 (define-public bzip2
-  (let ((build-shared-lib
-         ;; Build a shared library.
-         '(lambda* (#:key inputs #:allow-other-keys)
-            (patch-makefile-SHELL "Makefile-libbz2_so")
-            (zero? (system* "make" "-f" "Makefile-libbz2_so"))))
-        (install-shared-lib
-         '(lambda* (#:key outputs #:allow-other-keys)
-            (let* ((out    (assoc-ref outputs "out"))
-                   (libdir (string-append out "/lib")))
-              (for-each (lambda (file)
-                          (let ((base (basename file)))
-                            (format #t "installing `~a' to `~a'~%"
-                                    base libdir)
-                            (copy-file file
-                                       (string-append libdir "/" base))))
-                        (find-files "." "^libbz2\\.so")))))
-        (set-cross-environment
-         '(lambda* (#:key target #:allow-other-keys)
-            (substitute* (find-files "." "Makefile")
-              (("CC=.*$")
-               (string-append "CC = " target "-gcc\n"))
-              (("AR=.*$")
-               (string-append "AR = " target "-ar\n"))
-              (("RANLIB=.*$")
-               (string-append "RANLIB = " target "-ranlib\n"))
-              (("^all:(.*)test" _ prerequisites)
-               ;; Remove 'all' -> 'test' dependency.
-               (string-append "all:" prerequisites "\n"))))))
-    (package
-      (name "bzip2")
-      (version "1.0.6")
-      (source (origin
-               (method url-fetch)
-               (uri (string-append "http://www.bzip.org/" version "/bzip2-"
-                                   version ".tar.gz"))
-               (sha256
-                (base32
-                 "1kfrc7f0ja9fdn6j1y6yir6li818npy6217hvr3wzmnmzhs8z152"))))
-      (build-system gnu-build-system)
-      (arguments
-       `(#:modules ((guix build gnu-build-system)
-                    (guix build utils)
-                    (srfi srfi-1))
-         #:phases
-         ,(if (%current-target-system)
-
-              ;; Cross-compilation: use the cross tools.
-              `(alist-cons-before
-                'build 'build-shared-lib ,build-shared-lib
-                (alist-cons-after
-                 'install 'install-shared-lib ,install-shared-lib
-                 (alist-replace 'configure ,set-cross-environment
-                                %standard-phases)))
-
-              ;; Native compilation: build the shared library.
-              `(alist-cons-before
-                'build 'build-shared-lib ,build-shared-lib
-                (alist-cons-after
-                 'install 'install-shared-lib ,install-shared-lib
-                 (alist-delete 'configure %standard-phases))))
+  (package
+    (name "bzip2")
+    (version "1.0.6")
+    (source (origin
+              (method url-fetch)
+              (uri (string-append "http://www.bzip.org/" version "/bzip2-"
+                                  version ".tar.gz"))
+              (sha256
+               (base32
+                "1kfrc7f0ja9fdn6j1y6yir6li818npy6217hvr3wzmnmzhs8z152"))))
+    (build-system gnu-build-system)
+    (arguments
+     `(#:modules ((guix build gnu-build-system)
+                  (guix build utils)
+                  (srfi srfi-1))
+       #:phases
+       (modify-phases %standard-phases
+         (replace 'configure
+           (lambda* (#:key target #:allow-other-keys)
+             (if ,(%current-target-system)
+                 ;; Cross-compilation: use the cross tools.
+                 (substitute* (find-files "." "Makefile")
+                   (("CC=.*$")
+                    (string-append "CC = " target "-gcc\n"))
+                   (("AR=.*$")
+                    (string-append "AR = " target "-ar\n"))
+                   (("RANLIB=.*$")
+                    (string-append "RANLIB = " target "-ranlib\n"))
+                   (("^all:(.*)test" _ prerequisites)
+                    ;; Remove 'all' -> 'test' dependency.
+                    (string-append "all:" prerequisites "\n"))))))
+         (add-before 'build 'build-shared-lib
+           (lambda* (#:key inputs #:allow-other-keys)
+             (patch-makefile-SHELL "Makefile-libbz2_so")
+             (zero? (system* "make" "-f" "Makefile-libbz2_so"))))
+         (add-after 'install 'install-shared-lib
+           (lambda* (#:key outputs #:allow-other-keys)
+             (let* ((out    (assoc-ref outputs "out"))
+                    (libdir (string-append out "/lib")))
+               (for-each (lambda (file)
+                           (let ((base (basename file)))
+                             (format #t "installing `~a' to `~a'~%"
+                                     base libdir)
+                             (copy-file file
+                                        (string-append libdir "/" base))))
+                         (find-files "." "^libbz2\\.so"))))))
 
-         #:make-flags (list (string-append "PREFIX="
-                                           (assoc-ref %outputs "out")))
+       #:make-flags (list (string-append "PREFIX="
+                                         (assoc-ref %outputs "out")))
 
-         ;; Don't attempt to run the tests when cross-compiling.
-         ,@(if (%current-target-system)
-               '(#:tests? #f)
-               '())))
-      (synopsis "High-quality data compression program")
-      (description
-       "bzip2 is a freely available, patent free (see below), high-quality data
+       ;; Don't attempt to run the tests when cross-compiling.
+       ,@(if (%current-target-system)
+             '(#:tests? #f)
+             '())))
+    (synopsis "High-quality data compression program")
+    (description
+     "bzip2 is a freely available, patent free (see below), high-quality data
 compressor.  It typically compresses files to within 10% to 15% of the best
 available techniques (the PPM family of statistical compressors), whilst
 being around twice as fast at compression and six times faster at
 decompression.")
-      (license (license:non-copyleft "file://LICENSE"
-                                  "See LICENSE in the distribution."))
-      (home-page "http://www.bzip.org/"))))
+    (license (license:non-copyleft "file://LICENSE"
+                                   "See LICENSE in the distribution."))
+    (home-page "http://www.bzip.org/")))
 
 (define-public lbzip2
   (package
-- 
2.12.2

Message #14 received at 26730 <at> debbugs.gnu.org (full text, mbox):

From: Marius Bakke <mbakke <at> fastmail.com>
To: Christopher Baines <mail <at> cbaines.net>, 26730 <at> debbugs.gnu.org
Subject: Re: bug#26730: [PATCH] Fix bzip2 utilities
Date: Sat, 06 May 2017 17:38:45 +0200

[Message part 1 (text/plain, inline)]

Christopher Baines <mail <at> cbaines.net> writes:

> The bzip2 package includes wrappers around diff, grep and less/more.
> These shell scripts currently include /usr/bin (and sometimes /bin) on
> the PATH, and therefore fail if any of the commands that they rely on
> cannot be found.
>
> By substituting /usr/bin (and /bin) for the appropriate package paths,
> these scripts work much more reliably.

Most of these dependencies are available in environments where the bz*
tools will be executed. I think it would be better to simply remove the
absolute /usr/bin and /bin references such that grep, sed etc
invocations are picked up from PATH instead.

The "xz*" equivalent tools seem to do that. The rationale being that
bzip2 is often needed early in bootstrapping, and adding those inputs
would complicate the dependency tree. Although I admittedly haven't
looked at how bzip2 is used in Guix bootstrap. Thoughts?

[signature.asc (application/pgp-signature, inline)]

Message #17 received at 26730 <at> debbugs.gnu.org (full text, mbox):

From: ludo <at> gnu.org (Ludovic Courtès)
To: Marius Bakke <mbakke <at> fastmail.com>
Cc: Christopher Baines <mail <at> cbaines.net>, 26730 <at> debbugs.gnu.org
Subject: Re: bug#26730: [PATCH] Fix bzip2 utilities
Date: Tue, 09 May 2017 12:18:24 +0200

Marius Bakke <mbakke <at> fastmail.com> skribis:

> Christopher Baines <mail <at> cbaines.net> writes:
>
>> The bzip2 package includes wrappers around diff, grep and less/more.
>> These shell scripts currently include /usr/bin (and sometimes /bin) on
>> the PATH, and therefore fail if any of the commands that they rely on
>> cannot be found.
>>
>> By substituting /usr/bin (and /bin) for the appropriate package paths,
>> these scripts work much more reliably.
>
> Most of these dependencies are available in environments where the bz*
> tools will be executed. I think it would be better to simply remove the
> absolute /usr/bin and /bin references such that grep, sed etc
> invocations are picked up from PATH instead.

Agreed.  Otherwise we could end up retaining references to the bootstrap
Bash and Coreutils, for instance (remember that bzip2 is built early on,
in (gnu packages commencement)).

Ludo’.

Message #20 received at 26730 <at> debbugs.gnu.org (full text, mbox):

From: Christopher Baines <mail <at> cbaines.net>
To: 26730 <at> debbugs.gnu.org
Subject: [PATCH] gnu: bzip2: Patch bzip2 utilities.
Date: Mon, 15 May 2017 07:04:22 +0100

* gnu/packages/compression.scm (bzip2)[inputs]: Add grep, less, diffutils, sed
  and coreutils.
  [arguments]: Add patch-script phase.
---
 gnu/packages/compression.scm | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/gnu/packages/compression.scm b/gnu/packages/compression.scm
index 7d3a62e2f..829fd5053 100644
--- a/gnu/packages/compression.scm
+++ b/gnu/packages/compression.scm
@@ -252,7 +252,12 @@ file; as a result, it is often used in conjunction with \"tar\", resulting in
                                      base libdir)
                              (copy-file file
                                         (string-append libdir "/" base))))
-                         (find-files "." "^libbz2\\.so"))))))
+                         (find-files "." "^libbz2\\.so")))))
+         (add-after 'install-shared-lib 'patch-scripts
+           (lambda* (#:key outputs inputs #:allow-other-keys)
+             (let* ((out (assoc-ref outputs "out")))
+               (substitute* (string-append out "/bin/bzdiff")
+                 (("/bin/rm") "rm"))))))
 
        #:make-flags (list (string-append "PREFIX="
                                          (assoc-ref %outputs "out")))
-- 
2.13.0

Message #23 received at 26730 <at> debbugs.gnu.org (full text, mbox):

From: Christopher Baines <mail <at> cbaines.net>
To: 26730 <at> debbugs.gnu.org
Subject: [PATCH] gnu: bzip2: Patch bzip2 utilities.
Date: Mon, 15 May 2017 07:07:06 +0100

* gnu/packages/compression.scm (bzip2)[arguments]: Add patch-script phase to
  remove absolute reference to /bin/rm.
---
 gnu/packages/compression.scm | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/gnu/packages/compression.scm b/gnu/packages/compression.scm
index 7d3a62e2f..829fd5053 100644
--- a/gnu/packages/compression.scm
+++ b/gnu/packages/compression.scm
@@ -252,7 +252,12 @@ file; as a result, it is often used in conjunction with \"tar\", resulting in
                                      base libdir)
                              (copy-file file
                                         (string-append libdir "/" base))))
-                         (find-files "." "^libbz2\\.so"))))))
+                         (find-files "." "^libbz2\\.so")))))
+         (add-after 'install-shared-lib 'patch-scripts
+           (lambda* (#:key outputs inputs #:allow-other-keys)
+             (let* ((out (assoc-ref outputs "out")))
+               (substitute* (string-append out "/bin/bzdiff")
+                 (("/bin/rm") "rm"))))))
 
        #:make-flags (list (string-append "PREFIX="
                                          (assoc-ref %outputs "out")))
-- 
2.13.0

Message #26 received at 26730 <at> debbugs.gnu.org (full text, mbox):

From: Christopher Baines <mail <at> cbaines.net>
To: Ludovic Courtès <ludo <at> gnu.org>,
 Marius Bakke <mbakke <at> fastmail.com>
Cc: 26730 <at> debbugs.gnu.org
Subject: Re: bug#26730: [PATCH] Fix bzip2 utilities
Date: Mon, 15 May 2017 07:08:02 +0100

[Message part 1 (text/plain, inline)]

On 09/05/17 11:18, Ludovic Courtès wrote:
> Marius Bakke <mbakke <at> fastmail.com> skribis:
> 
>> Christopher Baines <mail <at> cbaines.net> writes:
>>
>>> The bzip2 package includes wrappers around diff, grep and less/more.
>>> These shell scripts currently include /usr/bin (and sometimes /bin) on
>>> the PATH, and therefore fail if any of the commands that they rely on
>>> cannot be found.
>>>
>>> By substituting /usr/bin (and /bin) for the appropriate package paths,
>>> these scripts work much more reliably.
>>
>> Most of these dependencies are available in environments where the bz*
>> tools will be executed. I think it would be better to simply remove the
>> absolute /usr/bin and /bin references such that grep, sed etc
>> invocations are picked up from PATH instead.
> 
> Agreed.  Otherwise we could end up retaining references to the bootstrap
> Bash and Coreutils, for instance (remember that bzip2 is built early on,
> in (gnu packages commencement)).

Ok, I've sent a couple of updates for the 2nd patch.

[signature.asc (application/pgp-signature, attachment)]

Message #29 received at 26730 <at> debbugs.gnu.org (full text, mbox):

From: Marius Bakke <mbakke <at> fastmail.com>
To: Christopher Baines <mail <at> cbaines.net>, 26730 <at> debbugs.gnu.org
Subject: Re: bug#26730: [PATCH 1/2] gnu: bzip2: Use 'modify-phases' syntax.
Date: Mon, 15 May 2017 17:43:06 +0200

[Message part 1 (text/plain, inline)]

Christopher Baines <mail <at> cbaines.net> writes:

> * gnu/packages/compression.scm (bzip2)[arguments]: Use 'modify-phases' syntax.

Thanks a lot for sorting this out! I only have a few nitpicks:

> +         (replace 'configure
> +           (lambda* (#:key target #:allow-other-keys)
> +             (if ,(%current-target-system)
> +                 ;; Cross-compilation: use the cross tools.
> +                 (substitute* (find-files "." "Makefile")
> +                   (("CC=.*$")
> +                    (string-append "CC = " target "-gcc\n"))
> +                   (("AR=.*$")
> +                    (string-append "AR = " target "-ar\n"))
> +                   (("RANLIB=.*$")
> +                    (string-append "RANLIB = " target "-ranlib\n"))
> +                   (("^all:(.*)test" _ prerequisites)
> +                    ;; Remove 'all' -> 'test' dependency.
> +                    (string-append "all:" prerequisites "\n"))))))

Noob question: What is returned here when (%current-target-system) is
false? Can we make it more explicit? We try to make sure all phases end
on a #t.

> +         (add-after 'install 'install-shared-lib
> +           (lambda* (#:key outputs #:allow-other-keys)
> +             (let* ((out    (assoc-ref outputs "out"))
> +                    (libdir (string-append out "/lib")))
> +               (for-each (lambda (file)
> +                           (let ((base (basename file)))
> +                             (format #t "installing `~a' to `~a'~%"
> +                                     base libdir)
> +                             (copy-file file
> +                                        (string-append libdir "/" base))))
> +                         (find-files "." "^libbz2\\.so"))))))

Similarly, if you send an updated patch, can you add a #t at the end of
this phase, since "for-each" has an unspecified return value? Otherwise
I can do so in a follow-up commit.

Apart from these "added-value" nitpicks LGTM. Tricky one!

[signature.asc (application/pgp-signature, inline)]

Message #32 received at 26730 <at> debbugs.gnu.org (full text, mbox):

From: Marius Bakke <mbakke <at> fastmail.com>
To: Christopher Baines <mail <at> cbaines.net>, 26730 <at> debbugs.gnu.org
Subject: Re: bug#26730: [PATCH] gnu: bzip2: Patch bzip2 utilities.
Date: Mon, 15 May 2017 17:46:05 +0200

[Message part 1 (text/plain, inline)]

Christopher Baines <mail <at> cbaines.net> writes:

> * gnu/packages/compression.scm (bzip2)[arguments]: Add patch-script phase to
>   remove absolute reference to /bin/rm.

Thanks for catching this! (substitute* ...) has an unspecified return
value, so I'll add a #t at the end of this new phase before pushing.

Waiting for feedback on the modify-phases patch first. We still have
~two weeks until core-updates starts rolling again, so no rush ;-)

[signature.asc (application/pgp-signature, inline)]

Message #35 received at 26730 <at> debbugs.gnu.org (full text, mbox):

From: Christopher Baines <mail <at> cbaines.net>
To: 26730 <at> debbugs.gnu.org
Subject: [PATCH 3/3] gnu: bzip2: Patch bzip2 utilities.
Date: Tue, 16 May 2017 21:36:33 +0100

* gnu/packages/compression.scm (bzip2)[arguments]: Add patch-script phase to
  remove absolute reference to /bin/rm.
---
 gnu/packages/compression.scm | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/gnu/packages/compression.scm b/gnu/packages/compression.scm
index 0ce9d88b7..3d2adcda5 100644
--- a/gnu/packages/compression.scm
+++ b/gnu/packages/compression.scm
@@ -254,6 +254,12 @@ file; as a result, it is often used in conjunction with \"tar\", resulting in
                              (copy-file file
                                         (string-append libdir "/" base))))
                          (find-files "." "^libbz2\\.so")))
+             #t))
+         (add-after 'install-shared-lib 'patch-scripts
+           (lambda* (#:key outputs inputs #:allow-other-keys)
+             (let* ((out (assoc-ref outputs "out")))
+               (substitute* (string-append out "/bin/bzdiff")
+                 (("/bin/rm") "rm")))
              #t)))
 
        #:make-flags (list (string-append "PREFIX="
-- 
2.13.0

Message #38 received at 26730 <at> debbugs.gnu.org (full text, mbox):

From: Christopher Baines <mail <at> cbaines.net>
To: 26730 <at> debbugs.gnu.org
Subject: [PATCH 2/3] gnu: bzip2: Add explicit return value for 2 phases.
Date: Tue, 16 May 2017 21:36:32 +0100

* gnu/packages/compression.scm (bzip2)[arguments]: Add explicit return values
  to the configure and build-shared-lib phases.
---
 gnu/packages/compression.scm | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/gnu/packages/compression.scm b/gnu/packages/compression.scm
index 7d3a62e2f..0ce9d88b7 100644
--- a/gnu/packages/compression.scm
+++ b/gnu/packages/compression.scm
@@ -237,7 +237,8 @@ file; as a result, it is often used in conjunction with \"tar\", resulting in
                     (string-append "RANLIB = " target "-ranlib\n"))
                    (("^all:(.*)test" _ prerequisites)
                     ;; Remove 'all' -> 'test' dependency.
-                    (string-append "all:" prerequisites "\n"))))))
+                    (string-append "all:" prerequisites "\n")))
+                 #t)))
          (add-before 'build 'build-shared-lib
            (lambda* (#:key inputs #:allow-other-keys)
              (patch-makefile-SHELL "Makefile-libbz2_so")
@@ -252,7 +253,8 @@ file; as a result, it is often used in conjunction with \"tar\", resulting in
                                      base libdir)
                              (copy-file file
                                         (string-append libdir "/" base))))
-                         (find-files "." "^libbz2\\.so"))))))
+                         (find-files "." "^libbz2\\.so")))
+             #t)))
 
        #:make-flags (list (string-append "PREFIX="
                                          (assoc-ref %outputs "out")))
-- 
2.13.0

Message #41 received at 26730 <at> debbugs.gnu.org (full text, mbox):

From: Christopher Baines <mail <at> cbaines.net>
To: 26730 <at> debbugs.gnu.org
Subject: [PATCH 1/3] gnu: bzip2: Use 'modify-phases' syntax.
Date: Tue, 16 May 2017 21:36:31 +0100

* gnu/packages/compression.scm (bzip2)[arguments]: Use 'modify-phases' syntax.
---
 gnu/packages/compression.scm | 130 +++++++++++++++++++------------------------
 1 file changed, 58 insertions(+), 72 deletions(-)

diff --git a/gnu/packages/compression.scm b/gnu/packages/compression.scm
index 11db2a66f..7d3a62e2f 100644
--- a/gnu/packages/compression.scm
+++ b/gnu/packages/compression.scm
@@ -207,84 +207,70 @@ file; as a result, it is often used in conjunction with \"tar\", resulting in
    (home-page "https://www.gnu.org/software/gzip/")))
 
 (define-public bzip2
-  (let ((build-shared-lib
-         ;; Build a shared library.
-         '(lambda* (#:key inputs #:allow-other-keys)
-            (patch-makefile-SHELL "Makefile-libbz2_so")
-            (zero? (system* "make" "-f" "Makefile-libbz2_so"))))
-        (install-shared-lib
-         '(lambda* (#:key outputs #:allow-other-keys)
-            (let* ((out    (assoc-ref outputs "out"))
-                   (libdir (string-append out "/lib")))
-              (for-each (lambda (file)
-                          (let ((base (basename file)))
-                            (format #t "installing `~a' to `~a'~%"
-                                    base libdir)
-                            (copy-file file
-                                       (string-append libdir "/" base))))
-                        (find-files "." "^libbz2\\.so")))))
-        (set-cross-environment
-         '(lambda* (#:key target #:allow-other-keys)
-            (substitute* (find-files "." "Makefile")
-              (("CC=.*$")
-               (string-append "CC = " target "-gcc\n"))
-              (("AR=.*$")
-               (string-append "AR = " target "-ar\n"))
-              (("RANLIB=.*$")
-               (string-append "RANLIB = " target "-ranlib\n"))
-              (("^all:(.*)test" _ prerequisites)
-               ;; Remove 'all' -> 'test' dependency.
-               (string-append "all:" prerequisites "\n"))))))
-    (package
-      (name "bzip2")
-      (version "1.0.6")
-      (source (origin
-               (method url-fetch)
-               (uri (string-append "http://www.bzip.org/" version "/bzip2-"
-                                   version ".tar.gz"))
-               (sha256
-                (base32
-                 "1kfrc7f0ja9fdn6j1y6yir6li818npy6217hvr3wzmnmzhs8z152"))))
-      (build-system gnu-build-system)
-      (arguments
-       `(#:modules ((guix build gnu-build-system)
-                    (guix build utils)
-                    (srfi srfi-1))
-         #:phases
-         ,(if (%current-target-system)
-
-              ;; Cross-compilation: use the cross tools.
-              `(alist-cons-before
-                'build 'build-shared-lib ,build-shared-lib
-                (alist-cons-after
-                 'install 'install-shared-lib ,install-shared-lib
-                 (alist-replace 'configure ,set-cross-environment
-                                %standard-phases)))
-
-              ;; Native compilation: build the shared library.
-              `(alist-cons-before
-                'build 'build-shared-lib ,build-shared-lib
-                (alist-cons-after
-                 'install 'install-shared-lib ,install-shared-lib
-                 (alist-delete 'configure %standard-phases))))
+  (package
+    (name "bzip2")
+    (version "1.0.6")
+    (source (origin
+              (method url-fetch)
+              (uri (string-append "http://www.bzip.org/" version "/bzip2-"
+                                  version ".tar.gz"))
+              (sha256
+               (base32
+                "1kfrc7f0ja9fdn6j1y6yir6li818npy6217hvr3wzmnmzhs8z152"))))
+    (build-system gnu-build-system)
+    (arguments
+     `(#:modules ((guix build gnu-build-system)
+                  (guix build utils)
+                  (srfi srfi-1))
+       #:phases
+       (modify-phases %standard-phases
+         (replace 'configure
+           (lambda* (#:key target #:allow-other-keys)
+             (if ,(%current-target-system)
+                 ;; Cross-compilation: use the cross tools.
+                 (substitute* (find-files "." "Makefile")
+                   (("CC=.*$")
+                    (string-append "CC = " target "-gcc\n"))
+                   (("AR=.*$")
+                    (string-append "AR = " target "-ar\n"))
+                   (("RANLIB=.*$")
+                    (string-append "RANLIB = " target "-ranlib\n"))
+                   (("^all:(.*)test" _ prerequisites)
+                    ;; Remove 'all' -> 'test' dependency.
+                    (string-append "all:" prerequisites "\n"))))))
+         (add-before 'build 'build-shared-lib
+           (lambda* (#:key inputs #:allow-other-keys)
+             (patch-makefile-SHELL "Makefile-libbz2_so")
+             (zero? (system* "make" "-f" "Makefile-libbz2_so"))))
+         (add-after 'install 'install-shared-lib
+           (lambda* (#:key outputs #:allow-other-keys)
+             (let* ((out    (assoc-ref outputs "out"))
+                    (libdir (string-append out "/lib")))
+               (for-each (lambda (file)
+                           (let ((base (basename file)))
+                             (format #t "installing `~a' to `~a'~%"
+                                     base libdir)
+                             (copy-file file
+                                        (string-append libdir "/" base))))
+                         (find-files "." "^libbz2\\.so"))))))
 
-         #:make-flags (list (string-append "PREFIX="
-                                           (assoc-ref %outputs "out")))
+       #:make-flags (list (string-append "PREFIX="
+                                         (assoc-ref %outputs "out")))
 
-         ;; Don't attempt to run the tests when cross-compiling.
-         ,@(if (%current-target-system)
-               '(#:tests? #f)
-               '())))
-      (synopsis "High-quality data compression program")
-      (description
-       "bzip2 is a freely available, patent free (see below), high-quality data
+       ;; Don't attempt to run the tests when cross-compiling.
+       ,@(if (%current-target-system)
+             '(#:tests? #f)
+             '())))
+    (synopsis "High-quality data compression program")
+    (description
+     "bzip2 is a freely available, patent free (see below), high-quality data
 compressor.  It typically compresses files to within 10% to 15% of the best
 available techniques (the PPM family of statistical compressors), whilst
 being around twice as fast at compression and six times faster at
 decompression.")
-      (license (license:non-copyleft "file://LICENSE"
-                                  "See LICENSE in the distribution."))
-      (home-page "http://www.bzip.org/"))))
+    (license (license:non-copyleft "file://LICENSE"
+                                   "See LICENSE in the distribution."))
+    (home-page "http://www.bzip.org/")))
 
 (define-public lbzip2
   (package
-- 
2.13.0

Message #44 received at 26730 <at> debbugs.gnu.org (full text, mbox):

From: Christopher Baines <mail <at> cbaines.net>
To: Marius Bakke <mbakke <at> fastmail.com>, 26730 <at> debbugs.gnu.org
Subject: Re: bug#26730: [PATCH] gnu: bzip2: Patch bzip2 utilities.
Date: Tue, 16 May 2017 21:41:24 +0100

[Message part 1 (text/plain, inline)]

On 15/05/17 16:46, Marius Bakke wrote:
> Christopher Baines <mail <at> cbaines.net> writes:
> 
>> * gnu/packages/compression.scm (bzip2)[arguments]: Add patch-script phase to
>>   remove absolute reference to /bin/rm.
> 
> Thanks for catching this! (substitute* ...) has an unspecified return
> value, so I'll add a #t at the end of this new phase before pushing.
> 
> Waiting for feedback on the modify-phases patch first. We still have
> ~two weeks until core-updates starts rolling again, so no rush ;-)

I've resent the patches with these changes, just in case that helps.

Thanks for your quick feedback,

Chris

[signature.asc (application/pgp-signature, attachment)]

Message #49 received at 26730-done <at> debbugs.gnu.org (full text, mbox):

From: Marius Bakke <mbakke <at> fastmail.com>
To: Christopher Baines <mail <at> cbaines.net>, 26730-done <at> debbugs.gnu.org
Subject: Re: bug#26730: [PATCH] gnu: bzip2: Patch bzip2 utilities.
Date: Wed, 17 May 2017 16:58:46 +0200

[Message part 1 (text/plain, inline)]

Christopher Baines <mail <at> cbaines.net> writes:

> On 15/05/17 16:46, Marius Bakke wrote:
>> Christopher Baines <mail <at> cbaines.net> writes:
>> 
>>> * gnu/packages/compression.scm (bzip2)[arguments]: Add patch-script phase to
>>>   remove absolute reference to /bin/rm.
>> 
>> Thanks for catching this! (substitute* ...) has an unspecified return
>> value, so I'll add a #t at the end of this new phase before pushing.
>> 
>> Waiting for feedback on the modify-phases patch first. We still have
>> ~two weeks until core-updates starts rolling again, so no rush ;-)
>
> I've resent the patches with these changes, just in case that helps.

Pushed to 'core-updates', thank you! Note: I squashed the first two.

[signature.asc (application/pgp-signature, inline)]

Message #52 received at 26730 <at> debbugs.gnu.org (full text, mbox):

From: Christopher Baines <mail <at> cbaines.net>
To: Marius Bakke <mbakke <at> fastmail.com>, 26730 <at> debbugs.gnu.org
Subject: Re: bug#26730: [PATCH] gnu: bzip2: Patch bzip2 utilities.
Date: Thu, 18 May 2017 19:46:19 +0100

[Message part 1 (text/plain, inline)]

On 17/05/17 15:58, Marius Bakke wrote:
> Christopher Baines <mail <at> cbaines.net> writes:
> 
>> On 15/05/17 16:46, Marius Bakke wrote:
>>> Christopher Baines <mail <at> cbaines.net> writes:
>>>
>>>> * gnu/packages/compression.scm (bzip2)[arguments]: Add patch-script phase to
>>>>   remove absolute reference to /bin/rm.
>>>
>>> Thanks for catching this! (substitute* ...) has an unspecified return
>>> value, so I'll add a #t at the end of this new phase before pushing.
>>>
>>> Waiting for feedback on the modify-phases patch first. We still have
>>> ~two weeks until core-updates starts rolling again, so no rush ;-)
>>
>> I've resent the patches with these changes, just in case that helps.
> 
> Pushed to 'core-updates', thank you! Note: I squashed the first two.

Great, thanks for your review :)

[signature.asc (application/pgp-signature, attachment)]

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.