Re: Transform options should error on nonexistant targets

[zimoun <zimon.toutoune@gmail.com>]

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

Hi Ludo,

On Thu, 2 Sept 2021 at 12:06, Ludovic Courtès <ludo@gnu.org> wrote:

> Unfortunately we cannot do that: rewriting happens lazily, when the
> various inputs fields (which are thunked) are accessed.  When PROC
> returns P, we still need to recurse into its inputs, until CUT? says we
> can stop.  (I’m surprised this change triggers only one test failure
> actually.)

Yeah.  After sending the email, I tried to fix stuff and I get the point. :-)


However, from my opinion, it is easy to check if the package-target is
a package or not, i.e.

  $ guix build foo --<transform>=package-target=new
  guix build: error: package-target: unknown package

For instance by using 'specification->package'; see attached patch.
But then, the test suite fails; I guess because 'dummy-package' and I
have not found the time to investigate.  From my point of view, this
kind of patch will fix one part of the initial issue reported by Ryan.

The other issue is to list if the transformation is applied or not.  I
think it is possible by traversing again the graph and check if a
property appears at least once; well it should be better to warn if
the 'mapping-property' is not found at least once.  I had some
headaches to implement it... and I moved to other "urgent" stuff. :-)


Last, speaking about transformations, the graph is walked too much
when several transformations is applied:

   guix build hello --with-latest=foo --with-input=bar=baz --with-latest=chouib

then the graph is walked 3 times, IIUC.  The options needs a rewrite
to pass a list of specs to 'package-with-latest-upstream' and not
twice a list with only one element.  This would reduce to 2 walks.
Then it could be nice to compose the transformation and then walk only
once (apply 'package-mapping' only once).
Well, maybe I miss something.

Cheers,
simon

[-- Attachment #2: 0001-transformations-Error-when-incorrect-specifications.patch --]
[-- Type: text/x-patch, Size: 4113 bytes --]

From c0fa86d316c91044630b85c9e789f9a455fd29f4 Mon Sep 17 00:00:00 2001
From: zimoun <zimon.toutoune@gmail.com>
Date: Fri, 27 Aug 2021 18:15:16 +0200
Subject: [PATCH] transformations: Error when incorrect specifications.

* guix/transformations.scm (transform-package-with-debug-info,
transform-package-latest, transform-package-tests)[rewrite]: Raise when
incorrect specification.
(options->transformation)[package-name?]: New procedure.
[applicable]: Use it.
---
 guix/transformations.scm | 41 ++++++++++++++++++++++++++++++++--------
 1 file changed, 33 insertions(+), 8 deletions(-)

diff --git a/guix/transformations.scm b/guix/transformations.scm
index 5122baa403..2546017d0d 100644
--- a/guix/transformations.scm
+++ b/guix/transformations.scm
@@ -1,5 +1,6 @@
 ;;; GNU Guix --- Functional package management for GNU
 ;;; Copyright © 2016, 2017, 2018, 2019, 2020, 2021 Ludovic Courtès <ludo@gnu.org>
+;;; Copyright © 2021 Simon Tournier <zimon.toutoune@gmail.com>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -432,9 +433,15 @@ to the same package but with #:strip-binaries? #f in its 'arguments' field."
                (replacement (loop next))))))))
 
   (define rewrite
-    (package-input-rewriting/spec (map (lambda (spec)
-                                         (cons spec package-with-debug-info))
-                                       specs)))
+    (package-input-rewriting/spec
+     (map (lambda (spec)
+            (match (string-tokenize spec %not-equal)
+              ((spec)
+               (cons spec package-with-debug-info))
+              (_
+               (raise
+                (formatted-message (G_ "~a: invalid specification") spec)))))
+          specs)))
 
   (lambda (obj)
     (if (package? obj)
@@ -451,9 +458,15 @@ to the same package but with #:strip-binaries? #f in its 'arguments' field."
          ((#:tests? _ #f) #f)))))
 
   (define rewrite
-    (package-input-rewriting/spec (map (lambda (spec)
-                                         (cons spec package-without-tests))
-                                       specs)))
+    (package-input-rewriting/spec
+     (map (lambda (spec)
+            (match (string-tokenize spec %not-equal)
+              ((spec)
+               (cons spec package-without-tests))
+              (_
+               (raise
+                (formatted-message (G_ "~a: invalid specification") spec)))))
+          specs)))
 
   (lambda (obj)
     (if (package? obj)
@@ -569,7 +582,12 @@ are replaced by their latest upstream version."
   (define rewrite
     (package-input-rewriting/spec
      (map (lambda (spec)
-            (cons spec package-with-latest-upstream))
+            (match (string-tokenize spec %not-equal)
+              ((spec)
+               (cons spec package-with-latest-upstream))
+              (_
+               (raise
+                (formatted-message (G_ "~a: invalid specification") spec)))))
           specs)))
 
   (lambda (obj)
@@ -695,6 +713,12 @@ the resulting objects.  OPTS must be a list of symbol/string pairs such as:
 
 Each symbol names a transformation and the corresponding string is an argument
 to that transformation."
+  (define (package-name? value)
+    ;; Return an error if value does not correspond to a package.
+    (match (string-tokenize value %not-equal)
+      ((name _ ...)
+       (specification->package name))))
+
   (define applicable
     ;; List of applicable transformations as symbol/procedure pairs in the
     ;; order in which they appear on the command line.
@@ -707,7 +731,8 @@ to that transformation."
                       ;; XXX: We used to pass TRANSFORM a list of several
                       ;; arguments, but we now pass only one, assuming that
                       ;; transform composes well.
-                      (list key value (transform (list value)))))))
+                      (and (package-name? value)
+                        (list key value (transform (list value))))))))
                 (reverse opts)))
 
   (define (package-with-transformation-properties p)

base-commit: 7125b0d8a1be58e3f5b66a00fc5912aed9b330e4
-- 
2.32.0