GNU bug report logs - #32153
[PATCH 0/2]: ruby-build-system: Error or return #t from all phases.

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 32153 in the body.
You can then email your comments to 32153 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 0/2]: ruby-build-system: Error or return #t from all phases.
Date: Sat, 14 Jul 2018 12:05:43 +0100

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

I'm trying to continue along with the Rails packaging (#30689), but I
noticed that currently if the tests fail for packages using the ruby
build system, then the package build doesn't fail.

These patches should get most of the packages using the ruby build
system to raise exceptions when there are errors, and return #t
otherwise.

I'm hopeful that this can be merged directly in to master, I build 180
packages in not that much time at all to test this change [1].

1: ./pre-inst-env guix package -s ruby- | recsel -P name | xargs ./pre-inst-env guix build


Christopher Baines (2):
  ruby-build-system: Error or return #t from all phases.
  gnu: ruby-options: Return #t from set-LIB phase.

 gnu/packages/ruby.scm            |   3 +-
 guix/build/ruby-build-system.scm | 109 ++++++++++++++++---------------
 2 files changed, 58 insertions(+), 54 deletions(-)

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

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

From: Christopher Baines <mail <at> cbaines.net>
To: 32153 <at> debbugs.gnu.org
Subject: [PATCH 1/2] ruby-build-system: Error or return #t from all phases.
Date: Sat, 14 Jul 2018 12:10:21 +0100

Previously, if the tests didn't pass, the check phase would evaluate to #f,
but the package would be built sucessfully. This changes all the phases to
raise exceptions if errors are encountered, and return #t otherwise.

This involves using invoke rather than system*, so that exceptions are raised
if the program exits with a status other than 0, and also returning #t at the
end of functions.

* gnu/build/ruby-build-system.scm (unpack): Use invoke rather than system*,
and return #t at the end.
(build, check): Use invoke rather than system*.
(install): Remove the use of "and", and rewrite the error handling to raise an
exception.
(wrap): Return #t.
---
 guix/build/ruby-build-system.scm | 109 ++++++++++++++++---------------
 1 file changed, 56 insertions(+), 53 deletions(-)

diff --git a/guix/build/ruby-build-system.scm b/guix/build/ruby-build-system.scm
index abef6937b..0a2b223db 100644
--- a/guix/build/ruby-build-system.scm
+++ b/guix/build/ruby-build-system.scm
@@ -52,18 +52,19 @@ directory."
 (define* (unpack #:key source #:allow-other-keys)
   "Unpack the gem SOURCE and enter the resulting directory."
   (if (gem-archive? source)
-      (and (zero? (system* "gem" "unpack" source))
-           ;; The unpacked gem directory is named the same as the archive,
-           ;; sans the ".gem" extension.  It is renamed to simply "gem" in an
-           ;; effort to keep file names shorter to avoid UNIX-domain socket
-           ;; file names and shebangs that exceed the system's fixed maximum
-           ;; length when running test suites.
-           (let ((dir (match:substring (string-match "^(.*)\\.gem$"
-                                                     (basename source))
-                                       1)))
-             (rename-file dir "gem")
-             (chdir "gem")
-             #t))
+      (begin
+        (invoke "gem" "unpack" source)
+        ;; The unpacked gem directory is named the same as the archive,
+        ;; sans the ".gem" extension.  It is renamed to simply "gem" in an
+        ;; effort to keep file names shorter to avoid UNIX-domain socket
+        ;; file names and shebangs that exceed the system's fixed maximum
+        ;; length when running test suites.
+        (let ((dir (match:substring (string-match "^(.*)\\.gem$"
+                                                  (basename source))
+                                    1)))
+          (rename-file dir "gem")
+          (chdir "gem"))
+        #t)
       ;; Use GNU unpack strategy for things that aren't gem archives.
       (gnu:unpack #:source source)))
 
@@ -104,7 +105,8 @@ generate the files list."
                   (write-char (read-char pipe) out))))
             #t)
           (lambda ()
-            (close-pipe pipe)))))))
+            (close-pipe pipe)))))
+    #t))
 
 (define* (build #:key source #:allow-other-keys)
   "Build a new gem using the gemspec from the SOURCE gem."
@@ -112,13 +114,13 @@ generate the files list."
   ;; Build a new gem from the current working directory.  This also allows any
   ;; dynamic patching done in previous phases to be present in the installed
   ;; gem.
-  (zero? (system* "gem" "build" (first-gemspec))))
+  (invoke "gem" "build" (first-gemspec)))
 
 (define* (check #:key tests? test-target #:allow-other-keys)
   "Run the gem's test suite rake task TEST-TARGET.  Skip the tests if TESTS?
 is #f."
   (if tests?
-      (zero? (system* "rake" test-target))
+      (invoke "rake" test-target)
       #t))
 
 (define* (install #:key inputs outputs (gem-flags '())
@@ -137,43 +139,43 @@ GEM-FLAGS are passed to the 'gem' invokation, if present."
                               0
                               (- (string-length gem-file-basename) 4))))
     (setenv "GEM_VENDOR" vendor-dir)
-    (and (let ((install-succeeded?
-                (zero?
-                 (apply system* "gem" "install" gem-file
-                        "--local" "--ignore-dependencies" "--vendor"
-                        ;; Executables should go into /bin, not
-                        ;; /lib/ruby/gems.
-                        "--bindir" (string-append out "/bin")
-                        gem-flags))))
-           (or install-succeeded?
-               (begin
-                 (simple-format #t "installation failed\n")
-                 (let ((failed-output-dir (string-append (getcwd) "/out")))
-                   (mkdir failed-output-dir)
-                   (copy-recursively out failed-output-dir))
-                 #f)))
-         (begin
-           ;; Remove the cached gem file as this is unnecessary and contains
-           ;; timestamped files rendering builds not reproducible.
-           (let ((cached-gem (string-append vendor-dir "/cache/" gem-file)))
-             (log-file-deletion cached-gem)
-             (delete-file cached-gem))
-           ;; For gems with native extensions, several Makefile-related files
-           ;; are created that contain timestamps or other elements making
-           ;; them not reproducible.  They are unnecessary so we remove them.
-           (if (file-exists? (string-append vendor-dir "/ext"))
-               (begin
-                 (for-each (lambda (file)
-                             (log-file-deletion file)
-                             (delete-file file))
-                           (append
-                            (find-files (string-append vendor-dir "/doc")
-                                        "page-Makefile.ri")
-                            (find-files (string-append vendor-dir "/extensions")
-                                        "gem_make.out")
-                            (find-files (string-append vendor-dir "/ext")
-                                        "Makefile")))))
-           #t))))
+
+    (or (zero?
+         (apply system* "gem" "install" gem-file
+                "--local" "--ignore-dependencies" "--vendor"
+                ;; Executables should go into /bin, not
+                ;; /lib/ruby/gems.
+                "--bindir" (string-append out "/bin")
+                gem-flags))
+        (begin
+          (let ((failed-output-dir (string-append (getcwd) "/out")))
+            (mkdir failed-output-dir)
+            (copy-recursively out failed-output-dir))
+          (error "installation failed")))
+
+    ;; Remove the cached gem file as this is unnecessary and contains
+    ;; timestamped files rendering builds not reproducible.
+    (let ((cached-gem (string-append vendor-dir "/cache/" gem-file)))
+      (log-file-deletion cached-gem)
+      (delete-file cached-gem))
+
+    ;; For gems with native extensions, several Makefile-related files
+    ;; are created that contain timestamps or other elements making
+    ;; them not reproducible.  They are unnecessary so we remove them.
+    (if (file-exists? (string-append vendor-dir "/ext"))
+        (begin
+          (for-each (lambda (file)
+                      (log-file-deletion file)
+                      (delete-file file))
+                    (append
+                     (find-files (string-append vendor-dir "/doc")
+                                 "page-Makefile.ri")
+                     (find-files (string-append vendor-dir "/extensions")
+                                 "gem_make.out")
+                     (find-files (string-append vendor-dir "/ext")
+                                 "Makefile")))))
+
+    #t))
 
 (define* (wrap-ruby-program prog #:key (gem-clear-paths #t) #:rest vars)
   "Make a wrapper for PROG.  VARS should look like this:
@@ -301,7 +303,8 @@ extended with definitions for VARS."
                 (let ((files (list-of-files dir)))
                   (for-each (cut wrap-ruby-program <> var)
                             files)))
-              bindirs)))
+              bindirs))
+  #t)
 
 (define (log-file-deletion file)
   (display (string-append "deleting '" file "' for reproducibility\n")))
-- 
2.17.1

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

From: Christopher Baines <mail <at> cbaines.net>
To: 32153 <at> debbugs.gnu.org
Subject: [PATCH 2/2] gnu: ruby-options: Return #t from set-LIB phase.
Date: Sat, 14 Jul 2018 12:10:22 +0100

* gnu/packages/ruby.scm (ruby-options)[arguments]: Return #t from the set-LIB
phase.
---
 gnu/packages/ruby.scm | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/gnu/packages/ruby.scm b/gnu/packages/ruby.scm
index 1602fd5d5..9a74f16c0 100644
--- a/gnu/packages/ruby.scm
+++ b/gnu/packages/ruby.scm
@@ -883,7 +883,8 @@ complexity.")
            (lambda _
              ;; This is used in the Rakefile, and setting it avoids an issue
              ;; with running the tests.
-             (setenv "LIB" "options"))))))
+             (setenv "LIB" "options")
+             #t)))))
     (synopsis "Ruby library to parse options from *args cleanly")
     (description
      "The @code{options} library helps with parsing keyword options in Ruby
-- 
2.17.1

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

From: Marius Bakke <mbakke <at> fastmail.com>
To: Christopher Baines <mail <at> cbaines.net>, 32153 <at> debbugs.gnu.org
Subject: Re: [bug#32153] [PATCH 1/2] ruby-build-system: Error or return #t
 from all phases.
Date: Sun, 15 Jul 2018 01:11:06 +0200

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

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

> Previously, if the tests didn't pass, the check phase would evaluate to #f,
> but the package would be built sucessfully. This changes all the phases to
> raise exceptions if errors are encountered, and return #t otherwise.
>
> This involves using invoke rather than system*, so that exceptions are raised
> if the program exits with a status other than 0, and also returning #t at the
> end of functions.
>
> * gnu/build/ruby-build-system.scm (unpack): Use invoke rather than system*,
> and return #t at the end.
> (build, check): Use invoke rather than system*.
> (install): Remove the use of "and", and rewrite the error handling to raise an
> exception.
> (wrap): Return #t.

Thanks!  The changes LGTM.  I will suggest a micro-improvement not
related to this patch since it was there from before:

> +    ;; For gems with native extensions, several Makefile-related files
> +    ;; are created that contain timestamps or other elements making
> +    ;; them not reproducible.  They are unnecessary so we remove them.
> +    (if (file-exists? (string-append vendor-dir "/ext"))
> +        (begin
> +          (for-each (lambda (file)
> +                      (log-file-deletion file)
> +                      (delete-file file))
> +                    (append
> +                     (find-files (string-append vendor-dir "/doc")
> +                                 "page-Makefile.ri")
> +                     (find-files (string-append vendor-dir "/extensions")
> +                                 "gem_make.out")
> +                     (find-files (string-append vendor-dir "/ext")
> +                                 "Makefile")))))
> +
> +    #t))

I was confused whether the #t was the "else" clause for the "if"
expression until I realized it didn't have one.

Could you turn this into a (when (file-exists? ...) (for-each ...))
while at it?  It also makes the (begin ...) redundant.

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

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

From: Marius Bakke <mbakke <at> fastmail.com>
To: Christopher Baines <mail <at> cbaines.net>, 32153 <at> debbugs.gnu.org
Subject: Re: [bug#32153] [PATCH 0/2]: ruby-build-system: Error or return #t
 from all phases.
Date: Sun, 15 Jul 2018 01:18:06 +0200

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

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

> I'm trying to continue along with the Rails packaging (#30689), but I
> noticed that currently if the tests fail for packages using the ruby
> build system, then the package build doesn't fail.
>
> These patches should get most of the packages using the ruby build
> system to raise exceptions when there are errors, and return #t
> otherwise.
>
> I'm hopeful that this can be merged directly in to master, I build 180
> packages in not that much time at all to test this change [1].
>
> 1: ./pre-inst-env guix package -s ruby- | recsel -P name | xargs ./pre-inst-env guix build

Thank you for fixing it!  Since this only affects gems, not ruby itself
(which has ~900 dependencies), I think it can go on 'master' too[1].

[1] guix refresh -l $(guix package -s ^ruby- | recsel -P name)

By the way, Ruby 2.5 is out.  Are you willing to try upgrading it for
'staging'?  :-)

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

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

From: Christopher Baines <mail <at> cbaines.net>
To: Marius Bakke <mbakke <at> fastmail.com>
Cc: 32153 <at> debbugs.gnu.org
Subject: Re: [bug#32153] [PATCH 0/2]: ruby-build-system: Error or return #t
 from all phases.
Date: Sun, 15 Jul 2018 09:23:29 +0100

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

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

> Christopher Baines <mail <at> cbaines.net> writes:
>
>> I'm trying to continue along with the Rails packaging (#30689), but I
>> noticed that currently if the tests fail for packages using the ruby
>> build system, then the package build doesn't fail.
>>
>> These patches should get most of the packages using the ruby build
>> system to raise exceptions when there are errors, and return #t
>> otherwise.
>>
>> I'm hopeful that this can be merged directly in to master, I build 180
>> packages in not that much time at all to test this change [1].
>>
>> 1: ./pre-inst-env guix package -s ruby- | recsel -P name | xargs ./pre-inst-env guix build
>
> Thank you for fixing it!  Since this only affects gems, not ruby itself
> (which has ~900 dependencies), I think it can go on 'master' too[1].
>
> [1] guix refresh -l $(guix package -s ^ruby- | recsel -P name)

Great :)

> By the way, Ruby 2.5 is out.  Are you willing to try upgrading it for
> 'staging'?  :-)

Sure :) I'll take a look.

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

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

From: Christopher Baines <mail <at> cbaines.net>
To: Marius Bakke <mbakke <at> fastmail.com>
Cc: 32153-done <at> debbugs.gnu.org
Subject: Re: [bug#32153] [PATCH 1/2] ruby-build-system: Error or return #t
 from all phases.
Date: Sun, 15 Jul 2018 22:27:03 +0100

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

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

> Christopher Baines <mail <at> cbaines.net> writes:
>
>> Previously, if the tests didn't pass, the check phase would evaluate to #f,
>> but the package would be built sucessfully. This changes all the phases to
>> raise exceptions if errors are encountered, and return #t otherwise.
>>
>> This involves using invoke rather than system*, so that exceptions are raised
>> if the program exits with a status other than 0, and also returning #t at the
>> end of functions.
>>
>> * gnu/build/ruby-build-system.scm (unpack): Use invoke rather than system*,
>> and return #t at the end.
>> (build, check): Use invoke rather than system*.
>> (install): Remove the use of "and", and rewrite the error handling to raise an
>> exception.
>> (wrap): Return #t.
>
> Thanks!  The changes LGTM.  I will suggest a micro-improvement not
> related to this patch since it was there from before:
>
>> +    ;; For gems with native extensions, several Makefile-related files
>> +    ;; are created that contain timestamps or other elements making
>> +    ;; them not reproducible.  They are unnecessary so we remove them.
>> +    (if (file-exists? (string-append vendor-dir "/ext"))
>> +        (begin
>> +          (for-each (lambda (file)
>> +                      (log-file-deletion file)
>> +                      (delete-file file))
>> +                    (append
>> +                     (find-files (string-append vendor-dir "/doc")
>> +                                 "page-Makefile.ri")
>> +                     (find-files (string-append vendor-dir "/extensions")
>> +                                 "gem_make.out")
>> +                     (find-files (string-append vendor-dir "/ext")
>> +                                 "Makefile")))))
>> +
>> +    #t))
>
> I was confused whether the #t was the "else" clause for the "if"
> expression until I realized it didn't have one.
>
> Could you turn this into a (when (file-exists? ...) (for-each ...))
> while at it?  It also makes the (begin ...) redundant.

I've made this change, and pushed the patches to master, thanks again
for taking a look :)

Chris

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

Previous Next

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