[bug#78233,1/2] gnu: Add nextpnr.

Message ID 29af3cc1065ab7b7c02dc36d68fa845db37e22d1.1746294265.git.csantosb@inventati.org
State New
Headers
Series Upgrade nextpnr. |

Commit Message

Cayetano Santos May 3, 2025, 6:20 p.m. UTC
* gnu/packages/fpga.scm (nextpnr): New variable.

Change-Id: Ic3476a6a4220ec20191897a6efb3d4aa347b51c2
---
 gnu/packages/fpga.scm | 78 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 78 insertions(+)
  

Comments

Maxim Cournoyer May 8, 2025, 12:43 p.m. UTC | #1
Hi!

Cayetano Santos <csantosb@inventati.org> writes:

> * gnu/packages/fpga.scm (nextpnr): New variable.
>
> Change-Id: Ic3476a6a4220ec20191897a6efb3d4aa347b51c2
> ---
>  gnu/packages/fpga.scm | 78 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 78 insertions(+)
>
> diff --git a/gnu/packages/fpga.scm b/gnu/packages/fpga.scm
> index 2298dde595..9dbd1a4564 100644
> --- a/gnu/packages/fpga.scm
> +++ b/gnu/packages/fpga.scm
> @@ -178,6 +178,84 @@ (define-public iverilog
>      ;; You have to accept both GPL2 and LGPL2.1+.
>      (license (list license:gpl2 license:lgpl2.1+))))
>  
> +(define nextpnr
> +  (package
> +    (name "nextpnr")
> +    (version "0.8")
> +    (source
> +     (origin
> +       (method git-fetch)
> +       (uri (git-reference
> +             (url "https://github.com/YosysHQ/nextpnr/")
> +             (commit (string-append "nextpnr-" version))
> +             ;; Required to get oourafft, json11, python-console and
> +             ;; QtPropertyBrowser, not packaged in Guix.
> +             (recursive? #t)))

I'd add a ';; TODO: Package the missing oourafft, json11, etc.'
dependencies.', so that people looking for things to do in the source
can find them.

> +       (file-name (git-file-name name version))
> +       (modules '((guix build utils)))
> +       (snippet
> +        ;; Remove bundled source code for which Guix has packages.
> +        '(with-directory-excursion "3rdparty"
> +           (for-each delete-file-recursively
> +                     '("googletest" "imgui" "pybind11" "qtimgui"
> +                       "sanitizers-cmake" "corrosion"))))

Great.  Perhaps not necessary here, but I've used defensive techniques
in the past to allow newer release to slip new dependencies unknown to
the packager, e.g. in retroarch-minimal:

--8<---------------cut here---------------start------------->8---
       (snippet
        #~(begin
            (use-modules (guix build utils)
                         (ice-9 ftw)
                         (srfi srfi-26))
            ;; XXX: 'delete-all-but' is copied from the turbovnc package.
            (define (delete-all-but directory . preserve)
              (define (directory? x)
                (and=> (stat x #f)
                       (compose (cut eq? 'directory <>) stat:type)))
              (with-directory-excursion directory
                (let* ((pred
                        (negate (cut member <> (append '("." "..") preserve))))
                       (items (scandir "." pred)))
                  (for-each (lambda (item)
                              (if (directory? item)
                                  (delete-file-recursively item)
                                  (delete-file item)))
                            items))))
            ;; Remove as much bundled sources as possible, shaving off about
            ;; 65 MiB.
            (delete-all-but "deps"
                            "feralgamemode" ;used in platform_unix.c
                            "mbedtls"       ;further refined below
                            "yxml")         ;used in rxml.c
            ;; This is an old root certificate used in net_socket_ssl_mbed.c,
            ;; not actually from mbedtls.
            (delete-all-but "deps/mbedtls" "cacert.h")))
--8<---------------cut here---------------end--------------->8---

> +       (sha256
> +        (base32 "0p53a2gl89hf3hfwdxs6pykxyrk82j4lqpwd1fqia2y0c9r2gjlm"))))
> +    (build-system qt-build-system)
> +    (arguments
> +     (list
> +      #:cmake cmake                     ;CMake 3.25 or higher is required

There's also cmake-next, which would be easier to grep, perhaps, when
the time comes to sed the repo after a core upgrade of cmake-minimal.

> +      #:configure-flags
> +      #~(list "-DBUILD_GUI=OFF"

Why do we not build the GUI?  We already have the Qt dependencies, it
seems.

> +              "-DUSE_OPENMP=yes"
> +              "-DBUILD_TESTS=ON"
> +              (string-append "-DCURRENT_GIT_VERSION=nextpnr-" #$version)
> +              "-DUSE_IPO=OFF")
> +      #:phases
> +      #~(modify-phases %standard-phases
> +          ;; Remove references to unbundled code and link against external
> +          ;; libraries instead.
> +          (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"))
> +                ;; Use the system googletest module
> +                (("^\\s+add_subdirectory\\(3rdparty/googletest.*")
> +                 "")
> +                ;; Use the system corrosion module
> +                (("^\\s+add_subdirectory\\(3rdparty/corrosion.*")
> +                 "")
> +                ;; replace gtest_main by gtest
> +                (("^(\\s+target_link_libraries.*)( gtest_main)" _ prefix suffix)
> +                 (string-append prefix " gtest")))
> +              ;; Use the system imgui module
> +              (substitute* "gui/CMakeLists.txt"
> +                (("\\$\\{CMAKE_SOURCE_DIR\\}(/3rdparty/imgui)")
> +                 (string-append #$(this-package-input "imgui")
> +                                "/include/imgui"))
> +                (("\\$\\{CMAKE_SOURCE_DIR\\}(/3rdparty/qtimgui)")
> +                 (string-append #$(this-package-input "qtimgui")
> +                                "/include/qtimgui"))
> +                (("^\\s+../3rdparty/(qt)?imgui.*")
> +                 "")))))))

Well done!  If the source changes often, a patch could be more
maintainable; ideally in way that can be forwarded upstream too, with
good chances of being merged.  That's less work for us in the long term.
  
Cayetano Santos May 9, 2025, 6:33 p.m. UTC | #2
>jeu. 08 mai 2025 at 21:43, Maxim Cournoyer <maxim.cournoyer@gmail.com> wrote:

> Why do we not build the GUI?  We already have the Qt dependencies, it
> seems.

Simply, I’m  unable to compile 0.8 with the gui flag on.

To me, the priority now is providing in Guix a place and route workflow
for all available nextpnr backends in cli mode, which is what most
engineers use. In a second time, we will investigate how to build the
gui and how activate other interesting options, but I’d rather prefer
this point not to be a big stopper to what really matters.

> Well done!  If the source changes often, a patch could be more
> maintainable; ideally in way that can be forwarded upstream too, with
> good chances of being merged.  That's less work for us in the long term.

Generally speaking, I see what you mean. In this very case, I don’t
understand how to replace current invocations to inputs
(this-package-input) with a patch; neither I see how upstream might
consider a patch to Guix only requirements, specially, as there is no
support to compilations not using the 3rd party modules they provide.

C.
  
Maxim Cournoyer May 11, 2025, 1:14 p.m. UTC | #3
Hi,

Cayetano Santos <csantosb@inventati.org> writes:

>>jeu. 08 mai 2025 at 21:43, Maxim Cournoyer <maxim.cournoyer@gmail.com> wrote:
>
>> Why do we not build the GUI?  We already have the Qt dependencies, it
>> seems.
>
> Simply, I’m  unable to compile 0.8 with the gui flag on.
>
> To me, the priority now is providing in Guix a place and route workflow
> for all available nextpnr backends in cli mode, which is what most
> engineers use. In a second time, we will investigate how to build the
> gui and how activate other interesting options, but I’d rather prefer
> this point not to be a big stopper to what really matters.

>> Well done!  If the source changes often, a patch could be more
>> maintainable; ideally in way that can be forwarded upstream too, with
>> good chances of being merged.  That's less work for us in the long term.

I think my comments were w.r.t. to patching the build system to use
system-provided libraries.  Patching commands to use the Guix-specific
file names should remain as they are as substitutions in phases.

> Generally speaking, I see what you mean. In this very case, I don’t
> understand how to replace current invocations to inputs
> (this-package-input) with a patch; neither I see how upstream might
> consider a patch to Guix only requirements, specially, as there is no
> support to compilations not using the 3rd party modules they provide.

That would mean contributing newly authored CMake glue code that would
make configuring using system-provided libraries possible.  It's more
work, but worth it in the long term.  I agree this doesn't need to block
the current submission.
  
Cayetano Santos May 11, 2025, 5:31 p.m. UTC | #4
>dim. 11 mai 2025 at 22:14, Maxim Cournoyer <maxim.cournoyer@gmail.com> wrote:

> Hi,
>
> Cayetano Santos <csantosb@inventati.org> writes:
>
>>>jeu. 08 mai 2025 at 21:43, Maxim Cournoyer <maxim.cournoyer@gmail.com> wrote:
>>
>>> Why do we not build the GUI?  We already have the Qt dependencies, it
>>> seems.
>>
>> Simply, I’m  unable to compile 0.8 with the gui flag on.
>>
>> To me, the priority now is providing in Guix a place and route workflow
>> for all available nextpnr backends in cli mode, which is what most
>> engineers use. In a second time, we will investigate how to build the
>> gui and how activate other interesting options, but I’d rather prefer
>> this point not to be a big stopper to what really matters.
>
>>> Well done!  If the source changes often, a patch could be more
>>> maintainable; ideally in way that can be forwarded upstream too, with
>>> good chances of being merged.  That's less work for us in the long term.
>
> I think my comments were w.r.t. to patching the build system to use
> system-provided libraries.  Patching commands to use the Guix-specific
> file names should remain as they are as substitutions in phases.
>
>> Generally speaking, I see what you mean. In this very case, I don’t
>> understand how to replace current invocations to inputs
>> (this-package-input) with a patch; neither I see how upstream might
>> consider a patch to Guix only requirements, specially, as there is no
>> support to compilations not using the 3rd party modules they provide.
>
> That would mean contributing newly authored CMake glue code that would
> make configuring using system-provided libraries possible.  It's more
> work, but worth it in the long term.  I agree this doesn't need to block
> the current submission.

Ok , I understand now. Upstream, they’re half way towards supporting
system libraries, instead of built-in. I’ll send them a PR to consider
the remaining libraries we need, so that next time we will simply this
package.

C.
  
Maxim Cournoyer May 12, 2025, 7 a.m. UTC | #5
Hi

Cayetano Santos <csantosb@inventati.org> writes:

>>jeu. 08 mai 2025 at 21:43, Maxim Cournoyer <maxim.cournoyer@gmail.com> wrote:
>
>> Why do we not build the GUI?  We already have the Qt dependencies, it
>> seems.
>
> Simply, I’m  unable to compile 0.8 with the gui flag on.
>
> To me, the priority now is providing in Guix a place and route workflow
> for all available nextpnr backends in cli mode, which is what most
> engineers use. In a second time, we will investigate how to build the
> gui and how activate other interesting options, but I’d rather prefer
> this point not to be a big stopper to what really matters.

Since the current variant of nextpnr-ice40 was building the GUI just
fine, and the error was just adjusting the headers location, I've gone
ahead and done that.  Otherwise it'd have been a regression or feature
removal, which we shouldn't do lightly with an update.

>> Well done!  If the source changes often, a patch could be more
>> maintainable; ideally in way that can be forwarded upstream too, with
>> good chances of being merged.  That's less work for us in the long term.
>
> Generally speaking, I see what you mean. In this very case, I don’t
> understand how to replace current invocations to inputs
> (this-package-input) with a patch; neither I see how upstream might
> consider a patch to Guix only requirements, specially, as there is no
> support to compilations not using the 3rd party modules they provide.

See commit 221899c2023, which introduces two patches [0, 1] to allow using
GoogleTest and ImGui/QtImGui from the system before falling back to the
bundled copies.

[0]  https://github.com/YosysHQ/nextpnr/pull/1478
[1]  https://github.com/YosysHQ/nextpnr/pull/1480
  

Patch

diff --git a/gnu/packages/fpga.scm b/gnu/packages/fpga.scm
index 2298dde595..9dbd1a4564 100644
--- a/gnu/packages/fpga.scm
+++ b/gnu/packages/fpga.scm
@@ -178,6 +178,84 @@  (define-public iverilog
     ;; You have to accept both GPL2 and LGPL2.1+.
     (license (list license:gpl2 license:lgpl2.1+))))
 
+(define nextpnr
+  (package
+    (name "nextpnr")
+    (version "0.8")
+    (source
+     (origin
+       (method git-fetch)
+       (uri (git-reference
+             (url "https://github.com/YosysHQ/nextpnr/")
+             (commit (string-append "nextpnr-" version))
+             ;; Required to get oourafft, json11, python-console and
+             ;; QtPropertyBrowser, not packaged in Guix.
+             (recursive? #t)))
+       (file-name (git-file-name name version))
+       (modules '((guix build utils)))
+       (snippet
+        ;; Remove bundled source code for which Guix has packages.
+        '(with-directory-excursion "3rdparty"
+           (for-each delete-file-recursively
+                     '("googletest" "imgui" "pybind11" "qtimgui"
+                       "sanitizers-cmake" "corrosion"))))
+       (sha256
+        (base32 "0p53a2gl89hf3hfwdxs6pykxyrk82j4lqpwd1fqia2y0c9r2gjlm"))))
+    (build-system qt-build-system)
+    (arguments
+     (list
+      #:cmake cmake                     ;CMake 3.25 or higher is required
+      #:configure-flags
+      #~(list "-DBUILD_GUI=OFF"
+              "-DUSE_OPENMP=yes"
+              "-DBUILD_TESTS=ON"
+              (string-append "-DCURRENT_GIT_VERSION=nextpnr-" #$version)
+              "-DUSE_IPO=OFF")
+      #:phases
+      #~(modify-phases %standard-phases
+          ;; Remove references to unbundled code and link against external
+          ;; libraries instead.
+          (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"))
+                ;; Use the system googletest module
+                (("^\\s+add_subdirectory\\(3rdparty/googletest.*")
+                 "")
+                ;; Use the system corrosion module
+                (("^\\s+add_subdirectory\\(3rdparty/corrosion.*")
+                 "")
+                ;; replace gtest_main by gtest
+                (("^(\\s+target_link_libraries.*)( gtest_main)" _ prefix suffix)
+                 (string-append prefix " gtest")))
+              ;; Use the system imgui module
+              (substitute* "gui/CMakeLists.txt"
+                (("\\$\\{CMAKE_SOURCE_DIR\\}(/3rdparty/imgui)")
+                 (string-append #$(this-package-input "imgui")
+                                "/include/imgui"))
+                (("\\$\\{CMAKE_SOURCE_DIR\\}(/3rdparty/qtimgui)")
+                 (string-append #$(this-package-input "qtimgui")
+                                "/include/qtimgui"))
+                (("^\\s+../3rdparty/(qt)?imgui.*")
+                 "")))))))
+    (native-inputs (list googletest sanitizers-cmake))
+    (inputs (list boost
+                  corrosion
+                  eigen
+                  imgui
+                  pybind11
+                  python
+                  qtbase-5
+                  qtimgui
+                  qtwayland-5))
+    (synopsis "Place-and-Route tool for FPGAs")
+    (description "Nextpnr is a portable FPGA place and route tool.")
+    (home-page "https://github.com/YosysHQ/nextpnr/")
+    (license license:asl2.0)))
+
 (define-public yosys
   (package
     (name "yosys")