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

[Philip McGrath <philip@philipmcgrath.com>]

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