GNU bug report logs - #49181
Fix missing phases in Emacs builds

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 49181 in the body.
You can then email your comments to 49181 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Carlo Zancanaro <carlo <at> zancanaro.id.au>
To: guix-patches <at> gnu.org
Subject: Fix missing phases in Emacs builds
Date: Wed, 23 Jun 2021 16:45:28 +1000

[Message part 1 (text/plain, inline)]

Hi there!

I went to install emacs-protobuf-mode today and found that the 
build wasn't working. I investigated and it looks like someone 
renamed the add-source-to-load-path phase to expand-load-path, but 
they left a few references behind. The first of my patches fixes 
that.

The second of my patches is more controversial. It changes 
modify-phases to error out if the asked-for phase doesn't exist in 
add-before/add-after clauses. I think this is the right move, 
because it's hard to imagine when the default behaviour of "add to 
the end of the phases list" is helpful. In most cases the extra 
phases are setup/transformation phases that we need to run before 
the final "install" phase, so it's far more useful to fail early 
than to add these to the end of the phases list.

Carlo

[0001-gnu-Fix-references-to-emacs-build-system-s-expand-lo.patch (text/x-diff, attachment)]

[0002-guix-Make-modify-phases-error-when-adding-before-aft.patch (text/x-diff, attachment)]

Message #8 received at 49181 <at> debbugs.gnu.org (full text, mbox):

From: Leo Prikler <leo.prikler <at> student.tugraz.at>
To: 49181 <at> debbugs.gnu.org
Cc: carlo <at> zancanaro.id.au
Subject: [PATCH 0/1] modify-phases: error when encountering missing phase
 (was: Fix missing phases in Emacs builds)
Date: Wed, 23 Jun 2021 09:25:09 +0200

Hi,

I've signed off your first patch and pushed it to master.  The second
patch looks useful, but I'd like others to state their opinion as well.
I'm resending it through git send-email, so that everyone can use their
preferred tooling for fetching stuff from the mailing list.

Regards,
Leo

Message #11 received at 49181 <at> debbugs.gnu.org (full text, mbox):

From: Leo Prikler <leo.prikler <at> student.tugraz.at>
To: 49181 <at> debbugs.gnu.org
Cc: carlo <at> zancanaro.id.au
Subject: [PATCH 1/1] guix: Make modify-phases error when adding before/after a
 missing phase
Date: Wed, 23 Jun 2021 09:25:10 +0200

From: Carlo Zancanaro <carlo <at> 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(-)

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 <at> netris.org>
 ;;; Copyright © 2018 Arun Isaac <arunisaac <at> systemreboot.net>
 ;;; Copyright © 2018, 2019 Ricardo Wurmus <rekado <at> elephly.net>
+;;; Copyright © 2021 Carlo Zancanaro <carlo <at> 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))
-- 
2.32.0

Message #14 received at 49181 <at> debbugs.gnu.org (full text, mbox):

From: Sarah Morgensen <iskarian <at> mgsn.dev>
To: Leo Prikler <leo.prikler <at> student.tugraz.at>
Cc: 49181 <at> debbugs.gnu.org, carlo <at> zancanaro.id.au
Subject: Re: bug#49181: Fix missing phases in Emacs builds
Date: Fri, 23 Jul 2021 14:32:08 -0700

Hi!

I'm glad to see this.  This behavior has caught me up a number of times
recently.

Leo Prikler <leo.prikler <at> 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

Message #17 received at 49181 <at> debbugs.gnu.org (full text, mbox):

From: Carlo Zancanaro <carlo <at> zancanaro.id.au>
To: 49181 <at> debbugs.gnu.org
Cc: Leo Prikler <leo.prikler <at> student.tugraz.at>
Subject: Re: [PATCH 1/1] guix: Make modify-phases error when adding
 before/after a missing phase
Date: Wed, 03 Nov 2021 09:58:52 +1100

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 <at> 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(-)
>
> ...

Message #20 received at 49181 <at> debbugs.gnu.org (full text, mbox):

From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
To: Carlo Zancanaro <carlo <at> zancanaro.id.au>
Cc: 49181 <at> debbugs.gnu.org, Leo Prikler <leo.prikler <at> student.tugraz.at>
Subject: Re: bug#49181: Fix missing phases in Emacs builds
Date: Tue, 28 Mar 2023 22:18:36 -0400

Hi Carlo,

Carlo Zancanaro <carlo <at> 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!

-- 
Thanks,
Maxim

Message #23 received at 49181 <at> debbugs.gnu.org (full text, mbox):

From: Carlo Zancanaro <carlo <at> zancanaro.id.au>
To: 49181 <at> debbugs.gnu.org
Cc: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Subject: [PATCH core-updates v2] guix: Make modify-phases error when adding
 before/after a missing phase
Date: Sat, 20 May 2023 23:05:06 +1000

* 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(-)

diff --git a/guix/build/utils.scm b/guix/build/utils.scm
index 2352a627e9..8e630ad586 100644
--- a/guix/build/utils.scm
+++ b/guix/build/utils.scm
@@ -9,6 +9,7 @@
 ;;; Copyright © 2020, 2021 Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
 ;;; Copyright © 2021, 2022 Maxime Devos <maximedevos <at> telenet.be>
 ;;; Copyright © 2021 Brendan Tildesley <mail <at> brendan.scot>
+;;; Copyright © 2023 Carlo Zancanaro <carlo <at> zancanaro.id.au>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -729,18 +730,22 @@ (define (every* pred lst)
 (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
+      ((_ _ ...)
+       (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 . _)
@@ -748,9 +753,7 @@ (define* (alist-cons-after reference key value alist
                        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 7f4f12ccc7..3babf5d544 100644
--- a/tests/build-utils.scm
+++ b/tests/build-utils.scm
@@ -41,17 +41,17 @@ (define-module (test build-utils)
   '((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))

base-commit: 24b6f94cf9b4ab97ef2eb70d05b2104a06776e62
-- 
2.40.1

Message #26 received at 49181 <at> debbugs.gnu.org (full text, mbox):

From: Carlo Zancanaro <carlo <at> zancanaro.id.au>
To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Cc: 49181 <at> debbugs.gnu.org
Subject: Re: bug#49181: Fix missing phases in Emacs builds
Date: Sat, 20 May 2023 23:13:12 +1000

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

Message #29 received at 49181 <at> debbugs.gnu.org (full text, mbox):

From: Carlo Zancanaro <carlo <at> zancanaro.id.au>
To: 49181 <at> debbugs.gnu.org
Cc: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Subject: Re: [bug#49181] [PATCH core-updates v2] guix: Make modify-phases
 error when adding before/after a missing phase
Date: Tue, 10 Oct 2023 11:50:06 +1100

Ping!

Given the upcoming planned world rebuild, it would be nice to get 
this into core-updates.

On Sat, May 20 2023, Carlo Zancanaro wrote:
> * 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(-)
> ...

Message #34 received at 49181-done <at> debbugs.gnu.org (full text, mbox):

From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
To: Carlo Zancanaro <carlo <at> zancanaro.id.au>
Cc: 49181-done <at> debbugs.gnu.org
Subject: Re: bug#49181: [PATCH core-updates v2] guix: Make modify-phases
 error when adding before/after a missing phase
Date: Mon, 09 Oct 2023 23:37:30 -0400

Hi,

Carlo Zancanaro <carlo <at> zancanaro.id.au> writes:

> * 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.

Installed to core-updates!

-- 
Thanks,
Maxim

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.