diff mbox series

[bug#49181] Fix missing phases in Emacs builds

Message ID 87k0mlaxkn.fsf@zancanaro.id.au
State New
Headers show
Series [bug#49181] Fix missing phases in Emacs builds | expand

Checks

Context Check Description
cbaines/comparison success View comparision
cbaines/git branch success View Git branch
cbaines/applying patch fail View Laminar job
cbaines/issue success View issue

Commit Message

Carlo Zancanaro June 23, 2021, 6:45 a.m. UTC 
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

Comments

Leo Prikler June 23, 2021, 7:25 a.m. UTC | #1 
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
diff mbox series

Patch

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