[bug#29745] [PATCH 0/3] Disallow phase returning <unspecified>.

[bug#29745] [PATCH 0/3] Disallow phase returning <unspecified>.

[Arun Isaac]

Patch 1 modifies gnu-build-system to explicitly check the return value of each
phase for an unspecified value, and if found consider that phase to have
failed.

Patch 2 fixes several phases in gnu-build-system by making them return #t
instead of an unspecified value.

Patch 3 fixes the phases in several packages to return #t instead of an
unspecified value. I was testing patch 1, by trying to build bash. In doing
so, I found that the custom phases of many packages were not properly
returning #t, and fixed all those packages. No doubt, there are many more
packages that need fixing.

Arun Isaac (3):
  build: gnu-build-system: Disallow phase returning <unspecified>.
  build: gnu-build-system: Return #t from phases.
  gnu: Return #t from phases.

 gnu/packages/attr.scm           |  3 ++-
 gnu/packages/autotools.scm      |  3 ++-
 gnu/packages/base.scm           |  3 ++-
 gnu/packages/bash.scm           |  3 ++-
 gnu/packages/commencement.scm   | 15 ++++++++++-----
 gnu/packages/gcc.scm            |  3 ++-
 gnu/packages/libffi.scm         |  3 ++-
 gnu/packages/m4.scm             |  3 ++-
 gnu/packages/ncurses.scm        |  6 ++++--
 gnu/packages/perl.scm           |  3 ++-
 gnu/packages/readline.scm       |  3 ++-
 guix/build/gnu-build-system.scm | 18 +++++++++++++-----
 12 files changed, 45 insertions(+), 21 deletions(-)

-- 
2.15.1

[bug#29745] [PATCH 1/3] build: gnu-build-system: Disallow phase returning <unspecified>.

[Arun Isaac]

* guix/build/gnu-build-system.scm (gnu-build): Treat a phase returning
<unspecified> as a failure.
---
 guix/build/gnu-build-system.scm | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/guix/build/gnu-build-system.scm b/guix/build/gnu-build-system.scm
index e37b75140..e1653a395 100644
--- a/guix/build/gnu-build-system.scm
+++ b/guix/build/gnu-build-system.scm
@@ -1,5 +1,6 @@
 ;;; GNU Guix --- Functional package management for GNU
 ;;; Copyright © 2012, 2013, 2014, 2015, 2016, 2017 Ludovic Courtès <ludo@gnu.org>
+;;; Copyright © 2017 Arun Isaac <arunisaac@systemreboot.net>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -669,6 +670,9 @@ in order.  Return #t if all the PHASES succeeded, #f otherwise."
       (+ (time-second diff)
          (/ (time-nanosecond diff) 1e9))))
 
+  (define (true? object)
+    (if (unspecified? object) #f object))
+
   (setvbuf (current-output-port) _IOLBF)
   (setvbuf (current-error-port) _IOLBF)
 
@@ -681,7 +685,7 @@ in order.  Return #t if all the PHASES succeeded, #f otherwise."
           ((name . proc)
            (let ((start (current-time time-monotonic)))
              (format #t "starting phase `~a'~%" name)
-             (let ((result (apply proc args))
+             (let ((result (true? (apply proc args)))
                    (end    (current-time time-monotonic)))
                (format #t "phase `~a' ~:[failed~;succeeded~] after ~,1f seconds~%"
                        name result
-- 
2.15.1

[bug#29745] [PATCH 2/3] build: gnu-build-system: Return #t from phases.

[Arun Isaac]

* guix/build/gnu-build-system.scm (patch-usr-bin-file, patch-source-shebangs,
patch-generated-file-shebangs, strip): Return #t from phases.
---
 guix/build/gnu-build-system.scm | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/guix/build/gnu-build-system.scm b/guix/build/gnu-build-system.scm
index e1653a395..92fa4f5fa 100644
--- a/guix/build/gnu-build-system.scm
+++ b/guix/build/gnu-build-system.scm
@@ -157,7 +157,8 @@ working directory."
       (and (if (string-suffix? ".zip" source)
                (zero? (system* "unzip" source))
                (zero? (system* "tar" "xvf" source)))
-           (chdir (first-subdirectory ".")))))
+           (chdir (first-subdirectory "."))
+           #t)))
 
 ;; See <http://bugs.gnu.org/17840>.
 (define* (patch-usr-bin-file #:key native-inputs inputs
@@ -184,7 +185,8 @@ $CONFIG_SHELL, but some don't, such as `mkinstalldirs' or Automake's
                         (lambda (file stat)
                           ;; Filter out symlinks.
                           (eq? 'regular (stat:type stat)))
-                        #:stat lstat)))
+                        #:stat lstat))
+  #t)
 
 (define (patch-generated-file-shebangs . rest)
   "Patch shebangs in generated files, including `SHELL' variables in
@@ -199,7 +201,8 @@ makefiles."
                         #:stat lstat))
 
   ;; Patch `SHELL' in generated makefiles.
-  (for-each patch-makefile-SHELL (find-files "." "^(GNU)?[mM]akefile$")))
+  (for-each patch-makefile-SHELL (find-files "." "^(GNU)?[mM]akefile$"))
+  #t)
 
 (define* (configure #:key build target native-inputs inputs outputs
                     (configure-flags '()) out-of-source?
@@ -413,7 +416,8 @@ makefiles."
                             ;; Ignore symlinks such as:
                             ;; libfoo.so -> libfoo.so.0.0.
                             (eq? 'regular (stat:type stat)))
-                          #:stat lstat)))
+                          #:stat lstat))
+    #t)
 
   (or (not strip-binaries?)
       (every strip-dir
-- 
2.15.1

[bug#29745] [PATCH 3/3] gnu: Return #t from phases.

[Arun Isaac]

* gnu/packages/attr.scm (attr): Return #t from phases.
* gnu/packages/autotools.scm (libltdl): Return #t from phases.
* gnu/packages/base.scm (coreutils): Return #t from phases.
* gnu/packages/bash.scm (bash): Return #t from phases.
* gnu/packages/commencement.scm (gnu-make-boot0, gcc-boot0, gettext-boot0,
libstdc++): Return #t from phases.
* gnu/packages/gcc.scm (make-libstdc++): Return #t from phases.
* gnu/packages/libffi.scm (libffi): Return #t from phases.
* gnu/packages/m4.scm (m4): Return #t from phases.
* gnu/packages/ncurses.scm (ncurses): Return #t from phases.
* gnu/packages/perl.scm (perl): Return #t from phases.
* gnu/packages/readline.scm (readline): Return #t from phases.
---
 gnu/packages/attr.scm         |  3 ++-
 gnu/packages/autotools.scm    |  3 ++-
 gnu/packages/base.scm         |  3 ++-
 gnu/packages/bash.scm         |  3 ++-
 gnu/packages/commencement.scm | 15 ++++++++++-----
 gnu/packages/gcc.scm          |  3 ++-
 gnu/packages/libffi.scm       |  3 ++-
 gnu/packages/m4.scm           |  3 ++-
 gnu/packages/ncurses.scm      |  6 ++++--
 gnu/packages/perl.scm         |  3 ++-
 gnu/packages/readline.scm     |  3 ++-
 11 files changed, 32 insertions(+), 16 deletions(-)

diff --git a/gnu/packages/attr.scm b/gnu/packages/attr.scm
index 4dbe09cec..7928c562b 100644
--- a/gnu/packages/attr.scm
+++ b/gnu/packages/attr.scm
@@ -42,7 +42,8 @@
        (modify-phases %standard-phases
          (add-after 'configure 'patch-makefile-SHELL
            (lambda _
-             (patch-makefile-SHELL "include/buildmacros")))
+             (patch-makefile-SHELL "include/buildmacros")
+             #t))
          (replace 'install
            (lambda _
              (zero? (system* "make"
diff --git a/gnu/packages/autotools.scm b/gnu/packages/autotools.scm
index 8a906a7d4..7454158bb 100644
--- a/gnu/packages/autotools.scm
+++ b/gnu/packages/autotools.scm
@@ -413,7 +413,8 @@ complexity of working with shared libraries across platforms.")
        #:phases (alist-cons-before
                  'configure 'change-directory
                  (lambda _
-                   (chdir "libltdl"))
+                   (chdir "libltdl")
+                   #t)
                  %standard-phases)))
 
     (synopsis "System-independent dlopen wrapper of GNU libtool")
diff --git a/gnu/packages/base.scm b/gnu/packages/base.scm
index 9cb628d8d..ddb9a5756 100644
--- a/gnu/packages/base.scm
+++ b/gnu/packages/base.scm
@@ -335,7 +335,8 @@ used to apply commands with arbitrarily long arguments.")
                        (format #f "~a/bin/sh" bash)))
                     (substitute* (find-files "tests" "\\.sh$")
                       (("#!/bin/sh")
-                       (format #f "#!~a/bin/sh" bash)))))
+                       (format #f "#!~a/bin/sh" bash))))
+                  #t)
                 %standard-phases)))
    (synopsis "Core GNU utilities (file, text, shell)")
    (description
diff --git a/gnu/packages/bash.scm b/gnu/packages/bash.scm
index b8b0ae58f..c50b4e693 100644
--- a/gnu/packages/bash.scm
+++ b/gnu/packages/bash.scm
@@ -159,7 +159,8 @@ number/base32-hash tuples, directly usable in the 'patch-series' form."
               ;; Add a `sh' -> `bash' link.
               (let ((out (assoc-ref outputs "out")))
                 (with-directory-excursion (string-append out "/bin")
-                  (symlink "bash" "sh")))))
+                  (symlink "bash" "sh")))
+              #t))
 
           (add-after 'install 'move-development-files
             (lambda* (#:key outputs #:allow-other-keys)
diff --git a/gnu/packages/commencement.scm b/gnu/packages/commencement.scm
index c5c00688e..ae845f2e7 100644
--- a/gnu/packages/commencement.scm
+++ b/gnu/packages/commencement.scm
@@ -85,7 +85,8 @@
                   (lambda* (#:key outputs #:allow-other-keys)
                     (let* ((out (assoc-ref outputs "out"))
                            (bin (string-append out "/bin")))
-                      (install-file "make" bin)))))))))
+                      (install-file "make" bin))
+                    #t)))))))
      (native-inputs '())                          ; no need for 'pkg-config'
      (inputs %bootstrap-inputs))))
 
@@ -260,7 +261,8 @@
                                         (package-full-name lib)
                                         char-set:letter)
                                       ,(package-name lib)))
-                          (list gmp-6.0 mpfr mpc))))
+                          (list gmp-6.0 mpfr mpc)))
+                 #t)
                (alist-cons-after
                 'install 'symlink-libgcc_eh
                 (lambda* (#:key outputs #:allow-other-keys)
@@ -271,7 +273,8 @@
                         (string-append out "/lib/gcc/"
                                        ,(boot-triplet)
                                        "/" ,(package-version gcc))
-                      (symlink "libgcc.a" "libgcc_eh.a"))))
+                      (symlink "libgcc.a" "libgcc_eh.a")))
+                  #t)
                 ,phases))))))
 
      (inputs `(("gmp-source" ,(package-source gmp-6.0))
@@ -612,7 +615,8 @@ exec ~a/bin/~a-~a -B~a/lib -Wl,-dynamic-linker -Wl,~a/~a \"$@\"~%"
                   ;; Build only the tools.
                   (add-after 'unpack 'chdir
                              (lambda _
-                               (chdir "gettext-tools")))
+                               (chdir "gettext-tools")
+                               #t))
 
                   ;; Some test programs require pthreads, which we don't have.
                   (add-before 'configure 'no-test-programs
@@ -692,7 +696,8 @@ exec ~a/bin/~a-~a -B~a/lib -Wl,-dynamic-linker -Wl,~a/~a \"$@\"~%"
         #:phases (alist-cons-before
                   'configure 'chdir
                   (lambda _
-                    (chdir "libstdc++-v3"))
+                    (chdir "libstdc++-v3")
+                    #t)
                   %standard-phases)
         #:configure-flags `("--disable-shared"
                             "--disable-libstdcxx-threads"
diff --git a/gnu/packages/gcc.scm b/gnu/packages/gcc.scm
index ad8992289..0151784e8 100644
--- a/gnu/packages/gcc.scm
+++ b/gnu/packages/gcc.scm
@@ -449,7 +449,8 @@ using compilers other than GCC."
        #:phases (alist-cons-before
                  'configure 'chdir
                  (lambda _
-                   (chdir "libstdc++-v3"))
+                   (chdir "libstdc++-v3")
+                   #t)
                  %standard-phases)
        #:configure-flags `("--disable-libstdcxx-pch"
                            ,(string-append "--with-gxx-include-dir="
diff --git a/gnu/packages/libffi.scm b/gnu/packages/libffi.scm
index 948dabb41..ed7628dc3 100644
--- a/gnu/packages/libffi.scm
+++ b/gnu/packages/libffi.scm
@@ -41,7 +41,8 @@
          '(lambda* (#:key outputs #:allow-other-keys)
             (define out (assoc-ref outputs "out"))
             (symlink (string-append out "/lib/libffi-3.2.1/include")
-                     (string-append out "/include")))))
+                     (string-append out "/include"))
+            #t)))
    (package
     (name "libffi")
     (version "3.2.1")
diff --git a/gnu/packages/m4.scm b/gnu/packages/m4.scm
index 32e7c2ae4..1162e0bb8 100644
--- a/gnu/packages/m4.scm
+++ b/gnu/packages/m4.scm
@@ -50,7 +50,8 @@
                     (substitute* (find-files "tests"
                                              "posix_spawn")
                       (("/bin/sh")
-                       (format #f "~a/bin/sh" bash)))))
+                       (format #f "~a/bin/sh" bash))))
+                  #t)
                 %standard-phases)))
    (synopsis "Macro processor")
    (description
diff --git a/gnu/packages/ncurses.scm b/gnu/packages/ncurses.scm
index 9f5905bc8..9aecc363b 100644
--- a/gnu/packages/ncurses.scm
+++ b/gnu/packages/ncurses.scm
@@ -54,7 +54,8 @@
      (let ((patch-makefile-phase
             '(lambda _
                (for-each patch-makefile-SHELL
-                         (find-files "." "Makefile.in"))))
+                         (find-files "." "Makefile.in"))
+               #t))
            (configure-phase
             ;; The 'configure' script does not understand '--docdir', so we must
             ;; override that and use '--mandir' instead.
@@ -141,7 +142,8 @@
                                            (when (file-exists? packagew.pc)
                                              (symlink packagew.pc package.pc))))
                                        '())))
-                             '("curses" "ncurses" "form" "panel" "menu")))))))
+                             '("curses" "ncurses" "form" "panel" "menu"))))
+               #t)))
        `(#:configure-flags
          ,(cons*
            'quasiquote
diff --git a/gnu/packages/perl.scm b/gnu/packages/perl.scm
index 7ff55546c..662fb90d2 100644
--- a/gnu/packages/perl.scm
+++ b/gnu/packages/perl.scm
@@ -114,7 +114,8 @@
                    (lib (string-append out "/lib")))
               (for-each (lambda (dso)
                           (chmod dso #o755))
-                        (find-files lib "\\.so$")))))
+                        (find-files lib "\\.so$")))
+            #t))
 
          (add-after 'install 'remove-extra-references
            (lambda* (#:key inputs outputs #:allow-other-keys)
diff --git a/gnu/packages/readline.scm b/gnu/packages/readline.scm
index f6ebbcc26..897657aa1 100644
--- a/gnu/packages/readline.scm
+++ b/gnu/packages/readline.scm
@@ -39,7 +39,8 @@
               (for-each (lambda (f) (chmod f #o755))
                         (find-files lib "\\.so"))
               (for-each (lambda (f) (chmod f #o644))
-                        (find-files lib "\\.a"))))))
+                        (find-files lib "\\.a")))
+            #t)))
     (package
       (name "readline")
       (version "7.0")
-- 
2.15.1

[bug#29745] [PATCH 0/3] Disallow phase returning <unspecified>.

[Ricardo Wurmus]

Hi,

> Patch 1 modifies gnu-build-system to explicitly check the return value of each
> phase for an unspecified value, and if found consider that phase to have
> failed.
>
> Patch 2 fixes several phases in gnu-build-system by making them return #t
> instead of an unspecified value.
>
> Patch 3 fixes the phases in several packages to return #t instead of an
> unspecified value. I was testing patch 1, by trying to build bash. In doing
> so, I found that the custom phases of many packages were not properly
> returning #t, and fixed all those packages. No doubt, there are many more
> packages that need fixing.

this is a step in the right direction.  Thank you for these patches.  In
my opinion, this can go straight to core-updates, but as it is a change
with wide-ranging consequences, I’d prefer for Ludovic to confirm that
it’s okay.

--
Ricardo

GPG: BCA6 89B6 3655 3801 C3C6  2150 197A 5888 235F ACAC
https://elephly.net

[bug#29745] [PATCH 0/3] Disallow phase returning <unspecified>.

[Ludovic Courtès]

Hi!

Arun Isaac <arunisaac@systemreboot.net> skribis:

> Patch 1 modifies gnu-build-system to explicitly check the return value of each
> phase for an unspecified value, and if found consider that phase to have
> failed.
>
> Patch 2 fixes several phases in gnu-build-system by making them return #t
> instead of an unspecified value.
>
> Patch 3 fixes the phases in several packages to return #t instead of an
> unspecified value. I was testing patch 1, by trying to build bash. In doing
> so, I found that the custom phases of many packages were not properly
> returning #t, and fixed all those packages. No doubt, there are many more
> packages that need fixing.
>
> Arun Isaac (3):
>   build: gnu-build-system: Disallow phase returning <unspecified>.
>   build: gnu-build-system: Return #t from phases.
>   gnu: Return #t from phases.

Even though I think it’s a good idea to explicitly return a Boolean
value from build phases for clarity, I’m reluctant to patch 1.  My
objection is that it ends up defining a notion of “true” that’s
different from that of Scheme (in Scheme, #f is the only false value;
everything else is true, including “the unspecified value”—see
“Disjointness of types” in the R5RS.)

Patches 2 and 3 LGTM.

However, ‘core-updates’ is supposedly frozen right now (I’m Cc’ing
Marius who is the de facto ‘core-updates’ manager :-)).  So perhaps we
should hold on until the next ‘core-updates’ is open.

What do people think?

Thanks,
Ludo’.

[bug#29745] [PATCH 0/3] Disallow phase returning <unspecified>.

[Arun Isaac]

Ludovic Courtès <ludo@gnu.org> writes:

> it ends up defining a notion of “true” that’s different from that of
> Scheme (in Scheme, #f is the only false value; everything else is
> true, including “the unspecified value”—see “Disjointness of types” in
> the R5RS.)

I agree. I don't think we should define a new notion of "true" that is
different from that of Scheme.

But, in that case, why do we explicitly add #t at the end of phases that
return an unspecified value? Can we not drop this convention and simply
accept unspecified values from phases as a success of that phase?

The way it is right now, we add #t at the end of phases that return an
unspecified value, but don't check for this anywhere. So, we have a lot
of custom phases where people have forgotten to return #t. Patches 2 and
3 fix some of these, but I'm sure there are many more. Wouldn't it be
simpler and more logical to accept unspecified values as "true" in line
with Scheme's notion of "true"?

[bug#29745] [PATCH 0/3] Disallow phase returning <unspecified>.

[Marius Bakke]

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

Ludovic Courtès <ludo@gnu.org> writes:

> However, ‘core-updates’ is supposedly frozen right now (I’m Cc’ing
> Marius who is the de facto ‘core-updates’ manager :-)).  So perhaps we
> should hold on until the next ‘core-updates’ is open.
>
> What do people think?

Many are already working on it and full-rebuilds are annoying.  So I
would prefer waiting until the next window.

But there are a few problems with core-updates still, so technically
there is a chance a full rebuild is necessary.  Maybe fork off
core-updates to core-updates-next so they are available?

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

[bug#29745] [PATCH 0/3] Disallow phase returning <unspecified>.

[Ludovic Courtès]

Hello,

Arun Isaac <arunisaac@systemreboot.net> skribis:

> Ludovic Courtès <ludo@gnu.org> writes:
>
>> it ends up defining a notion of “true” that’s different from that of
>> Scheme (in Scheme, #f is the only false value; everything else is
>> true, including “the unspecified value”—see “Disjointness of types” in
>> the R5RS.)
>
> I agree. I don't think we should define a new notion of "true" that is
> different from that of Scheme.
>
> But, in that case, why do we explicitly add #t at the end of phases that
> return an unspecified value? Can we not drop this convention and simply
> accept unspecified values from phases as a success of that phase?
>
> The way it is right now, we add #t at the end of phases that return an
> unspecified value, but don't check for this anywhere. So, we have a lot
> of custom phases where people have forgotten to return #t. Patches 2 and
> 3 fix some of these, but I'm sure there are many more. Wouldn't it be
> simpler and more logical to accept unspecified values as "true" in line
> with Scheme's notion of "true"?

That’s a good question.  My take on it is that it’s clearer: the return
value of ‘substitute*’ for instance is unspecified, it’s not necessarily
*the* unspecified value.

I’m not opposed to changing things though.  For instance we could change
the return value of ‘substitute*’ to be #t on success; similarly for
‘install-file’.

What do people think?

Ludo’.

[bug#29745] [PATCH 0/3] Disallow phase returning <unspecified>.

[Ricardo Wurmus]

Ludovic Courtès <ludo@gnu.org> writes:

> Hello,
>
> Arun Isaac <arunisaac@systemreboot.net> skribis:
>
>> Ludovic Courtès <ludo@gnu.org> writes:
>>
>>> it ends up defining a notion of “true” that’s different from that of
>>> Scheme (in Scheme, #f is the only false value; everything else is
>>> true, including “the unspecified value”—see “Disjointness of types” in
>>> the R5RS.)
>>
>> I agree. I don't think we should define a new notion of "true" that is
>> different from that of Scheme.
>>
>> But, in that case, why do we explicitly add #t at the end of phases that
>> return an unspecified value? Can we not drop this convention and simply
>> accept unspecified values from phases as a success of that phase?
>>
>> The way it is right now, we add #t at the end of phases that return an
>> unspecified value, but don't check for this anywhere. So, we have a lot
>> of custom phases where people have forgotten to return #t. Patches 2 and
>> 3 fix some of these, but I'm sure there are many more. Wouldn't it be
>> simpler and more logical to accept unspecified values as "true" in line
>> with Scheme's notion of "true"?
>
> That’s a good question.  My take on it is that it’s clearer: the return
> value of ‘substitute*’ for instance is unspecified, it’s not necessarily
> *the* unspecified value.
>
> I’m not opposed to changing things though.  For instance we could change
> the return value of ‘substitute*’ to be #t on success; similarly for
> ‘install-file’.

I would like to change the return value for these two things.  It would
be good for “substitute*” to return #f when a pattern could not be
matched, but this would require a few changes to a number of packages
where we replace a pattern in many files at once.

--
Ricardo

GPG: BCA6 89B6 3655 3801 C3C6  2150 197A 5888 235F ACAC
https://elephly.net

[bug#29745] [PATCH 0/3] Disallow phase returning <unspecified>.

[Arun Isaac]

> For instance we could change the return value of ‘substitute*’ to be
> #t on success; similarly for ‘install-file’.

Yes, changing the return value of these functions is a good idea,
regardless of what we decide here in this thread. But, there will always
be phases that do not end with a call to these functions, and we won't
be able to cover those cases. So, this is not a complete solution to the
problem being discussed in this thread.

>> Wouldn't it be simpler and more logical to accept unspecified values
>> as "true" in line with Scheme's notion of "true"?

Also, during patch review, the reviewer has to go to the extra trouble
of manually checking if all phases return #t. If either the build system
strictly checks for #t, or if #t were not strictly required (meaning
#<unspecified> was tolerated), we would save ourselves some tedious
manual work.

> That’s a good question.  My take on it is that it’s clearer: the return
> value of ‘substitute*’ for instance is unspecified, it’s not necessarily
> *the* unspecified value.

My take is that we have two options:

- disallow #<unspecified> and strictly check for #t in the build system
even though this creates a new notion of "true"
- allow #<unspecified> as "true" in line with the rest of Scheme, and
stop appending #t at the end of phases

I have no particular preference about which option we should
choose. But, we do have to choose one of these options
completely. Partial solutions might only add to the existing troubles.

[bug#29745] [PATCH 0/3] Disallow phase returning <unspecified>.

[Andreas Enge]

On Mon, Dec 18, 2017 at 09:01:01AM +0100, Ludovic Courtès wrote:
> I’m not opposed to changing things though.  For instance we could change
> the return value of ‘substitute*’ to be #t on success; similarly for
> ‘install-file’.
> 
> What do people think?

I agree, since I never understood why the function does not return a truth
value, if it is something we are interested in later for our phases.

Andreas

[bug#29745] [PATCH 0/3] Disallow phase returning <unspecified>.

[Ludovic Courtès]

Hi,

Arun Isaac <arunisaac@systemreboot.net> skribis:

>> For instance we could change the return value of ‘substitute*’ to be
>> #t on success; similarly for ‘install-file’.
>
> Yes, changing the return value of these functions is a good idea,
> regardless of what we decide here in this thread.

OK, let’s do that in the next ‘core-updates’.

> But, there will always be phases that do not end with a call to these
> functions, and we won't be able to cover those cases. So, this is not
> a complete solution to the problem being discussed in this thread.

True.

>>> Wouldn't it be simpler and more logical to accept unspecified values
>>> as "true" in line with Scheme's notion of "true"?
>
> Also, during patch review, the reviewer has to go to the extra trouble
> of manually checking if all phases return #t. If either the build system
> strictly checks for #t, or if #t were not strictly required (meaning
> #<unspecified> was tolerated), we would save ourselves some tedious
> manual work.

I agree.

>> That’s a good question.  My take on it is that it’s clearer: the return
>> value of ‘substitute*’ for instance is unspecified, it’s not necessarily
>> *the* unspecified value.
>
> My take is that we have two options:
>
> - disallow #<unspecified> and strictly check for #t in the build system
> even though this creates a new notion of "true"
> - allow #<unspecified> as "true" in line with the rest of Scheme, and
> stop appending #t at the end of phases
>
> I have no particular preference about which option we should
> choose. But, we do have to choose one of these options
> completely. Partial solutions might only add to the existing troubles.

I get your point.

I don’t have a strong preference either, to be honest.  I think Mark
did, so I’m Cc’ing him.  Mark?

Ludo’.

[bug#29745] [PATCH 0/3] Disallow phase returning <unspecified>.

[Mark H Weaver]

ludo@gnu.org (Ludovic Courtès) writes:
> I don’t have a strong preference either, to be honest.  I think Mark
> did, so I’m Cc’ing him.  Mark?

Thanks for bringing my attention to this bug report.  I've just recently
outlined an alternative proposal in the related thread on guix-devel:

  https://lists.gnu.org/archive/html/guix-devel/2017-12/msg00278.html

What do you think?

      Mark

bug#29745: [PATCH 0/3] Disallow phase returning <unspecified>.

[Arun Isaac]

I guess this discussion is over. I'm closing this bug report and
discarding all 3 patches.