| Message ID | 20210623072509.7165-2-leo.prikler@student.tugraz.at |
|---|---|
| State | New |
| Headers | show |
| Series | modify-phases: error when encountering missing phase (was: Fix missing phases in Emacs builds) | expand |
| Context | Check | Description |
|---|---|---|
| cbaines/comparison | success | View comparision |
| cbaines/git branch | success | View Git branch |
| cbaines/applying patch | success | View Laminar job |
| cbaines/issue | success | View issue |
Hi! I'm glad to see this. This behavior has caught me up a number of times recently. Leo Prikler <leo.prikler@student.tugraz.at> writes: [...] @@ -571,18 +572,22 @@ effects, such as displaying warnings or error messages." > (define* (alist-cons-before reference key value alist > #:optional (key=? equal?)) > "Insert the KEY/VALUE pair before the first occurrence of a pair whose key > -is REFERENCE in ALIST. Use KEY=? to compare keys." > +is REFERENCE in ALIST. Use KEY=? to compare keys. An error is raised when no > +such pair exists." > (let-values (((before after) > (break (match-lambda > ((k . _) > (key=? k reference))) > alist))) > - (append before (alist-cons key value after)))) > + (match after > + ((reference after ...) > + (append before (alist-cons key value (cons reference after))))))) This can probably just be (untested): ((reference after* ...) (append before (alist-cons key value after)))))) Or if we want to avoid extraneous bindings completely (also untested): ((_ _ ...) (append before (alist-cons key value after)))))) > > (define* (alist-cons-after reference key value alist > #:optional (key=? equal?)) > "Insert the KEY/VALUE pair after the first occurrence of a pair whose key > -is REFERENCE in ALIST. Use KEY=? to compare keys." > +is REFERENCE in ALIST. Use KEY=? to compare keys. An error is raised when > +no such pair exists." > (let-values (((before after) > (break (match-lambda > ((k . _) > @@ -590,9 +595,7 @@ is REFERENCE in ALIST. Use KEY=? to compare keys." > alist))) > (match after > ((reference after ...) > - (append before (cons* reference `(,key . ,value) after))) > - (() > - (append before `((,key . ,value))))))) > + (append before (cons* reference `(,key . ,value) after)))))) > > (define* (alist-replace key value alist #:optional (key=? equal?)) > "Replace the first pair in ALIST whose car is KEY with the KEY/VALUE pair. Other than that it looks good to me. Pushing this should also close #32661. This should be a patch for core-updates, though, since it changes derivations for any package that (directly or indirectly) uses ALIST-CONS-BEFORE or ALIST-CONS-AFTER. -- Sarah
This patch has been sitting around for a while, and I think it would be good to merge it. It still applies cleanly on top of master. What can I do to get this merged? Carlo On Wed, Jun 23 2021, Leo Prikler wrote: > From: Carlo Zancanaro <carlo@zancanaro.id.au> > > * guix/build/utils.scm (alist-cons-before, alist-cons-after): > Cause a match failure if the > reference is not found, rather than appending to the alist. > * tests/build-utils.scm: Update tests to match. > --- > guix/build/utils.scm | 15 +++++++++------ > tests/build-utils.scm | 12 ++++++------ > 2 files changed, 15 insertions(+), 12 deletions(-) > > ...
Hi Carlo, Carlo Zancanaro <carlo@zancanaro.id.au> writes: > This patch has been sitting around for a while, and I think it would > be good to merge it. It still applies cleanly on top of master. > > What can I do to get this merged? Have you seen the good comments from Sarah up thread? Perhaps you could send a rebased v2 integrating them. And then ping us in #guix if it falls into the cracks again!
Hi Maxim, On Tue, Mar 28 2023, Maxim Cournoyer wrote: > Have you seen the good comments from Sarah up thread? Perhaps > you could send a rebased v2 integrating them. I've sent an updated patch with the fix that Sarah mentioned. I also added "core updates" to the PATCH bit, because I imagine Sarah is right that this would trigger a bunch of rebuilds. I'm not sure that these changes warranted a two year wait on this patch, but I still think the patch is worth merging. Carlo
diff --git a/guix/build/utils.scm b/guix/build/utils.scm index 419c10195b..6e052a7ea1 100644 --- a/guix/build/utils.scm +++ b/guix/build/utils.scm @@ -5,6 +5,7 @@ ;;; Copyright © 2015, 2018 Mark H Weaver <mhw@netris.org> ;;; Copyright © 2018 Arun Isaac <arunisaac@systemreboot.net> ;;; Copyright © 2018, 2019 Ricardo Wurmus <rekado@elephly.net> +;;; Copyright © 2021 Carlo Zancanaro <carlo@zancanaro.id.au> ;;; ;;; This file is part of GNU Guix. ;;; @@ -571,18 +572,22 @@ effects, such as displaying warnings or error messages." (define* (alist-cons-before reference key value alist #:optional (key=? equal?)) "Insert the KEY/VALUE pair before the first occurrence of a pair whose key -is REFERENCE in ALIST. Use KEY=? to compare keys." +is REFERENCE in ALIST. Use KEY=? to compare keys. An error is raised when no +such pair exists." (let-values (((before after) (break (match-lambda ((k . _) (key=? k reference))) alist))) - (append before (alist-cons key value after)))) + (match after + ((reference after ...) + (append before (alist-cons key value (cons reference after))))))) (define* (alist-cons-after reference key value alist #:optional (key=? equal?)) "Insert the KEY/VALUE pair after the first occurrence of a pair whose key -is REFERENCE in ALIST. Use KEY=? to compare keys." +is REFERENCE in ALIST. Use KEY=? to compare keys. An error is raised when +no such pair exists." (let-values (((before after) (break (match-lambda ((k . _) @@ -590,9 +595,7 @@ is REFERENCE in ALIST. Use KEY=? to compare keys." alist))) (match after ((reference after ...) - (append before (cons* reference `(,key . ,value) after))) - (() - (append before `((,key . ,value))))))) + (append before (cons* reference `(,key . ,value) after)))))) (define* (alist-replace key value alist #:optional (key=? equal?)) "Replace the first pair in ALIST whose car is KEY with the KEY/VALUE pair. diff --git a/tests/build-utils.scm b/tests/build-utils.scm index 654b480ed9..c694b479b3 100644 --- a/tests/build-utils.scm +++ b/tests/build-utils.scm @@ -38,17 +38,17 @@ '((a . 1) (x . 42) (b . 2) (c . 3)) (alist-cons-before 'b 'x 42 '((a . 1) (b . 2) (c . 3)))) -(test-equal "alist-cons-before, reference not found" - '((a . 1) (b . 2) (c . 3) (x . 42)) - (alist-cons-before 'z 'x 42 '((a . 1) (b . 2) (c . 3)))) +(test-assert "alist-cons-before, reference not found" + (not (false-if-exception + (alist-cons-before 'z 'x 42 '((a . 1) (b . 2) (c . 3)))))) (test-equal "alist-cons-after" '((a . 1) (b . 2) (x . 42) (c . 3)) (alist-cons-after 'b 'x 42 '((a . 1) (b . 2) (c . 3)))) -(test-equal "alist-cons-after, reference not found" - '((a . 1) (b . 2) (c . 3) (x . 42)) - (alist-cons-after 'z 'x 42 '((a . 1) (b . 2) (c . 3)))) +(test-assert "alist-cons-after, reference not found" + (not (false-if-exception + (alist-cons-after 'z 'x 42 '((a . 1) (b . 2) (c . 3)))))) (test-equal "alist-replace" '((a . 1) (b . 77) (c . 3))
From: Carlo Zancanaro <carlo@zancanaro.id.au> * guix/build/utils.scm (alist-cons-before, alist-cons-after): Cause a match failure if the reference is not found, rather than appending to the alist. * tests/build-utils.scm: Update tests to match. --- guix/build/utils.scm | 15 +++++++++------ tests/build-utils.scm | 12 ++++++------ 2 files changed, 15 insertions(+), 12 deletions(-)