diff mbox series

[bug#63839] gnu: riscv-pk: don't target riscv64-linux-gnu when current system is riscv64.

Message ID tencent_D0FFDA97CD8454B0ACE9BF6F9DB7F29E1207@qq.com
State New
Headers show
Series [bug#63839] gnu: riscv-pk: don't target riscv64-linux-gnu when current system is riscv64. | expand

Commit Message

Z572 June 2, 2023, 12:10 p.m. UTC 
* gnu/packages/virtualization.scm (riscv-pk): don't target riscv64-linux-gnu
when current system is riscv64.
---
 gnu/packages/virtualization.scm | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)


base-commit: 75bdd4b05253c0e6ca5399f60e424f0f00fdb673

Comments

Josselin Poiret June 6, 2023, 3:48 p.m. UTC | #1 
Hi,

Z572 via Guix-patches via <guix-patches@gnu.org> writes:


> +      ;; riscv-pk can only be built for riscv64.
> +      (if (string-prefix? "riscv64" (%current-system))
> +          '()
> +          (list
> +           #:target "riscv64-linux-gnu"))

I don't think that's a good idea, in general, consumers of the package
should be the ones specifying for which arch this package builds.  For
that purpose, there's the supported-systems field of the package record
type.  It's not good in general to hardcode targets in package
definitions.

Best,
Z572 June 11, 2023, 2:48 p.m. UTC | #2 
Josselin Poiret via Guix-patches via <guix-patches@gnu.org> writes:

> [[PGP Signed Part:Undecided]]
> Hi,
>
> Z572 via Guix-patches via <guix-patches@gnu.org> writes:
>
>
>> +      ;; riscv-pk can only be built for riscv64.
>> +      (if (string-prefix? "riscv64" (%current-system))
>> +          '()
>> +          (list
>> +           #:target "riscv64-linux-gnu"))
>
> I don't think that's a good idea, in general, consumers of the package
> should be the ones specifying for which arch this package builds.  For
> that purpose, there's the supported-systems field of the package record
> type.  It's not good in general to hardcode targets in package
> definitions.
look it's name.

because it doesn't make sense to target other targets, like `u-boot-sifive-unmatched' or `opensbi-generic'.

>
> Best,
Josselin Poiret Aug. 25, 2023, 1:11 p.m. UTC | #3 
Hi,

Any news on this patch?  I still stand by what I said: I believe it's
wrong for packages to force a target, this should be specified by the
caller instead, using `--target=`.  One can add the supported systems to
supported-systems to note that this package only works for some systems
though.

Best,
diff mbox series

Patch

diff --git a/gnu/packages/virtualization.scm b/gnu/packages/virtualization.scm
index c88fd31146..1257bb531d 100644
--- a/gnu/packages/virtualization.scm
+++ b/gnu/packages/virtualization.scm
@@ -28,6 +28,7 @@ 
 ;;; Copyright © 2022 Ekaitz Zarraga <ekaitz@elenq.tech>
 ;;; Copyright © 2022 Arun Isaac <arunisaac@systemreboot.net>
 ;;; Copyright © 2022 Zhu Zihao <all_but_last@163.com>
+;;; Copyright © 2023 Zheng Junjie <873216071@qq.com>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -2554,16 +2555,21 @@  (define-public riscv-pk
          "1cc0rz4q3a1zw8756b8yysw8lb5g4xbjajh5lvqbjix41hbdx6xz"))))
     (build-system gnu-build-system)
     (arguments
-     (list #:out-of-source? #t
-           ;; riscv-pk can only be built for riscv64.
-           #:target "riscv64-linux-gnu"
-           #:make-flags #~(list (string-append "INSTALLDIR=" #$output))
-           ;; Add flags to keep symbols fromhost and tohost. These symbols are
-           ;; required for the correct functioning of pk.
-           #:strip-flags #~(list "--strip-unneeded"
-                                 "--keep-symbol=fromhost"
-                                 "--keep-symbol=tohost"
-                                 "--enable-deterministic-archives")))
+     (append
+      ;; riscv-pk can only be built for riscv64.
+      (if (string-prefix? "riscv64" (%current-system))
+          '()
+          (list
+           #:target "riscv64-linux-gnu"))
+      (list #:out-of-source? #t
+
+            #:make-flags #~(list (string-append "INSTALLDIR=" #$output))
+            ;; Add flags to keep symbols fromhost and tohost. These symbols are
+            ;; required for the correct functioning of pk.
+            #:strip-flags #~(list "--strip-unneeded"
+                                  "--keep-symbol=fromhost"
+                                  "--keep-symbol=tohost"
+                                  "--enable-deterministic-archives"))))
     (home-page "https://github.com/riscv-software-src/riscv-pk")
     (synopsis "RISC-V Proxy Kernel")
     (description "The RISC-V Proxy Kernel, @command{pk}, is a lightweight