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

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!

Racket's variant of Chez Scheme does or will soon run even on systems
for which native code generation is not supported: see discussion at
<https://racket.discourse.group/t/950>. This helps us to eliminate
unused functions (that may not have been quite right, anyway).

* gnu/packages/chez.scm (chez-machine->nonthreaded,
chez-machine->threaded, %nix-arch-to-chez-alist, %nix-os-to-chez-alist,
chez-machine->nix-system, nix-system->chez-machine): Replace with ...
(%chez-features-table, target-chez-arch, target-chez-os): ... these new
variables.
(chez-upstream-features-for-system): Adapt accordingly.
(chez-scheme-bootstrap-bootfiles)[supported-systems]: Use enhanced
'chez-upstream-features-for-system'.
(chez-scheme-for-system): Likewise, and use 'chez-scheme-for-racket' on
systems without native code generation.
(racket-cs-native-supported-system?): New variable.
(chez-scheme-for-racket)[supported-systems]: Use it.
* gnu/packages/racket.scm (racket-vm-for-system): Likewise.
---
 gnu/packages/chez.scm   | 269 +++++++++++++++++++++++-----------------
 gnu/packages/racket.scm |   7 +-
 2 files changed, 159 insertions(+), 117 deletions(-)

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

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

>  
> -(define (chez-machine->nonthreaded machine)
> -  "Given a string MACHINE naming a Chez Scheme machine type, returns
> a string
> -naming the nonthreaded machine type for the same architecture and OS
> as
> -MACHINE.  The returned string may share storage with MACHINE."
> -  ;; Chez Scheme documentation consistently uses "nonthreaded"
> rather than
> -  ;; e.g. "unthreaded"
> -  (if (eqv? #\t (string-ref machine 0))
> -      (substring machine 1)
> -      machine))
> -(define (chez-machine->threaded machine)
> -  "Like @code{chez-machine->nonthreaded}, but returns the threaded
> machine
> -type."
> -  (if (eqv? #\t (string-ref machine 0))
> -      machine
> -      (string-append "t" machine)))
> -
> -;; Based on the implementation from raco-cross-
> lib/private/cross/platform.rkt
> -;; in https://github.com/racket/raco-cross.
> -;; For supported platforms, refer to
> release_notes/release_notes.stex in the
> -;; upstream Chez Scheme repository or to
> racket/src/ChezScheme/README.md
> -;; in https://github.com/racket/racket.
> -(define %nix-arch-to-chez-alist
> -  `(("x86_64" . "a6")
> -    ("i386" . "i3")
> -    ("aarch64" . "arm64")
> -    ("armhf" . "arm32") ;; Chez supports ARM v6+
> -    ("ppc" . "ppc32")))
> -(define %nix-os-to-chez-alist
> -  `(("w64-mingw32" . "nt")
> -    ("darwin" . "osx")
> -    ("linux" . "le")
> -    ("freebsd" . "fb")
> -    ("openbsd" . "ob")
> -    ("netbsd" . "nb")
> -    ("solaris" . "s2")))
> -
> -(define (chez-machine->nix-system machine)
> -  "Return the Nix system type corresponding to the Chez Scheme
> machine type
> -MACHINE.  If MACHINE is not a string representing a known machine
> type, an
> -exception is raised.  This function does not distinguish between
> threaded and
> -nonthreaded variants of MACHINE.
> -
> -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.)"
> -  (let ((machine (chez-machine->nonthreaded machine)))
> -    (let find-arch ((alist %nix-arch-to-chez-alist))
> -      (match alist
> -        (((nix . chez) . alist)
> -         (if (string-prefix? chez machine)
> -             (string-append
> -              nix "-" (let ((machine-os
> -                             (substring machine (string-length
> chez))))
> -                        (let find-os ((alist %nix-os-to-chez-alist))
> -                          (match alist
> -                            (((nix . chez) . alist)
> -                             (if (equal? chez machine-os)
> -                                 nix
> -                                 (find-os alist)))))))
> -             (find-arch alist)))))))
> -
> -(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.

>  (define* (chez-upstream-features-for-system #:optional
>                                              (system
>                                               (or (%current-target-
> system)
> @@ -172,20 +91,150 @@ (define* (chez-upstream-features-for-system
> #:optional
>  does not support SYSTEM at all.
>  
>  If native threads are supported, the returned list will include
> -@code{'threads}.  Other feature symbols may be added in the future."
> +@code{'threads}.  If bootstrap bootfiles for SYSTEM are distributed
> in the
> +upstream Chez Scheme repository, the returned list will include
> +@code{'bootstrap-bootfiles}.  Other feature symbols may be added in
> the
> +future."
> +  (let ((chez-arch (target-chez-arch system))
> +        (chez-os (target-chez-os system)))
> +    (and=> (assoc-ref %chez-features-table chez-os)
> +           (cut assoc-ref <> chez-arch))))
> +
> +(define* (racket-cs-native-supported-system? #:optional
> +                                             (system
> +                                              (or (%current-target-
> system)
> +                                                  (%current-
> system))))
> +  "Can Racket's variant of Chez Scheme generate native code for
> SYSTEM?
> +Otherwise, SYSTEM can use only the ``portable bytecode'' backends."
> +  (let ((chez-arch (target-chez-arch system))
> +        (chez-os (target-chez-os system)))
> +    (and (and=> (assoc-ref %chez-features-table chez-os)
> +                ;; NOT assoc-ref: supported even if cdr is #f
> +                (cut assoc chez-arch <>))
> +         #t)))
> +
> +(define %chez-features-table
> +  ;; An alist of alists mapping:
> +  ;;   os -> arch -> (or/c #f (listof symbol?))
> +  ;; where:
> +  ;;  - `os` is a string for the OS part of a Chez Scheme machine
> type; and
> +  ;;  - `arch` is a string for the architecture part of a Chez
> machine type.
> +  ;;
> +  ;; The absence of an entry for a given arch--os pair means that
> neither
> +  ;; upstream Chez Scheme nor the Racket variant can generate native
> code for
> +  ;; that system.  (The Racket variant can still provide support via
> its
> +  ;; ``portable bytecode'' backends and optional compilation to C.) 
> A value
> +  ;; of `#f` means that upstream Chez Scheme does not support the
> arch--os
> +  ;; pair at all, but the Racket variant does.  A list has the same
> meaning as
> +  ;; a result from `chez-upstream-features-for-system`.
> +  ;;
> +  ;; The arch--os pairs marked "commented out" have been commented
> out in the
> +  ;; STeX source for the upstream release notes since the initial
> release as
> +  ;; free software, but they are reported to work and/or have been
> described
> +  ;; as supported by upstream maintainers.
> +  ;;
> +  ;; For this overall approach to make sense, we assume that
> Racket's variant
> +  ;; of Chez Scheme can generate native code for a superset of the
> platforms
> +  ;; supported upstream, supports threads on all platforms it
> supports at all
> +  ;; (because they are needed for Racket), and doesn't need
> bootstrap
> +  ;; bootfiles.  Those assumptions have held for several years.
> +  '(;; Linux
> +    ("le"
> +     ("i3" threads bootstrap-bootfiles)
> +     ("a6" threads bootstrap-bootfiles)
> +     ("arm32" bootstrap-bootfiles)
> +     ("arm64" . #f)
> +     ("ppc32" threads))
> +    ;; FreeBSD
> +    ("fb"
> +     ("i3" threads) ;; commented out
> +     ("a6" threads) ;; commented out
> +     ("arm32" . #f)
> +     ("arm64" . #f)
> +     ("ppc32" . #f))
> +    ;; OpenBSD
> +    ("ob"
> +     ("i3" threads) ;; commented out
> +     ("a6" threads) ;; commented out
> +     ("arm32" . #f)
> +     ("arm64" . #f)
> +     ("ppc32" . #f))
> +    ;; NetBSD
> +    ("nb"
> +     ("i3" threads) ;; commented out
> +     ("a6" threads) ;; commented out
> +     ("arm32" . #f)
> +     ("arm64" . #f)
> +     ("ppc32" . #f))
> +    ;; OpenSolaris / OpenIndiana / Illumos
> +    ("s2"
> +     ("i3" threads) ;; commented out
> +     ("a6" threads)) ;; commented out
> +    ;; Windows
> +    ("nt"
> +     ("i3" threads bootstrap-bootfiles)
> +     ("a6" threads bootstrap-bootfiles)
> +     ;; ^ threads "experiemental", but reportedly fine
> +     ("arm64" . #f))
> +    ;; Darwin
> +    ("osx"
> +     ("i3" threads bootstrap-bootfiles)
> +     ("a6" threads bootstrap-bootfiles)
> +     ("arm64" . #f)
> +     ("ppc32" . #f))))
> +
> +(define* (target-chez-arch #:optional (system
> +                                       (or (%current-target-system)
> +                                           (%current-system))))
> +  "Return a string representing the architecture of SYSTEM as used
> in Chez
> +Scheme machine types, or '#f' if none is defined."
>    (cond
> -   ((not (nix-system->chez-machine system))
> -    #f)
> +   ((target-x86-64? system)
> +    "a6")
> +   ((target-x86-32? system)
> +    "i3")
>     ((target-aarch64? system)
> -    #f)
> +    "arm64")
>     ((target-arm32? system)
> -    (and (target-linux? system)
> -         '()))
> +    "arm32")
> +   ((target-ppc64le? system)
> +    #f)
>     ((target-ppc32? system)
> -    (and (target-linux? system)
> -         '(threads)))
> +    "ppc32")
> +   ((target-riscv64? system)
> +    #f)
>     (else
> -    '(threads))))
> +    #f)))
> +
> +(define* (target-chez-os #:optional (system (or (%current-target-
> system)
> +                                                (%current-system))))
> +  "Return a string representing the operating system kernel of
> SYSTEM as used
> +in Chez Scheme machine types, or '#f' if none is defined."
> +  ;; e.g. "le" includes both GNU/Linux and Android
> +  (cond
> +   ((target-linux? system)
> +    "le")
> +   ((target-hurd? system)
> +    #f)
> +   ((target-mingw? system)
> +    "nt")
> +   ;; missing (guix utils) predicates
> +   ;; cf.
> https://github.com/NixOS/nixpkgs/blob/master/lib/systems/doubles.nix
> +   ((string-suffix? "-darwin" system)
> +    "osx")
> +   ((string-suffix? "-freebsd" system)
> +    "fb")
> +   ((string-suffix? "-openbsd" system)
> +    "ob")
> +   ((string-suffix? "-netbsd" system)
> +    "nb")
> +   ;; Nix says "x86_64-solaris", but accommodate "-solaris2"
> +   ((string-contains system "solaris")
> +    "s2")
> +   ;; unknown
> +   (else
> +    #f)))
> +
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.

>  
>  ;;
>  ;; Chez Scheme:
> @@ -365,14 +414,9 @@ (define-public chez-scheme
>                    ((pth)
>                     (symlink pth
>                              "csug.pdf")))))))))
> -    ;; Chez Scheme does not have a  MIPS backend.
> -    ;; FIXME: Debian backports patches to get armhf working.
> -    ;; We should too. It is the Chez machine type arm32le
> -    ;; (no threaded version upstream yet, though there is in
> -    ;; Racket's fork), more specifically (per the release notes)
> ARMv6.
>      (supported-systems
>       (delete
> -      "armhf-linux" ;; <-- should work, but reportedly broken
> +      "armhf-linux" ;; XXX is this still broken?
I'd say "XXX: reportedly broken, needs checking"
>        (filter chez-upstream-features-for-system
>                %supported-systems)))
>      (home-page "https://cisco.github.io/ChezScheme/")
> @@ -418,7 +462,9 @@ (define-public chez-scheme-for-racket
>                (add-after 'unpack 'chdir
>                  (lambda args
>                    (chdir "racket/src/ChezScheme"))))))))
> -    (supported-systems (filter nix-system->chez-machine
> +    ;; TODO: How to build pbarch/pbchunks for other systems?
> +    ;; See https://racket.discourse.group/t/950
> +    (supported-systems (filter racket-cs-native-supported-system?
>                                 %supported-systems))
>      (home-page "https://github.com/racket/ChezScheme")
>      ;; ^ This is downstream of https://github.com/racket/racket,
> @@ -471,16 +517,9 @@ (define-public chez-scheme-bootstrap-bootfiles
>       (list #:install-plan
>             #~`(("boot/" "lib/chez-scheme-bootfiles"))))
>      (supported-systems
> -     ;; Upstream only distributes pre-built bootfiles for
> -     ;; arm32le and t?(i3|a6)(le|nt|osx)
>       (filter (lambda (system)
> -               (let ((machine (and=> (nix-system->chez-machine
> system)
> -                                     chez-machine->nonthreaded)))
> -                 (or (equal? "arm32le" machine)
> -                     (and machine
> -                          (member (substring machine 0 2) '("i3"
> "a6"))
> -                          (or-map (cut string-suffix? <> machine)
> -                                  '("le" "nt" "osx"))))))
> +               (and=> (chez-upstream-features-for-system system)
> +                      (cut memq 'bootstrap-bootfiles <>)))
Yup, that's simpler.
>               %supported-systems))
>      (synopsis "Chez Scheme bootfiles (binary seed)")
>      (description
> diff --git a/gnu/packages/racket.scm b/gnu/packages/racket.scm
> index 2f4f7cebd8..41f45f4215 100644
> --- a/gnu/packages/racket.scm
> +++ b/gnu/packages/racket.scm
> @@ -190,8 +190,11 @@ (define-module (gnu packages racket)
>  (define* (racket-vm-for-system #:optional
>                                 (system (or (%current-target-system)
>                                             (%current-system))))
> -  "Return 'racket-vm-cs' if it supports SYSTEM; 'racket-vm-bc'
> otherwise."
> -  (if (nix-system->chez-machine system)
> +  "Return 'racket-vm-cs' we are able to build it for SYSTEM;
> 'racket-vm-bc'
> +otherwise."
> +  ;; Once we figure out the issues in
> https://racket.discourse.group/t/950,
> +  ;; we can use 'racket-vm-cs' everywhere.
> +  (if (racket-cs-native-supported-system? system)
>        racket-vm-cs
>        racket-vm-bc))
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?

Patches 2-5 mostly LGTM, at least I don't see any glaring issues in
this iteration.

Cheers

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.


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

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


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

>>   
>>   ;;
>>   ;; Chez Scheme:
>> @@ -365,14 +414,9 @@ (define-public chez-scheme
>>                     ((pth)
>>                      (symlink pth
>>                               "csug.pdf")))))))))
>> -    ;; Chez Scheme does not have a  MIPS backend.
>> -    ;; FIXME: Debian backports patches to get armhf working.
>> -    ;; We should too. It is the Chez machine type arm32le
>> -    ;; (no threaded version upstream yet, though there is in
>> -    ;; Racket's fork), more specifically (per the release notes)
>> ARMv6.
>>       (supported-systems
>>        (delete
>> -      "armhf-linux" ;; <-- should work, but reportedly broken
>> +      "armhf-linux" ;; XXX is this still broken?
> I'd say "XXX: reportedly broken, needs checking"

That seems better, particularly given e.g. 
<https://github.com/cisco/ChezScheme/issues/622#issuecomment-1110290004>:


 > > it is likely musl-related since I assume that arm32le is well tested
 > in conjunction with glibc
 >
 > That's probably not the best assumption... arm32le is not tested in
 > GitHub automation, and the last work that I know for sure was done on
 > it was for a project that is now defunct. I'm sure it was working and
 > tested at some point, but bit rot may have set in.
 >


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

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.

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

-Philip

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?

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