Package: guix-patches;
Reported by: Cayetano Santos <csantosb <at> inventati.org>
Date: Sat, 3 May 2025 17:52:02 UTC
Severity: normal
Tags: patch
Done: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
To reply to this bug, email your comments to 78233 AT debbugs.gnu.org.
There is no need to reopen the bug first.
Toggle the display of automated, internal messages from the tracker.
View this report as an mbox folder, status mbox, maintainer mbox
csantosb <at> inventati.org, ekaitz <at> elenq.tech, gabriel <at> erlikon.ch, maxim.cournoyer <at> gmail.com, guix-patches <at> gnu.org
:bug#78233
; Package guix-patches
.
(Sat, 03 May 2025 17:52:02 GMT) Full text and rfc822 format available.Cayetano Santos <csantosb <at> inventati.org>
:csantosb <at> inventati.org, ekaitz <at> elenq.tech, gabriel <at> erlikon.ch, maxim.cournoyer <at> gmail.com, guix-patches <at> gnu.org
.
(Sat, 03 May 2025 17:52:02 GMT) Full text and rfc822 format available.Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):
From: Cayetano Santos <csantosb <at> inventati.org> To: guix-patches <at> gnu.org Cc: Cayetano Santos <csantosb <at> inventati.org> Subject: [PATCH 0/2 electronics-team] Upgrade nextpnr. Date: Sat, 3 May 2025 19:46:49 +0200
Hi, This patch series adds a new nextpnr package (strongly based on former nextpnt-ice40), common to all derived nextpnr-xxx, which will inherit from it. Other device packages will follow (nextpnr-ecp5, which uses prjtrellis as backend, for example). See [0] First commit - updates to 0.8 - updates substitutions in Makefiles Second commit modifies the device specific nextpnr-ice40, which now inherits from nextpnr, using icestorm as a backend. With respect to nextpnr: - adds icestorm as propagated input - adds yosys as native input, for tests - addapts config flags to ice40 architecture - uses tests from icestorm/examples [0] https://github.com/YosysHQ/nextpnr/blob/master/README.md Cayetano Santos (2): gnu: Add nextpnr. gnu: nextpnr-ice40: Update to 0.8. gnu/packages/fpga.scm | 216 +++++++++++++++++++++++------------------- 1 file changed, 119 insertions(+), 97 deletions(-) base-commit: fa1149d3fd8d2ce94968dd05d5dc08561cb283ed -- 2.49.0
csantosb <at> inventati.org, ekaitz <at> elenq.tech, gabriel <at> erlikon.ch, maxim.cournoyer <at> gmail.com, guix-patches <at> gnu.org
:bug#78233
; Package guix-patches
.
(Sat, 03 May 2025 18:20:01 GMT) Full text and rfc822 format available.Message #8 received at 78233 <at> debbugs.gnu.org (full text, mbox):
From: Cayetano Santos <csantosb <at> inventati.org> To: 78233 <at> debbugs.gnu.org Cc: Cayetano Santos <csantosb <at> inventati.org> Subject: [PATCH 1/2] gnu: Add nextpnr. Date: Sat, 3 May 2025 20:20:05 +0200
* 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))) + (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") -- 2.49.0
csantosb <at> inventati.org, ekaitz <at> elenq.tech, gabriel <at> erlikon.ch, maxim.cournoyer <at> gmail.com, guix-patches <at> gnu.org
:bug#78233
; Package guix-patches
.
(Sat, 03 May 2025 18:20:02 GMT) Full text and rfc822 format available.Message #11 received at 78233 <at> debbugs.gnu.org (full text, mbox):
From: Cayetano Santos <csantosb <at> inventati.org> To: 78233 <at> debbugs.gnu.org Cc: Cayetano Santos <csantosb <at> inventati.org> Subject: [PATCH 2/2] gnu: nextpnr-ice40: Update to 0.8. Date: Sat, 3 May 2025 20:20:06 +0200
* 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(-) diff --git a/gnu/packages/fpga.scm b/gnu/packages/fpga.scm index 9dbd1a4564..e5c3313487 100644 --- a/gnu/packages/fpga.scm +++ b/gnu/packages/fpga.scm @@ -256,6 +256,47 @@ (define nextpnr (home-page "https://github.com/YosysHQ/nextpnr/") (license license:asl2.0))) +(define-public nextpnr-ice40 + (package + (inherit nextpnr) + (name "nextpnr-ice40") + (arguments + (substitute-keyword-arguments (package-arguments nextpnr) + ;; tests + ((#:phases phases #~%standard-phases) + #~(modify-phases #$phases + ;; get icestorm/examples + (add-after 'compress-documentation 'get-icestorm + (lambda* (#:key inputs #:allow-other-keys) + (copy-recursively + #$(origin (inherit (package-source icestorm))) + "icestorm"))) + ;; run all icestorm/examples as tests + (add-after 'get-icestorm 'tests-icestorm-examples + (lambda* _ + (let ((dir (opendir "icestorm/examples"))) + (do ((entry (readdir dir) + (readdir dir))) + ((eof-object? entry)) + (when (not (member entry '("." ".."))) + (setenv "PATH" + (string-append (string-append #$output "/bin") + ":" + (getenv "PATH"))) + (invoke "make" "-C" + (string-append "icestorm/examples/" entry)))) + (closedir dir)))))) + ((#: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))) + ;; tests + (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)))) - (define-public gtkwave (package (name "gtkwave") -- 2.49.0
guix-patches <at> gnu.org
:bug#78233
; Package guix-patches
.
(Wed, 07 May 2025 13:30:03 GMT) Full text and rfc822 format available.Message #14 received at 78233 <at> debbugs.gnu.org (full text, mbox):
From: Gabriel Wicki <gabriel <at> erlikon.ch> To: Cayetano Santos <csantosb <at> inventati.org> Cc: 78233 <at> debbugs.gnu.org, Maxim Cournoyer <maxim.cournoyer <at> gmail.com>, Ekaitz Zarraga <ekaitz <at> elenq.tech> Subject: Re: [bug#78233] [PATCH 0/2 electronics-team] Upgrade nextpnr. Date: Wed, 7 May 2025 15:29:38 +0200
Hi! Thanks for the patch! It LGTM and I appreciate the initiative. Not sure if the diff looked somewhat more clear if the two commits were merged into one and the new definitions came to lay where the nextpnr-ice40 is currently.
guix-patches <at> gnu.org
:bug#78233
; Package guix-patches
.
(Thu, 08 May 2025 12:44:02 GMT) Full text and rfc822 format available.Message #17 received at 78233 <at> debbugs.gnu.org (full text, mbox):
From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> To: Cayetano Santos <csantosb <at> inventati.org> Cc: 78233 <at> debbugs.gnu.org, Gabriel Wicki <gabriel <at> erlikon.ch>, Ekaitz Zarraga <ekaitz <at> elenq.tech> Subject: Re: [bug#78233] [PATCH 1/2] gnu: Add nextpnr. Date: Thu, 08 May 2025 21:43:13 +0900
Hi! Cayetano Santos <csantosb <at> 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. -- Thanks, Maxim
guix-patches <at> gnu.org
:bug#78233
; Package guix-patches
.
(Thu, 08 May 2025 13:08:02 GMT) Full text and rfc822 format available.Message #20 received at 78233 <at> debbugs.gnu.org (full text, mbox):
From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> To: Cayetano Santos <csantosb <at> inventati.org> Cc: 78233 <at> debbugs.gnu.org, Gabriel Wicki <gabriel <at> erlikon.ch>, Ekaitz Zarraga <ekaitz <at> elenq.tech> Subject: Re: [bug#78233] [PATCH 2/2] gnu: nextpnr-ice40: Update to 0.8. Date: Thu, 08 May 2025 22:07:12 +0900
Hi, Cayetano Santos <csantosb <at> 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. -- Thanks, Maxim
guix-patches <at> gnu.org
:bug#78233
; Package guix-patches
.
(Fri, 09 May 2025 18:34:02 GMT) Full text and rfc822 format available.Message #23 received at 78233 <at> debbugs.gnu.org (full text, mbox):
From: Cayetano Santos <csantosb <at> inventati.org> To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> Cc: 78233 <at> debbugs.gnu.org, Gabriel Wicki <gabriel <at> erlikon.ch>, Ekaitz Zarraga <ekaitz <at> elenq.tech> Subject: Re: [bug#78233] [PATCH 1/2] gnu: Add nextpnr. Date: Fri, 09 May 2025 20:33:21 +0200
[Message part 1 (text/plain, inline)]
>jeu. 08 mai 2025 at 21:43, Maxim Cournoyer <maxim.cournoyer <at> 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.
[signature.asc (application/pgp-signature, inline)]
guix-patches <at> gnu.org
:bug#78233
; Package guix-patches
.
(Fri, 09 May 2025 18:52:02 GMT) Full text and rfc822 format available.Message #26 received at 78233 <at> debbugs.gnu.org (full text, mbox):
From: Cayetano Santos <csantosb <at> inventati.org> To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> Cc: 78233 <at> debbugs.gnu.org, Gabriel Wicki <gabriel <at> erlikon.ch>, Ekaitz Zarraga <ekaitz <at> elenq.tech> Subject: Re: [bug#78233] [PATCH 2/2] gnu: nextpnr-ice40: Update to 0.8. Date: Fri, 09 May 2025 20:51:36 +0200
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 <at> 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.
csantosb <at> inventati.org, ekaitz <at> elenq.tech, gabriel <at> erlikon.ch, maxim.cournoyer <at> gmail.com, guix-patches <at> gnu.org
:bug#78233
; Package guix-patches
.
(Sun, 11 May 2025 09:21:01 GMT) Full text and rfc822 format available.Message #29 received at 78233 <at> debbugs.gnu.org (full text, mbox):
From: Cayetano Santos <csantosb <at> inventati.org> To: 78233 <at> debbugs.gnu.org Cc: Cayetano Santos <csantosb <at> inventati.org> Subject: [PATCH v2 1/2] gnu: Add nextpnr. Date: Sun, 11 May 2025 11:18:38 +0200
* gnu/packages/fpga.scm (nextpnt): New variable. Change-Id: Ic3476a6a4220ec20191897a6efb3d4aa347b51c2 --- gnu/packages/fpga.scm | 79 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 79 insertions(+) diff --git a/gnu/packages/fpga.scm b/gnu/packages/fpga.scm index 2298dde595..083faf7d7a 100644 --- a/gnu/packages/fpga.scm +++ b/gnu/packages/fpga.scm @@ -356,6 +356,85 @@ (define-public icestorm files.") (license license:isc)))) +(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)) + ;; TODO: Package next packages, not yet in Guix. + ;; Required to get missing oourafft, json11, python-console and + ;; QtPropertyBrowser. + (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-next ;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 nextpnr-ice40 (let* ((version "0.7") (tag (string-append "nextpnr-" version))) base-commit: 4fe484ee7e9d598a9d0a249c375b75a14b95d1b4 -- 2.49.0
csantosb <at> inventati.org, ekaitz <at> elenq.tech, gabriel <at> erlikon.ch, maxim.cournoyer <at> gmail.com, guix-patches <at> gnu.org
:bug#78233
; Package guix-patches
.
(Sun, 11 May 2025 09:21:02 GMT) Full text and rfc822 format available.Message #32 received at 78233 <at> debbugs.gnu.org (full text, mbox):
From: Cayetano Santos <csantosb <at> inventati.org> To: 78233 <at> debbugs.gnu.org Cc: Cayetano Santos <csantosb <at> inventati.org> Subject: [PATCH v2 2/2] gnu: nextpnr-ice40: Update to 0.8. Date: Sun, 11 May 2025 11:18:39 +0200
* gnu/packages/fpga.scm (nextpnr-ice40): Update to 0.8. Change-Id: I8d1c3528679f38ec4593f90aec0f1c4321dc7e44 --- gnu/packages/fpga.scm | 128 +++++++++++------------------------------- 1 file changed, 33 insertions(+), 95 deletions(-) diff --git a/gnu/packages/fpga.scm b/gnu/packages/fpga.scm index 083faf7d7a..77f39559cf 100644 --- a/gnu/packages/fpga.scm +++ b/gnu/packages/fpga.scm @@ -436,102 +436,39 @@ (define nextpnr (license license:asl2.0))) (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)))) + (package + (inherit nextpnr) + (name "nextpnr-ice40") + (arguments + (substitute-keyword-arguments (package-arguments nextpnr) + ((#:phases phases #~%standard-phases) + #~(modify-phases #$phases + (add-after 'unpack 'get-icestorm + (lambda _ + (copy-recursively #$(package-source icestorm) "icestorm"))) + ;; Run examples included in icestorm backend. + (add-after 'compress-documentation 'run-icestorm-examples + (lambda _ + (let* ((examples "../source/icestorm/examples/") + (dir (opendir examples)) + (path-env (getenv "PATH"))) + (setenv "PATH" (string-append #$output "/bin" ":" path-env)) + (do ((entry (readdir dir) (readdir dir))) + ((eof-object? entry)) + (unless (member entry '("." "..")) + (invoke "make" "-C" (string-append examples entry)))) + (closedir dir)))))) + ((#: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))) + (native-inputs + (modify-inputs (package-native-inputs nextpnr) + (prepend yosys))))) (define-public gtkwave (package -- 2.49.0
guix-patches <at> gnu.org
:bug#78233
; Package guix-patches
.
(Sun, 11 May 2025 13:15:02 GMT) Full text and rfc822 format available.Message #35 received at 78233 <at> debbugs.gnu.org (full text, mbox):
From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> To: Cayetano Santos <csantosb <at> inventati.org> Cc: 78233 <at> debbugs.gnu.org, Gabriel Wicki <gabriel <at> erlikon.ch>, Ekaitz Zarraga <ekaitz <at> elenq.tech> Subject: Re: [bug#78233] [PATCH 1/2] gnu: Add nextpnr. Date: Sun, 11 May 2025 22:14:43 +0900
Hi, Cayetano Santos <csantosb <at> inventati.org> writes: >>jeu. 08 mai 2025 at 21:43, Maxim Cournoyer <maxim.cournoyer <at> 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. -- Thanks, Maxim
guix-patches <at> gnu.org
:bug#78233
; Package guix-patches
.
(Sun, 11 May 2025 17:32:02 GMT) Full text and rfc822 format available.Message #38 received at 78233 <at> debbugs.gnu.org (full text, mbox):
From: Cayetano Santos <csantosb <at> inventati.org> To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> Cc: 78233 <at> debbugs.gnu.org, Gabriel Wicki <gabriel <at> erlikon.ch>, Ekaitz Zarraga <ekaitz <at> elenq.tech> Subject: Re: [bug#78233] [PATCH 1/2] gnu: Add nextpnr. Date: Sun, 11 May 2025 19:31:42 +0200
[Message part 1 (text/plain, inline)]
>dim. 11 mai 2025 at 22:14, Maxim Cournoyer <maxim.cournoyer <at> gmail.com> wrote: > Hi, > > Cayetano Santos <csantosb <at> inventati.org> writes: > >>>jeu. 08 mai 2025 at 21:43, Maxim Cournoyer <maxim.cournoyer <at> 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.
[signature.asc (application/pgp-signature, inline)]
csantosb <at> inventati.org, ekaitz <at> elenq.tech, gabriel <at> erlikon.ch, maxim.cournoyer <at> gmail.com, guix-patches <at> gnu.org
:bug#78233
; Package guix-patches
.
(Sun, 11 May 2025 18:22:02 GMT) Full text and rfc822 format available.Message #41 received at 78233 <at> debbugs.gnu.org (full text, mbox):
From: Cayetano Santos <csantosb <at> inventati.org> To: 78233 <at> debbugs.gnu.org Cc: Cayetano Santos <csantosb <at> inventati.org> Subject: [PATCH v3 1/2] gnu: Add nextpnr. Date: Sun, 11 May 2025 20:20:05 +0200
* gnu/packages/fpga.scm (nextpnr): New variable. Change-Id: Ic3476a6a4220ec20191897a6efb3d4aa347b51c2 --- gnu/packages/fpga.scm | 90 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 90 insertions(+) diff --git a/gnu/packages/fpga.scm b/gnu/packages/fpga.scm index 2298dde595..519807e847 100644 --- a/gnu/packages/fpga.scm +++ b/gnu/packages/fpga.scm @@ -356,6 +356,96 @@ (define-public icestorm files.") (license license:isc)))) +(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)) + ;; TODO: Package next packages, not yet in Guix. + ;; Required to get missing oourafft, json11, python-console and + ;; QtPropertyBrowser. + (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-next ;CMake 3.25 or higher is required. + #:configure-flags + #~(list "-DBUILD_GUI=OFF" + "-DUSE_OPENMP=ON" + "-DBUILD_TESTS=ON" + (string-append "-DCURRENT_GIT_VERSION=nextpnr-" #$version) + "-DUSE_IPO=OFF") + #:phases + #~(modify-phases %standard-phases + ;; We need sources to build the gui. + (add-after 'unpack 'get-imgui-src + (lambda _ + (copy-recursively #$(package-source imgui-1.86) "imgui") + (copy-recursively #$(package-source qtimgui) "qtimgui"))) + ;; Remove references to unbundled code and link against external + ;; libraries instead. + (add-after 'get-imgui-src '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.*") + "") + (("\\$\\{CMAKE_SOURCE_DIR\\}/3rdparty/googletest/googletest/include") + (string-append #$(this-package-native-input "googletest") + "/include")) + ;; 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"))) + (substitute* "gui/CMakeLists.txt" + ;; Use the system imgui module. + (("\\$\\{CMAKE_SOURCE_DIR\\}(/3rdparty/imgui)") + (string-append #$(this-package-input "imgui") + "/include/imgui")) + (("^\\s+../3rdparty/imgui") + "../imgui") + ;; Use the system qtimgui module. + (("\\$\\{CMAKE_SOURCE_DIR\\}(/3rdparty/qtimgui)") + (string-append #$(this-package-input "qtimgui") + "/include/qtimgui")) + (("^\\s+../3rdparty/qtimgui") + "../qtimgui/src"))))))) + (native-inputs (list googletest sanitizers-cmake)) + (inputs (list boost + corrosion + eigen + imgui-1.86 ;qtimgui depends on 1.86. + 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 nextpnr-ice40 (let* ((version "0.7") (tag (string-append "nextpnr-" version))) base-commit: 300d29e69988caf23684f08effa51f621c7ec083 -- 2.49.0
csantosb <at> inventati.org, ekaitz <at> elenq.tech, gabriel <at> erlikon.ch, maxim.cournoyer <at> gmail.com, guix-patches <at> gnu.org
:bug#78233
; Package guix-patches
.
(Sun, 11 May 2025 18:22:02 GMT) Full text and rfc822 format available.Message #44 received at 78233 <at> debbugs.gnu.org (full text, mbox):
From: Cayetano Santos <csantosb <at> inventati.org> To: 78233 <at> debbugs.gnu.org Cc: Cayetano Santos <csantosb <at> inventati.org> Subject: [PATCH v3 2/2] gnu: nextpnr-ice40: Update to 0.8. Date: Sun, 11 May 2025 20:20:06 +0200
* gnu/packages/fpga.scm (nextpnr-ice40): Update to 0.8. Change-Id: I8d1c3528679f38ec4593f90aec0f1c4321dc7e44 --- gnu/packages/fpga.scm | 128 +++++++++++------------------------------- 1 file changed, 33 insertions(+), 95 deletions(-) diff --git a/gnu/packages/fpga.scm b/gnu/packages/fpga.scm index 519807e847..4d05420d66 100644 --- a/gnu/packages/fpga.scm +++ b/gnu/packages/fpga.scm @@ -447,102 +447,39 @@ (define nextpnr (license license:asl2.0))) (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)))) + (package + (inherit nextpnr) + (name "nextpnr-ice40") + (arguments + (substitute-keyword-arguments (package-arguments nextpnr) + ((#:phases phases #~%standard-phases) + #~(modify-phases #$phases + (add-after 'unpack 'get-icestorm + (lambda _ + (copy-recursively #$(package-source icestorm) "icestorm"))) + ;; Run examples included in icestorm backend. + (add-after 'compress-documentation 'run-icestorm-examples + (lambda _ + (let* ((examples "../source/icestorm/examples/") + (dir (opendir examples)) + (path-env (getenv "PATH"))) + (setenv "PATH" (string-append #$output "/bin" ":" path-env)) + (do ((entry (readdir dir) (readdir dir))) + ((eof-object? entry)) + (unless (member entry '("." "..")) + (invoke "make" "-C" (string-append examples entry)))) + (closedir dir)))))) + ((#: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))) + (native-inputs + (modify-inputs (package-native-inputs nextpnr) + (prepend yosys))))) (define-public gtkwave (package -- 2.49.0
guix-patches <at> gnu.org
:bug#78233
; Package guix-patches
.
(Mon, 12 May 2025 07:02:02 GMT) Full text and rfc822 format available.Message #47 received at 78233 <at> debbugs.gnu.org (full text, mbox):
From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> To: Cayetano Santos <csantosb <at> inventati.org> Cc: 78233 <at> debbugs.gnu.org, Gabriel Wicki <gabriel <at> erlikon.ch>, Ekaitz Zarraga <ekaitz <at> elenq.tech> Subject: Re: [bug#78233] [PATCH 1/2] gnu: Add nextpnr. Date: Mon, 12 May 2025 16:00:55 +0900
Hi Cayetano Santos <csantosb <at> inventati.org> writes: >>jeu. 08 mai 2025 at 21:43, Maxim Cournoyer <maxim.cournoyer <at> 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 -- Thanks, Maxim
Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
:Cayetano Santos <csantosb <at> inventati.org>
:Message #52 received at 78233-done <at> debbugs.gnu.org (full text, mbox):
From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> To: Cayetano Santos <csantosb <at> inventati.org> Cc: 78233-done <at> debbugs.gnu.org, Gabriel Wicki <gabriel <at> erlikon.ch>, Ekaitz Zarraga <ekaitz <at> elenq.tech> Subject: Re: [bug#78233] [PATCH 2/2] gnu: nextpnr-ice40: Update to 0.8. Date: Mon, 12 May 2025 16:14:12 +0900
Hi, Cayetano Santos <csantosb <at> 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 <at> 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. -- Thanks, Maxim
guix-patches <at> gnu.org
:bug#78233
; Package guix-patches
.
(Mon, 12 May 2025 08:26:01 GMT) Full text and rfc822 format available.Message #55 received at 78233-done <at> debbugs.gnu.org (full text, mbox):
From: Cayetano Santos <csantosb <at> inventati.org> To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> Cc: 78233-done <at> debbugs.gnu.org, Gabriel Wicki <gabriel <at> erlikon.ch>, Ekaitz Zarraga <ekaitz <at> elenq.tech> Subject: Re: [bug#78233] [PATCH 2/2] gnu: nextpnr-ice40: Update to 0.8. Date: Mon, 12 May 2025 10:25:43 +0200
[Message part 1 (text/plain, inline)]
>lun. 12 mai 2025 at 16:14, Maxim Cournoyer <maxim.cournoyer <at> 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
[signature.asc (application/pgp-signature, inline)]
guix-patches <at> gnu.org
:bug#78233
; Package guix-patches
.
(Mon, 12 May 2025 11:55:01 GMT) Full text and rfc822 format available.Message #58 received at 78233-done <at> debbugs.gnu.org (full text, mbox):
From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> To: Cayetano Santos <csantosb <at> inventati.org> Cc: 78233-done <at> debbugs.gnu.org, Gabriel Wicki <gabriel <at> erlikon.ch>, Ekaitz Zarraga <ekaitz <at> elenq.tech> Subject: Re: [bug#78233] [PATCH 2/2] gnu: nextpnr-ice40: Update to 0.8. Date: Mon, 12 May 2025 20:54:11 +0900
Hi Cayetano, Cayetano Santos <csantosb <at> inventati.org> writes: >>lun. 12 mai 2025 at 16:14, Maxim Cournoyer <maxim.cournoyer <at> 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? -- Thanks, Maxim
guix-patches <at> gnu.org
:bug#78233
; Package guix-patches
.
(Mon, 12 May 2025 12:25:02 GMT) Full text and rfc822 format available.Message #61 received at 78233-done <at> debbugs.gnu.org (full text, mbox):
From: Cayetano Santos <csantosb <at> inventati.org> To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> Cc: 78233-done <at> debbugs.gnu.org, Gabriel Wicki <gabriel <at> erlikon.ch>, Ekaitz Zarraga <ekaitz <at> elenq.tech> Subject: Re: [bug#78233] [PATCH 2/2] gnu: nextpnr-ice40: Update to 0.8. Date: Mon, 12 May 2025 14:24:05 +0200
[Message part 1 (text/plain, inline)]
>lun. 12 mai 2025 at 20:54, Maxim Cournoyer <maxim.cournoyer <at> gmail.com> wrote: > Hi Cayetano, > > Cayetano Santos <csantosb <at> inventati.org> writes: > >>>lun. 12 mai 2025 at 16:14, Maxim Cournoyer <maxim.cournoyer <at> 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.
[signature.asc (application/pgp-signature, inline)]
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.