[bug#49181] Fix missing phases in Emacs builds

[bug#49181] Fix missing phases in Emacs builds

[Carlo Zancanaro]

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

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


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-gnu-Fix-references-to-emacs-build-system-s-expand-lo.patch --]
[-- Type: text/x-diff, Size: 2698 bytes --]

From d07b375d6d0a1c354587b285a2547aee8f6e9f96 Mon Sep 17 00:00:00 2001
From: Carlo Zancanaro <carlo@zancanaro.id.au>
Date: Wed, 23 Jun 2021 15:57:40 +1000
Subject: [PATCH 1/2] gnu: Fix references to emacs-build-system's
 expand-load-path phase

* gnu/packages/emacs-xyz.scm (emacs-pdf-tools): Change reference from
emacs-add-source-to-load-path to emacs-expand-load-path
* gnu/packages/erlang.scm (emacs-erlang): Change reference from
add-source-to-load-path to expand-load-path
* gnu/packages/protobuf.scm (emacs-protobuf-mode): Change reference from
add-source-to-load-path to expand-load-path
---
 gnu/packages/emacs-xyz.scm | 2 +-
 gnu/packages/erlang.scm    | 2 +-
 gnu/packages/protobuf.scm  | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/gnu/packages/emacs-xyz.scm b/gnu/packages/emacs-xyz.scm
index eef33fd437..28b87d49dd 100644
--- a/gnu/packages/emacs-xyz.scm
+++ b/gnu/packages/emacs-xyz.scm
@@ -3310,7 +3310,7 @@ during idle time, while Emacs is doing nothing else.")
                  ("pdf-tools-handle-upgrades" '()))))
            (add-after 'emacs-patch-variables 'emacs-expand-load-path
              (assoc-ref emacs:%standard-phases 'expand-load-path))
-           (add-after 'emacs-add-source-to-load-path 'emacs-install
+           (add-after 'emacs-expand-load-path 'emacs-install
              (assoc-ref emacs:%standard-phases 'install))
            (add-after 'emacs-install 'emacs-build
              (assoc-ref emacs:%standard-phases 'build))
diff --git a/gnu/packages/erlang.scm b/gnu/packages/erlang.scm
index 7b5dc70b5d..d52d5c3983 100644
--- a/gnu/packages/erlang.scm
+++ b/gnu/packages/erlang.scm
@@ -232,7 +232,7 @@ built-in support for concurrency, distribution and fault tolerance.")
     (arguments
      `(#:phases
        (modify-phases %standard-phases
-         (add-before 'add-source-to-load-path 'change-working-directory
+         (add-before 'expand-load-path 'change-working-directory
            (lambda _ (chdir "lib/tools/emacs") #t)))))
     (home-page "https://www.erlang.org/")
     (synopsis "Erlang major mode for Emacs")
diff --git a/gnu/packages/protobuf.scm b/gnu/packages/protobuf.scm
index 857adf1703..9c6dcb51cc 100644
--- a/gnu/packages/protobuf.scm
+++ b/gnu/packages/protobuf.scm
@@ -311,7 +311,7 @@ structured data.")
     (arguments
      `(#:phases
        (modify-phases %standard-phases
-         (add-before 'add-source-to-load-path 'change-working-directory
+         (add-before 'expand-load-path 'change-working-directory
            (lambda _ (chdir "editors") #t)))))
     (home-page "https://github.com/protocolbuffers/protobuf")
     (synopsis "Protocol buffers major mode for Emacs")
-- 
2.32.0


[-- Attachment #3: 0002-guix-Make-modify-phases-error-when-adding-before-aft.patch --]
[-- Type: text/x-diff, Size: 3948 bytes --]

From b272a65b12175a3d0354f18e19247f9ed72f5f8e Mon Sep 17 00:00:00 2001
From: Carlo Zancanaro <carlo@zancanaro.id.au>
Date: Wed, 23 Jun 2021 15:59:37 +1000
Subject: [PATCH 2/2] guix: Make modify-phases error when adding before/after a
 missing phase

* 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@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))
-- 
2.32.0

[bug#49181] [PATCH 0/1] modify-phases: error when encountering missing phase (was: Fix missing phases in Emacs builds)

[Leo Prikler]

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

[bug#49181] [PATCH 1/1] guix: Make modify-phases error when adding before/after a missing phase

[Leo Prikler]

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

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))
-- 
2.32.0

[bug#49181] Fix missing phases in Emacs builds

[Sarah Morgensen]

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

[bug#49181] [PATCH 1/1] guix: Make modify-phases error when adding before/after a missing phase

[Carlo Zancanaro]

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

[bug#49181] Fix missing phases in Emacs builds

[Maxim Cournoyer]

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!

-- 
Thanks,
Maxim

[bug#49181] [PATCH core-updates v2] guix: Make modify-phases error when adding before/after a missing phase

[Carlo Zancanaro]

* 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@gmail.com>
 ;;; Copyright © 2021, 2022 Maxime Devos <maximedevos@telenet.be>
 ;;; Copyright © 2021 Brendan Tildesley <mail@brendan.scot>
+;;; Copyright © 2023 Carlo Zancanaro <carlo@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

[bug#49181] Fix missing phases in Emacs builds

[Carlo Zancanaro]

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

[bug#49181] [PATCH core-updates v2] guix: Make modify-phases error when adding before/after a missing phase

[Carlo Zancanaro]

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

bug#49181: [PATCH core-updates v2] guix: Make modify-phases error when adding before/after a missing phase

[Maxim Cournoyer]

Hi,

Carlo Zancanaro <carlo@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