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

[Liliana Marie Prikler]

Hi Philip,

Am Mittwoch, dem 27.04.2022 um 16:03 -0400 schrieb Philip McGrath:
> [...]
> 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.
As stated above, I find the things below to be "cleanup"s, which I will
explain below.

> > 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.
While that is true, it comes at the cost of dumping literally
everything in a standard non-standard directory, which in and of
itself, is bad.  At the time I agreed to doing so because it moved
things forward to the inclusion of Racket 8.4 (and possibly 8.5 pp as
well), but getting things into an actual layout should be a long-term
goal for both the Guix packaging of Racket and Racket itself.

In particular, the parts of Racket that make it currently impossible to
build it in a more FHS-conforming way (such as layers) should be
implemented in terms of features we have and that are acceptable, such
as search paths.  I personally find it baffling that whatever is
achieved by layers needs those in the first place and can not be
achieved in a more standardized manner.

> >   (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:
This should tell you more so that you ought not place stuff in /opt. 
Just look at the code you wrote.  You have to resort to an ugly hack to
get indentation close to where you want and even then it's a painful
read.  I can not recall ever being fine with this.  Most of the phases
should not need patching with prefix set.  For the rest...

> ```
> 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>.
Note, that you would have to look for the ld.so.cache in /etc, not
/opt/wherever/etc.  That currently changes little, as /opt is not
searched for constructing the cache.  With /opt being a free for all,
that is sane behaviour, though.  If we find that we need to accomodate
further bin-directories, perhaps we might adjust make-dynamic-linker-
cache, but I wouldn't count on that necessity.

> >       (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'.)
Since racket-minimal does not suffice for cross-compilation as per this
comment and your words here, I decided to communicate this more clearly
by not having native-inputs – which itself suggests, that cross-
compilation was not yet thought of (hard enough).

> 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.
I understand that, but for me as a reviewer and a committer it doesn't
make an awful lot of sense to commit a huge chunk of garbage that will
be reverted in the next.  (Don't take garbage personal here, it refers
more so to the fact that these are gratuitous lines of diffs that
*everyone* will have to pull, which is not climate friendly.)

As for discussing changes, I am personally also a fan of communicating
more clearly, but I was urged for time and there is some consensus that
silently improving things is okay.

I would heavily advise you to not completely change things once more
for Racket 8.5.  That would just serve to show how bad packaging Racket
really is.  Improvements concerning build systems etc. are welcome, but
they should overall result in less of a mess than we had before
(particularly also considering the /opt mess).

I know this reply was dominated by me hating on /opt and I apologize
for that, but despite Guix itself not strictly adhering to FHS given
the /gnu/store directory, we don't disagree with the standard in the
sense that we believe separating binaries from libraries and data is a
bad thing.  Au contraire, this separation is important information and
should be embraced!

Cheers