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

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

[Christopher Baines]

[-- Attachment #1: Type: text/plain, Size: 862 bytes --]

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(-)

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

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

[Christopher Baines]

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

[bug#32153] [PATCH 2/2] gnu: ruby-options: Return #t from set-LIB phase.

[Christopher Baines]

* 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

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

[Marius Bakke]

[-- Attachment #1: Type: text/plain, Size: 1943 bytes --]

Christopher Baines <mail@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.

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

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

[Marius Bakke]

[-- Attachment #1: Type: text/plain, Size: 921 bytes --]

Christopher Baines <mail@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'?  :-)

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

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

[Christopher Baines]

[-- Attachment #1: Type: text/plain, Size: 1030 bytes --]


Marius Bakke <mbakke@fastmail.com> writes:

> Christopher Baines <mail@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.

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

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

[Christopher Baines]

[-- Attachment #1: Type: text/plain, Size: 2137 bytes --]


Marius Bakke <mbakke@fastmail.com> writes:

> Christopher Baines <mail@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

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