[bug#55248] [PATCH 7/7] gnu: chez-scheme-for-system: Adjust support logic.

[Liliana Marie Prikler]

Am Donnerstag, dem 05.05.2022 um 16:42 -0400 schrieb Philip McGrath:
> Hi,
> 
> On 5/4/22 03:21, Liliana Marie Prikler wrote:
> > Am Dienstag, dem 03.05.2022 um 14:33 -0400 schrieb Philip McGrath:
> > 
> > > This is a follow-up to commit
> > > b8fc9169515ef1a6d6037c84e30ad308e5418b6f:
> > > see <https://issues.guix.gnu.org/54292>. Thanks to Liliana Marie
> > > Prikler for pointing out various issues, e.g. that being able to
> > > represent a Nix system as a Chez Scheme machine type does not
> > > necessarily mean the system is supported!
> > The issue in that commit is a different one: nix-system->chez-machine
> > can fail if there's no conversion.  Anyway...
> > 
> 
> The issue fixed in the commit is different, but this issue hadn't 
> occurred to me until you wrote in
> <https://issues.guix.gnu.org/54292#6>:
> 
> > I pushed that definition upstream, but a rewrite is still needed.  I
> > also think this logic should be a little decoupled from the
> > question of whether or not a given nix-system is supported.  While
> > surely this function returning #f means it's not, there are still
> > other questions to consider.
Ahh, in that case the commit message is pointing people to the wrong
location.  I think this needs to be communicated more clearly, e.g.

"This commit is a follow-up to
b8fc9169515ef1a6d6037c84e30ad308e5418b6f.  While that commit did fix a
breaking build, this one addresses the assumptions that lead to the
failure, see also <https://issues.guix.gnu.org/54292#6>."

Thereafter go on to describe what's actually done.


> > > [...]
> > > ;; Commentary:
> > > @@ -73,96 +71,17 @@ (define* (chez-scheme-for-system #:optional
> > >                                                (%current-
> > > system))))
> > >     "Return 'chez-scheme' unless only 'chez-scheme-for-racket'
> > > supports SYSTEM,
> > >   including support for native threads."
> > > -  (if (or
> > > -       ;; full support upstream
> > > -       (and=> (chez-upstream-features-for-system system)
> > > -              (cut memq 'threads <>))
> > > -       ;; no support anywhere
> > > -       (not (nix-system->chez-machine system)))
> > > +  (if (and=> (chez-upstream-features-for-system system)
> > > +             (lambda (features)
> > > +               (every (cut memq <> features)
> > > +                      '(threads
> > > +                        ;; We can cross-compile for platforms
> > > without
> > > +                        ;; bootstrap bootfiles, but we can't
> > > self-
> > > host
> > > +                        ;; on them short of adding more binary
> > > seeds.
> > > +                        bootstrap-bootfiles))))
> > >         chez-scheme
> > >         chez-scheme-for-racket))
> > Does it make sense to require 'threads always?
> > 
> 
> I guess there are a few notions of "always".
> 
> In 'chez-scheme-for-racket', yes, because Racket CS needs thread
> support for "futures" and "places". (Racket BC had a notion of
> platforms where those features were not available, but AFAIK there
> isn't support for a non-threaded configuration of Racket CS.)
> 
> For 'chez-scheme', every distribution I'm aware of packages the
> threaded version (only) on platforms where thread support is
> available. The only reason to use the nonthreaded version is if you
> know for sure that your application doesn't use threads---IIRC, that
> may even include any FFI libraries not using threads internally---AND
> the small performance gain from not implementing thread safety
> internally makes a difference.
> 
> For 'chez-scheme-for-system', I don't have a strong view, but the
> fact that I think the benefits of thread support are significant
> makes me lean that way. Concretely, the answer to this question only
> affects armhf-linux, so I think we should not change this at least
> until we re-enable it in upstream Chez's 'supported-system'.
In other words, there aren't that many uses of Chez scheme in embedded
spaces, so we might as well always require threads?

> > > -(define* (nix-system->chez-machine #:optional
> > > -                                   (system (or (%current-target-
> > > system)
> > > -                                               (%current-
> > > system))))
> > > -  "Return the Chez Scheme machine type corresponding to the Nix
> > > system
> > > -identifier SYSTEM, or @code{#f} if the translation of SYSTEM to
> > > a
> > > Chez Scheme
> > > -machine type is undefined.
> > > -
> > > -It is unspecified whether the resulting string will name a
> > > threaded
> > > or a
> > > -nonthreaded machine type: when the distinction is relevant, use
> > > -@code{chez-machine->nonthreaded} or @code{chez-machine-
> > > >threaded} to
> > > adjust
> > > -the result."
> > > -  (let* ((hyphen (string-index system #\-))
> > > -         (nix-arch (substring system 0 hyphen))
> > > -         (nix-os (substring system (+ 1 hyphen)))
> > > -         (chez-arch (assoc-ref %nix-arch-to-chez-alist nix-
> > > arch))
> > > -         (chez-os (assoc-ref %nix-os-to-chez-alist nix-os)))
> > > -    (and chez-arch chez-os (string-append chez-arch chez-os))))
> > > -
> > The replacement code should go here for readability imho.  At the
> > very least I was confused why this was first above and now below.
> > 
> 
> Happy to move things. Specifically, do you want 'target-chez-arch'
> and 'target-chez-os' (and '%chez-features-table'?) before 
> 'chez-upstream-features-for-system' and 
> 'racket-cs-native-supported-system?'?
This is my personal bias coming from a C background, but I read source
files top to bottom with helper procedures at the top and the main
thing at the bottom.  If you look closely, much of Guix also follows
that pattern.  For instance, build systems have their phases declared
at the bottom, "guix build" expects the last line to evaluate to a
package, and so on.

> > > +
> > For the sake of completeness, we might want to still have nix-
> > system-
> > > chez-machine (with a threaded? argument) defined in terms of
> > > target-
> > chez-arch and target-chez-os.  See 6/7 for motivation.
> > 
> 
> Eventually, I imagine we will want to have a function like 
> 'nix-system->chez-machine', but I think it would be better to wait
> until we have a concrete use-case. In particular, what I'd written
> here:
> 
>  >> -Note that this function only handles Chez Scheme machine types
> in
>  >> the
>  >> -strictest sense, not other kinds of descriptors sometimes used
> in
>  >> place of a
>  >> -Chez Scheme machine type by Racket, such as @code{\"pb\"},
>  >> @code{#f}, or
>  >> -@code{\"racket\"}.  (When using such extensions, the Chez Scheme
>  >> machine type
>  >> -for the host system is often still relevant.)"
> 
> is no longer necessarily true, thanks to the improvements in the 
> "portable bytecode" backends.
In other words, nix-system->chez-scheme would get an extra #:features
argument, which would be a sequence of 'threads and 'portable-bytecode,
no?  This question also has relevance w.r.t. 6/7 and potentially a 
chez-build-system, where this machine-type could actually be a
discriminating factor.

> > 
> [...]
> > All in all, the individual logic of this patch seems fine, but
> > overall it appears as though it's doing three separate things
> > (chez-scheme-for-system, chez features, racket-cs stuff).  IMO it
> > would make sense to split this patch according to those lines. 
> > WDYT?
> > 
> 
> I don't think I'm picturing what you have in mind.
> 
> The way I've been thinking of this patch is replacing the Chez
> features and machine type functions based on '%chez-features-table',
> then updating other things accordingly.
Sure, but if we retain nix-system->chez-machine as a function, I think
we can make a cut here and proceed with the second patch thereafter.

> I guess there is a distinguishable change to the behavior of 
> 'chez-scheme-for-system' for systems with no native-code backed. I
> could separate that, if you want. On the other hand, it continues to
> return a package that can't actually be built for the specified
> system, so the change seems mostly theoretical.
This should be the second patch imo.  Regardless of theoreticness in
value, I think the change itself is one that deserves its own commit
message.  It would also be easier to review and reason about later that
way.

> In terms of "racket-cs stuff", 'racket-cs-native-supported-system?' 
> seemed better than any name I could come up with based on 
> 'chez-scheme-for-racket', but the answer is based only on Racket's 
> variant of Chez scheme. The old version based on 
> 'nix-system->chez-machine' was just wrong (it would falsely claim to 
> support e.g. "powerpc-w64-mingw32"), and we didn't have a way to 
> implement a correct function until adding the information in 
> '%chez-features-table'.
This would be the third patch according to my initial suggestion.  That
way, racket-cs-native-supported-system? would remain wrong for patch
7/9, but be corrected in patch 9/9, which imo would more clearly
communicate that it was previously wrong.

WDYT?