GNU bug report logs - #75330
[PATCH v2] gnu: nim: Update to 2.2.2

Previous Next

Full log

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

From: Ludovic Courtès <ludo <at> gnu.org>
To: ashish.is <at> lostca.se
Cc: 75330 <at> debbugs.gnu.org
Subject: Re: [bug#75330] [PATCH] gnu: nim: Update to 2.2.0.
Date: Sun, 23 Feb 2025 23:58:00 +0100

Hi,

ashish.is <at> lostca.se skribis:

> From: Ashish SHUKLA <ashish.is <at> lostca.se>
>
> * 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’.

Previous Next

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