[PATCH core-updates 1/?] build: substitute: Don't fail silently.

  • Open
  • quality assurance status badge
Details
5 participants
  • Jakub K?dzio?ka
  • Ludovic Courtès
  • Maxim Cournoyer
  • Bruno Victal
  • Simon Tournier
Owner
unassigned
Submitted by
Jakub K?dzio?ka
Severity
normal
J
J
Jakub K?dzio?ka wrote on 1 Jul 2020 00:09
(address . guix-patches@gnu.org)(address . mbakke@fastmail.com)
20200630220913.14665-1-kuba@kadziolka.net
* guix/build/utils.scm (substitute, substitute*)[require-matches?]: New
argument.
---
Of course, this will require some changes in the build recipes. However,
many of the bugs I have seen before could've been prevented by this
behavior, so I believe it is worth it.

I am currently running 'guix build hello' with these changes; I will
send any follow-up fixes needed to this bug#.

guix/build/utils.scm | 47 +++++++++++++++++++++++++++++++++-----------
1 file changed, 35 insertions(+), 12 deletions(-)

Toggle diff (96 lines)
diff --git a/guix/build/utils.scm b/guix/build/utils.scm
index dc55c6745d..1bfb774c60 100644
--- a/guix/build/utils.scm
+++ b/guix/build/utils.scm
@@ -6,6 +6,7 @@
;;; Copyright © 2018 Arun Isaac <arunisaac@systemreboot.net>
;;; Copyright © 2018, 2019 Ricardo Wurmus <rekado@elephly.net>
;;; Copyright © 2020 Efraim Flashner <efraim@flashner.co.il>
+;;; Copyright © 2020 Jakub K?dzio?ka <kuba@kadziolka.net>
;;;
;;; This file is part of GNU Guix.
;;;
@@ -747,29 +748,42 @@ PROC's result is returned."
(lambda (key . args)
(false-if-exception (delete-file template))))))
-(define (substitute file pattern+procs)
+(define* (substitute file pattern+procs #:key (require-matches? #t))
"PATTERN+PROCS is a list of regexp/two-argument-procedure pairs. For each
line of FILE, and for each PATTERN that it matches, call the corresponding
PROC as (PROC LINE MATCHES); PROC must return the line that will be written as
a substitution of the original line. Be careful about using '$' to match the
-end of a line; by itself it won't match the terminating newline of a line."
- (let ((rx+proc (map (match-lambda
- (((? regexp? pattern) . proc)
- (cons pattern proc))
- ((pattern . proc)
- (cons (make-regexp pattern regexp/extended)
- proc)))
- pattern+procs)))
+end of a line; by itself it won't match the terminating newline of a line.
+
+By default, SUBSTITUTE will raise a &message condition if one of the patterns
+fails to match. If a lack of matches is acceptable, pass #:require-matches? #f
+to disable this check."
+ (let ((rx+proc (map (match-lambda
+ (((? regexp? pattern) . proc)
+ (cons* #f "<unknown>" pattern proc))
+ ((pattern . proc)
+ (cons* #f pattern (make-regexp pattern regexp/extended)
+ proc)))
+ pattern+procs)))
(with-atomic-file-replacement file
(lambda (in out)
(let loop ((line (read-line in 'concat)))
(if (eof-object? line)
- #t
+ (when require-matches?
+ (for-each
+ (match-lambda
+ ((#f pat . _)
+ (raise (condition
+ (&message
+ (message (format #f "substitute: ~a: pattern failed to match: ~a" file pat))))))
+ ((#t . _) #t))
+ rx+proc))
(let ((line (fold (lambda (r+p line)
(match r+p
- ((regexp . proc)
+ ((_ _ regexp . proc)
(match (list-matches regexp line)
((and m+ (_ _ ...))
+ (set-car! r+p #t)
(proc line m+))
(_ line)))))
line
@@ -814,9 +828,17 @@ match substring.
Alternatively, FILE may be a list of file names, in which case they are
all subject to the substitutions.
+By default, SUBSTITUTE* will raise a &message condition if one of the patterns
+fails to match on one of the files. If a lack of matches is acceptable,
+add #:require-matches? #f after FILE to disable this check.
+
Be careful about using '$' to match the end of a line; by itself it won't
match the terminating newline of a line."
((substitute* file ((regexp match-var ...) body ...) ...)
+ (substitute* file #:require-matches? #t
+ ((regexp match-var ...) body ...) ...))
+ ((substitute* file #:require-matches? require-matches?
+ ((regexp match-var ...) body ...) ...)
(let ()
(define (substitute-one-file file-name)
(substitute
@@ -840,7 +862,8 @@ match the terminating newline of a line."
(begin body ...)
(substring l o (match:start m))
r))))))))
- ...)))
+ ...)
+ #:require-matches? require-matches?))
(match file
((files (... ...))
--
2.26.2
J
J
Jakub K?dzio?ka wrote on 1 Jul 2020 00:11
[PATCH 2/?] build: bootstrap-configure: Allow lack of matches in substitute.
(address . 42146@debbugs.gnu.org)
20200630221121.16086-1-kuba@kadziolka.net
Matches are not required here, as not every file will use every
variable.

* guix/build/gnu-bootstrap.scm (bootstrap-configure): Pass
#:require-matches? #f to substitute*.
---
guix/build/gnu-bootstrap.scm | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)

Toggle diff (31 lines)
diff --git a/guix/build/gnu-bootstrap.scm b/guix/build/gnu-bootstrap.scm
index 1cb9dc5512..6ee520e301 100644
--- a/guix/build/gnu-bootstrap.scm
+++ b/guix/build/gnu-bootstrap.scm
@@ -1,5 +1,6 @@
;;; GNU Guix --- Functional package management for GNU
;;; Copyright © 2020 Timothy Sample <samplet@ngyro.com>
+;;; Copyright © 2020 Jakub K?dzio?ka <kuba@kadziolka.net>
;;;
;;; This file is part of GNU Guix.
;;;
@@ -49,7 +50,7 @@ and object directories."
(format #t "Configuring ~a~%" template)
(let ((target (string-drop-right template 3)))
(copy-file template target)
- (substitute* target
+ (substitute* target #:require-matches? #f
(("@VERSION@") version))))
(find-files modules
(lambda (fn st)
@@ -58,7 +59,7 @@ and object directories."
(format #t "Configuring ~a~%" template)
(let ((target (string-drop-right template 3)))
(copy-file template target)
- (substitute* target
+ (substitute* target #:require-matches? #f
(("@GUILE@") guile)
(("@MODDIR@") moddir)
(("@GODIR@") godir))
--
2.26.2
J
J
Jakub K?dzio?ka wrote on 1 Jul 2020 00:11
[PATCH 3/?] gnu: commencement: Build fix for %bootstrap-mes-rewired.
(address . 42146@debbugs.gnu.org)
20200630221134.16350-1-kuba@kadziolka.net
Not every file has to refer to all dependencies.

* gnu/packages/commencement.scm (%bootstrap-mes-rewired)[builder]: Allow
lack of matches in substitute*.
---
gnu/packages/commencement.scm | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

Toggle diff (23 lines)
diff --git a/gnu/packages/commencement.scm b/gnu/packages/commencement.scm
index ef250e037b..1c7cf3d810 100644
--- a/gnu/packages/commencement.scm
+++ b/gnu/packages/commencement.scm
@@ -8,6 +8,7 @@
;;; Copyright © 2018, 2019, 2020 Jan (janneke) Nieuwenhuizen <janneke@gnu.org>
;;; Copyright © 2019, 2020 Marius Bakke <mbakke@fastmail.com>
;;; Copyright © 2020 Timothy Sample <samplet@ngyro.com>
+;;; Copyright © 2020 Jakub K?dzio?ka <kuba@kadziolka.net>
;;;
;;; This file is part of GNU Guix.
;;;
@@ -283,7 +284,7 @@ pure Scheme to Tar and decompression in one easy step.")
(mescc (string-append bin "/mescc"))
(module (string-append out "/share/mes/module")))
(define (rewire file)
- (substitute* file
+ (substitute* file #:require-matches? #f
((mes) out)
(("/gnu/store[^ ]+mes-minimal-[^/)}\"]*") out)
(("/gnu/store[^ ]+guile-[^/]*/bin/guile") guile)
--
2.26.2
J
J
Jakub K?dzio?ka wrote on 1 Jul 2020 00:50
Re: [PATCH core-updates 1/?] build: substitute: Don't fail silently.
(address . 42146@debbugs.gnu.org)
20200630225007.tw2rcfbmzh3wvthf@gravity
The error messages I'm getting from this are quite indirect - there are
wrapped in quite a few layers. Is there a better way to report errors
inside a build utility?

ice-9/eval.scm:387:11: In procedure eval:
ice-9/eval.scm:387:11: Throw to key `srfi-34' with args `(#<condition &message [message: "substit ute: install.sh: pattern failed to match: -xf"] 11aa480>)'.
builder for `/gnu/store/wdyzzh7rkg47hfp434w72ly9nay1yva1-mes-boot-0.22.drv' failed with exit code 1

Regards,
Jakub K?dzio?ka
-----BEGIN PGP SIGNATURE-----

iQIzBAABCAAdFiEE5Xa/ss9usT31cTO54xWnWEYTFWQFAl77wZ8ACgkQ4xWnWEYT
FWRIhxAAhBBY1dtp5/7VfFJ2cKcDK8X1xTJucuTGpM5AflHMVarCBdba/7pofenb
/cpRR52LfhwQU2HMornopoRIeBnm7DR81vSE4A/V4XO7Z6+BhG2AXAU8biQ6Mzqu
1SH0ykadjyxZSOC+JZ4XfqSc/SDb79aIUh5O4Mw22xCir+2ed1zJcHrhjm4jOhMW
e3ZPoL1+WDhe26LYdhcjUXciBTf0TEhkb4ZCmHgdbe9cVaLq97gA25MsJRiV8Q1d
IoSgf7Fg5FUktl4xCZdMSQ/N3f5mkpS94P6X0EGZQRu3V2KaQH4Yut661W8X00Yr
F2OtlTu4RHivHk/sdkGWETb9W/EqAqquoSg+KENRzkbbASn2upS79/fG74kPooIq
ULNcSW0qCqbSUuELSBiRvP/G+AvSlrQ6w0Tfsur8KF2Rkh4f8m4hzCHkccUSP0FZ
DRsF1sjILlBDjjtb/dgm4uuWr7NOBUddp9s0DvCKpSdHVq9ms7Q9QNyRyZ7sEgzr
eqFkmF2Mv4IZWISy5K1XITWmL+ypctm8chceoQmwrhSU2YQ+2peiLykB7GxcymrB
UkkeULOJ2OZDE5Nl0Mgrz5WOxzN7AuH5C9FAE8JnNJ32mgZsgtPJ1GN4YpL4tmBv
KxwHdl55Rbc/PV3+oiVFnsZ6UOt/bL4wF9DqmPV25KXog63pNQ0=
=BRqx
-----END PGP SIGNATURE-----


J
J
Jakub K?dzio?ka wrote on 1 Jul 2020 02:42
[PATCH 4/?] gnu: mes-boot: Use verbosity settings integrated into buildsystem
(address . 42146@debbugs.gnu.org)
20200701004234.18050-1-kuba@kadziolka.net
* gnu/packages/commencement.scm (mes-boot): Set the V environment
variable. Don't substitute in the install script.
---
gnu/packages/commencement.scm | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)

Toggle diff (26 lines)
diff --git a/gnu/packages/commencement.scm b/gnu/packages/commencement.scm
index 1c7cf3d810..295a378d11 100644
--- a/gnu/packages/commencement.scm
+++ b/gnu/packages/commencement.scm
@@ -407,6 +407,7 @@ $MES -e '(mescc)' module/mescc.scm -- \"$@\"
(string-append
mes "/share/mes/module"
":" dir "/nyacc-0.99.0/module"))
+ (setenv "V" "1")
(invoke "gash" "configure.sh"
(string-append "--prefix=" out)
(string-append "--host=i686-linux-gnu")))))
@@ -416,11 +417,6 @@ $MES -e '(mescc)' module/mescc.scm -- \"$@\"
(delete 'check)
(replace 'install
(lambda _
- (substitute* "install.sh" ; show some progress
- ((" -xf") " -xvf")
- (("^( *)((cp|mkdir|tar) [^']*[^\\])\n" all space cmd)
- (string-append space "echo '" cmd "'\n"
- space cmd "\n")))
(invoke "sh" "install.sh")
;; Keep ASCII output, for friendlier comparison and bisection
(let* ((out (assoc-ref %outputs "out"))
--
2.26.2
J
J
Jakub K?dzio?ka wrote on 1 Jul 2020 02:42
[PATCH 5/?] build-system/gnu: Allow lack of matches in substitution phases.
(address . 42146@debbugs.gnu.org)
20200701004242.18245-1-kuba@kadziolka.net
* guix/build/utils.scm (patch-makefile-SHELL, patch-/usr/bin/file),
guix/build/gnu-build-system.scm (patch-dot-desktop-files):
Allow lack of matches in substitute*.
---
guix/build/gnu-build-system.scm | 3 ++-
guix/build/utils.scm | 4 ++--
2 files changed, 4 insertions(+), 3 deletions(-)

Toggle diff (45 lines)
diff --git a/guix/build/gnu-build-system.scm b/guix/build/gnu-build-system.scm
index 2e7dff2034..4b96761233 100644
--- a/guix/build/gnu-build-system.scm
+++ b/guix/build/gnu-build-system.scm
@@ -2,6 +2,7 @@
;;; Copyright © 2012, 2013, 2014, 2015, 2016, 2017, 2018, 2019 Ludovic Courtès <ludo@gnu.org>
;;; Copyright © 2018 Mark H Weaver <mhw@netris.org>
;;; Copyright © 2020 Brendan Tildesley <mail@brendan.scot>
+;;; Copyright © 2020 Jakub K?dzio?ka <kuba@kadziolka.net>
;;;
;;; This file is part of GNU Guix.
;;;
@@ -724,7 +725,7 @@ which cannot be found~%"
;; '.desktop' files contain translations and are always
;; UTF-8-encoded.
(with-fluids ((%default-port-encoding "UTF-8"))
- (substitute* files
+ (substitute* files #:require-matches? #f
(("^Exec=([^/[:blank:]\r\n]*)(.*)$" _ binary rest)
(string-append "Exec=" (which binary) rest))
(("^TryExec=([^/[:blank:]\r\n]*)(.*)$" _ binary rest)
diff --git a/guix/build/utils.scm b/guix/build/utils.scm
index 1bfb774c60..a8218a7743 100644
--- a/guix/build/utils.scm
+++ b/guix/build/utils.scm
@@ -1005,7 +1005,7 @@ When KEEP-MTIME? is true, the atime/mtime of FILE are kept unchanged."
(let ((st (stat file)))
;; Consider FILE is using an 8-bit encoding to avoid errors.
(with-fluids ((%default-port-encoding #f))
- (substitute* file
+ (substitute* file #:require-matches? #f
(("^ *SHELL[[:blank:]]*:?=[[:blank:]]*([[:graph:]]*/)([[:graph:]]+)(.*)$"
_ dir shell args)
(let* ((old (string-append dir shell))
@@ -1033,7 +1033,7 @@ no replacement 'file' command, doing nothing~%")
(let ((st (stat file)))
;; Consider FILE is using an 8-bit encoding to avoid errors.
(with-fluids ((%default-port-encoding #f))
- (substitute* file
+ (substitute* file #:require-matches? #f
(("/usr/bin/file")
(begin
(format (current-error-port)
--
2.26.2
J
J
Jakub K?dzio?ka wrote on 1 Jul 2020 11:55
[PATCH 6/6] gnu: glibc-mesboot0: Fixup the fixup-configure phase.
(address . 42146@debbugs.gnu.org)
20200701095523.9501-1-kuba@kadziolka.net
* gnu/packages/commencement.scm (glibc-mesboot0)[fixup-configure]: Don't
repeat substitution. Fix indentation.
---
This is an interesting cleanup commit. I believe this concludes a nice
sampling of the consequences of this patch, and thus subsequent fixups
are unlikely to benefit from a review. I will be pushing my work to


gnu/packages/commencement.scm | 24 +++++++++++-------------
1 file changed, 11 insertions(+), 13 deletions(-)

Toggle diff (37 lines)
diff --git a/gnu/packages/commencement.scm b/gnu/packages/commencement.scm
index 295a378d11..837f4e2ecc 100644
--- a/gnu/packages/commencement.scm
+++ b/gnu/packages/commencement.scm
@@ -1418,19 +1418,17 @@ ac_cv_c_float_format='IEEE (little-endian)'
(format (current-error-port)
"running ./configure ~a\n" (string-join configure-flags))
(apply invoke "./configure" configure-flags)))
- (add-after 'configure 'fixup-configure
- (lambda _
- (let* ((out (assoc-ref %outputs "out"))
- (bash (assoc-ref %build-inputs "bash"))
- (shell (string-append bash "/bin/bash")))
- (substitute* "config.make"
- (("INSTALL = scripts/") "INSTALL = $(..)./scripts/"))
- (substitute* "config.make"
- (("INSTALL = scripts/") "INSTALL = $(..)./scripts/")
- (("BASH = ") (string-append
- "SHELL = " shell "
- BASH = ")))
- #t))))))))
+ (add-after 'configure 'fixup-configure
+ (lambda _
+ (let* ((out (assoc-ref %outputs "out"))
+ (bash (assoc-ref %build-inputs "bash"))
+ (shell (string-append bash "/bin/bash")))
+ (substitute* "config.make"
+ (("INSTALL = scripts/") "INSTALL = $(..)./scripts/")
+ (("BASH = ") (string-append
+ "SHELL = " shell "
+BASH = ")))
+ #t))))))))
(define gcc-mesboot0
(package
--
2.26.2
M
M
Maxim Cournoyer wrote on 19 Oct 2023 15:14
Re: bug#42146: [PATCH core-updates 1/?] build: substitute: Don't fail silently.
(name . Jakub K?dzio?ka)(address . kuba@kadziolka.net)(address . 42146@debbugs.gnu.org)
87wmvioit0.fsf_-_@gmail.com
Hi Jakub,

Sorry for the late reply; nice series!

Jakub K?dzio?ka <kuba@kadziolka.net> writes:

Toggle quote (8 lines)
> The error messages I'm getting from this are quite indirect - there are
> wrapped in quite a few layers. Is there a better way to report errors
> inside a build utility?
>
> ice-9/eval.scm:387:11: In procedure eval:
> ice-9/eval.scm:387:11: Throw to key `srfi-34' with args `(#<condition &message [message: "substit ute: install.sh: pattern failed to match: -xf"] 11aa480>)'.
> builder for `/gnu/store/wdyzzh7rkg47hfp434w72ly9nay1yva1-mes-boot-0.22.drv' failed with exit code 1

I don't think it's possible currently; I guess we'd have to serialize
the exceptions in the builder to recover them on the host to present
them in a nicer fashion.

--
Thanks,
Maxim
M
M
Maxim Cournoyer wrote on 19 Oct 2023 22:33
[PATCH 1/3] build: Relocate <regexp*> record and associated procedures here.
(address . 42146@debbugs.gnu.org)(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)
ff182177822369c7d31698ecbf0cb5dcbca37644.1697747385.git.maxim.cournoyer@gmail.com
* etc/teams.scm.in (<regexp*>, make-regexp*, regexp*-exec): Move to...
* guix/build/utils.scm: ... here.
(list-matches*): New procedure.

Change-Id: I566ac372f7d8ba08de94e19b54dcc68da2106a23
---
etc/teams.scm.in | 19 +------------------
guix/build/utils.scm | 39 +++++++++++++++++++++++++++++++++++++++
2 files changed, 40 insertions(+), 18 deletions(-)

Toggle diff (110 lines)
diff --git a/etc/teams.scm.in b/etc/teams.scm.in
index 55242caad1..8af25b9802 100644
--- a/etc/teams.scm.in
+++ b/etc/teams.scm.in
@@ -34,29 +34,12 @@
(srfi srfi-9)
(srfi srfi-26)
(ice-9 format)
- (ice-9 regex)
(ice-9 match)
(ice-9 rdelim)
+ (guix build utils)
(guix ui)
(git))
-(define-record-type <regexp*>
- (%make-regexp* pat flag rx)
- regexp*?
- (pat regexp*-pattern)
- (flag regexp*-flag)
- (rx regexp*-rx))
-
-;;; Work around regexp implementation.
-;;; This record allows to track the regexp pattern and then display it.
-(define* (make-regexp* pat #:optional (flag regexp/extended))
- "Alternative to `make-regexp' producing annotated <regexp*> objects."
- (%make-regexp* pat flag (make-regexp pat flag)))
-
-(define (regexp*-exec rx* str)
- "Execute the RX* regexp, a <regexp*> object."
- (regexp-exec (regexp*-rx rx*) str))
-
(define-record-type <team>
(make-team id name description members scope)
team?
diff --git a/guix/build/utils.scm b/guix/build/utils.scm
index 8e630ad586..2b3a8e278b 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 © 2022 Simon Tournier <zimon.toutoune@gmail.com>
;;; Copyright © 2023 Carlo Zancanaro <carlo@zancanaro.id.au>
;;;
;;; This file is part of GNU Guix.
@@ -28,6 +29,7 @@
(define-module (guix build utils)
#:use-module (srfi srfi-1)
+ #:use-module (srfi srfi-9)
#:use-module (srfi srfi-11)
#:use-module (srfi srfi-26)
#:use-module (srfi srfi-34)
@@ -55,6 +57,14 @@ (define-module (guix build utils)
package-name->name+version
parallel-job-count
+ make-regexp*
+ regexp*-exec
+ regexp*?
+ regexp*-pattern
+ regexp*-flag
+ regexp*-rx
+ list-matches*
+
compressor
tarball?
%xz-parallel-args
@@ -163,6 +173,35 @@ (define-syntax-rule (define-constant name val)
(module-replace! (current-module) '(setvbuf)))
(else #f))
+
+;;;
+;;; Extend regexp objects with a pattern field.
+;;;
+(define-record-type <regexp*>
+ (%make-regexp* pat flag rx)
+ regexp*?
+ (pat regexp*-pattern) ;the regexp pattern, a string
+ (flag regexp*-flag) ;regexp flags
+ (rx regexp*-rx)) ;the compiled regexp object
+
+;;; Work around regexp implementation.
+;;; This record allows to track the regexp pattern and then display it.
+(define* (make-regexp* pat #:optional (flag regexp/extended))
+ "Alternative to `make-regexp' producing annotated <regexp*> objects."
+ (%make-regexp* pat flag (make-regexp pat flag)))
+
+(define (regexp*-exec rx* str)
+ "Execute the RX* regexp, a <regexp*> object."
+ (regexp-exec (regexp*-rx rx*) str))
+
+(define* (list-matches* regexp str #:optional (flags regexp/extended))
+ "Like 'list-matches', but also accepting a regexp* as REGEXP."
+ (match regexp
+ ((or (? string?) (? regexp?))
+ (list-matches regexp str flags))
+ ((? regexp*?)
+ (list-matches (regexp*-rx regexp) str flags))))
+
;;;
;;; Compression helpers.

base-commit: d59653b7c9e43ebdbba20e2ca071429507f94c67
--
2.41.0
M
M
Maxim Cournoyer wrote on 19 Oct 2023 22:33
[PATCH 2/3] build: substitute: Error when no substitutions were done.
(address . 42146@debbugs.gnu.org)
cafb034c8454eff36ab6f8c40df6fc1699915923.1697747385.git.maxim.cournoyer@gmail.com
From: Jakub K?dzio?ka <kuba@kadziolka.net>

* guix/build/utils.scm (substitute, substitute*)
[require-matches?]: New argument.
* tests/build-utils.scm ("substitute*"): New test group.
("substitute*, no match error")
("substitute*, partial no match error"): New tests.

Co-authored-by: Maxim Cournoyer <maxim.cournoyer@gmail.com>
Change-Id: I66ed33d72aa73cd35e5642521efec70bf756f86e
---
guix/build/utils.scm | 94 +++++++++++++++++++++++++++++++++----------
tests/build-utils.scm | 63 +++++++++++++++++++----------
2 files changed, 114 insertions(+), 43 deletions(-)

Toggle diff (230 lines)
diff --git a/guix/build/utils.scm b/guix/build/utils.scm
index 2b3a8e278b..7bfb6560e1 100644
--- a/guix/build/utils.scm
+++ b/guix/build/utils.scm
@@ -6,7 +6,8 @@
;;; Copyright © 2018, 2022 Arun Isaac <arunisaac@systemreboot.net>
;;; Copyright © 2018, 2019 Ricardo Wurmus <rekado@elephly.net>
;;; Copyright © 2020 Efraim Flashner <efraim@flashner.co.il>
-;;; Copyright © 2020, 2021 Maxim Cournoyer <maxim.cournoyer@gmail.com>
+;;; Copyright © 2020 Jakub K?dzio?ka <kuba@kadziolka.net>
+;;; Copyright © 2020, 2021, 2023 Maxim Cournoyer <maxim.cournoyer@gmail.com>
;;; Copyright © 2021, 2022 Maxime Devos <maximedevos@telenet.be>
;;; Copyright © 2021 Brendan Tildesley <mail@brendan.scot>
;;; Copyright © 2022 Simon Tournier <zimon.toutoune@gmail.com>
@@ -971,24 +972,53 @@ (define (replace-char c1 c2 s)
c))
s)))
-(define (substitute file pattern+procs)
+(define* (substitute file pattern+procs #:key (require-matches? #t))
"PATTERN+PROCS is a list of regexp/two-argument-procedure pairs. For each
line of FILE, and for each PATTERN that it matches, call the corresponding
PROC as (PROC LINE MATCHES); PROC must return the line that will be written as
a substitution of the original line. Be careful about using '$' to match the
-end of a line; by itself it won't match the terminating newline of a line."
- (let ((rx+proc (map (match-lambda
- (((? regexp? pattern) . proc)
+end of a line; by itself it won't match the terminating newline of a line.
+
+By default, SUBSTITUTE will raise a &message condition if one of the patterns
+fails to match. REQUIRE-MATCHES? can be set to false when lack of matches is
+acceptable (e.g. if you have multiple potential patterns not guaranteed to be
+found in FILE)."
+ (define (rx->pattern m)
+ (match m
+ ((? regexp? pattern)
+ "<unknown pattern (regexp)>")
+ ((? regexp*? pattern)
+ (regexp*-pattern pattern))
+ ((? string? pattern)
+ pattern)))
+
+ (define (make-condition failed-matches)
+ (condition
+ (&message
+ (message (format #f "substitute: `~a': no match for patterns `~a'"
+ file failed-matches)))))
+
+ (let ((rx+proc (map (match-lambda
+ (((or (? regexp? pattern) (? regexp*? pattern)) . proc)
(cons pattern proc))
((pattern . proc)
- (cons (make-regexp pattern regexp/extended)
- proc)))
- pattern+procs)))
+ (cons (make-regexp* pattern regexp/extended) proc)))
+ pattern+procs)))
(with-atomic-file-replacement file
(lambda (in out)
- (let loop ((line (read-line in 'concat)))
+ (let loop ((line (read-line in 'concat))
+ (ok-matches '())
+ (failed-matches '()))
(if (eof-object? line)
- #t
+ (when require-matches?
+ (let ((failed-patterns (lset-difference
+ string=?
+ (delete-duplicates
+ (map rx->pattern failed-matches))
+ (delete-duplicates
+ (map rx->pattern ok-matches)))))
+ (when (not (null? failed-patterns))
+ (raise (make-condition failed-patterns)))))
;; Work around the fact that Guile's regexp-exec does not handle
;; NUL characters (a limitation of the underlying GNU libc's
;; regexec) by temporarily replacing them by an unused private
@@ -998,19 +1028,30 @@ (define (substitute file pattern+procs)
(unused-private-use-code-point line))
#\nul))
(line* (replace-char #\nul nul* line))
- (line1* (fold (lambda (r+p line)
- (match r+p
- ((regexp . proc)
- (match (list-matches regexp line)
- ((and m+ (_ _ ...))
- (proc line m+))
- (_ line)))))
- line*
- rx+proc))
+ (results ;line, ok-matches and failed-matches
+ (fold (lambda (r+p results)
+ (let ((line (first results))
+ (ok-matches (second results))
+ (failed-matches (third results)))
+ (match r+p
+ ((regexp . proc)
+ (match (list-matches* regexp line)
+ ((and m+ (_ _ ...))
+ (list (proc line m+)
+ (cons regexp ok-matches)
+ failed-matches))
+ (_
+ (list line
+ ok-matches
+ (cons regexp failed-matches))))))))
+ (list line* '() '())
+ rx+proc))
+ (line1* (first results))
+ (ok-matches (second results))
+ (failed-matches (third results))
(line1 (replace-char nul* #\nul line1*)))
(display line1 out)
- (loop (read-line in 'concat)))))))))
-
+ (loop (read-line in 'concat) ok-matches failed-matches))))))))
(define-syntax let-matches
;; Helper macro for `substitute*'.
@@ -1048,9 +1089,17 @@ (define-syntax substitute*
Alternatively, FILE may be a list of file names, in which case they are
all subject to the substitutions.
+By default, SUBSTITUTE* will raise a &message condition if one of the patterns
+fails to match on one of the files; REQUIRE-MATCHES? may be set to false to
+avoid an error being raised in such condition.
+
Be careful about using '$' to match the end of a line; by itself it won't
match the terminating newline of a line."
((substitute* file ((regexp match-var ...) body ...) ...)
+ (substitute* file #:require-matches? #t
+ ((regexp match-var ...) body ...) ...))
+ ((substitute* file #:require-matches? require-matches?
+ ((regexp match-var ...) body ...) ...)
(let ()
(define (substitute-one-file file-name)
(substitute
@@ -1074,7 +1123,8 @@ (define-syntax substitute*
(begin body ...)
(substring l o (match:start m))
r))))))))
- ...)))
+ ...)
+ #:require-matches? require-matches?))
(match file
((files (... ...))
diff --git a/tests/build-utils.scm b/tests/build-utils.scm
index 3babf5d544..890fbca16f 100644
--- a/tests/build-utils.scm
+++ b/tests/build-utils.scm
@@ -1,7 +1,7 @@
;;; GNU Guix --- Functional package management for GNU
;;; Copyright © 2012, 2015, 2016, 2019, 2020 Ludovic Courtès <ludo@gnu.org>
;;; Copyright © 2019 Ricardo Wurmus <rekado@elephly.net>
-;;; Copyright © 2021 Maxim Cournoyer <maxim.cournoyer@gmail.com>
+;;; Copyright © 2021, 2023 Maxim Cournoyer <maxim.cournoyer@gmail.com>
;;; Copyright © 2021 Maxime Devos <maximedevos@telenet.be>
;;; Copyright © 2021 Brendan Tildesley <mail@brendan.scot>
;;;
@@ -289,26 +289,47 @@ (define (arg-test bash-args)
(test-assert "wrap-script, argument handling, bash --norc"
(arg-test " --norc"))
-(test-equal "substitute*, text contains a NUL byte, UTF-8"
- "c\0d"
- (with-fluids ((%default-port-encoding "UTF-8")
- (%default-port-conversion-strategy 'error))
- ;; The GNU libc is locale sensitive. Depending on the value of LANG, the
- ;; test could fail with "string contains #\\nul character: ~S" or "cannot
- ;; convert wide string to output locale".
- (setlocale LC_ALL "en_US.UTF-8")
- (call-with-temporary-output-file
- (lambda (file port)
- (format port "a\0b")
- (flush-output-port port)
-
- (substitute* file
- (("a") "c")
- (("b") "d"))
-
- (with-input-from-file file
- (lambda _
- (get-string-all (current-input-port))))))))
+(test-group "substitute*"
+ (define-syntax-rule (define-substitute*-test test-type name expected
+ content clauses ...)
+ (test-type
+ name
+ expected
+ (with-fluids ((%default-port-encoding "UTF-8")
+ (%default-port-conversion-strategy 'error))
+ ;; The GNU libc is locale sensitive. Depending on the value of LANG,
+ ;; the test could fail with "string contains #\\nul character: ~S" or
+ ;; "cannot convert wide string to output locale".
+ (setlocale LC_ALL "en_US.UTF-8")
+ (call-with-temporary-output-file
+ (lambda (file port)
+ (format port content)
+ (flush-output-port port)
+
+ (substitute* file
+ clauses ...)
+
+ (with-input-from-file file
+ (lambda _
+ (get-string-all (current-input-port)))))))))
+
+ (define-substitute*-test test-equal
+ "substitute*, text contains a NUL byte, UTF-8"
+ "c\0d" ;expected
+ "a\0b" ;content
+ (("a") "c")
+ (("b") "d"))
+
+ (define-substitute*-test test-error "substitute*, no match error"
+ #t ;expected
+ "a\0b" ;content
+ (("Oops!") "c"))
+
+ (define-substitute*-test test-error "substitute*, partial no match error"
+ #t ;expected
+ "a\0b" ;content
+ (("a") "c"
+ ("Oops!") "c")))
(test-equal "search-input-file: exception if not found"
`((path)
--
2.41.0
M
M
Maxim Cournoyer wrote on 19 Oct 2023 22:33
[PATCH 3/3] build: bootstrap-configure: Allow lack of matches in substitute.
(address . 42146@debbugs.gnu.org)
28fd2bd2039b4a4c0fe8fdaf1adbfb5956f683d5.1697747385.git.maxim.cournoyer@gmail.com
From: Jakub K?dzio?ka <kuba@kadziolka.net>

Matches are not required here, as not every file will use every
variable.

* guix/build/gnu-bootstrap.scm (bootstrap-configure): Pass
#:require-matches? #f to substitute*.

Signed-off-by: Maxim Cournoyer <maxim.cournoyer@gmail.com>
---
guix/build/gnu-bootstrap.scm | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)

Toggle diff (38 lines)
diff --git a/guix/build/gnu-bootstrap.scm b/guix/build/gnu-bootstrap.scm
index b4257a3717..d044c8acd9 100644
--- a/guix/build/gnu-bootstrap.scm
+++ b/guix/build/gnu-bootstrap.scm
@@ -1,5 +1,6 @@
;;; GNU Guix --- Functional package management for GNU
;;; Copyright © 2020, 2022 Timothy Sample <samplet@ngyro.com>
+;;; Copyright © 2020 Jakub K?dzio?ka <kuba@kadziolka.net>
;;;
;;; This file is part of GNU Guix.
;;;
@@ -51,6 +52,7 @@ (define (bootstrap-configure name version modules scripts)
(let ((target (string-drop-right template 3)))
(copy-file template target)
(substitute* target
+ #:require-matches? #f
(("@PACKAGE_NAME@") name)
(("@VERSION@") version))))
(append-map (lambda (dir) (find-files dir "\\.in$"))
@@ -60,14 +62,14 @@ (define (bootstrap-configure name version modules scripts)
(let ((target (string-drop-right template 3)))
(copy-file template target)
(substitute* target
+ #:require-matches? #f
(("@GUILE@") guile)
(("@MODDIR@") moddir)
(("@GODIR@") godir))
(chmod target #o755)))
(find-files scripts
(lambda (fn st)
- (string-suffix? ".in" fn))))
- #t)))
+ (string-suffix? ".in" fn)))))))
(define (bootstrap-build modules)
"Create a procedure that builds an early bootstrap package. The
--
2.41.0
L
L
Ludovic Courtès wrote on 19 Oct 2023 22:40
Re: bug#42146: [PATCH core-updates 1/?] build: substitute: Don't fail silently.
(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)
87lebyxs4y.fsf_-_@gnu.org
Hi,

Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:

Toggle quote (5 lines)
> * etc/teams.scm.in (<regexp*>, make-regexp*, regexp*-exec): Move to...
> * guix/build/utils.scm: ... here.
> (list-matches*): New procedure.


[...]

Toggle quote (2 lines)
> +++ b/guix/build/utils.scm

[...]

Toggle quote (14 lines)
> +;;;
> +;;; Extend regexp objects with a pattern field.
> +;;;
> +(define-record-type <regexp*>
> + (%make-regexp* pat flag rx)
> + regexp*?
> + (pat regexp*-pattern) ;the regexp pattern, a string
> + (flag regexp*-flag) ;regexp flags
> + (rx regexp*-rx)) ;the compiled regexp object
> +
> +;;; Work around regexp implementation.
> +;;; This record allows to track the regexp pattern and then display it.
> +(define* (make-regexp* pat #:optional (flag regexp/extended))

I’m skeptical about the concrete benefits. I would not include it in
(guix build utils), or at least not in this patch series.

(I tend to be super conservative about (guix build utils) because we
rarely get a chance to change it.)

Ludo’.
L
L
Ludovic Courtès wrote on 19 Oct 2023 22:49
(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)
878r7yxrpz.fsf_-_@gnu.org
Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:

Toggle quote (11 lines)
> From: Jakub K?dzio?ka <kuba@kadziolka.net>
>
> * guix/build/utils.scm (substitute, substitute*)
> [require-matches?]: New argument.
> * tests/build-utils.scm ("substitute*"): New test group.
> ("substitute*, no match error")
> ("substitute*, partial no match error"): New tests.
>
> Co-authored-by: Maxim Cournoyer <maxim.cournoyer@gmail.com>
> Change-Id: I66ed33d72aa73cd35e5642521efec70bf756f86e

[...]

Toggle quote (5 lines)
> -(define (substitute file pattern+procs)
> +(define* (substitute file pattern+procs #:key (require-matches? #t))
> "PATTERN+PROCS is a list of regexp/two-argument-procedure pairs. For each
> line of FILE, and for each PATTERN that it matches, call the corresponding

As discussed on IRC recently, I’d suggest:

#:key (require-matches? (%substitute-requires-matches?))

where:

(define %substitute-requires-matches?
(make-parameter #t))

That way it’ll be easier to change the default for entire builds if we
need to.

Toggle quote (8 lines)
> + (when require-matches?
> + (let ((failed-patterns (lset-difference
> + string=?
> + (delete-duplicates
> + (map rx->pattern failed-matches))
> + (delete-duplicates
> + (map rx->pattern ok-matches)))))

That’s potentially costly.

Would it be enough to thread a list of unmatched regexps, and to
(delq rx unmatched) every time RX is matched? (This is O(N) but N, the
number of regexps, is typically a handful.)

Then at the end, we’d check whether UNMATCHED is empty.

Toggle quote (3 lines)
> + (when (not (null? failed-patterns))
> + (raise (make-condition failed-patterns)))))

SRFI-35 ‘make-condition’ expects different arguments.

Should probably be: (raise (condition (&substitute-error …))).

Toggle quote (3 lines)
> + ((substitute* file #:require-matches? require-matches?
> + ((regexp match-var ...) body ...) ...)

Maybe rather:

(substitute* file
((regexp match-var ...) body ...) ...
#:require-matches? require-matches?)

That way we formatting remains unchanged.

Toggle quote (2 lines)
> +(test-group "substitute*"

I’d avoid groups: they’re not super useful and the output with the
Automake driver is terrible.

Ludo’.
L
L
Ludovic Courtès wrote on 19 Oct 2023 22:51
(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)
874jimxrmr.fsf_-_@gnu.org
Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:

Toggle quote (10 lines)
> From: Jakub K?dzio?ka <kuba@kadziolka.net>
>
> Matches are not required here, as not every file will use every
> variable.
>
> * guix/build/gnu-bootstrap.scm (bootstrap-configure): Pass
> #:require-matches? #f to substitute*.
>
> Signed-off-by: Maxim Cournoyer <maxim.cournoyer@gmail.com>

To me these two special cases suggest we may discover other more cases
where it’s okay to not match and we’d think. We’ll need a dedicated
branch built on ci.guix to assess that.

Thanks for working on it!

Ludo’.
M
M
Maxim Cournoyer wrote on 20 Oct 2023 01:54
(name . Ludovic Courtès)(address . ludo@gnu.org)
87pm1anp6a.fsf_-_@gmail.com
Hi Ludo,

Ludovic Courtès <ludo@gnu.org> writes:

Toggle quote (35 lines)
> Hi,
>
> Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:
>
>> * etc/teams.scm.in (<regexp*>, make-regexp*, regexp*-exec): Move to...
>> * guix/build/utils.scm: ... here.
>> (list-matches*): New procedure.
>
>
> [...]
>
>> +++ b/guix/build/utils.scm
>
> [...]
>
>> +;;;
>> +;;; Extend regexp objects with a pattern field.
>> +;;;
>> +(define-record-type <regexp*>
>> + (%make-regexp* pat flag rx)
>> + regexp*?
>> + (pat regexp*-pattern) ;the regexp pattern, a string
>> + (flag regexp*-flag) ;regexp flags
>> + (rx regexp*-rx)) ;the compiled regexp object
>> +
>> +;;; Work around regexp implementation.
>> +;;; This record allows to track the regexp pattern and then display it.
>> +(define* (make-regexp* pat #:optional (flag regexp/extended))
>
> I’m skeptical about the concrete benefits. I would not include it in
> (guix build utils), or at least not in this patch series.
>
> (I tend to be super conservative about (guix build utils) because we
> rarely get a chance to change it.)

The original users are substitute* and the teams.scm script. Since
substitute* is from (guix build utils), it makes sense to add it there
as well, since they are coupled.

The benefit is concrete: it makes it possible to show which regexp
pattern failed to match (its textual representation), instead of
something much less useful such as a generic placeholder such as
"<unknown> (regexp)".

It's in actual use if you look at the definition of substitute:

Toggle snippet (9 lines)
(let ((rx+proc (map (match-lambda
(((or (? regexp? pattern) (? regexp*? pattern)) . proc)
(cons pattern proc))
((pattern . proc)
(cons (make-regexp* pattern regexp/extended) proc)))
pattern+procs)))

The previous version followed a different approach, annotating the
rx+proc list with the raw pattern; I think the approach here is a bit
cleaner, and it should also enable users to pass pre-computed regexp*
objects to substitute* and have useful error messages produced.

--
Thanks,
Maxim
M
M
Maxim Cournoyer wrote on 20 Oct 2023 02:57
[PATCH v3 2/3] build: substitute: Error when no substitutions were done.
(address . 42146@debbugs.gnu.org)
d80dd0e1134cf608c69f3aaa0769f95a37f35e55.1697763444.git.maxim.cournoyer@gmail.com
From: Jakub K?dzio?ka <kuba@kadziolka.net>

* guix/build/utils.scm (substitute, substitute*)
[require-matches?]: New argument.
* tests/build-utils.scm ("substitute*"): New test group.
("substitute*, no match error")
("substitute*, partial no match error"): New tests.

Co-authored-by: Maxim Cournoyer <maxim.cournoyer@gmail.com>
Change-Id: I66ed33d72aa73cd35e5642521efec70bf756f86e
---
guix/build/utils.scm | 93 +++++++++++++++++++++++++++++++++----------
tests/build-utils.scm | 68 +++++++++++++++++++++----------
2 files changed, 118 insertions(+), 43 deletions(-)

Toggle diff (243 lines)
diff --git a/guix/build/utils.scm b/guix/build/utils.scm
index 2b3a8e278b..8e4b8321dd 100644
--- a/guix/build/utils.scm
+++ b/guix/build/utils.scm
@@ -6,7 +6,8 @@
;;; Copyright © 2018, 2022 Arun Isaac <arunisaac@systemreboot.net>
;;; Copyright © 2018, 2019 Ricardo Wurmus <rekado@elephly.net>
;;; Copyright © 2020 Efraim Flashner <efraim@flashner.co.il>
-;;; Copyright © 2020, 2021 Maxim Cournoyer <maxim.cournoyer@gmail.com>
+;;; Copyright © 2020 Jakub K?dzio?ka <kuba@kadziolka.net>
+;;; Copyright © 2020, 2021, 2023 Maxim Cournoyer <maxim.cournoyer@gmail.com>
;;; Copyright © 2021, 2022 Maxime Devos <maximedevos@telenet.be>
;;; Copyright © 2021 Brendan Tildesley <mail@brendan.scot>
;;; Copyright © 2022 Simon Tournier <zimon.toutoune@gmail.com>
@@ -111,8 +112,14 @@ (define-module (guix build utils)
modify-phases
with-atomic-file-replacement
+ %substitute-requires-matches?
substitute
substitute*
+ &substitute-error
+ substitute-error?
+ substitute-error-file
+ substitute-error-patterns
+
dump-port
set-file-time
patch-shebang
@@ -971,24 +978,51 @@ (define (replace-char c1 c2 s)
c))
s)))
-(define (substitute file pattern+procs)
+(define-condition-type &substitute-error &error
+ substitute-error?
+ (file substitute-error-file)
+ (patterns substitute-error-patterns))
+
+(define %substitute-requires-matches?
+ (make-parameter #t))
+
+(define* (substitute file pattern+procs
+ #:key (require-matches? (%substitute-requires-matches?)))
"PATTERN+PROCS is a list of regexp/two-argument-procedure pairs. For each
line of FILE, and for each PATTERN that it matches, call the corresponding
PROC as (PROC LINE MATCHES); PROC must return the line that will be written as
a substitution of the original line. Be careful about using '$' to match the
-end of a line; by itself it won't match the terminating newline of a line."
- (let ((rx+proc (map (match-lambda
- (((? regexp? pattern) . proc)
+end of a line; by itself it won't match the terminating newline of a line.
+
+By default, SUBSTITUTE will raise a &substitute-error condition if one of the
+patterns fails to match. REQUIRE-MATCHES? can be set to false when lack of
+matches is acceptable (e.g. if you have multiple potential patterns not
+guaranteed to be found in FILE)."
+ (define (rx->pattern m)
+ (match m
+ ((? regexp? pattern)
+ "<unknown pattern (regexp)>")
+ ((? regexp*? pattern)
+ (regexp*-pattern pattern))
+ ((? string? pattern)
+ pattern)))
+
+ (let ((rx+proc (map (match-lambda
+ (((or (? regexp? pattern) (? regexp*? pattern)) . proc)
(cons pattern proc))
((pattern . proc)
- (cons (make-regexp pattern regexp/extended)
- proc)))
- pattern+procs)))
+ (cons (make-regexp* pattern regexp/extended) proc)))
+ pattern+procs)))
(with-atomic-file-replacement file
(lambda (in out)
- (let loop ((line (read-line in 'concat)))
+ (let loop ((line (read-line in 'concat))
+ (unmatched-regexps (map first rx+proc)))
(if (eof-object? line)
- #t
+ (when (and require-matches? (not (null? unmatched-regexps)))
+ (raise (condition
+ (&substitute-error
+ (file file)
+ (patterns (map rx->pattern unmatched-regexps))))))
;; Work around the fact that Guile's regexp-exec does not handle
;; NUL characters (a limitation of the underlying GNU libc's
;; regexec) by temporarily replacing them by an unused private
@@ -998,19 +1032,23 @@ (define (substitute file pattern+procs)
(unused-private-use-code-point line))
#\nul))
(line* (replace-char #\nul nul* line))
- (line1* (fold (lambda (r+p line)
- (match r+p
- ((regexp . proc)
- (match (list-matches regexp line)
- ((and m+ (_ _ ...))
- (proc line m+))
- (_ line)))))
- line*
- rx+proc))
+ (results ;line, unmatched-regexps
+ (fold (lambda (r+p results)
+ (let ((line (first results))
+ (unmatched (second results)))
+ (match r+p
+ ((regexp . proc)
+ (match (list-matches* regexp line)
+ ((and m+ (_ _ ...))
+ (list (proc line m+)
+ (delq regexp unmatched)))
+ (_ (list line unmatched)))))))
+ (list line* unmatched-regexps)
+ rx+proc))
+ (line1* (first results))
(line1 (replace-char nul* #\nul line1*)))
(display line1 out)
- (loop (read-line in 'concat)))))))))
-
+ (loop (read-line in 'concat) (second results)))))))))
(define-syntax let-matches
;; Helper macro for `substitute*'.
@@ -1048,9 +1086,19 @@ (define-syntax substitute*
Alternatively, FILE may be a list of file names, in which case they are
all subject to the substitutions.
+By default, SUBSTITUTE* will raise a &message condition if one of the patterns
+fails to match on one of the files; REQUIRE-MATCHES? may be set to false to
+avoid an error being raised in such condition.
+
Be careful about using '$' to match the end of a line; by itself it won't
match the terminating newline of a line."
((substitute* file ((regexp match-var ...) body ...) ...)
+ (substitute* file
+ ((regexp match-var ...) body ...) ...
+ #:require-matches? #t))
+ ((substitute* file
+ ((regexp match-var ...) body ...) ...
+ #:require-matches? require-matches?)
(let ()
(define (substitute-one-file file-name)
(substitute
@@ -1074,7 +1122,8 @@ (define-syntax substitute*
(begin body ...)
(substring l o (match:start m))
r))))))))
- ...)))
+ ...)
+ #:require-matches? require-matches?))
(match file
((files (... ...))
diff --git a/tests/build-utils.scm b/tests/build-utils.scm
index 3babf5d544..35c66faa3c 100644
--- a/tests/build-utils.scm
+++ b/tests/build-utils.scm
@@ -1,7 +1,7 @@
;;; GNU Guix --- Functional package management for GNU
;;; Copyright © 2012, 2015, 2016, 2019, 2020 Ludovic Courtès <ludo@gnu.org>
;;; Copyright © 2019 Ricardo Wurmus <rekado@elephly.net>
-;;; Copyright © 2021 Maxim Cournoyer <maxim.cournoyer@gmail.com>
+;;; Copyright © 2021, 2023 Maxim Cournoyer <maxim.cournoyer@gmail.com>
;;; Copyright © 2021 Maxime Devos <maximedevos@telenet.be>
;;; Copyright © 2021 Brendan Tildesley <mail@brendan.scot>
;;;
@@ -289,26 +289,52 @@ (define (arg-test bash-args)
(test-assert "wrap-script, argument handling, bash --norc"
(arg-test " --norc"))
-(test-equal "substitute*, text contains a NUL byte, UTF-8"
- "c\0d"
- (with-fluids ((%default-port-encoding "UTF-8")
- (%default-port-conversion-strategy 'error))
- ;; The GNU libc is locale sensitive. Depending on the value of LANG, the
- ;; test could fail with "string contains #\\nul character: ~S" or "cannot
- ;; convert wide string to output locale".
- (setlocale LC_ALL "en_US.UTF-8")
- (call-with-temporary-output-file
- (lambda (file port)
- (format port "a\0b")
- (flush-output-port port)
-
- (substitute* file
- (("a") "c")
- (("b") "d"))
-
- (with-input-from-file file
- (lambda _
- (get-string-all (current-input-port))))))))
+(define-syntax-rule (define-substitute*-test test-type name expected
+ content clauses ...)
+ (test-type
+ name
+ expected
+ (with-fluids ((%default-port-encoding "UTF-8")
+ (%default-port-conversion-strategy 'error))
+ ;; The GNU libc is locale sensitive. Depending on the value of LANG,
+ ;; the test could fail with "string contains #\\nul character: ~S" or
+ ;; "cannot convert wide string to output locale".
+ (setlocale LC_ALL "en_US.UTF-8")
+ (call-with-temporary-output-file
+ (lambda (file port)
+ (format port content)
+ (flush-output-port port)
+
+ (substitute* file
+ clauses ...)
+
+ (with-input-from-file file
+ (lambda _
+ (get-string-all (current-input-port)))))))))
+
+(define-substitute*-test test-equal
+ "substitute*, text contains a NUL byte, UTF-8"
+ "c\0d" ;expected
+ "a\0b" ;content
+ (("a") "c")
+ (("b") "d"))
+
+(define-substitute*-test test-error "substitute*, no match error"
+ #t ;expected
+ "a\0b" ;content
+ (("Oops!") "c"))
+
+(define-substitute*-test test-equal "substitute*, no match, ignored"
+ "abc" ;expected
+ "abc" ;content
+ (("Oops!") "c")
+ #:require-matches? #f)
+
+(define-substitute*-test test-error "substitute*, partial no match error"
+ #t ;expected
+ "a\0b" ;content
+ (("a") "c"
+ ("Oops!") "c"))
(test-equal "search-input-file: exception if not found"
`((path)
--
2.41.0
M
M
Maxim Cournoyer wrote on 20 Oct 2023 02:57
[PATCH v3 1/3] build: Relocate <regexp*> record and associated procedures here.
(address . 42146@debbugs.gnu.org)(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)
ff182177822369c7d31698ecbf0cb5dcbca37644.1697763444.git.maxim.cournoyer@gmail.com
* etc/teams.scm.in (<regexp*>, make-regexp*, regexp*-exec): Move to...
* guix/build/utils.scm: ... here.
(list-matches*): New procedure.

Change-Id: I566ac372f7d8ba08de94e19b54dcc68da2106a23
---
etc/teams.scm.in | 19 +------------------
guix/build/utils.scm | 39 +++++++++++++++++++++++++++++++++++++++
2 files changed, 40 insertions(+), 18 deletions(-)

Toggle diff (110 lines)
diff --git a/etc/teams.scm.in b/etc/teams.scm.in
index 55242caad1..8af25b9802 100644
--- a/etc/teams.scm.in
+++ b/etc/teams.scm.in
@@ -34,29 +34,12 @@
(srfi srfi-9)
(srfi srfi-26)
(ice-9 format)
- (ice-9 regex)
(ice-9 match)
(ice-9 rdelim)
+ (guix build utils)
(guix ui)
(git))
-(define-record-type <regexp*>
- (%make-regexp* pat flag rx)
- regexp*?
- (pat regexp*-pattern)
- (flag regexp*-flag)
- (rx regexp*-rx))
-
-;;; Work around regexp implementation.
-;;; This record allows to track the regexp pattern and then display it.
-(define* (make-regexp* pat #:optional (flag regexp/extended))
- "Alternative to `make-regexp' producing annotated <regexp*> objects."
- (%make-regexp* pat flag (make-regexp pat flag)))
-
-(define (regexp*-exec rx* str)
- "Execute the RX* regexp, a <regexp*> object."
- (regexp-exec (regexp*-rx rx*) str))
-
(define-record-type <team>
(make-team id name description members scope)
team?
diff --git a/guix/build/utils.scm b/guix/build/utils.scm
index 8e630ad586..2b3a8e278b 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 © 2022 Simon Tournier <zimon.toutoune@gmail.com>
;;; Copyright © 2023 Carlo Zancanaro <carlo@zancanaro.id.au>
;;;
;;; This file is part of GNU Guix.
@@ -28,6 +29,7 @@
(define-module (guix build utils)
#:use-module (srfi srfi-1)
+ #:use-module (srfi srfi-9)
#:use-module (srfi srfi-11)
#:use-module (srfi srfi-26)
#:use-module (srfi srfi-34)
@@ -55,6 +57,14 @@ (define-module (guix build utils)
package-name->name+version
parallel-job-count
+ make-regexp*
+ regexp*-exec
+ regexp*?
+ regexp*-pattern
+ regexp*-flag
+ regexp*-rx
+ list-matches*
+
compressor
tarball?
%xz-parallel-args
@@ -163,6 +173,35 @@ (define-syntax-rule (define-constant name val)
(module-replace! (current-module) '(setvbuf)))
(else #f))
+
+;;;
+;;; Extend regexp objects with a pattern field.
+;;;
+(define-record-type <regexp*>
+ (%make-regexp* pat flag rx)
+ regexp*?
+ (pat regexp*-pattern) ;the regexp pattern, a string
+ (flag regexp*-flag) ;regexp flags
+ (rx regexp*-rx)) ;the compiled regexp object
+
+;;; Work around regexp implementation.
+;;; This record allows to track the regexp pattern and then display it.
+(define* (make-regexp* pat #:optional (flag regexp/extended))
+ "Alternative to `make-regexp' producing annotated <regexp*> objects."
+ (%make-regexp* pat flag (make-regexp pat flag)))
+
+(define (regexp*-exec rx* str)
+ "Execute the RX* regexp, a <regexp*> object."
+ (regexp-exec (regexp*-rx rx*) str))
+
+(define* (list-matches* regexp str #:optional (flags regexp/extended))
+ "Like 'list-matches', but also accepting a regexp* as REGEXP."
+ (match regexp
+ ((or (? string?) (? regexp?))
+ (list-matches regexp str flags))
+ ((? regexp*?)
+ (list-matches (regexp*-rx regexp) str flags))))
+
;;;
;;; Compression helpers.

base-commit: d59653b7c9e43ebdbba20e2ca071429507f94c67
--
2.41.0
M
M
Maxim Cournoyer wrote on 20 Oct 2023 02:57
[PATCH v3 3/3] build: bootstrap-configure: Allow lack of matches in substitute.
(address . 42146@debbugs.gnu.org)
0d8764ba1b635d49c07cc1d47f3dcc620bbf482d.1697763444.git.maxim.cournoyer@gmail.com
From: Jakub K?dzio?ka <kuba@kadziolka.net>

Matches are not required here, as not every file will use every
variable.

* guix/build/gnu-bootstrap.scm (bootstrap-configure): Pass
#:require-matches? #f to substitute*.

Signed-off-by: Maxim Cournoyer <maxim.cournoyer@gmail.com>
Change-Id: I7eee433a3af5fa310b3f2e8b8b58ac9befb4b56a
---
guix/build/gnu-bootstrap.scm | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)

Toggle diff (39 lines)
diff --git a/guix/build/gnu-bootstrap.scm b/guix/build/gnu-bootstrap.scm
index b4257a3717..b097f410ad 100644
--- a/guix/build/gnu-bootstrap.scm
+++ b/guix/build/gnu-bootstrap.scm
@@ -1,5 +1,6 @@
;;; GNU Guix --- Functional package management for GNU
;;; Copyright © 2020, 2022 Timothy Sample <samplet@ngyro.com>
+;;; Copyright © 2020 Jakub K?dzio?ka <kuba@kadziolka.net>
;;;
;;; This file is part of GNU Guix.
;;;
@@ -52,7 +53,8 @@ (define (bootstrap-configure name version modules scripts)
(copy-file template target)
(substitute* target
(("@PACKAGE_NAME@") name)
- (("@VERSION@") version))))
+ (("@VERSION@") version)
+ #:require-matches? #f)))
(append-map (lambda (dir) (find-files dir "\\.in$"))
modules))
(for-each (lambda (template)
@@ -62,12 +64,12 @@ (define (bootstrap-configure name version modules scripts)
(substitute* target
(("@GUILE@") guile)
(("@MODDIR@") moddir)
- (("@GODIR@") godir))
+ (("@GODIR@") godir)
+ #:require-matches? #f)
(chmod target #o755)))
(find-files scripts
(lambda (fn st)
- (string-suffix? ".in" fn))))
- #t)))
+ (string-suffix? ".in" fn)))))))
(define (bootstrap-build modules)
"Create a procedure that builds an early bootstrap package. The
--
2.41.0
S
S
Simon Tournier wrote on 20 Oct 2023 17:11
Re: [bug#42146] [PATCH core-updates 1/?] build: substitute: Don't fail silently.
874jilmira.fsf@gmail.com
Hi Ludo,

On Thu, 19 Oct 2023 at 22:40, Ludovic Courtès <ludo@gnu.org> wrote:

Toggle quote (20 lines)
>> +;;;
>> +;;; Extend regexp objects with a pattern field.
>> +;;;
>> +(define-record-type <regexp*>
>> + (%make-regexp* pat flag rx)
>> + regexp*?
>> + (pat regexp*-pattern) ;the regexp pattern, a string
>> + (flag regexp*-flag) ;regexp flags
>> + (rx regexp*-rx)) ;the compiled regexp object
>> +
>> +;;; Work around regexp implementation.
>> +;;; This record allows to track the regexp pattern and then display it.
>> +(define* (make-regexp* pat #:optional (flag regexp/extended))
>
> I’m skeptical about the concrete benefits. I would not include it in
> (guix build utils), or at least not in this patch series.
>
> (I tend to be super conservative about (guix build utils) because we
> rarely get a chance to change it.)

If I remember correctly, the record was introduced in #58660 [1].
Basically, if you have,

(make-regexp "^gnu/packages/python(-.+|)\\.scm$")

then you only have access to some #<regexp 7f6315fb3500>. Other said,
you lost the human-readable "^gnu/packages/python(-.+|)\\.scm$" regexp
pattern. The workaround just stores this human-readable regexp pattern.
Later, it is thus possible to display it; for debugging or else.

For the location of such feature, I do not have an opinion. For the
concrete benefits, I have one. :-)

Well, maybe the feature – keep an access to the human-readable regexp
pattern – could be implemented on Guile-side.

Or maybe there is another simpler way that I am not aware of?

Cheers,
simon


M
M
Maxim Cournoyer wrote on 20 Oct 2023 17:39
(name . Simon Tournier)(address . zimon.toutoune@gmail.com)
8734y5i9rb.fsf@gmail.com
Hi,

Simon Tournier <zimon.toutoune@gmail.com> writes:

Toggle quote (40 lines)
> Hi Ludo,
>
> On Thu, 19 Oct 2023 at 22:40, Ludovic Courtès <ludo@gnu.org> wrote:
>
>>> +;;;
>>> +;;; Extend regexp objects with a pattern field.
>>> +;;;
>>> +(define-record-type <regexp*>
>>> + (%make-regexp* pat flag rx)
>>> + regexp*?
>>> + (pat regexp*-pattern) ;the regexp pattern, a string
>>> + (flag regexp*-flag) ;regexp flags
>>> + (rx regexp*-rx)) ;the compiled regexp object
>>> +
>>> +;;; Work around regexp implementation.
>>> +;;; This record allows to track the regexp pattern and then display it.
>>> +(define* (make-regexp* pat #:optional (flag regexp/extended))
>>
>> I’m skeptical about the concrete benefits. I would not include it in
>> (guix build utils), or at least not in this patch series.
>>
>> (I tend to be super conservative about (guix build utils) because we
>> rarely get a chance to change it.)
>
> If I remember correctly, the record was introduced in #58660 [1].
> Basically, if you have,
>
> (make-regexp "^gnu/packages/python(-.+|)\\.scm$")
>
> then you only have access to some #<regexp 7f6315fb3500>. Other said,
> you lost the human-readable "^gnu/packages/python(-.+|)\\.scm$" regexp
> pattern. The workaround just stores this human-readable regexp pattern.
> Later, it is thus possible to display it; for debugging or else.
>
> For the location of such feature, I do not have an opinion. For the
> concrete benefits, I have one. :-)
>
> Well, maybe the feature – keep an access to the human-readable regexp
> pattern – could be implemented on Guile-side.

Agreed, for the long haul (as with many cool bits that are Guix specific
currently).

--
Thanks,
Maxim
B
B
Bruno Victal wrote on 20 Oct 2023 20:50
(name . Ludovic Courtès)(address . ludo@gnu.org)
3dad7ba9-75b4-4c48-b468-07c2a1fd603d@makinata.eu
Hi Ludo’,

On 2023-10-19 21:49, Ludovic Courtès wrote:
Toggle quote (5 lines)
>> +(test-group "substitute*"
>
> I’d avoid groups: they’re not super useful and the output with the
> Automake driver is terrible.

I personally like using groups for structuring the logic of the tests.

The output issue sounds either like a bug to me that should be fixed
or a place for improvement.

--
Furthermore, I consider that nonfree software must be eradicated.

Cheers,
Bruno.
M
M
Maxim Cournoyer wrote on 20 Oct 2023 23:11
(name . Bruno Victal)(address . mirai@makinata.eu)
877cnhgfti.fsf@gmail.com
Hi,

Bruno Victal <mirai@makinata.eu> writes:

Toggle quote (13 lines)
> Hi Ludo’,
>
> On 2023-10-19 21:49, Ludovic Courtès wrote:
>>> +(test-group "substitute*"
>>
>> I’d avoid groups: they’re not super useful and the output with the
>> Automake driver is terrible.
>
> I personally like using groups for structuring the logic of the tests.
>
> The output issue sounds either like a bug to me that should be fixed
> or a place for improvement.

Ludo, could you refresh my memory about what is wrong with the Automake
SRFI-64 driver when it comes to tests? I think it used to masks the
tests in its output, but I'm not seeing this now?

Consider the following tests, where the "elm->package-name" and
"infer-elm-package-name" tests are in the "round trip" test group itself
nested in the "elm->package-name and infer-elm-package-name" test group.

tests/configuration.scm contains a "duplicated/conflicting entries" test
group that contains "duplicate sanitizer", "duplicate serializer" and
"conflicting use of serializer + empty-serializer":

Toggle snippet (4 lines)
make check TESTS='tests/services/configuration.scm tests/elm.scm' \
SCM_LOG_DRIVER_FLAGS="--brief=no"

PASS: tests/services/configuration.scm - default value, no serialization
PASS: tests/services/configuration.scm - wrong type for a field
PASS: tests/services/configuration.scm - default value, custom serializer
PASS: tests/services/configuration.scm - no default value, provided
PASS: tests/services/configuration.scm - no default value, not provided
PASS: tests/services/configuration.scm - serialize-configuration
PASS: tests/services/configuration.scm - serialize-configuration [deprecated]
PASS: tests/services/configuration.scm - serialize-configuration with no-serialization
PASS: tests/services/configuration.scm - serialize-configuration with prefix
PASS: tests/services/configuration.scm - default value, sanitizer
PASS: tests/services/configuration.scm - string value, sanitized to number
PASS: tests/services/configuration.scm - default value, serializer literal
PASS: tests/services/configuration.scm - empty-serializer as literal
PASS: tests/services/configuration.scm - empty-serializer as procedure
PASS: tests/services/configuration.scm - default value, sanitizer, permutation
PASS: tests/services/configuration.scm - default value, serializer, permutation
PASS: tests/services/configuration.scm - string value sanitized to number, permutation
PASS: tests/services/configuration.scm - default value, sanitizer, permutation 2
PASS: tests/services/configuration.scm - default value, serializer, permutation 2
PASS: tests/services/configuration.scm - duplicate sanitizer
PASS: tests/services/configuration.scm - duplicate serializer
PASS: tests/services/configuration.scm - conflicting use of serializer + empty-serializer
PASS: tests/services/configuration.scm - Mix of bare serializer and new syntax
PASS: tests/services/configuration.scm - Mix of bare serializer and new syntax, permutation)
PASS: tests/services/configuration.scm - maybe value serialization
PASS: tests/services/configuration.scm - maybe value serialization of the instance
PASS: tests/services/configuration.scm - maybe value serialization of the instance, unspecified
PASS: tests/services/configuration.scm - symbol maybe value serialization, unspecified
PASS: tests/services/configuration.scm - maybe value without serialization no procedure bound
PASS: tests/services/configuration.scm - maybe type, no default
PASS: tests/services/configuration.scm - maybe type, with default
PASS: tests/elm.scm - elm->package-name
PASS: tests/elm.scm - infer-elm-package-name
PASS: tests/elm.scm - elm->package-name
PASS: tests/elm.scm - infer-elm-package-name
PASS: tests/elm.scm - elm->package-name
PASS: tests/elm.scm - infer-elm-package-name
PASS: tests/elm.scm - elm->package-name
PASS: tests/elm.scm - infer-elm-package-name
PASS: tests/elm.scm - elm->package-name
PASS: tests/elm.scm - infer-elm-package-name
PASS: tests/elm.scm - elm->package-name
PASS: tests/elm.scm - infer-elm-package-name
PASS: tests/elm.scm - elm->package-name
PASS: tests/elm.scm - infer-elm-package-name
PASS: tests/elm.scm - elm->package-name
PASS: tests/elm.scm - infer-elm-package-name
PASS: tests/elm.scm - elm->package-name
PASS: tests/elm.scm - infer-elm-package-name
PASS: tests/elm.scm - elm->package-name
PASS: tests/elm.scm - infers other name
PASS: tests/elm.scm - infered name round-trips
PASS: tests/elm.scm - elm->package-name
PASS: tests/elm.scm - infers other name
PASS: tests/elm.scm - infered name round-trips
PASS: tests/elm.scm - elm->package-name
PASS: tests/elm.scm - infers other name
PASS: tests/elm.scm - infered name round-trips
PASS: tests/elm.scm - elm->package-name
PASS: tests/elm.scm - infers other name
PASS: tests/elm.scm - infered name round-trips
PASS: tests/elm.scm - elm->package-name
PASS: tests/elm.scm - infers other name
PASS: tests/elm.scm - infered name round-trips
PASS: tests/elm.scm - elm->package-name
PASS: tests/elm.scm - infers other name
PASS: tests/elm.scm - infered name round-trips
PASS: tests/elm.scm - elm->package-name
PASS: tests/elm.scm - infers other name
PASS: tests/elm.scm - infered name round-trips
PASS: tests/elm.scm - elm->package-name
PASS: tests/elm.scm - infers other name
PASS: tests/elm.scm - infered name round-trips
PASS: tests/elm.scm - elm->package-name
PASS: tests/elm.scm - infers other name
PASS: tests/elm.scm - infered name round-trips
PASS: tests/elm.scm - elm->package-name
PASS: tests/elm.scm - infers other name
PASS: tests/elm.scm - infered name round-trips
PASS: tests/elm.scm - elm
PASS: tests/elm.scm - guile
PASS: tests/elm.scm - gcc-toolchain
PASS: tests/elm.scm - font-adobe-source-sans-pro
PASS: tests/elm.scm - (elm->guix-package "elm/core")
PASS: tests/elm.scm - (elm-recursive-import "elm-guix/demo")
============================================================================
Testsuite summary for GNU Guix 1.3.0.48706-b42e6315-dirty
============================================================================
# TOTAL: 85
# PASS: 85
# SKIP: 0
# XFAIL: 0
# FAIL: 0
# XPASS: 0
# ERROR: 0
============================================================================

Observation: the output says nothing about the groups, but at least the
nested tests are correctly listed.

--
Thanks,
Maxim
L
L
Ludovic Courtès wrote on 23 Oct 2023 10:42
Re: bug#42146: [PATCH core-updates 1/?] build: substitute: Don't fail silently.
(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)
875y2xivbz.fsf_-_@gnu.org
Hi Maxim,

Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:

Toggle quote (2 lines)
> Ludovic Courtès <ludo@gnu.org> writes:

[...]

Toggle quote (16 lines)
>>> +;;; Extend regexp objects with a pattern field.
>>> +;;;
>>> +(define-record-type <regexp*>
>>> + (%make-regexp* pat flag rx)
>>> + regexp*?
>>> + (pat regexp*-pattern) ;the regexp pattern, a string
>>> + (flag regexp*-flag) ;regexp flags
>>> + (rx regexp*-rx)) ;the compiled regexp object
>>> +
>>> +;;; Work around regexp implementation.
>>> +;;; This record allows to track the regexp pattern and then display it.
>>> +(define* (make-regexp* pat #:optional (flag regexp/extended))
>>
>> I’m skeptical about the concrete benefits. I would not include it in
>> (guix build utils), or at least not in this patch series.

[...]

Toggle quote (5 lines)
> The benefit is concrete: it makes it possible to show which regexp
> pattern failed to match (its textual representation), instead of
> something much less useful such as a generic placeholder such as
> "<unknown> (regexp)".

Yes, okay.

Can we keep the <regexp*> interface private, then? To me it’s an
implementation detail and perhaps a bit of a hack that I’d rather not
commit to maintaining.

The cost might be to duplicate it in teams.scm, but that’s an acceptable
cost IMO.

Toggle quote (15 lines)
> It's in actual use if you look at the definition of substitute:
>
>
> (let ((rx+proc (map (match-lambda
> (((or (? regexp? pattern) (? regexp*? pattern)) . proc)
> (cons pattern proc))
> ((pattern . proc)
> (cons (make-regexp* pattern regexp/extended) proc)))
> pattern+procs)))
>
> The previous version followed a different approach, annotating the
> rx+proc list with the raw pattern; I think the approach here is a bit
> cleaner, and it should also enable users to pass pre-computed regexp*
> objects to substitute* and have useful error messages produced.

Note that ‘substitute*’ only takes string literals as patterns, not
regexp objects. The pattern part of clauses has sometimes been abused
to pass code, typically ‘string-append’ calls, but that’s definitely not
the spirit.

Another approach would have been, in ‘substitute*’, to capture the
regexp-as-string as well as other useful debugging info, such as its
source location.

Thanks,
Ludo’.
L
L
Ludovic Courtès wrote on 23 Oct 2023 10:48
(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)
871qdliv1v.fsf_-_@gnu.org
Hi,

Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:

Toggle quote (11 lines)
> From: Jakub K?dzio?ka <kuba@kadziolka.net>
>
> * guix/build/utils.scm (substitute, substitute*)
> [require-matches?]: New argument.
> * tests/build-utils.scm ("substitute*"): New test group.
> ("substitute*, no match error")
> ("substitute*, partial no match error"): New tests.
>
> Co-authored-by: Maxim Cournoyer <maxim.cournoyer@gmail.com>
> Change-Id: I66ed33d72aa73cd35e5642521efec70bf756f86e

[…]

Toggle quote (8 lines)
> +(define-condition-type &substitute-error &error
> + substitute-error?
> + (file substitute-error-file)
> + (patterns substitute-error-patterns))
> +
> +(define %substitute-requires-matches?
> + (make-parameter #t))

[…]

Toggle quote (6 lines)
> @@ -1048,9 +1086,19 @@ (define-syntax substitute*
> Alternatively, FILE may be a list of file names, in which case they are
> all subject to the substitutions.
>
> +By default, SUBSTITUTE* will raise a &message condition if one of the patterns

I think it’s ‘&substitute-error’ rather than ‘&message’.

Toggle quote (6 lines)
> +(define-substitute*-test test-error "substitute*, partial no match error"
> + #t ;expected
> + "a\0b" ;content
> + (("a") "c"
> + ("Oops!") "c"))

Maybe it’s not all that important here, but I think ‘test-error’ tests
for any exception, whereas we’re looking specifically for a
‘&substitute-error’ condition filled with the right values.

Ludo’.
L
L
Ludovic Courtès wrote on 23 Oct 2023 10:51
(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)(address . 42146@debbugs.gnu.org)
87v8axhgci.fsf_-_@gnu.org
BTW, as far as merging is concerned, how about doing an
x86_64-linux-only build of a branch containing these patches on ci.guix
first, as proposed on IRC a few days ago?

That would allow us to see, within less than 24h, whether there’s a
massive fallout that we didn’t think of, for instance because key
packages rely on silent match failures.

Thanks,
Ludo’.
M
M
Maxim Cournoyer wrote on 23 Oct 2023 17:05
(name . Ludovic Courtès)(address . ludo@gnu.org)
87ttqh751g.fsf@gmail.com
Hi Ludovic,

Ludovic Courtès <ludo@gnu.org> writes:

Toggle quote (37 lines)
> Hi Maxim,
>
> Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:
>
>> Ludovic Courtès <ludo@gnu.org> writes:
>
> [...]
>
>>>> +;;; Extend regexp objects with a pattern field.
>>>> +;;;
>>>> +(define-record-type <regexp*>
>>>> + (%make-regexp* pat flag rx)
>>>> + regexp*?
>>>> + (pat regexp*-pattern) ;the regexp pattern, a string
>>>> + (flag regexp*-flag) ;regexp flags
>>>> + (rx regexp*-rx)) ;the compiled regexp object
>>>> +
>>>> +;;; Work around regexp implementation.
>>>> +;;; This record allows to track the regexp pattern and then display it.
>>>> +(define* (make-regexp* pat #:optional (flag regexp/extended))
>>>
>>> I’m skeptical about the concrete benefits. I would not include it in
>>> (guix build utils), or at least not in this patch series.
>
> [...]
>
>> The benefit is concrete: it makes it possible to show which regexp
>> pattern failed to match (its textual representation), instead of
>> something much less useful such as a generic placeholder such as
>> "<unknown> (regexp)".
>
> Yes, okay.
>
> Can we keep the <regexp*> interface private, then? To me it’s an
> implementation detail and perhaps a bit of a hack that I’d rather not
> commit to maintaining.

If you can think of a more elegant way to define it, I'm all ears.
Ideally, as Simon pointed out, that's something that Guile would support
natively (the ability to pull back the raw pattern from a regexp
object). In Python for example, the re.Pattern object has a 'pattern'
property [0]


Otherwise I don't think "the bit of a hack" is substantiated enough :-).

Toggle quote (3 lines)
> The cost might be to duplicate it in teams.scm, but that’s an acceptable
> cost IMO.

I dislike copying bits around just to "hide" them from the "official"
interface, and have no fear maintaining that simple piece of code, so
I'd rather keep it at one place and expose it as a public API.

Toggle quote (21 lines)
>> It's in actual use if you look at the definition of substitute:
>>
>>
>> (let ((rx+proc (map (match-lambda
>> (((or (? regexp? pattern) (? regexp*? pattern)) . proc)
>> (cons pattern proc))
>> ((pattern . proc)
>> (cons (make-regexp* pattern regexp/extended) proc)))
>> pattern+procs)))
>>
>> The previous version followed a different approach, annotating the
>> rx+proc list with the raw pattern; I think the approach here is a bit
>> cleaner, and it should also enable users to pass pre-computed regexp*
>> objects to substitute* and have useful error messages produced.
>
> Note that ‘substitute*’ only takes string literals as patterns, not
> regexp objects. The pattern part of clauses has sometimes been abused
> to pass code, typically ‘string-append’ calls, but that’s definitely not
> the spirit.
>

It's true that 'substitute*' only accepts literal patterns, but that's
not the case with 'substitute' itself, which is also exposed in the API
of (guix build utils): it accepts regexp objects as well, and thus
there's value in this change -- users could in theory use make-regexp*
objects and pass that to 'substitute' and benefit from more precises
error messages.

Toggle quote (4 lines)
> Another approach would have been, in ‘substitute*’, to capture the
> regexp-as-string as well as other useful debugging info, such as its
> source location.

That'd be a nice solution, but it doesn't take into account
'substitute', as mentioned above. As it has exactly zero (!) users
outside of substitute*, perhaps it could be yanked from the public API
and then we could consider doing the above for substitute* ?

--
Thanks,
Maxim
M
M
Maxim Cournoyer wrote on 23 Oct 2023 18:25
(name . Ludovic Courtès)(address . ludo@gnu.org)
87a5s971c8.fsf@gmail.com
Hi Ludo,

Ludovic Courtès <ludo@gnu.org> writes:

[...]

Toggle quote (8 lines)
>> @@ -1048,9 +1086,19 @@ (define-syntax substitute*
>> Alternatively, FILE may be a list of file names, in which case they are
>> all subject to the substitutions.
>>
>> +By default, SUBSTITUTE* will raise a &message condition if one of the patterns
>
> I think it’s ‘&substitute-error’ rather than ‘&message’.

Good catch. Fixed.

Toggle quote (10 lines)
>> +(define-substitute*-test test-error "substitute*, partial no match error"
>> + #t ;expected
>> + "a\0b" ;content
>> + (("a") "c"
>> + ("Oops!") "c"))
>
> Maybe it’s not all that important here, but I think ‘test-error’ tests
> for any exception, whereas we’re looking specifically for a
> ‘&substitute-error’ condition filled with the right values.

I thought test-error was limited in srfi-64 that made it not work with
condition types, but it seems that's not the case and it actually does
work:

Toggle snippet (24 lines)
modified tests/build-utils.scm
@@ -320,8 +320,8 @@ (define-substitute*-test test-equal
(("b") "d"))
(define-substitute*-test test-error "substitute*, no match error"
- #t ;expected
- "a\0b" ;content
+ &substitute-error ;expected
+ "a\0b" ;content
(("Oops!") "c"))
(define-substitute*-test test-equal "substitute*, no match, ignored"
@@ -331,8 +331,8 @@ (define-substitute*-test test-equal "substitute*, no match, ignored"
#:require-matches? #f)
(define-substitute*-test test-error "substitute*, partial no match error"
- #t ;expected
- "a\0b" ;content
+ &substitute-error ;expected
+ "a\0b" ;content
(("a") "c"
("Oops!") "c"))

and all tests still pass :-). I'll send a v4.

--
Thanks,
Maxim
?