| Message ID | 20230114041903.7121-2-maxim.cournoyer@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [bug#60802,v2,1/2] platforms: Raise an exception when no suitable platform is found. | expand |
Hi, Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis: > This was motivated by #60786, which produced a cryptic, hard to understand > backtrace. > > * guix/platform.scm (&platform-not-found-error): New exception type. > (make-platform-not-found-error): New exception constructor. > (platform-not-found-error?): New predicate. > (false-if-platform-not-found): New syntax. > (lookup-platform-by-system): Raise an exception when no platform is found. > Update documentation. > (lookup-platform-by-target): Likewise. > (lookup-platform-by-target-or-system): Likewise, and guard lookup calls with > false-if-platform-not-found. Sounds like a good idea! > +++ b/gnu/packages/bootstrap.scm > @@ -315,7 +315,7 @@ (define* (glibc-dynamic-linker > (%current-system)))) > "Return the name of Glibc's dynamic linker for SYSTEM." > ;; See the 'SYSDEP_KNOWN_INTERPRETER_NAMES' cpp macro in libc. > - (let ((platform (lookup-platform-by-system system))) > + (let ((platform (false-if-exception (lookup-platform-by-system system)))) Maybe we don’t need to protect here, because it’s a > +++ b/guix/platform.scm > @@ -21,6 +21,7 @@ (define-module (guix platform) > #:use-module (guix memoization) > #:use-module (guix records) > #:use-module (guix ui) > + #:use-module (ice-9 exceptions) So far the we use (srfi srfi-35) exclusively to define condition types; I think we should do the same here, for consistency. > + &platform-not-found-error > + make-platform-not-found-error The constructor doesn’t need to be exposed. > +;;; > +;;; Exceptions. > +;;; > +(define-exception-type &platform-not-found-error &error > + make-platform-not-found-error platform-not-found-error?) So this would become (define-condition-type …). Otherwise LGTM, thanks! Ludo’.
Hi, Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis: > This was motivated by #60786, which produced a cryptic, hard to understand > backtrace. > > * guix/platform.scm (&platform-not-found-error): New exception type. > (make-platform-not-found-error): New exception constructor. > (platform-not-found-error?): New predicate. > (false-if-platform-not-found): New syntax. > (lookup-platform-by-system): Raise an exception when no platform is found. > Update documentation. > (lookup-platform-by-target): Likewise. > (lookup-platform-by-target-or-system): Likewise, and guard lookup calls with > false-if-platform-not-found. Sounds like a good idea! > +++ b/guix/platform.scm > @@ -21,6 +21,7 @@ (define-module (guix platform) > #:use-module (guix memoization) > #:use-module (guix records) > #:use-module (guix ui) > + #:use-module (ice-9 exceptions) So far the we use (srfi srfi-35) exclusively to define condition types; I think we should do the same here, for consistency. > + &platform-not-found-error > + make-platform-not-found-error The constructor doesn’t need to be exposed. > +;;; > +;;; Exceptions. > +;;; > +(define-exception-type &platform-not-found-error &error > + make-platform-not-found-error platform-not-found-error?) So this would become (define-condition-type …). Otherwise LGTM, thanks! Ludo’.
Hi Ludovic, Ludovic Courtès <ludo@gnu.org> writes: > Hi, > > Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis: > >> This was motivated by #60786, which produced a cryptic, hard to understand >> backtrace. >> >> * guix/platform.scm (&platform-not-found-error): New exception type. >> (make-platform-not-found-error): New exception constructor. >> (platform-not-found-error?): New predicate. >> (false-if-platform-not-found): New syntax. >> (lookup-platform-by-system): Raise an exception when no platform is found. >> Update documentation. >> (lookup-platform-by-target): Likewise. >> (lookup-platform-by-target-or-system): Likewise, and guard lookup calls with >> false-if-platform-not-found. > > Sounds like a good idea! Good! >> +++ b/gnu/packages/bootstrap.scm >> @@ -315,7 +315,7 @@ (define* (glibc-dynamic-linker >> (%current-system)))) >> "Return the name of Glibc's dynamic linker for SYSTEM." >> ;; See the 'SYSDEP_KNOWN_INTERPRETER_NAMES' cpp macro in libc. >> - (let ((platform (lookup-platform-by-system system))) >> + (let ((platform (false-if-exception (lookup-platform-by-system system)))) > > Maybe we don’t need to protect here, because it’s a We cannot do this, otherwise the other cond clauses would never been evaluated: --8<---------------cut here---------------start------------->8--- (let ((platform (false-if-exception (lookup-platform-by-system system)))) (cond ((platform? platform) (platform-glibc-dynamic-linker platform)) --> Clauses below here are evaluated when platform was not found. ;; TODO: Define those as platforms. ((string=? system "i686-gnu") "/lib/ld.so.1") ((string=? system "powerpc64-linux") "/lib/ld64.so.1") ((string=? system "alpha-linux") "/lib/ld-linux.so.2") ;; XXX: This one is used bare-bones, without a libc, so add a case ;; here just so we can keep going. ((string=? system "arm-elf") "no-ld.so") ((string=? system "arm-eabi") "no-ld.so") ((string=? system "xtensa-elf") "no-ld.so") ((string=? system "avr") "no-ld.so") ((string=? system "propeller-elf") "no-ld.so") ((string=? system "i686-mingw") "no-ld.so") ((string=? system "x86_64-mingw") "no-ld.so") ((string=? system "vc4-elf") "no-ld.so") (else (error "dynamic linker name not known for this system" system)))) --8<---------------cut here---------------end--------------->8--- I'll change it to use false-if-platform-not-found though. >> +++ b/guix/platform.scm >> @@ -21,6 +21,7 @@ (define-module (guix platform) >> #:use-module (guix memoization) >> #:use-module (guix records) >> #:use-module (guix ui) >> + #:use-module (ice-9 exceptions) > > So far the we use (srfi srfi-35) exclusively to define condition types; > I think we should do the same here, for consistency. Could we instead start migrating away from srfi-35 to (ice-9 exceptions), which is the new native way to use exceptions in Guile? I think it'd be nicer to use it in the future, to avoid newcomers being confusing about the 3 or 4 ways to manage exceptions in Guile (recommended read on the topic: https://vijaymarupudi.com/blog/2022-02-13-error-handling-in-guile.html). Migrating the whole code at once doesn't seem a good idea, so gradually transitioning (such as using (ice-9 exceptions) for new code) appears a good idea to me, if we agree on the direction! >> + &platform-not-found-error >> + make-platform-not-found-error > > The constructor doesn’t need to be exposed. Good point. Fixed in the latest revision.
Hi, Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis: >>> +++ b/gnu/packages/bootstrap.scm >>> @@ -315,7 +315,7 @@ (define* (glibc-dynamic-linker >>> (%current-system)))) >>> "Return the name of Glibc's dynamic linker for SYSTEM." >>> ;; See the 'SYSDEP_KNOWN_INTERPRETER_NAMES' cpp macro in libc. >>> - (let ((platform (lookup-platform-by-system system))) >>> + (let ((platform (false-if-exception (lookup-platform-by-system system)))) >> >> Maybe we don’t need to protect here, because it’s a > > We cannot do this, otherwise the other cond clauses would never been > evaluated: Yes sorry, I realized later but hit “Send” before deleting the sentence prototype. :-) >> So far the we use (srfi srfi-35) exclusively to define condition types; >> I think we should do the same here, for consistency. > > Could we instead start migrating away from srfi-35 to (ice-9 > exceptions), which is the new native way to use exceptions in Guile? Maybe, but I’m not convinced it’s worth it. > I think it'd be nicer to use it in the future, to avoid newcomers > being confusing about the 3 or 4 ways to manage exceptions in Guile That’s why I suggest sticking to one way in Guix, which is currently SRFI-34/35. > Migrating the whole code at once doesn't seem a good idea, so gradually > transitioning (such as using (ice-9 exceptions) for new code) appears a > good idea to me, if we agree on the direction! I’m not convinced it’s worth transitioning, and I’m even less convinced about a gradual transition, as that would make it harder for newcomers to join, people wouldn’t be sure which style to use, etc. It’s a discussion worth having though, but I think we can have it separately. Thanks, Ludo’.
Hello, Ludovic Courtès <ludo@gnu.org> writes: > Hi, > > Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis: > >> This was motivated by #60786, which produced a cryptic, hard to understand >> backtrace. >> >> * guix/platform.scm (&platform-not-found-error): New exception type. >> (make-platform-not-found-error): New exception constructor. >> (platform-not-found-error?): New predicate. >> (false-if-platform-not-found): New syntax. >> (lookup-platform-by-system): Raise an exception when no platform is found. >> Update documentation. >> (lookup-platform-by-target): Likewise. >> (lookup-platform-by-target-or-system): Likewise, and guard lookup calls with >> false-if-platform-not-found. > > Sounds like a good idea! > >> +++ b/guix/platform.scm >> @@ -21,6 +21,7 @@ (define-module (guix platform) >> #:use-module (guix memoization) >> #:use-module (guix records) >> #:use-module (guix ui) >> + #:use-module (ice-9 exceptions) > > So far the we use (srfi srfi-35) exclusively to define condition types; > I think we should do the same here, for consistency. > >> + &platform-not-found-error >> + make-platform-not-found-error > > The constructor doesn’t need to be exposed. > >> +;;; >> +;;; Exceptions. >> +;;; >> +(define-exception-type &platform-not-found-error &error >> + make-platform-not-found-error platform-not-found-error?) > > So this would become (define-condition-type …). > > Otherwise LGTM, thanks! The latest version incorporate these changes and use srfi-34/35 as is currently custom in Guix. I've now applied it.
diff --git a/gnu/packages/bootstrap.scm b/gnu/packages/bootstrap.scm index d2914fb5a7..772be7e5e4 100644 --- a/gnu/packages/bootstrap.scm +++ b/gnu/packages/bootstrap.scm @@ -315,7 +315,7 @@ (define* (glibc-dynamic-linker (%current-system)))) "Return the name of Glibc's dynamic linker for SYSTEM." ;; See the 'SYSDEP_KNOWN_INTERPRETER_NAMES' cpp macro in libc. - (let ((platform (lookup-platform-by-system system))) + (let ((platform (false-if-exception (lookup-platform-by-system system)))) (cond ((platform? platform) (platform-glibc-dynamic-linker platform)) diff --git a/guix/platform.scm b/guix/platform.scm index f873913fe0..2e61c6827a 100644 --- a/guix/platform.scm +++ b/guix/platform.scm @@ -21,6 +21,7 @@ (define-module (guix platform) #:use-module (guix memoization) #:use-module (guix records) #:use-module (guix ui) + #:use-module (ice-9 exceptions) #:use-module (srfi srfi-1) #:export (platform platform? @@ -29,6 +30,11 @@ (define-module (guix platform) platform-linux-architecture platform-glibc-dynamic-linker + &platform-not-found-error + make-platform-not-found-error + platform-not-found-error? + false-if-platform-not-found + platform-modules platforms lookup-platform-by-system @@ -70,6 +76,19 @@ (define-record-type* <platform> platform make-platform (default #false)) (glibc-dynamic-linker platform-glibc-dynamic-linker)) + +;;; +;;; Exceptions. +;;; +(define-exception-type &platform-not-found-error &error + make-platform-not-found-error platform-not-found-error?) + +(define-syntax-rule (false-if-platform-not-found exp) + "Evaluate EXP but return #f if it raises a platform-not-found-error? +exception." + (guard (ex ((platform-not-found-error? ex) #f)) + exp)) + ;;; ;;; Platforms. @@ -94,23 +113,29 @@ (define platforms (platform-modules))))) (define (lookup-platform-by-system system) - "Return the platform corresponding to the given SYSTEM." - (find (lambda (platform) - (let ((s (platform-system platform))) - (and (string? s) (string=? s system)))) - (platforms))) + "Return the platform corresponding to the given SYSTEM. Raise +&PLATFORM-NOT-FOUND-ERROR when no platform could be found." + (or (find (lambda (platform) + (let ((s (platform-system platform))) + (and (string? s) (string=? s system)))) + (platforms)) + (raise-exception (make-platform-not-found-error)))) (define (lookup-platform-by-target target) - "Return the platform corresponding to the given TARGET." - (find (lambda (platform) - (let ((t (platform-target platform))) - (and (string? t) (string=? t target)))) - (platforms))) + "Return the platform corresponding to the given TARGET. Raise +&PLATFORM-NOT-FOUND-ERROR when no platform could be found." + (or (find (lambda (platform) + (let ((t (platform-target platform))) + (and (string? t) (string=? t target)))) + (platforms)) + (raise-exception (make-platform-not-found-error)))) (define (lookup-platform-by-target-or-system target-or-system) - "Return the platform corresponding to the given TARGET or SYSTEM." - (or (lookup-platform-by-target target-or-system) - (lookup-platform-by-system target-or-system))) + "Return the platform corresponding to the given TARGET or SYSTEM. Raise +&PLATFORM-NOT-FOUND-ERROR when no platform could be found." + (or (false-if-platform-not-found (lookup-platform-by-target target-or-system)) + (false-if-platform-not-found (lookup-platform-by-system target-or-system)) + (raise-exception (make-platform-not-found-error)))) (define (platform-system->target system) "Return the target matching the given SYSTEM if it exists or false