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>
Bug is archived. No further changes may be made.
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
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.