[bug#53878] Fwd: [PATCH v7 00/24] Update Racket to 8.4. Adjust Chez Scheme packages.

[Philip McGrath <philip@philipmcgrath.com>]

(Hopefully I've made the necessary debbugs incantations for this to go 
through this time.)

-------- Forwarded Message --------
Subject: Re: [PATCH v7 00/24] Update Racket to 8.4. Adjust Chez Scheme 
packages.
Date: Wed, 27 Apr 2022 15:52:49 -0400
To: Liliana Marie Prikler <liliana.prikler@gmail.com>, 53878@debbugs.gnu.org

Hi,

On 3/4/22 17:59, Liliana Marie Prikler wrote:
> Hi Philip,
> 
> Am Sonntag, dem 27.02.2022 um 16:28 -0500 schrieb Philip McGrath:
>> Rather than debate it, I'm sending a v7 with the change to patch
>> 03/24 that Liliana requested in
>> <https://issues.guix.gnu.org/53878#187>.
> Sorry for the delay.  I cleaned up some of your commits and their
> messages, but apart from that pushed v7 without major changes.  I'm
> marking this as done now; if you feel I've made a mistake somewhere,
> don't hesitate to reopen.
> 

While getting ready for the upcoming Racket 8.5 release, I noticed some 
differences between my issue 53878 v7 patch series and the commits as 
applied to Guix that I hadn't noticed at the time. I don't want to make 
a big a deal out of it---I'm sure everyone was acting in good faith, and 
it isn't enormously consequential in the grand scheme of things---but it 
took me by surprise.

Considering a diff between my v7 and the series as merged:

> diff --cc gnu/packages/racket.scm
> index 8d44241414,471a11dd48..0000000000
> --- a/gnu/packages/racket.scm
> +++ b/gnu/packages/racket.scm
> @@@ -248,7 -248,10 +248,14 @@@
>         ,(string-append "CPPFLAGS=-DGUIX_RKTIO_PATCH_BIN_SH="
>                         #$(file-append bash-minimal "/bin/sh"))
>         "--disable-strip"
> ++<<<<<<< HEAD
>  +      "--enable-origtree"))
> ++=======
> +       ;; XXX: origtree layout is required by some other packages down the
> +       ;; bootstrap chain.  Remove these flags as soon as we can do without them.
> +       "--enable-origtree"
> +       ,(string-append "--prefix=" #$output "/opt/racket-vm")))
> ++>>>>>>> 992ed3b4ce20335ca61df0d29bfd02495dee87e6
>   

This comment was what first gave me pause: I was concerned it meant 
something had gone, so I went looking through the Git blame to find when 
it was added, and Git blamed me.

I think the comment is not quite right, factually (or, at least, is only 
true for very specific definitions of "required" and "bootstrap chain"), 
and, more significantly, I disagree that it should be a goal to "remove 
these flags": IMO, adding these flags brought us closer to being able to 
build the Racket VM, Racket packages, and Racket installation layers in 
a well-organized fashion.

>   (define-public racket-vm-cgc
>     ;; Eventually, it may make sense for some vm packages to not be hidden,
> @@@ -282,69 -285,36 +289,102 @@@
>          #:strip-directories #~'("opt/racket-vm/bin"
>                                  "opt/racket-vm/lib")
>          #:phases
> ++<<<<<<< HEAD
>  +       #~(let ()
>  +           (define* ((wrap-racket-vm-outputs phase) . args)
>  +             (apply
>  +              phase
>  +              (let loop ((args args))
>  +                (match args
>  +                  ((#:outputs outputs . args)
>  +                   `(#:outputs
>  +                     ,(let loop ((outputs outputs))
>  +                        (match outputs
>  +                          ((("out" . out) . outputs)
>  +                           `(("out" . ,(string-append out "/opt/racket-vm/"))
>  +                             ,@outputs))
>  +                          ((other . outputs)
>  +                           (cons other (loop outputs)))))
>  +                     ,@args))
>  +                  ((arg . args)
>  +                   (cons arg (loop args)))))))
>  +           (modify-phases %standard-phases
>  +             (add-before 'configure 'initialize-config.rktd
>  +               (lambda* (#:key inputs #:allow-other-keys)
>  +                 (define (write-racket-hash alist)
>  +                   ;; inside must use dotted pair notation
>  +                   (display "#hash(")
>  +                   (for-each (match-lambda
>  +                               ((k . v)
>  +                                (format #t "(~s . ~s)" k v)))
>  +                             alist)
>  +                   (display ")\n"))
>  +                 (define maybe-release-catalog
>  +                   (let ((v #$(package-version this-package)))
>  +                     (if (string-match "^[0-9]+\\.[0-9]+($|\\.[0-8][0-9]*$)"
>  +                                       v)
>  +                         `(,(string-append
>  +                             "https://download.racket-lang.org/releases/"
>  +                             v
>  +                             "/catalog/"))
>  +                         '())))
>  +                 (mkdir-p "racket/etc")
>  +                 (with-output-to-file "racket/etc/config.rktd"
>  +                   (lambda ()
>  +                     (write-racket-hash
>  +                      `((build-stamp . "")
>  +                        (catalogs ,@maybe-release-catalog
>  +                                  #f)))))))
>  +             (add-before 'configure 'chdir
>  +               (lambda _
>  +                 (chdir "racket/src")))
>  +             (replace 'configure
>  +               (wrap-racket-vm-outputs
>  +                (assoc-ref %standard-phases 'configure)))
>  +             (replace 'patch-shebangs
>  +               (wrap-racket-vm-outputs
>  +                (assoc-ref %standard-phases 'patch-shebangs)))
>  +             (replace 'validate-runpath
>  +               (wrap-racket-vm-outputs
>  +                (assoc-ref %standard-phases 'validate-runpath)))
>  +             (replace 'make-dynamic-linker-cache
>  +               (wrap-racket-vm-outputs
>  +                (assoc-ref %standard-phases 'make-dynamic-linker-cache)))
>  +             (replace 'patch-dot-desktop-files
>  +               (wrap-racket-vm-outputs
>  +                (assoc-ref %standard-phases 'patch-dot-desktop-files)))))))
> ++=======
> +        #~(modify-phases %standard-phases
> +            (add-before 'configure 'initialize-config.rktd
> +              (lambda* (#:key inputs #:allow-other-keys)
> +                (define (write-racket-hash alist)
> +                  ;; inside must use dotted pair notation
> +                  (display "#hash(")
> +                  (for-each (match-lambda
> +                              ((k . v)
> +                               (format #t "(~s . ~s)" k v)))
> +                            alist)
> +                  (display ")\n"))
> +                (define maybe-release-catalog
> +                  (let ((v #$(package-version this-package)))
> +                    (if (string-match "^[0-9]+\\.[0-9]+($|\\.[0-8][0-9]*$)"
> +                                      v)
> +                        `(,(string-append
> +                            "https://download.racket-lang.org/releases/"
> +                            v
> +                            "/catalog/"))
> +                        '())))
> +                (mkdir-p "racket/etc")
> +                (with-output-to-file "racket/etc/config.rktd"
> +                  (lambda ()
> +                    (write-racket-hash
> +                     `((build-stamp . "")
> +                       (catalogs ,@maybe-release-catalog
> +                                 #f)))))))
> +            (add-before 'configure 'chdir
> +              (lambda _
> +                (chdir "racket/src"))))))
> ++>>>>>>> 992ed3b4ce20335ca61df0d29bfd02495dee87e6
>        (home-page "https://racket-lang.org")
>        (synopsis "Old Racket implementation used for bootstrapping")
>        (description "This variant of the Racket BC (``before Chez'' or
> @@@ -599,6 -569,7 +639,10 @@@ DrRacket IDE, are not included."

More concretely, removing `wrap-racket-vm-outputs` here without a 
replacement means that the phases in question did not run on the 
relevant files/directories. For example, this interaction illustrates 
that the patches as applied don't generate a `ld.so.cache` for Racket:

```
philip@bastet:~$ echo v7
v7
philip@bastet:~$ ls $(guix time-machine 
--url=https://gitlab.com/philip1/guix-patches.git 
--disable-authentication 
--branch=racket-chez-refactor-guix-issue-53878-v7 -- build -e "(@ (gnu 
packages racket) racket-vm-cs)" 2>/dev/null | tail -n 1)/opt/racket-vm/etc
config.rktd  ld.so.cache
philip@bastet:~$ echo as applied
as applied
philip@bastet:~$ ls $(guix time-machine 
--commit=992ed3b4ce20335ca61df0d29bfd02495dee87e6 -- build -e "(@ (gnu 
packages racket) racket-vm-cs)" 2>/dev/null | tail -n 1)/opt/racket-vm/etc
config.rktd
philip@bastet:~$
```

I thought we had discussed the reason for this e.g. in 
<https://issues.guix.gnu.org/53878#32>.

>       (inherit racket-minimal)
>       (name "racket")
>       (source #f)
> ++<<<<<<< HEAD
> ++=======
> +     (native-inputs (list racket-minimal)) ; XXX: conservative estimate, untested
> ++>>>>>>> 992ed3b4ce20335ca61df0d29bfd02495dee87e6
>       (inputs
>        (list
>         cairo

(For completeness, this was also a change: it really isn't a big deal, 
but much more will be needed to be able to cross-compile Racket code, 
and I think it would be best addressed in the context of a 
'racket-build-system'.)

I certainly don't want anything that would further burden reviewers or 
slow down the process (this patch series having taken 7 revisions and 
almost a month as it is). For me, at least, it would have been easier to 
notice these changes if they had been in their own commit (or commits), 
rather than mixed in with changes that had been revised and discussed 
several times.

-Philip