GNU bug report logs - #78063
[PATCH electronics-team] gnu: Add prjtrellis.

Previous Next

Full log

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

From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
To: Cayetano Santos <csantosb <at> inventati.org>
Cc: 78063 <at> debbugs.gnu.org, Ekaitz Zarraga <ekaitz <at> elenq.tech>
Subject: Re: [bug#78063] [PATCH electronics-team] gnu: Add prjtrellis.
Date: Fri, 02 May 2025 16:19:09 +0900

Hi,

Cayetano Santos <csantosb <at> inventati.org> writes:

> * gnu/packages/electronics.scm (prjtrellis): New variable.

[...]

> +(define-public prjtrellis
> +  (package
> +    (name "prjtrellis")
> +    (version "1.4")
> +    (source
> +     (origin
> +       (method git-fetch)
> +       (uri (git-reference
> +             (url "https://github.com/YosysHQ/prjtrellis/")
> +             (commit version)
> +             (recursive? #t)))

This needs an explanation, explaining why the bundled libraries/sources
in the checkout cannot be packaged separately: our standard practice is
to try to unbundle everything and build the components as distinct
packages.  Here it looks like it pulls some kind of database files used
by the project.  That's fine to use a recursive? #t for that, the reason
just needs to be explained.

[...]

> +      #:tests? #f ; tests are to be run from nextpnr-ecp5

nitpick: leave at least two spaces between the code and the ';' margin
comment, as ensured by Emacs with 'M-;'.  Also, I'm not sure I get what
it means: the test are run from another package?  Could you explain a
bit more in a standalone comment above the argument?

> +      #:phases
> +      #~(modify-phases %standard-phases
> +          (add-after 'unpack 'chdir
> +            (lambda _
> +              (chdir "libtrellis")))
> +          ;; Remove bundled source code for which Guix has packages.
> +          (add-after 'chdir 'remove-deps
> +            (lambda _
> +              (with-directory-excursion "3rdparty"
> +                (for-each delete-file-recursively
> +                          '("pybind11")))))

This would benefit from being done in the source directly, as a snippet
attached to the origin, to remove the extraneous files from the source
(which needlessly adds weigh to the source checkout).

But as I suggested above, perhaps recursive? #t can be removed, and the
few dependencies packaged separately, if they can be built as libraries.

> +          (add-after 'remove-deps 'setenv-pybind11
> +            (lambda* (#:key inputs #:allow-other-keys)
> +              (setenv "PYBIND11_INCLUDE_DIR"
> +                      (string-append #$(this-package-input "pybind11")
> +                                     "/include/pybind11")))))))
> +    (native-inputs (list python))
> +    (inputs (list openocd boost pybind11))
> +    (synopsis "Placement and routing for ECP5 FPGAs")
> +    (description
> +     "Project Trellis is a Nextpnr backend compatible with ECP5 FPGAs.")

The description is a bit too terse.  It'd be nice to know what features
are included the software provides, for example, or what are the various
commands are included (if there are many), and what they do.  You can
use a '@table @command' for this.

Otherwise it looks good to me.  Could you please submit a v2?

-- 
Thanks,
Maxim

Previous Next

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