Fix missing phases in Emacs builds

  • Done
  • quality assurance status badge
Details
4 participants
  • Carlo Zancanaro
  • Sarah Morgensen
  • Leo Prikler
  • Maxim Cournoyer
Owner
unassigned
Submitted by
Carlo Zancanaro
Severity
normal
C
C
Carlo Zancanaro wrote on 23 Jun 2021 08:45
(address . guix-patches@gnu.org)
87k0mlaxkn.fsf@zancanaro.id.au
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
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(-)

Toggle diff (41 lines)
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
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(-)

Toggle diff (79 lines)
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
L
L
Leo Prikler wrote on 23 Jun 2021 09:25
[PATCH 0/1] modify-phases: error when encountering missing phase (was: Fix missing phases in Emacs builds)
(address . 49181@debbugs.gnu.org)(address . carlo@zancanaro.id.au)
20210623072509.7165-1-leo.prikler@student.tugraz.at
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
L
L
Leo Prikler wrote on 23 Jun 2021 09:25
[PATCH 1/1] guix: Make modify-phases error when adding before/after a missing phase
(address . 49181@debbugs.gnu.org)(address . carlo@zancanaro.id.au)
20210623072509.7165-2-leo.prikler@student.tugraz.at
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(-)

Toggle diff (79 lines)
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
S
S
Sarah Morgensen wrote on 23 Jul 2021 23:32
Re: bug#49181: Fix missing phases in Emacs builds
(name . Leo Prikler)(address . leo.prikler@student.tugraz.at)
867dhgra1z.fsf_-_@mgsn.dev
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."
Toggle quote (16 lines)
> (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))))))

Toggle quote (22 lines)
>
> (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
C
C
Carlo Zancanaro wrote on 2 Nov 2021 23:58
Re: [PATCH 1/1] guix: Make modify-phases error when adding before/after a missing phase
(address . 49181@debbugs.gnu.org)(name . Leo Prikler)(address . leo.prikler@student.tugraz.at)
87ilxa5fev.fsf@zancanaro.id.au
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:
Toggle quote (12 lines)
> 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(-)
>
> ...
M
M
Maxim Cournoyer wrote on 29 Mar 2023 04:18
Re: bug#49181: Fix missing phases in Emacs builds
(name . Carlo Zancanaro)(address . carlo@zancanaro.id.au)
87cz4s47wj.fsf_-_@gmail.com
Hi Carlo,

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

Toggle quote (5 lines)
> 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
C
C
Carlo Zancanaro wrote on 20 May 2023 15:05
[PATCH core-updates v2] guix: Make modify-phases error when adding before/after a missing phase
(address . 49181@debbugs.gnu.org)(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)
ce2c9e10e21b105396225081718a82a139c0334f.1684587905.git.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(-)

Toggle diff (81 lines)
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
C
C
Carlo Zancanaro wrote on 20 May 2023 15:13
Re: bug#49181: Fix missing phases in Emacs builds
(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)(address . 49181@debbugs.gnu.org)
875y8n2lb3.fsf@zancanaro.id.au
Hi Maxim,

On Tue, Mar 28 2023, Maxim Cournoyer wrote:
Toggle quote (3 lines)
> 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
C
C
Carlo Zancanaro wrote on 10 Oct 2023 02:50
Re: [bug#49181] [PATCH core-updates v2] guix: Make modify-phases error when adding before/after a missing phase
(address . 49181@debbugs.gnu.org)(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)
87o7h79tx7.fsf@zancanaro.id.au
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:
Toggle quote (9 lines)
> * 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(-)
> ...
M
M
Maxim Cournoyer wrote on 10 Oct 2023 05:37
Re: bug#49181: [PATCH core-updates v2] guix: Make modify-phases error when adding before/after a missing phase
(name . Carlo Zancanaro)(address . carlo@zancanaro.id.au)(address . 49181-done@debbugs.gnu.org)
87h6mz6tc5.fsf_-_@gmail.com
Hi,

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

Toggle quote (4 lines)
> * 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
Closed
?