Why should build phases not return unspecified values?

Why should build phases not return unspecified values?

[Arun Isaac]

Whenever we have a build phase that ends with a call (for example, to
substitute, chdir, symlink, etc) that returns an unspecified value, we
append a #t so that the return value is a boolean. However, the build
system, as it stands currently, does not mind an unspecified value, and
treats it as a success. As a result, forgetting to add a #t at the end
of custom phases is a common mistake. To fix this, I have submitted a
patch at https://debbugs.gnu.org/cgi/bugreport.cgi?bug=29745 that
modifies the build system to reject unspecified values as
failures.

However, IMO, the addition of #t at the end of certain phases, does not
contribute anything of value and we should simply be at peace with
phases returning unspecified values. Am I missing something here?

WDYT?

Re: Why should build phases not return unspecified values?

[Pjotr Prins]

On Sun, Dec 17, 2017 at 04:58:15AM +0530, Arun Isaac wrote:
> 
> Whenever we have a build phase that ends with a call (for example, to
> substitute, chdir, symlink, etc) that returns an unspecified value, we
> append a #t so that the return value is a boolean. However, the build
> system, as it stands currently, does not mind an unspecified value, and
> treats it as a success. As a result, forgetting to add a #t at the end
> of custom phases is a common mistake. To fix this, I have submitted a
> patch at https://debbugs.gnu.org/cgi/bugreport.cgi?bug=29745 that
> modifies the build system to reject unspecified values as
> failures.
> 
> However, IMO, the addition of #t at the end of certain phases, does not
> contribute anything of value and we should simply be at peace with
> phases returning unspecified values. Am I missing something here?

If an unspecified value is treated as #t I would prefer allowing an
unspecified value. The less visual 'noise' we have the better. The
build will break if something goes wrong. But then in strictly typed
languages you would need to be explicit... It is a matter of taste in
the end.

I have wondered why I would need to put in a #t there.

Pj.
-- 

Re: Why should build phases not return unspecified values?

[Alex Vong]

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

Arun Isaac <arunisaac@systemreboot.net> writes:

> Whenever we have a build phase that ends with a call (for example, to
> substitute, chdir, symlink, etc) that returns an unspecified value, we
> append a #t so that the return value is a boolean. However, the build
> system, as it stands currently, does not mind an unspecified value, and
> treats it as a success. As a result, forgetting to add a #t at the end
> of custom phases is a common mistake. To fix this, I have submitted a
> patch at https://debbugs.gnu.org/cgi/bugreport.cgi?bug=29745 that
> modifies the build system to reject unspecified values as
> failures.
>
> However, IMO, the addition of #t at the end of certain phases, does not
> contribute anything of value and we should simply be at peace with
> phases returning unspecified values. Am I missing something here?
>
> WDYT?

I think the problem is that when the scheme standard says "the returned
value is unspecified", it means anything can be returned. In this case,
guile choose to return an unspecified value to avoid returning an
arbitary value.

I think the answer written by soegaard in [0] explains it pretty well.

[0]: https://stackoverflow.com/questions/28910911/detecting-unspecified-in-scheme-list

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

Re: Why should build phases not return unspecified values?

[Arun Isaac]

Alex Vong <alexvong1995@gmail.com> writes:

> I think the problem is that when the scheme standard says "the returned
> value is unspecified", it means anything can be returned. In this case,
> guile choose to return an unspecified value to avoid returning an
> arbitary value.
>
> I think the answer written by soegaard in [0] explains it pretty well.
>
> [0]: https://stackoverflow.com/questions/28910911/detecting-unspecified-in-scheme-list

This is new to me. But, since we only use the Guile implementation, I
think we should be ok with phases returning #<unspecified>.

Re: Why should build phases not return unspecified values?

[Clément Lassieur]

Arun Isaac <arunisaac@systemreboot.net> writes:

> Alex Vong <alexvong1995@gmail.com> writes:
>
>> I think the problem is that when the scheme standard says "the returned
>> value is unspecified", it means anything can be returned. In this case,
>> guile choose to return an unspecified value to avoid returning an
>> arbitary value.
>>
>> I think the answer written by soegaard in [0] explains it pretty well.
>>
>> [0]: https://stackoverflow.com/questions/28910911/detecting-unspecified-in-scheme-list
>
> This is new to me. But, since we only use the Guile implementation, I
> think we should be ok with phases returning #<unspecified>.

But is there any guarantee that a Guile function returning
#<unspecified> won't return, say, #f in later versions?  If it does,
there would be a bug in phases that don't add #t at the end.

Re: Why should build phases not return unspecified values?

[Andy Wingo]

On Sun 17 Dec 2017 00:28, Arun Isaac <arunisaac@systemreboot.net> writes:

> Whenever we have a build phase that ends with a call (for example, to
> substitute, chdir, symlink, etc) that returns an unspecified value, we
> append a #t so that the return value is a boolean. However, the build
> system, as it stands currently, does not mind an unspecified value, and
> treats it as a success. As a result, forgetting to add a #t at the end
> of custom phases is a common mistake. To fix this, I have submitted a
> patch at https://debbugs.gnu.org/cgi/bugreport.cgi?bug=29745 that
> modifies the build system to reject unspecified values as
> failures.
>
> However, IMO, the addition of #t at the end of certain phases, does not
> contribute anything of value and we should simply be at peace with
> phases returning unspecified values. Am I missing something here?

I agree entirely.

IMO the continuation of a build phase should be:

  (define build-phase-cont
    (case-lambda
      (() #t)
      ((ret . _) (and ret #t))))

I.e. the phase could return 0 values, that's fine, we count as success.
Quite possible if your build phase ends up tail-calling to some
procedure you don't care about which returns 0 values for its own
reasons (arguably nicer than returning "the unspecified value", blah);
currently this doesn't work though.  Similarly for build phases that
return more than 1 value; extra values should be ignored (this is
currently the case).  Making a build phase return a single boolean value
in a language like Scheme is busy-work IMO.

Andy

Re: Why should build phases not return unspecified values?

[Mark H Weaver]

Arun Isaac <arunisaac@systemreboot.net> writes:

> Whenever we have a build phase that ends with a call (for example, to
> substitute, chdir, symlink, etc) that returns an unspecified value, we
> append a #t so that the return value is a boolean. However, the build
> system, as it stands currently, does not mind an unspecified value, and
> treats it as a success. As a result, forgetting to add a #t at the end
> of custom phases is a common mistake. To fix this, I have submitted a
> patch at https://debbugs.gnu.org/cgi/bugreport.cgi?bug=29745 that
> modifies the build system to reject unspecified values as
> failures.
>
> However, IMO, the addition of #t at the end of certain phases, does not
> contribute anything of value and we should simply be at peace with
> phases returning unspecified values. Am I missing something here?

I don't think we should rely on every "unspecified value" being treated
as #true in future versions of Guile.  That is merely an accident of the
current implementation.

However, I also agree that the current situation is a mess in need of
cleaning up.

My preference would be to deprecate the practice of returning explicit
boolean results from phases and snippets, and transition to reporting
errors exclusively using exceptions.

We would create a variant of the 'system*' procedure that raises an
exception in case of a non-zero status code.  This would eliminate the
many awkward code segments where we need to check multiple 'system*'
calls and manually handle propagating the errors upward, e.g. this one
from our 'sicp' package:

       (and (zero?
             (system* "makeinfo" "--output"
                      (string-append info-dir "/sicp.info")
                      (string-append source "/sicp-pocket.texi")))
            (every zero?
                   (map (cut system* "gzip" "-9n" <>)
                        (find-files info-dir))))

Most of the Guix veterans have surely seen or written bits of code like
this, and in my opinion it's an embarrassment.  We're using a language
that supports exceptions, and the overwhelming majority of our errors
are already being reported that way.  However, for historical reasons
we've ended up with this hybrid error reporting strategy where we
awkwardly use both exceptions and explicit error plumbing.  In addition
to the awkwardness, we have a great many bugs related to this.  We
surely still have a great many packages that are relying on "unspecified
value" being treated as #true.  We also surely have cases where the
results of 'system*' are not being checked at all.  This entire way of
doing things is error-prone.

Let's clean this up! [*]

Here's a transition plan: We could start by making the new
exception-throwing 'system*' variant, and switching existing packages to
use it, while removing the related error-code plumbing.  Once that work
is done, we could change the code that calls snippets or phase
procedures to ignore the result of those calls.  Finally, we could
remove the trailing #t's.

What do you think?

      Mark


[*] Incidentally, the clean up proposed here would not be possible if we
    had frozen our existing internal APIs to support external package
    repositories.

Re: Why should build phases not return unspecified values?

[Danny Milosavljevic]

Hi Mark,

I agree that exceptions are the way to go here.

Btw, Ludo and I put "invoke" into ./guix/build/utils.scm somewhen in June (now in master).

(define (invoke program . args)
  "Invoke PROGRAM with the given ARGS.  Raise an error if the exit
code is non-zero; otherwise return #t."
  (let ((status (apply system* program args)))
    (unless (zero? status)
      (error (format #f "program ~s exited with non-zero code" program)
             status))
    #t))

So we could actually use "invoke" in phases where we use "system*" now.  

There are 1331 lines with "system*" in gnu/packages - that would take some time to port.  Or we could substitute (zero? (system* ...)) by (invoke ...) with the editor - I don't see much risk with doing the latter, in core-updates-next or something.

In the far future, phase API could change to not examine the result of a phase and "invoke" would still work fine.

Re: Why should build phases not return unspecified values?

[Ludovic Courtès]

Howdy,

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

> I don't think we should rely on every "unspecified value" being treated
> as #true in future versions of Guile.  That is merely an accident of the
> current implementation.
>
> However, I also agree that the current situation is a mess in need of
> cleaning up.
>
> My preference would be to deprecate the practice of returning explicit
> boolean results from phases and snippets, and transition to reporting
> errors exclusively using exceptions.

Yes, that sounds more in line with what we usually do.

> We would create a variant of the 'system*' procedure that raises an
> exception in case of a non-zero status code.

Indeed.  Like Danny wrote, we can already start migrating to ‘invoke’,
which does exactly that.

> Here's a transition plan: We could start by making the new
> exception-throwing 'system*' variant, and switching existing packages to
> use it, while removing the related error-code plumbing.  Once that work
> is done, we could change the code that calls snippets or phase
> procedures to ignore the result of those calls.  Finally, we could
> remove the trailing #t's.
>
> What do you think?

That sounds good to me!

Concretely, we can:

  1. Encourage use of ‘invoke’ when reviewing or writing new package
     definitions;

  2. Gradually migrate packages (we can do a bit of that in
     ‘core-updates’, though we won’t do full rebuilds at this stage).

How does that sound?

Thanks,
Ludo’.

[bug#29745] Why should build phases not return unspecified values?

[Ludovic Courtès]

Howdy,

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

> I don't think we should rely on every "unspecified value" being treated
> as #true in future versions of Guile.  That is merely an accident of the
> current implementation.
>
> However, I also agree that the current situation is a mess in need of
> cleaning up.
>
> My preference would be to deprecate the practice of returning explicit
> boolean results from phases and snippets, and transition to reporting
> errors exclusively using exceptions.

Yes, that sounds more in line with what we usually do.

> We would create a variant of the 'system*' procedure that raises an
> exception in case of a non-zero status code.

Indeed.  Like Danny wrote, we can already start migrating to ‘invoke’,
which does exactly that.

> Here's a transition plan: We could start by making the new
> exception-throwing 'system*' variant, and switching existing packages to
> use it, while removing the related error-code plumbing.  Once that work
> is done, we could change the code that calls snippets or phase
> procedures to ignore the result of those calls.  Finally, we could
> remove the trailing #t's.
>
> What do you think?

That sounds good to me!

Concretely, we can:

  1. Encourage use of ‘invoke’ when reviewing or writing new package
     definitions;

  2. Gradually migrate packages (we can do a bit of that in
     ‘core-updates’, though we won’t do full rebuilds at this stage).

How does that sound?

Thanks,
Ludo’.

Re: Why should build phases not return unspecified values?

[Ricardo Wurmus]

Hi Ludo,

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

> Concretely, we can:
>
>   1. Encourage use of ‘invoke’ when reviewing or writing new package
>      definitions;
>
>   2. Gradually migrate packages (we can do a bit of that in
>      ‘core-updates’, though we won’t do full rebuilds at this stage).
>
> How does that sound?

I’m all for it!

I was convinced by what Mark wrote, and the migration seems simple
enough.

--
Ricardo

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

[bug#29745] Why should build phases not return unspecified values?

[Ricardo Wurmus]

Hi Ludo,

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

> Concretely, we can:
>
>   1. Encourage use of ‘invoke’ when reviewing or writing new package
>      definitions;
>
>   2. Gradually migrate packages (we can do a bit of that in
>      ‘core-updates’, though we won’t do full rebuilds at this stage).
>
> How does that sound?

I’m all for it!

I was convinced by what Mark wrote, and the migration seems simple
enough.

--
Ricardo

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

Re: Why should build phases not return unspecified values?

[Arun Isaac]

> Concretely, we can:
>
>   1. Encourage use of ‘invoke’ when reviewing or writing new package
>      definitions;
>
>   2. Gradually migrate packages (we can do a bit of that in
>      ‘core-updates’, though we won’t do full rebuilds at this stage).
>
> How does that sound?

Sounds good. I agree to all the above.