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

[Philip McGrath]

Hi,

On 5/6/22 03:08, Liliana Marie Prikler wrote:
> 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.

Yes, that seems better. (I was trying to focus the summary on 
user-facing functions, rather than replaced internals, but I think it 
can be better.)

> > > > [...]
> > > > ;; 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?

At least, I think it's a sufficiently niche use-case that its reasonable 
for those users to use a package transformation (and check that it works 
for all of the libraries they use) while allowing general Chez code to 
assume threads are available, since they are well supported overall.

(I have heard stories about embedded Chez, e.g. to control a Disney 
World virtual reality ride.[1] But I'm not aware of any free software 
Chez projects that don't work with 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.

My personal preference vacillates between defining helpers before using 
them and putting high-level or public definitions above internal utilities.

> > > > +
> > > 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.

It may end up being more complex than that, depending on how many of the 
underlying options we want to expose via Guix. For example, there is 
"pb" for a fully machine-independent bytecode, but e.g. "tpb64l" for a 
specialized bytecode for 64-bit little-endian machines with threads. I 
also don't yet understand when, if ever, we might want to supply a 
native machine type (if one is defined) in addition to a pb-based 
machine types.

The uncertainty is why I'd rather avoid nix-system->chez-machine until 
we actually need it.

> > >
> > [...]
> > > 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 don't think we should retain nix-system->chez-machine beyond this 
series, but I guess we can delay removing it to split here.

> > 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.

This is the part that makes the most sense to me to put in its own commit.

> > 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?

I will try to come up with a v2 more or less along these lines.

-Philip

[1]: 
https://groups.google.com/g/comp.lang.scheme/c/Xud6nGrF0Ss/m/BaJDopHMYYAJ