Subject: Re: [bug#78233] [PATCH 2/2] gnu: nextpnr-ice40: Update to 0.8.

From: Maxim Cournoyer <[email-masked]>
Date: Thu, 08 May 2025 22:07:12 +0900

Hi,

Cayetano Santos <[email-masked]> writes:

> * gnu/packages/fpga.scm (nextpnr-ice40): Update to 0.8.

[...]

> +(define-public nextpnr-ice40
> +  (package
> +    (inherit nextpnr)
> +    (name "nextpnr-ice40")
> +    (arguments
> +     (substitute-keyword-arguments (package-arguments nextpnr)
> +       ;; tests

I'd drop this comment.  It's not a complete sentence, and doesn't help.

> +       ((#:phases phases #~%standard-phases)
> +        #~(modify-phases #$phases
> +            ;; get icestorm/examples

> +            (add-after 'compress-documentation 'get-icestorm

I'd drop the comment and rename the phase to 'copy-icestorm-source

> +              (lambda* (#:key inputs #:allow-other-keys)
> +                (copy-recursively
> +                 #$(origin (inherit (package-source icestorm)))

You can drop the origin + inherit; package-source returns an
origin too.

> +                 "icestorm")))

The `inputs' keyword is not used.  The lambda signature can be just

 (lambda _
  ...)

Why order after compress-documentation?  Typically if something needs to
be patched or copied, I do so after the `unpack' phase.
  
> +            ;; run all icestorm/examples as tests
> +            (add-after 'get-icestorm 'tests-icestorm-examples

I'd drop the above comment and rename the phase to 'build-icestorm-examples

> +              (lambda* _

s/lambda*/lambda

> +                (let ((dir (opendir "icestorm/examples")))
> +                  (do ((entry (readdir dir)
> +                              (readdir dir)))
> +                      ((eof-object? entry))
> +                    (when (not (member entry '("." "..")))

Anytime you see (when (not ...)), turn it into (unless ...).

> +                      (setenv "PATH"
> +                              (string-append (string-append #$output "/bin")

No need to nest 'string-append'; the first one alone suffices.

> +                                             ":"
> +                                             (getenv "PATH")))
> +                      (invoke "make" "-C"
> +                              (string-append "icestorm/examples/" entry))))
> +                  (closedir dir))))))

Interesting, I had never seen the opendir/closedir approach, perhaps
because the code base in Guix embraces functional rather than imperative
style.  It'd be more conventional (in Guix at least) to use (ice-9
ftw)'s scandir to list the files and apply for-each to its value, and I
think it'd be more succinct too.

Something like:

(with-directory-excursion "icestorm/examples"
                       (for-each (cut invoke "make" "-C" <>)
                                 (scandir "."
                                          (negate (cut member <> '("." ".."))))))

You'll need to import the the (srfi srfi-26) module (for 'cut') and
(ice-9 ftw) (for 'scandir').

> +       ((#:configure-flags original-flags #~(list))
> +        #~(append #$original-flags
> +                  `("-DARCH=ice40"
> +                    ,(string-append "-DICESTORM_INSTALL_PREFIX="
> +                                    #$(this-package-input "icestorm")))))))
> +    (propagated-inputs (modify-inputs (package-propagated-inputs nextpnr)
> +                         (prepend icestorm)))

It's just a styling nitpick, but I like to format this like:

  (propagated-inputs
   (modify-inputs (package-propagated-inputs nextpnr)
    (prepend icestorm))

I find that it mixes better with the often-found:

  (propagated-inputs
   (list one
         two
         three
         ...))

forms.

> +    ;; tests

Stray comment?

> +    (native-inputs (modify-inputs (package-native-inputs nextpnr)
> +                     (prepend yosys)))))
> +
>  (define-public yosys
>    (package
>      (name "yosys")
> @@ -434,103 +475,6 @@ (define-public icestorm
>  files.")
>        (license license:isc))))
>  
> -(define-public nextpnr-ice40
> -  (let* ((version "0.7")
> -         (tag (string-append "nextpnr-" version)))
> -    (package
> -      (name "nextpnr-ice40")
> -      (version version)
> -      (source
> -       (origin
> -         (method git-fetch)
> -         (uri (git-reference
> -               (url "https://github.com/YosysHQ/nextpnr")
> -               (commit tag)
> -               (recursive? #t)))
> -         (file-name (git-file-name name version))
> -         (sha256
> -          (base32
> -           "0sbhqscgmlk4q2207rsqsw99qx4fyrxx1hsd669lrk42gmk3s9lm"))
> -         (modules '((guix build utils)))
> -         (snippet
> -          #~(begin
> -              ;; Remove bundled source code for which Guix has packages.
> -              ;; Note the bundled copies of json11 and python-console contain
> -              ;; modifications, while QtPropertyBrowser appears to be
> -              ;; abandoned and without an official source.
> -              ;; fpga-interchange-schema is used only by the
> -              ;; "fpga_interchange" architecture target, which this package
> -              ;; doesn't build.
> -              (with-directory-excursion "3rdparty"
> -                (for-each delete-file-recursively
> -                          '("googletest" "imgui" "pybind11" "qtimgui"
> -                            "sanitizers-cmake")))
> -
> -              ;; Remove references to unbundled code and link against external
> -              ;; libraries instead.
> -              (substitute* "CMakeLists.txt"
> -                (("^\\s+add_subdirectory\\(3rdparty/googletest.*") "")
> -                (("^(\\s+target_link_libraries.*)( gtest_main\\))"
> -                  _ prefix suffix)
> -                 (string-append prefix " gtest" suffix)))
> -              (substitute* "gui/CMakeLists.txt"
> -                (("^\\s+../3rdparty/(qt)?imgui.*") "")
> -                (("^(target_link_libraries.*)\\)" _ prefix)
> -                 (string-append prefix " imgui qt_imgui_widgets)")))))))
> -      (native-inputs
> -       (list googletest sanitizers-cmake))
> -      (inputs
> -       (list boost
> -             eigen
> -             icestorm
> -             imgui-1.86
> -             pybind11
> -             python
> -             qtbase-5
> -             qtwayland-5
> -             qtimgui
> -             yosys))
> -      (build-system qt-build-system)
> -      (arguments
> -       (list
> -        #:configure-flags
> -        #~(list "-DARCH=ice40"
> -                "-DBUILD_GUI=ON"
> -                "-DBUILD_TESTS=ON"
> -                (string-append "-DCURRENT_GIT_VERSION=" #$tag)
> -                (string-append "-DICESTORM_INSTALL_PREFIX="
> -                               #$(this-package-input "icestorm"))
> -                "-DUSE_IPO=OFF")
> -        #:phases
> -        #~(modify-phases %standard-phases
> -            (add-after 'unpack 'patch-source
> -              (lambda* (#:key inputs #:allow-other-keys)
> -                (substitute* "CMakeLists.txt"
> -                  ;; Use the system sanitizers-cmake module.
> -                  (("\\$\\{CMAKE_SOURCE_DIR\\}/3rdparty/sanitizers-cmake/cmake")
> -                   (string-append
> -                    #$(this-package-native-input "sanitizers-cmake")
> -                    "/share/sanitizers-cmake/cmake")))
> -                (substitute* "gui/CMakeLists.txt"
> -                  ;; Compile with system imgui and qtimgui headers.
> -                  (("^(target_include_directories.*)../3rdparty/imgui(.*)$"
> -                    _ prefix suffix)
> -                   (string-append prefix
> -                                  (search-input-directory inputs
> -                                                          "include/imgui")
> -                                  suffix))
> -                  (("^(target_include_directories.*)../3rdparty/qtimgui/(.*)$"
> -                    _ prefix suffix)
> -                   (string-append prefix
> -                                  (search-input-directory inputs
> -                                                          "include/qtimgui")
> -                                  suffix))))))))
> -      (synopsis "Place-and-Route tool for FPGAs")
> -      (description "Nextpnr aims to be a vendor neutral, timing driven, FOSS
> -FPGA place and route tool.")
> -      (home-page "https://github.com/YosysHQ/nextpnr")
> -      (license license:expat))))

Hm, this relocation in the same diff as the changes muddled the scope.
I thought it was a new entry above, so reviewed it as such.  Could you
please split the relocation as a prior commit with your actual changes
to review in a 2nd commit on top?  This should give a nicer view, as
Gabriel also noted.

-- 
Thanks,
Maxim