Subject: Re: [bug#75330] [PATCH] gnu: nim: Update to 2.2.0.

From: Ludovic Courtès <[email-masked]>
Date: Sun, 23 Feb 2025 23:58:00 +0100

Hi,

[email-masked] skribis:

> From: Ashish SHUKLA <[email-masked]>
>
> * gnu/packages/nim.scm (%atlas-commit, %sat-commit, atlas, sat): New
> variables.
> (nim): Update to 2.2.0. [arguments]: Use G-expressions. <#:phases>:
> Add phase "copy-deps". Update phases "patch-installer",
> "patch-dynamic-libraries", and "patch-more-shebangs".
>
> Change-Id: Ibd8fdaf7f033755ada3e4638a9f3a9295cd5e3b2

[...]

> +;; referenced in koch.nim
> +(define %atlas-commit "5faec3e9a33afe99a7d22377dd1b45a5391f5504")
> +(define %sat-commit "faf1617f44d7632ee9601ebc13887644925dcc01")

Since there’s only one reference to each of these variables, I think the
commit ID can go directly in the ‘commit’ field, without defining these
variables.

> +             (add-after 'unpack 'copy-deps:www
> +               (lambda _
> +                 (copy-recursively #$atlas "dist/atlas"
> +                                   #:keep-permissions? #f)
> +                 (copy-recursively #$sat "dist/atlas/dist/sat"
> +                                   #:keep-permissions? #f)))

Could you add a ‘file-name to ‘atlas’ and ‘sat’, add the both to
‘inputs’ of this package, and then refer to them in this phase like so:

  (copy-recursively #$(this-package-input "atlas-checkout") "dist/atlas"
                    #:keep-permissions? #f)

… where “atlas-checkout” must match the ‘file-name’ field of ‘atlas’.

That way, those inputs are visible to code the traverses the package
graph.

> +             (add-after 'patch-source-shebangs 'patch-more-shebangs
> +               (lambda _
> +                 (use-modules (ice-9 rdelim))
> +                 (use-modules (ice-9 regex))

Please use the #:modules parameters instead of ‘use-modules’ in the
middle of the phase.

> +                 (define sh (which "sh"))
> +                 (define sh-len (string-length sh))
> +
> +                 (define rx1 (make-regexp "^(.*NIM_CHAR data\\[)7(\\+1\\];.*)$" regexp/extended))
> +                 ;; } TM__HZdw8BhppcTQo8DIK46LSg_5 = { 7 | NIM_STRLIT_FLAG, "/bin/sh" };
> +                 (define rx2 (make-regexp
> +                               (string-append "^(\\} )"
> +                                               "([^[:space:]]+)"
> +                                               "( = \\{ )"
> +                                               "7"
> +                                               "( [|] NIM_STRLIT_FLAG, )"
> +                                               "\"/bin/sh\""
> +                                               "(.*)$")
> +                               regexp/extended))
> +
> +                 (define (fixup-1 matches out)
> +                   (format out "~a~a~a\n"
> +                           (match:substring matches 1)
> +                           sh-len
> +                           (match:substring matches 2))
> +                   #f)
> +
> +                 (define (fixup-2 matches out)
> +                   (format out "~a~a~a~a~a~s~a\n"
> +                           (match:substring matches 1)
> +                           (match:substring matches 2)
> +                           (match:substring matches 3)
> +                           sh-len
> +                           (match:substring matches 4)
> +                           sh
> +                           (match:substring matches 5))
> +                   (match:substring matches 2))
> +
> +                 (define fixups
> +                   (list (cons rx1 fixup-1)
> +                         (cons rx2 fixup-2)))

I think this whole part could use some comments, at the very least for
the cryptic ‘rx1’.

> +                 (define (rx-match rx line in out)
> +                   (if (null? rx)
> +                       (begin
> +                         (format out "~a\n" line)
> +                         #f)
> +
> +                       (let ((matches (regexp-exec (caar rx) line)))
> +                         (if (regexp-match? matches)
> +                             ((cdar rx) matches out)
> +                             (rx-match (cdr rx) line in out)))))

Please use ‘match’ instead of cdar & co. (info "(guix) Data Types and
Pattern Matching").

Apart from that LGTM.

Could you send an updated patch?

Thanks,
Ludo’.