Discussion:
Why should build phases not return unspecified values?
Add Reply
Arun Isaac
2017-12-16 23:28:15 UTC
Reply
Permalink
Raw Message
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?
Pjotr Prins
2017-12-17 07:03:13 UTC
Reply
Permalink
Raw Message
Post by 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?
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.
--
Alex Vong
2017-12-17 07:10:19 UTC
Reply
Permalink
Raw Message
Post by 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?
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
Arun Isaac
2017-12-17 08:22:22 UTC
Reply
Permalink
Raw Message
Post by Alex Vong
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>.
Clément Lassieur
2017-12-17 10:35:12 UTC
Reply
Permalink
Raw Message
Post by Arun Isaac
Post by Alex Vong
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.
Andy Wingo
2017-12-18 09:40:00 UTC
Reply
Permalink
Raw Message
Post by 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?
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
Mark H Weaver
2017-12-19 21:35:34 UTC
Reply
Permalink
Raw Message
Post by 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?
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.
Danny Milosavljevic
2017-12-20 02:15:36 UTC
Reply
Permalink
Raw Message
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.
Ludovic Courtès
2017-12-20 09:27:16 UTC
Reply
Permalink
Raw Message
Howdy,
Post by Mark H Weaver
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.
Post by Mark H Weaver
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.
Post by Mark H Weaver
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’.
Arun Isaac
2017-12-20 10:27:58 UTC
Reply
Permalink
Raw Message
Post by Ludovic Courtès
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.
Ricardo Wurmus
2017-12-20 10:15:33 UTC
Reply
Permalink
Raw Message
Hi Ludo,
Post by Ludovic Courtès
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

Loading...