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

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

Change-Id: I8d1c3528679f38ec4593f90aec0f1c4321dc7e44
---
 gnu/packages/fpga.scm | 138 +++++++++++++-----------------------------
 1 file changed, 41 insertions(+), 97 deletions(-)
  

Hi,

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

n<#secure method=pgpmime mode=sign>

Hi Maxim,

Thanks for all your comments !  They’re (almost) included in v2 series.

>jeu. 08 mai 2025 at 22:07, Maxim Cournoyer <maxim.cournoyer@gmail.com> wrote:

> 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 <> '("." ".."))))))

I just learned about cut existence, thanks again; I understand its utility
by looking at examples all around the code. No doubt, this is the correct
alternative.

Unfortunately, I’m unable at this point to use it, I miss the necessary
skills: I fall in frequent "unbound variable <>" errors that I don’t
known yet how to handle, I suspect gexps add a extra layer of complexity
which becomes a barrier to me, sorry for that.

> 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.

Let’s forget the relocation by now, as it complicates things with no
real utility by itself. We will sort things out later on.

Just consider that, in reality, nextpnr replaces former nextpnr-ice40
(it is strongly based on it); new nextpnr-ice40 is a new package (even
if it carries the name of previous one).  This is not evident with two commits.

Two commits, without breaking things up, are necessarily somehow
confusing (and kind of useless, to that matter). To me, the only means
of achieving a nicer diff view is using a single commit, where one may
compare new nextpnr with old nextpnr-ice40, at the same time as it becomes
evident that the new nextpnr-ice40 is an addition.

V2 is in its way.

C.
  

Hi,

Cayetano Santos <csantosb@inventati.org> writes:

> n<#secure method=pgpmime mode=sign>
>
> Hi Maxim,
>
> Thanks for all your comments !  They’re (almost) included in v2 series.
>
>>jeu. 08 mai 2025 at 22:07, Maxim Cournoyer <maxim.cournoyer@gmail.com> wrote:
>
>> 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 <> '("." ".."))))))
>
> I just learned about cut existence, thanks again; I understand its utility
> by looking at examples all around the code. No doubt, this is the correct
> alternative.
>
> Unfortunately, I’m unable at this point to use it, I miss the necessary
> skills: I fall in frequent "unbound variable <>" errors that I don’t
> known yet how to handle, I suspect gexps add a extra layer of complexity
> which becomes a barrier to me, sorry for that.
>
>> 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.
>
> Let’s forget the relocation by now, as it complicates things with no
> real utility by itself. We will sort things out later on.

Change clarity/transparency and easing the review is worth the extra
effort, in my opinion.

> Just consider that, in reality, nextpnr replaces former nextpnr-ice40
> (it is strongly based on it); new nextpnr-ice40 is a new package (even
> if it carries the name of previous one).  This is not evident with two commits.
>
> Two commits, without breaking things up, are necessarily somehow
> confusing (and kind of useless, to that matter). To me, the only means
> of achieving a nicer diff view is using a single commit, where one may
> compare new nextpnr with old nextpnr-ice40, at the same time as it becomes
> evident that the new nextpnr-ice40 is an addition.
>
> V2 is in its way.

Thanks for V2, I've used it as the base of nextpnr-ice40 package, for
which I've also added patches, as mentioned previously.  By first
updating nextpnr-ice40 to v0.8 and related changes, and later renaming
it to nextpnr with few adjustments needed, the commit history is as
clear as it can be.  Please have a look/test to make sure everything
still works as intended.

I didn't keep the phase building the examples as the package already has
a test suite, and I didn't see the gain/effort ratio of doing so as
worth it.
  

>lun. 12 mai 2025 at 16:14, Maxim Cournoyer <maxim.cournoyer@gmail.com> wrote:

> Thanks for V2, I've used it as the base of nextpnr-ice40 package, for
> which I've also added patches, as mentioned previously.

Thanks a lot for this ! I’m sure that providing a whole pipeline for fpga
development (yosys+nextpnr+fpgaloader) in Guix is a rewarding effort.

> By first updating nextpnr-ice40 to v0.8 and related changes, and later
> renaming it to nextpnr with few adjustments needed, the commit history
> is as clear as it can be. Please have a look/test to make sure
> everything still works as intended.

Well, I’m afraid I was not clear enough about it, but the whole original idea
was to get a different package per device:

  - nextpnr (not public) :: generic package
  - nextpnr-ice40 (public, inherits from nextpnr) :: icestorm backend
  - nextpnr-ecp5 (public, inherits from nextpnr) :: prjtrellis backend
  - nextpnr-generic (public, inherits from nextpnr) :: no backend
  - nextpnr-himbaechel-gowin (public, inherits from nextpnr) ::
  - nextpnr-himbaechel-ng-ultra (public, inherits from nextpnr) ::
    prjbeyond-db backend
  - nextpnr-nexus :: ...
  - etc.

You can see the overall concept in my testing playground [0].

As it is now, we’d be using a single package for all devices, which is
possible, but complicates things as we would need a huge package
definition. In addition, this approach difficults maintenance, IMO,
whereas a different package per device/backend eases future upgrades
and fixes.

Let me know how do you prefer I proceed.

> I didn't keep the phase building the examples as the package already has
> a test suite, and I didn't see the gain/effort ratio of doing so as
> worth it.

Consider that nextpnr testing is different from nextpnr-ice40 testing,
which again differs from nextpnr-ecp5 testing, etc.

Thanks again,

Cayetano

[0] https://git.sr.ht/~csantosb/guix.channel-electronics/tree/main/item/electronics/packages/pnr.scm
  

Hi Cayetano,

Cayetano Santos <csantosb@inventati.org> writes:

>>lun. 12 mai 2025 at 16:14, Maxim Cournoyer <maxim.cournoyer@gmail.com> wrote:
>
>> Thanks for V2, I've used it as the base of nextpnr-ice40 package, for
>> which I've also added patches, as mentioned previously.
>
> Thanks a lot for this ! I’m sure that providing a whole pipeline for fpga
> development (yosys+nextpnr+fpgaloader) in Guix is a rewarding effort.
>
>> By first updating nextpnr-ice40 to v0.8 and related changes, and later
>> renaming it to nextpnr with few adjustments needed, the commit history
>> is as clear as it can be. Please have a look/test to make sure
>> everything still works as intended.
>
> Well, I’m afraid I was not clear enough about it, but the whole original idea
> was to get a different package per device:
>
>   - nextpnr (not public) :: generic package
>   - nextpnr-ice40 (public, inherits from nextpnr) :: icestorm backend
>   - nextpnr-ecp5 (public, inherits from nextpnr) :: prjtrellis backend
>   - nextpnr-generic (public, inherits from nextpnr) :: no backend
>   - nextpnr-himbaechel-gowin (public, inherits from nextpnr) ::
>   - nextpnr-himbaechel-ng-ultra (public, inherits from nextpnr) ::
>     prjbeyond-db backend
>   - nextpnr-nexus :: ...
>   - etc.
>
> You can see the overall concept in my testing playground [0].

I see.  That's neat, but I think I'd rather try a single 'nextpnr'
package that does it all first, and complicate later if we find it
becomes unpractical.  The benefits:

1. all the executables share the same library, so space and compute
wise, it makes sense they are packaged together.

2. There doesn't seem to be many different inputs to be added to build
all the backends, so the maintenance cost should similar.  Inheritance
also introduce some complexity, by having to keep everything in sync
while the Guix tooling doesn't make the relationship easy to discover
(via 'guix refresh --list-transitive say').

3. It's done elsewhere, e.g. in nixpkgs, so we can compare in case of
problems.

What do you think?
  

>lun. 12 mai 2025 at 20:54, Maxim Cournoyer <maxim.cournoyer@gmail.com> wrote:

> Hi Cayetano,
>
> Cayetano Santos <csantosb@inventati.org> writes:
>
>>>lun. 12 mai 2025 at 16:14, Maxim Cournoyer <maxim.cournoyer@gmail.com> wrote:
>>
>>> Thanks for V2, I've used it as the base of nextpnr-ice40 package, for
>>> which I've also added patches, as mentioned previously.
>>
>> Thanks a lot for this ! I’m sure that providing a whole pipeline for fpga
>> development (yosys+nextpnr+fpgaloader) in Guix is a rewarding effort.
>>
>>> By first updating nextpnr-ice40 to v0.8 and related changes, and later
>>> renaming it to nextpnr with few adjustments needed, the commit history
>>> is as clear as it can be. Please have a look/test to make sure
>>> everything still works as intended.
>>
>> Well, I’m afraid I was not clear enough about it, but the whole original idea
>> was to get a different package per device:
>>
>>   - nextpnr (not public) :: generic package
>>   - nextpnr-ice40 (public, inherits from nextpnr) :: icestorm backend
>>   - nextpnr-ecp5 (public, inherits from nextpnr) :: prjtrellis backend
>>   - nextpnr-generic (public, inherits from nextpnr) :: no backend
>>   - nextpnr-himbaechel-gowin (public, inherits from nextpnr) ::
>>   - nextpnr-himbaechel-ng-ultra (public, inherits from nextpnr) ::
>>     prjbeyond-db backend
>>   - nextpnr-nexus :: ...
>>   - etc.
>>
>> You can see the overall concept in my testing playground [0].
>
> I see.  That's neat, but I think I'd rather try a single 'nextpnr'
> package that does it all first, and complicate later if we find it
> becomes unpractical.  The benefits:
>
> 1. all the executables share the same library, so space and compute
> wise, it makes sense they are packaged together.
>
> 2. There doesn't seem to be many different inputs to be added to build
> all the backends, so the maintenance cost should similar.  Inheritance
> also introduce some complexity, by having to keep everything in sync
> while the Guix tooling doesn't make the relationship easy to discover
> (via 'guix refresh --list-transitive say').
>
> 3. It's done elsewhere, e.g. in nixpkgs, so we can compare in case of
> problems.
>
> What do you think?

Fine with me; just keep in mind that we are not testing the binaries
(nextpnr-himbaechel, etc.).

This is it: #78390. It builds and performs as expected in all 4
architectures.

C.