[bug#70985] [PATCH 0/4] Use specific errors for unsupported targets

[bug#70985] [PATCH 0/4] Use specific errors for unsupported targets

[Christopher Baines]

[-- Attachment #1: Type: text/plain, Size: 1106 bytes --]

I think when computing derivations any errors except from a small list
should be treated as bugs.

This has become more of an issue with the avr, or1k-elf and
xtensa-ath9k-elf targets since these targets both seem to not support a
large number of packages, and lead to generic errors or crashes.

I'm seeing this because logs relating to errors computing package
derivations are now taking up most of the data service logs around
processing revisions.


Christopher Baines (4):
  guix: packages: Add new &package-unsupported-target-error.
  gnu: tls: Raise conditions from target->openssl-target.
  gnu: cross-libc*: Raise conditions rather than returning #f.
  guix: build-system: meson: Don't error on unsupported targets.

 gnu/packages/cross-base.scm |  12 +++-
 gnu/packages/tls.scm        |  17 ++++--
 guix/build-system/meson.scm | 115 +++++++++++++++++++-----------------
 guix/packages.scm           |   7 +++
 guix/ui.scm                 |   7 +++
 5 files changed, 98 insertions(+), 60 deletions(-)


base-commit: 5a624adfd7b14c3717237d137bd0766c77f0f570
-- 
2.41.0

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 987 bytes --]

[bug#70985] [PATCH 1/4] guix: packages: Add new &package-unsupported-target-error.

[Christopher Baines]

Some packages don't support cross building to specific targets, so add a error
type to signal this.

* guix/packages.scm (&package-unsupported-target-error): New condition type.
[package-unsupported-target-error? package-unsupported-target-error-target):
New procedures.
* guix/ui.scm (call-with-error-handling): Handle this new condition type.

Change-Id: Ib47813399e04b20d616a95f545b6aabe25736e92
---
 guix/packages.scm | 7 +++++++
 guix/ui.scm       | 7 +++++++
 2 files changed, 14 insertions(+)

diff --git a/guix/packages.scm b/guix/packages.scm
index abe89cdb07..7fbb8d126b 100644
--- a/guix/packages.scm
+++ b/guix/packages.scm
@@ -173,6 +173,9 @@ (define-module (guix packages)
             package-error-invalid-input
             &package-cross-build-system-error
             package-cross-build-system-error?
+            &package-unsupported-target-error
+            package-unsupported-target-error?
+            package-unsupported-target-error-target
 
             package->bag
             bag->derivation
@@ -850,6 +853,10 @@ (define-condition-type &package-cyclic-dependency-error &package-error
 (define-condition-type &package-cross-build-system-error &package-error
   package-cross-build-system-error?)
 
+(define-condition-type &package-unsupported-target-error &package-error
+  package-unsupported-target-error?
+  (target package-unsupported-target-error-target))
+
 (define* (package-full-name package #:optional (delimiter "@"))
   "Return the full name of PACKAGE--i.e., `NAME@VERSION'.  By specifying
 DELIMITER (a string), you can customize what will appear between the name and
diff --git a/guix/ui.scm b/guix/ui.scm
index d82fa533cc..0bb1b3b3ba 100644
--- a/guix/ui.scm
+++ b/guix/ui.scm
@@ -756,6 +756,13 @@ (define (call-with-error-handling thunk)
                        (location->string loc)
                        (package-full-name package)
                        (build-system-name system))))
+             ((package-unsupported-target-error? c)
+              (let* ((package (package-error-package c))
+                     (loc     (package-location package)))
+                (leave (G_ "~a: ~a: does not support target `~a'~%")
+                       (location->string loc)
+                       (package-full-name package)
+                       (package-unsupported-target-error-target c))))
              ((gexp-input-error? c)
               (let ((input (gexp-error-invalid-input c)))
                 (leave (G_ "~s: invalid G-expression input~%")

base-commit: 5a624adfd7b14c3717237d137bd0766c77f0f570
-- 
2.41.0

[bug#70985] [PATCH 2/4] gnu: tls: Raise conditions from target->openssl-target.

[Christopher Baines]

Rather than rasising generic errors.

* gnu/packages/tls.scm (target->openssl-target): Raise conditions rather than
generic errors.
(openssl-1.1): Call target->openssl-target with the package.

Change-Id: I13c63328cdf6bc177b20879805246ad94ff2665b
---
 gnu/packages/tls.scm | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/gnu/packages/tls.scm b/gnu/packages/tls.scm
index 719da8e113..f04e171a9b 100644
--- a/gnu/packages/tls.scm
+++ b/gnu/packages/tls.scm
@@ -84,7 +84,9 @@ (define-module (gnu packages tls)
   #:use-module (gnu packages time)
   #:use-module (gnu packages version-control)
   #:use-module (gnu packages base)
-  #:use-module (srfi srfi-1))
+  #:use-module (srfi srfi-1)
+  #:use-module (srfi srfi-34)
+  #:use-module (srfi srfi-35))
 
 (define-public libtasn1
   (package
@@ -390,7 +392,7 @@ (define-public guile2.2-gnutls
      (modify-inputs (package-inputs guile-gnutls)
        (replace "guile" guile-2.2)))))
 
-(define (target->openssl-target target)
+(define (target->openssl-target pkg target)
   "Return the value to set CONFIGURE_TARGET_ARCH to when cross-compiling
 OpenSSL for TARGET."
   ;; Keep this code outside the build code,
@@ -411,7 +413,10 @@ (define (target->openssl-target target)
                    ((target-linux? target)
                     "linux")
                    (else
-                    (error "unsupported openssl target kernel"))))
+                    (raise (condition
+                            (&package-unsupported-target-error
+                             (package pkg)
+                             (target target)))))))
             (arch
              (cond
               ((target-x86-32? target)
@@ -438,7 +443,10 @@ (define (target->openssl-target target)
               ((target-64bit? target)
                "generic64")
               (else
-               (error "unsupported openssl target architecture")))))
+               (raise (condition
+                       (&package-unsupported-target-error
+                        (package pkg)
+                        (target target))))))))
         (string-append kernel "-" arch))))
 
 (define-public openssl-1.1
@@ -488,6 +496,7 @@ (define-public openssl-1.1
                         (setenv "CROSS_COMPILE" (string-append target "-"))
                         (setenv "CONFIGURE_TARGET_ARCH"
                                 #$(target->openssl-target
+                                   this-package
                                    (%current-target-system))))))
                  #~())
           #$@(if (target-hurd?)
-- 
2.41.0

[bug#70985] [PATCH 3/4] gnu: cross-libc*: Raise conditions rather than returning #f.

[Christopher Baines]

As this means that the error will be clearer for targets which are
unsupported.

* gnu/packages/cross-base.scm (cross-libc*): Raise conditions rather than
returning #f.

Change-Id: I820780ad738d85a98950de5608f3019e961ff7c8
---
 gnu/packages/cross-base.scm | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/gnu/packages/cross-base.scm b/gnu/packages/cross-base.scm
index 2cc5f52e47..0488e397ef 100644
--- a/gnu/packages/cross-base.scm
+++ b/gnu/packages/cross-base.scm
@@ -46,6 +46,8 @@ (define-module (gnu packages cross-base)
   #:use-module (guix gexp)
   #:use-module (srfi srfi-1)
   #:use-module (srfi srfi-26)
+  #:use-module (srfi srfi-34)
+  #:use-module (srfi srfi-35)
   #:use-module (ice-9 match)
   #:use-module (ice-9 regex)
   #:export (cross-binutils
@@ -671,8 +673,8 @@ (define* (cross-libc* target
                       (xbinutils (cross-binutils target))
                       (xheaders (cross-kernel-headers target)))
   "Return LIBC cross-built for TARGET, a GNU triplet. Use XGCC and XBINUTILS
-and the cross tool chain.  If TARGET doesn't have a standard C library #f is
-returned."
+and the cross tool chain.  If TARGET doesn't have a standard C library an
+exception is raised."
   (match target
    ((? target-mingw?)
     (let ((machine (substring target 0 (string-index target #\-))))
@@ -745,7 +747,11 @@ (define* (cross-libc* target
    ((? target-avr?)
     (make-avr-libc #:xbinutils xbinutils
                    #:xgcc xgcc))
-   (else #f)))
+   (else
+    (raise (condition
+            (&package-unsupported-target-error
+             (package libc)
+             (target target)))))))
 
 (define* (cross-gcc-toolchain/implementation target
                                              #:key
-- 
2.41.0

[bug#70985] [PATCH 3/4] gnu: cross-libc*: Raise conditions rather than returning #f.

[Ludovic Courtès]

Christopher Baines <mail@cbaines.net> writes:

> As this means that the error will be clearer for targets which are
> unsupported.
>
> * gnu/packages/cross-base.scm (cross-libc*): Raise conditions rather than
> returning #f.
>
> Change-Id: I820780ad738d85a98950de5608f3019e961ff7c8

[...]

> -   (else #f)))
> +   (else
> +    (raise (condition
> +            (&package-unsupported-target-error
> +             (package libc)
> +             (target target)))))))

What I'm unsure is whether this works with libc-less triplets such as
‘avr’ or ‘or1k-elf’.

If it does, LGTM.

Ludo’.

[bug#70985] [PATCH 3/4] gnu: cross-libc*: Raise conditions rather than returning #f.

[Christopher Baines]

[-- Attachment #1: Type: text/plain, Size: 1193 bytes --]

Ludovic Courtès <ludo@gnu.org> writes:

> Christopher Baines <mail@cbaines.net> writes:
>
>> As this means that the error will be clearer for targets which are
>> unsupported.
>>
>> * gnu/packages/cross-base.scm (cross-libc*): Raise conditions rather than
>> returning #f.
>>
>> Change-Id: I820780ad738d85a98950de5608f3019e961ff7c8
>
> [...]
>
>> -   (else #f)))
>> +   (else
>> +    (raise (condition
>> +            (&package-unsupported-target-error
>> +             (package libc)
>> +             (target target)))))))
>
> What I'm unsure is whether this works with libc-less triplets such as
> ‘avr’ or ‘or1k-elf’.

Hmm, it seems like this is erroring in some cases where it didn't
before, I'm not sure why though.

I ended up here through rust-sysroot-for-... since it calls
cross-libc. Handling cross-libc returning #f in the packages which use
it seems error prone, hence adding the exception inside of cross-libc*.

It looks liek cross-gcc-toolchain/implementation is expecting cross-libc
to return #f in some cases though, so maybe I need to add a guard there
somehow to handle the exception and ignore it, as happens when it
returns #f.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 987 bytes --]

[bug#70985] [PATCH 4/4] guix: build-system: meson: Don't error on unsupported targets.

[Christopher Baines]

Rather than raising generic errors.

* guix/build-system/meson.scm (make-machine-alist): Return #f if the triplet
is unsupported.
(lower): Return #f if the machine alist is #f.

Change-Id: If6a1f8d1c2073e43107406ac186aa9c845005a95
---
 guix/build-system/meson.scm | 115 +++++++++++++++++++-----------------
 1 file changed, 62 insertions(+), 53 deletions(-)

diff --git a/guix/build-system/meson.scm b/guix/build-system/meson.scm
index bf9ca15ecc..b321417773 100644
--- a/guix/build-system/meson.scm
+++ b/guix/build-system/meson.scm
@@ -46,39 +46,46 @@ (define (make-machine-alist triplet)
   "Make an association list describing what should go into
 the ‘host_machine’ section of the cross file when cross-compiling
 for TRIPLET."
-  `((system . ,(cond ((target-hurd? triplet) "gnu")
-                     ((target-linux? triplet) "linux")
-                     ((target-mingw? triplet) "windows")
-                     ((target-avr? triplet) "none")
-                     (#t (error "meson: unknown operating system"))))
-    (cpu_family . ,(cond ((target-x86-32? triplet) "x86")
+  (let ((system
+         (cond
+          ((target-hurd? triplet) "gnu")
+          ((target-linux? triplet) "linux")
+          ((target-mingw? triplet) "windows")
+          ((target-avr? triplet) "none")
+          (else #f)))
+        (cpu-family
+         (cond ((target-x86-32? triplet) "x86")
+               ((target-x86-64? triplet) "x86_64")
+               ((target-arm32? triplet) "arm")
+               ((target-aarch64? triplet) "aarch64")
+               ((target-avr? triplet) "avr")
+               ((target-mips64el? triplet) "mips64")
+               ((target-powerpc? triplet)
+                (if (target-64bit? triplet)
+                    "ppc64"
+                    "ppc"))
+               ((target-riscv64? triplet) "riscv64")
+               (else #f))))
+    (and system
+         cpu-family
+         `((system . ,system)
+           (cpu_family . ,cpu-family)
+           (cpu . ,(cond ((target-x86-32? triplet) ; i386, ..., i686
+                          (substring triplet 0 4))
                          ((target-x86-64? triplet) "x86_64")
-                         ((target-arm32? triplet) "arm")
-                         ((target-aarch64? triplet) "aarch64")
+                         ((target-aarch64? triplet) "armv8-a")
+                         ((target-arm32? triplet) "armv7")
                          ((target-avr? triplet) "avr")
-                         ((target-mips64el? triplet) "mips64")
-                         ((target-powerpc? triplet)
-                          (if (target-64bit? triplet)
-                              "ppc64"
-                              "ppc"))
-                         ((target-riscv64? triplet) "riscv64")
-                         (#t (error "meson: unknown architecture"))))
-    (cpu . ,(cond ((target-x86-32? triplet) ; i386, ..., i686
-                   (substring triplet 0 4))
-                  ((target-x86-64? triplet) "x86_64")
-                  ((target-aarch64? triplet) "armv8-a")
-                  ((target-arm32? triplet) "armv7")
-                  ((target-avr? triplet) "avr")
-                  ;; According to #mesonbuild on OFTC, there does not appear
-                  ;; to be an official-ish list of CPU types recognised by
-                  ;; Meson, the "cpu" field is not used by Meson itself and
-                  ;; most software doesn't look at this field, except perhaps
-                  ;; for selecting optimisations, so set it to something
-                  ;; arbitrary.
-                  (#t "strawberries")))
-    (endian . ,(if (target-little-endian? triplet)
-                   "little"
-                   "big"))))
+                         ;; According to #mesonbuild on OFTC, there does not appear
+                         ;; to be an official-ish list of CPU types recognised by
+                         ;; Meson, the "cpu" field is not used by Meson itself and
+                         ;; most software doesn't look at this field, except perhaps
+                         ;; for selecting optimisations, so set it to something
+                         ;; arbitrary.
+                         (#t "strawberries")))
+           (endian . ,(if (target-little-endian? triplet)
+                          "little"
+                          "big"))))))
 
 (define (make-binaries-alist triplet)
   "Make an associatoin list describing what should go into
@@ -146,29 +153,31 @@ (define* (lower name
             '()
             '(#:target))))
 
-  (bag
-    (name name)
-    (system system) (target target)
-    (build-inputs `(("meson" ,meson)
-                    ("ninja" ,ninja)
-                    ,@native-inputs
-                    ,@(if target '() inputs)
-                    ;; Keep the standard inputs of 'gnu-build-system'.
-                    ,@(if target
-                          (standard-cross-packages target 'host)
+  (and
+   (make-machine-alist target)
+   (bag
+     (name name)
+     (system system) (target target)
+     (build-inputs `(("meson" ,meson)
+                     ("ninja" ,ninja)
+                     ,@native-inputs
+                     ,@(if target '() inputs)
+                     ;; Keep the standard inputs of 'gnu-build-system'.
+                     ,@(if target
+                           (standard-cross-packages target 'host)
+                           '())
+                     ,@(standard-packages)))
+     (host-inputs `(,@(if source
+                          `(("source" ,source))
                           '())
-                    ,@(standard-packages)))
-    (host-inputs `(,@(if source
-                         `(("source" ,source))
-                         '())
-                   ,@(if target inputs '())))
-    ;; Keep the standard inputs of 'gnu-buid-system'.
-    (target-inputs (if target
-                       (standard-cross-packages target 'target)
-                       '()))
-    (outputs outputs)
-    (build (if target meson-cross-build meson-build))
-    (arguments (strip-keyword-arguments private-keywords arguments))))
+                    ,@(if target inputs '())))
+     ;; Keep the standard inputs of 'gnu-buid-system'.
+     (target-inputs (if target
+                        (standard-cross-packages target 'target)
+                        '()))
+     (outputs outputs)
+     (build (if target meson-cross-build meson-build))
+     (arguments (strip-keyword-arguments private-keywords arguments)))))
 
 (define* (meson-build name inputs
                       #:key
-- 
2.41.0

[bug#70985] [PATCH 4/4] guix: build-system: meson: Don't error on unsupported targets.

[Ludovic Courtès]

Hi,

Christopher Baines <mail@cbaines.net> writes:

> Rather than raising generic errors.
>
> * guix/build-system/meson.scm (make-machine-alist): Return #f if the triplet
> is unsupported.
> (lower): Return #f if the machine alist is #f.
>
> Change-Id: If6a1f8d1c2073e43107406ac186aa9c845005a95

[...]

> +  (and
> +   (make-machine-alist target)

I think this call is unnecessary (and kinda confusing because
‘make-machine-alist’ is a pure function) because ‘meson-cross-build’
calls ‘make-cross-file’, which calls ‘make-machine-alist’.

Ludo’.

[bug#70985] [PATCH 4/4] guix: build-system: meson: Don't error on unsupported targets.

[Christopher Baines]

[-- Attachment #1: Type: text/plain, Size: 1354 bytes --]

Ludovic Courtès <ludo@gnu.org> writes:

> Christopher Baines <mail@cbaines.net> writes:
>
>> Rather than raising generic errors.
>>
>> * guix/build-system/meson.scm (make-machine-alist): Return #f if the triplet
>> is unsupported.
>> (lower): Return #f if the machine alist is #f.
>>
>> Change-Id: If6a1f8d1c2073e43107406ac186aa9c845005a95
>
> [...]
>
>> +  (and
>> +   (make-machine-alist target)
>
> I think this call is unnecessary (and kinda confusing because
> ‘make-machine-alist’ is a pure function) because ‘meson-cross-build’
> calls ‘make-cross-file’, which calls ‘make-machine-alist’.

The situation here is that the meson build system does support cross
builds, but make-machine-alist calls error for some targets which I'm
treating as a bug here, because error isn't really machine readable.

I think this line should actually be (or (not target)
(make-machine-alist target)) to handle the case where the target is #f,
but this is sort of helpful in that it changes the error to the usual
error when the build system doesn't support cross builds.

This is still far from perfect though as the UI would say that "build
system `meson' does not support cross builds", which is obviously wrong,
it just doesn't support cross builds for the given target. I'm not sure
how to signal this though?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 987 bytes --]

[bug#70985] [PATCH 0/4] Use specific errors for unsupported targets

[Ludovic Courtès]

Christopher Baines <mail@cbaines.net> writes:

> I think when computing derivations any errors except from a small list
> should be treated as bugs.
>
> This has become more of an issue with the avr, or1k-elf and
> xtensa-ath9k-elf targets since these targets both seem to not support a
> large number of packages, and lead to generic errors or crashes.
>
> I'm seeing this because logs relating to errors computing package
> derivations are now taking up most of the data service logs around
> processing revisions.

Looks to me like a much welcome improvement, modulo the minor issues I
commented on.  Thanks!

Ludo'.

[bug#70985] [PATCH 3/4] gnu: cross-libc*: Raise conditions rather than returning #f.

[Jean-Pierre De Jesus Diaz]

Hello,

>> Christopher Baines <mail@cbaines.net> writes:
>>
>>> As this means that the error will be clearer for targets which are
>>> unsupported.
>>>
>>> * gnu/packages/cross-base.scm (cross-libc*): Raise conditions rather than
>>> returning #f.
>>>
>>> Change-Id: I820780ad738d85a98950de5608f3019e961ff7c8
>>
>> [...]
>>
>>> -   (else #f)))
>>> +   (else
>>> +    (raise (condition
>>> +            (&package-unsupported-target-error
>>> +             (package libc)
>>> +             (target target)))))))
>>
>> What I'm unsure is whether this works with libc-less triplets such as
>> ‘avr’ or ‘or1k-elf’.
>
>Hmm, it seems like this is erroring in some cases where it didn't
>before, I'm not sure why though.
>
>I ended up here through rust-sysroot-for-... since it calls
>cross-libc. Handling cross-libc returning #f in the packages which use
>it seems error prone, hence adding the exception inside of cross-libc*.
>
>It looks liek cross-gcc-toolchain/implementation is expecting cross-libc
>to return #f in some cases though, so maybe I need to add a guard there
>somehow to handle the exception and ignore it, as happens when it
>returns #f.

This is because some targets as Ludovic mentioned don't have a proper
libc, like most *-elf targets.  Also, gnu-build-system also depends on
cross-libc returning #f to check for those targets that don't have a
libc in `cross-standard-packages'.

Thanks,

Jean-Pierre