GNU bug report logs - #49280
[PATCH v2 0/3] gnu: racket: Update to 8.2. Bootstrap from C.

Previous Next

Full log

Message #23 received at 49280 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Philip McGrath <philip <at> philipmcgrath.com>
Cc: 49280 <at> debbugs.gnu.org
Subject: Re: bug#49280: [PATCH 0/4] gnu: racket: Add racket-next. Bootstrap
 from C.
Date: Thu, 08 Jul 2021 23:43:39 +0200

Hi,

Philip McGrath <philip <at> philipmcgrath.com> skribis:

> This commit bootstraps the Racket compiler and runtime system from source,
> including Racket CS as well as both variants of Racket BC. (One remaining
> limitation is discussed in comments added to gnu/packages/racket.scm.)
>
> In the process, it moves to building minimal Racket from the Git repository,
> rather than the packaged source tarballs. The Git repository is slightly
> better as the ``corresponding source'':
>
>  1. A few packages especially closely tied to the Racket core implementation
>     (like "compiler-lib", "base", and "racket-doc") are developed in the
>     same Git repository. Having them use the same Guix origin, too, will
>     help to keep them in sync.
>
>  2. The top-level Makefile in the Git repository is an important
>     ``script[] used to control compilation and installation.''
>     In particular, it cooperates with the "distro-build" package to
>     create the source tarballs and installers for a Racket distribution.
>     (Racket supports a notion of custom distributions.)
>
>  3. It is ``the preferred form ... for making modifications'' to the core
>     Racket implementation.
>
> Racket releases are tagged in the Git repository (e.g. "v8.1"). At the
> beginning of each release cycle, a branch is created to stabalize a version
> for extra testing. Active development happens on the "master" branch.
>
> * gnu/packages/racket-next-minimal-sh-via-rktio.patch: New file, coppied
> from racket-sh-via-rktio.patch to accomodate an extra directory layer.
> When racket-next-minimal becomes racket-minimal, this version will be
> the only one needed.
> * gnu/local.mk (dist_patch_DATA): Add it.
> * gnu/local/racket.scm (cfg-flag:sh-for-rktio, cfg-flag:enable-lt,
> cfg-flag:enable-racket, unpack-nanopass+stex,
> %main-repo-main-distribution-pkgs): New private variables.
> * gnu/local/racket.scm (racket-next-minimal)[source]: Use Git.
> [source](snippet): Unbundle nanopass, stex, and libffi.
> [inputs]: List explicitly.
> [native-inputs]: Use racket-next-bootstrap-bootfiles, plus its
> dependencies (for Chez, plus a Racket for bootstrappig).
> [arguments]: Revise extensively.
> * gnu/local/racket.scm (racket-next-minimal-bc-3m,
> racket-next-minimal-bc-cgc): New packages, hidden at least for now.
> (racket-next-bootstrap-chez-bootfiles): Another new package, but this one
> is especially likely to stay hidden.
> * gnu/local/racket.scm (racket-next)[origin](snippet): Unbundle packages
> developed in the main Git repository, but leave their links.rktd and
> pkgs.rktd entries in place.
> [native-inputs]: Add the main Racket Git repository.
> [arguments](#:phases): Adjust 'unpack-packages to also unpack package
> sources from the main Racket Git repository.

Exciting!

[...]

> +       (method git-fetch)
> +       (uri (git-reference
> +             (url "https://github.com/racket/racket")
> +             (commit "0874b76de4f147ada46607857d8acf8445a1073d")))
>         (sha256
>          (base32
> -         "0dm849wvlaxpfgz2qmgy2kwdslyi515rxn1m1yff38lagbn21vxq"))
> -       (uri (string-append %pre-release-installers
> -                           "racket-minimal-src.tgz"))))))
> +         "0gy6rwyrpaij5k5pcyiif821b4vffqiaxg1vpg4iykw2c5ypfp43"))
> +       (file-name
> +        (git-file-name name version))
> +       (patches
> +        (search-patches
> +         "racket-next-minimal-sh-via-rktio.patch"))

Please keep these on a single line, as in:

  (file-name (git-file-name name version))

> +         (replace 'configure
> +           (let ((inner (assq-ref %standard-phases 'configure)))
> +             (lambda args
> +               (chdir "racket/src")
> +               (apply inner args))))

I’d find it clearer like this:

  (add-before 'configure 'change-directory
    (lambda _
      (chdir "racket/src")))

> +         (add-after 'install 'remove-pkgs-directory
> +           ;; otherwise, e.g., `raco pkg show` will try and fail to
> +           ;; create a lock file
> +           (lambda* (#:key outputs #:allow-other-keys)
> +             ;; rmdir because we want an error if it isn't empty
> +             (rmdir (string-append (assoc-ref outputs "out")
> +                                   "/share/racket/pkgs"))
> +             #t)))))

Please write full sentences with a bit more context (“Remove package
directory, otherwise ‘raco pkg show’ …”).

> +(define-public racket-next-minimal-bc-3m
> +  (hidden-package
> +   (package/inherit racket-next-minimal
> +     (name "racket-next-minimal-bc-3m")

This is “-next” because it’s targeting 8.1, which is not released yet,
right?

Since it’s only used for bootstrapping, perhaps use ‘define’ instead of
‘define-public’ and remove the call to ‘hidden-package’.

It should also be (package (inherit …) …) rather than (package/inherit
…).  The latter is only useful when defining variants of a package (same
version, same code) where the same security updates would apply.

> +     (inputs
> +      `(("libffi" ,libffi) ;; <- only for BC variants
> +        ,@(filter (match-lambda
> +                    ((label . _)
> +                     (not (member label
> +                                  '("zlib" "zlib:static"
> +                                    "lz4" "lz4:static")))))
> +                  (package-inputs racket-next-minimal))))

Please use this more common idiom:

  ,@(fold alist-delete (package-inputs racket-next-minimal) '("zlib" …))

(It matters notably because ‘guix style’ recognizes it:
<https://issues.guix.gnu.org/49169>.)

> +     (synopsis "Minimal Racket with the BC [3M] runtime system")
> +     (description "The Racket BC (``before Chez'' or ``bytecode'') implementation was the default before Racket 8.0. It uses a compiler written in C targeting architecture-independent bytecode, plus a JIT compiler on most platforms. Racket BC has a different C API than the newer runtune system (Racket CS) supports a slightly different set of architectures than the current runtime system, Racket CS (based on ``Chez Scheme'').
> +
> +This packackage is the normal implementation of Racket BC with a precise garbage collector, 3M (``Moving Memory Mana
        ^
Typo here, and lines too long (here and in other places).  :-)

Please also check what ‘guix lint’ thinks!

> +(define-public racket-next-minimal-bc-cgc
> +  (package/inherit racket-next-minimal-bc-3m
> +    (name "racket-next-minimal-bc-cgc")
> +    (native-inputs
> +     (filter (match-lambda
> +               (("racket" . _)
> +                #f)
> +               (_
> +                #t))
> +             (package-native-inputs racket-next-minimal-bc-3m)))

Rather: (alist-delete "racket" (package-native-inputs racket-next-minimal-bc-3m))

> +     (license (package-license chez-scheme)))))

You cannot do that since here since potentially we could end up with
circular top-level references from these two modules.

Instead, restate what the license is.

Thanks for all this!

Ludo’.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.