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

[Maxim Cournoyer]

Hi,

Cayetano Santos <csantosb@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