Simplifications enabled by switching to 'invoke'

Simplifications enabled by switching to 'invoke'

[Mark H Weaver]

Hello Kei, and other fellow Guix,

kkebreau@posteo.net (Kei Kebreau) writes:
> commit 0af6ffdd8d81f86a232902a54f99d4cfcd369490
> Author: Kei Kebreau <kkebreau@posteo.net>
> Date:   Tue Jan 23 17:44:53 2018 -0500
>
>     gnu: qscintilla: Update to 2.10.2.
>     
>     * gnu/packages/qt.scm (qscintilla, python-qscintilla, python-pyqt+qscintilla):
>     Update to 2.10.2.
[...]
> @@ -1715,8 +1715,8 @@ indicators, code completion and call tips.")
>           (replace 'configure
>             (lambda* (#:key outputs configure-flags #:allow-other-keys)
>               (chdir "Python")
> -             (and (zero? (apply system* "python3" "configure.py"
> -                                configure-flags))
> +             (and (apply invoke "python3" "configure.py"
> +                         configure-flags)
>                    ;; Install to the right directory
>                    (begin
>                      (substitute* '("Makefile"

Kei, I appreciate that you took this opportunity to switch from
'system*' to 'invoke' here while doing this update.  I think it makes
sense to do this whenever we update a package.

However, it's worth noting that you missed an important further
simplification that the switch to 'invoke' enables.  Since 'invoke'
never returns #false, the surrounding code that arranges for plumbing of
its result code can be removed entirely.

Step by step:

* Since 'invoke' never returns #false, it can be moved above the 'and'.

* This leaves the 'and' with only one remaining argument.  An 'and' with
  only one argument is equivalent to that argument, so the 'and' can be
  removed, replaced by its argument.

* Since the 'begin' is now within a body (whereas previously it was an
  operand), the 'begin' can now be removed, replaced by its contents.

This is what I did in commit 76c7fc436a151236f5e1ff966fd99172d85ee422 on
master, which I've attached below.

    Thanks,
      Mark


From 76c7fc436a151236f5e1ff966fd99172d85ee422 Mon Sep 17 00:00:00 2001
From: Mark H Weaver <mhw@netris.org>
Date: Wed, 24 Jan 2018 06:35:29 -0500
Subject: [PATCH] gnu: python-qscintilla: Remove result code plumbing.

* gnu/packages/qt.scm (python-qscintilla)[arguments]: In the 'configure'
phase, remove result code plumbing that is no longer needed, since 'invoke'
never returns #false.
---
 gnu/packages/qt.scm | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/gnu/packages/qt.scm b/gnu/packages/qt.scm
index 596006080..34938b9c0 100644
--- a/gnu/packages/qt.scm
+++ b/gnu/packages/qt.scm
@@ -1715,15 +1715,14 @@ indicators, code completion and call tips.")
          (replace 'configure
            (lambda* (#:key outputs configure-flags #:allow-other-keys)
              (chdir "Python")
-             (and (apply invoke "python3" "configure.py"
-                         configure-flags)
-                  ;; Install to the right directory
-                  (begin
-                    (substitute* '("Makefile"
-                                   "Qsci/Makefile")
-                      (("\\$\\(INSTALL_ROOT\\)/gnu/store/[^/]+")
-                       (assoc-ref outputs "out")))
-                    #t)))))))
+             (apply invoke "python3" "configure.py"
+                    configure-flags)
+             ;; Install to the right directory
+             (substitute* '("Makefile"
+                            "Qsci/Makefile")
+               (("\\$\\(INSTALL_ROOT\\)/gnu/store/[^/]+")
+                (assoc-ref outputs "out")))
+             #t)))))
     (inputs
      `(("qscintilla" ,qscintilla)
        ("python" ,python)
-- 
2.16.1

question regarding substitute* and #t (was: Simplifications enabled by switching to 'invoke')

[Andy Wingo]

Hi!

On Wed 24 Jan 2018 13:06, Mark H Weaver <mhw@netris.org> writes:

> +             ;; Install to the right directory
> +             (substitute* '("Makefile"
> +                            "Qsci/Makefile")
> +               (("\\$\\(INSTALL_ROOT\\)/gnu/store/[^/]+")
> +                (assoc-ref outputs "out")))
> +             #t)))))

I guess once we switch over all instances of "system" and "system*" to
use invoke, does that mean we will also be able to remove these
vestigial "#t" returns?

Andy

Re: question regarding substitute* and #t

[Mark H Weaver]

Hi Andy,

Andy Wingo <wingo@igalia.com> writes:

> On Wed 24 Jan 2018 13:06, Mark H Weaver <mhw@netris.org> writes:
>
>> +             ;; Install to the right directory
>> +             (substitute* '("Makefile"
>> +                            "Qsci/Makefile")
>> +               (("\\$\\(INSTALL_ROOT\\)/gnu/store/[^/]+")
>> +                (assoc-ref outputs "out")))
>> +             #t)))))
>
> I guess once we switch over all instances of "system" and "system*" to
> use invoke, does that mean we will also be able to remove these
> vestigial "#t" returns?

After we switch to using 'invoke' everywhere, or more precisely, after
we arrange to never return #false from any phase or snippet, then there
should be one more step before removing the vestigial #true returns: we
should change the code that calls phases or snippets to ignore the
value(s) returned by those procedures.  When that is done, then the #t's
will truly be vestigial.  Does that make sense?

      Mark

Re: question regarding substitute* and #t

[Andy Wingo]

On Wed 24 Jan 2018 14:28, Mark H Weaver <mhw@netris.org> writes:

> Andy Wingo <wingo@igalia.com> writes:
>
>> On Wed 24 Jan 2018 13:06, Mark H Weaver <mhw@netris.org> writes:
>>
>>> +             ;; Install to the right directory
>>> +             (substitute* '("Makefile"
>>> +                            "Qsci/Makefile")
>>> +               (("\\$\\(INSTALL_ROOT\\)/gnu/store/[^/]+")
>>> +                (assoc-ref outputs "out")))
>>> +             #t)))))
>>
>> I guess once we switch over all instances of "system" and "system*" to
>> use invoke, does that mean we will also be able to remove these
>> vestigial "#t" returns?
>
> After we switch to using 'invoke' everywhere, or more precisely, after
> we arrange to never return #false from any phase or snippet, then there
> should be one more step before removing the vestigial #true returns: we
> should change the code that calls phases or snippets to ignore the
> value(s) returned by those procedures.  When that is done, then the #t's
> will truly be vestigial.  Does that make sense?

Sure, makes sense.  Thanks for thinking it through with me :)

Andy

Re: question regarding substitute* and #t

[Kei Kebreau]

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

Andy Wingo <wingo@igalia.com> writes:

> On Wed 24 Jan 2018 14:28, Mark H Weaver <mhw@netris.org> writes:
>
>> Andy Wingo <wingo@igalia.com> writes:
>>
>>> On Wed 24 Jan 2018 13:06, Mark H Weaver <mhw@netris.org> writes:
>>>
>>>> +             ;; Install to the right directory
>>>> +             (substitute* '("Makefile"
>>>> +                            "Qsci/Makefile")
>>>> +               (("\\$\\(INSTALL_ROOT\\)/gnu/store/[^/]+")
>>>> +                (assoc-ref outputs "out")))
>>>> +             #t)))))
>>>
>>> I guess once we switch over all instances of "system" and "system*" to
>>> use invoke, does that mean we will also be able to remove these
>>> vestigial "#t" returns?
>>
>> After we switch to using 'invoke' everywhere, or more precisely, after
>> we arrange to never return #false from any phase or snippet, then there
>> should be one more step before removing the vestigial #true returns: we
>> should change the code that calls phases or snippets to ignore the
>> value(s) returned by those procedures.  When that is done, then the #t's
>> will truly be vestigial.  Does that make sense?
>
> Sure, makes sense.  Thanks for thinking it through with me :)
>
> Andy

Thanks to you both for this thread!

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

Re: question regarding substitute* and #t

[Maxim Cournoyer]

Andy Wingo <wingo@igalia.com> writes:

> On Wed 24 Jan 2018 14:28, Mark H Weaver <mhw@netris.org> writes:
>
>> Andy Wingo <wingo@igalia.com> writes:
>>
>>> On Wed 24 Jan 2018 13:06, Mark H Weaver <mhw@netris.org> writes:
>>>
>>>> +             ;; Install to the right directory
>>>> +             (substitute* '("Makefile"
>>>> +                            "Qsci/Makefile")
>>>> +               (("\\$\\(INSTALL_ROOT\\)/gnu/store/[^/]+")
>>>> +                (assoc-ref outputs "out")))
>>>> +             #t)))))
>>>
>>> I guess once we switch over all instances of "system" and "system*" to
>>> use invoke, does that mean we will also be able to remove these
>>> vestigial "#t" returns?
>>
>> After we switch to using 'invoke' everywhere, or more precisely, after
>> we arrange to never return #false from any phase or snippet, then there
>> should be one more step before removing the vestigial #true returns: we
>> should change the code that calls phases or snippets to ignore the
>> value(s) returned by those procedures.  When that is done, then the #t's
>> will truly be vestigial.  Does that make sense?
>
> Sure, makes sense.  Thanks for thinking it through with me :)
>
> Andy

Where does this `invoke' comes from? Geiser is unhelpful at finding it,
and it doesn't seem to be documented in the Guile Reference?

Thanks,

Maxim

Re: question regarding substitute* and #t

[Andy Wingo]

On Thu 25 Jan 2018 06:31, Maxim Cournoyer <maxim.cournoyer@gmail.com> writes:

> Where does this `invoke' comes from? Geiser is unhelpful at finding it,
> and it doesn't seem to be documented in the Guile Reference?

https://lists.gnu.org/archive/html/guix-devel/2018-01/msg00163.html

Re: question regarding substitute* and #t

[Maxim Cournoyer]

Andy Wingo <wingo@igalia.com> writes:

> On Thu 25 Jan 2018 06:31, Maxim Cournoyer <maxim.cournoyer@gmail.com> writes:
>
>> Where does this `invoke' comes from? Geiser is unhelpful at finding it,
>> and it doesn't seem to be documented in the Guile Reference?
>
> https://lists.gnu.org/archive/html/guix-devel/2018-01/msg00163.html

OK, so `invoke' is defined in (guix build utils), and it's docstring is:

   "Invoke PROGRAM with the given ARGS.  Raise an error if the exit code
   is non-zero; otherwise return #t."

Thanks,

Maxim

Re: question regarding substitute* and #t

[Arun Isaac]

Mark H Weaver <mhw@netris.org> writes:

> After we switch to using 'invoke' everywhere, or more precisely, after
> we arrange to never return #false from any phase or snippet, then
> there should be one more step before removing the vestigial #true
> returns: we should change the code that calls phases or snippets to
> ignore the value(s) returned by those procedures.  When that is done,
> then the #t's will truly be vestigial.  Does that make sense?

I think we should start removing the vestigial #true right away. Why
wait until we can make the code that calls phases ignore the values
returned by those phases? As it stands, that code errors out only when a
phase returns #false, not when it returns any other value (even
unspecified). WDYT?

The #true is already vestigial. In fact, #true being vestigial is what
annoyed me and made me start the original thread discussing ways to get
rid of it.

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

And, it so happened that we concluded the best way to go forward was to
deprecate boolean results altogether and transition to an exception
based system.

Re: question regarding substitute* and #t

[Mark H Weaver]

Arun Isaac <arunisaac@systemreboot.net> writes:

> Mark H Weaver <mhw@netris.org> writes:
>
>> After we switch to using 'invoke' everywhere, or more precisely, after
>> we arrange to never return #false from any phase or snippet, then
>> there should be one more step before removing the vestigial #true
>> returns: we should change the code that calls phases or snippets to
>> ignore the value(s) returned by those procedures.  When that is done,
>> then the #t's will truly be vestigial.  Does that make sense?
>
> I think we should start removing the vestigial #true right away. Why
> wait until we can make the code that calls phases ignore the values
> returned by those phases? As it stands, that code errors out only when a
> phase returns #false, not when it returns any other value (even
> unspecified). WDYT?
>
> The #true is already vestigial.

They are not vestigial if we care about code correctness.

Phases and snippets are currently specified to return a boolean, and
furthermore we must return the _appropriate_ boolean to indicate success
for failure.  I consider it unacceptable to not bother returning
anything, allowing a completely unspecified value to be returned, and
think that this is okay because it happens to work, for now, because of
an internal implementation detail of Guile.

This (unfortunately widespread) practice of sloppiness in software
engineering is how we ended up in the mess we are in today, where our
software is drowning in bugs and our systems are hopelessly insecure.

Let the annoyance that you and others feel about these unsightly #t's
supply the motivation to fix this issue properly.

      Mark

Re: question regarding substitute* and #t

[Hartmut Goebel]

Am 24.01.2018 um 13:14 schrieb Andy Wingo:
> On Wed 24 Jan 2018 13:06, Mark H Weaver <mhw@netris.org> writes:
>
>> +             ;; Install to the right directory
>> +             (substitute* '("Makefile"
>> +                            "Qsci/Makefile")
>> +               (("\\$\\(INSTALL_ROOT\\)/gnu/store/[^/]+")
>> +                (assoc-ref outputs "out")))
>> +             #t)))))
> I guess once we switch over all instances of "system" and "system*" to
> use invoke, does that mean we will also be able to remove these
> vestigial "#t" returns?
I wonder why substitute* not simply returns #t?!

-- 
Regards
Hartmut Goebel

| Hartmut Goebel          | h.goebel@crazy-compilers.com               |
| www.crazy-compilers.com | compilers which you thought are impossible |

Re: question regarding substitute* and #t

[Andy Wingo]

On Wed 24 Jan 2018 15:45, Hartmut Goebel <h.goebel@crazy-compilers.com> writes:

> Am 24.01.2018 um 13:14 schrieb Andy Wingo:
>> On Wed 24 Jan 2018 13:06, Mark H Weaver <mhw@netris.org> writes:
>>
>>> +             ;; Install to the right directory
>>> +             (substitute* '("Makefile"
>>> +                            "Qsci/Makefile")
>>> +               (("\\$\\(INSTALL_ROOT\\)/gnu/store/[^/]+")
>>> +                (assoc-ref outputs "out")))
>>> +             #t)))))
>> I guess once we switch over all instances of "system" and "system*" to
>> use invoke, does that mean we will also be able to remove these
>> vestigial "#t" returns?
> I wonder why substitute* not simply returns #t?!

There was a proposal to make it return #t!  However then someone pointed
out that actually instead of making phases return boolean results, we
should instead signal problems via exceptions, and that drove the shift
from system / system* to invoke.  In the future world where completion
means success, it doesn't matter what substitute* returns.

However!  Because it doesn't matter, perhaps in the interest of
transition we should make substitute* return #t, so that once we switch
to the new exception-based error signalling, that we have less code to
clean up later.

Andy

Re: question regarding substitute* and #t

[Ricardo Wurmus]

Andy Wingo <wingo@igalia.com> writes:

>> I wonder why substitute* not simply returns #t?!
>
> There was a proposal to make it return #t!  However then someone pointed
> out that actually instead of making phases return boolean results, we
> should instead signal problems via exceptions, and that drove the shift
> from system / system* to invoke.  In the future world where completion
> means success, it doesn't matter what substitute* returns.

There is also the question what it means for substitute* to fail.  Does
it fail when one of the substitutions couldn’t be performed in one of
the files?  Or does it only fail when no substitutions succeeded?  Or
something in between?

I’m sure we have a bunch of places in package definitions where old
substitions actually no longer apply to current versions of the sources.
Cleaning this up will take quite some time.

(I think that it would be helpful if substitute* failed when any
substitution failed, but this would make some shortcuts impossible.)

--
Ricardo

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