Re: Transform options should error on nonexistant targets

[zimoun]

Hi Ryan,

On Wed, 18 Aug 2021 at 03:57, Ryan Prior <rprior@protonmail.com> wrote:
> I learned today that Guix will chug happily along applying a transform to a 
> nonexistent package.
>
> For example, I can run:
> guix environment --with-latest=not-exist --ad-hoc which
>
> This shows no warning or errors. I think it would be beneficial to show an 
> error & bail if the target of a transformation is a package that doesn't 
> exist, as this is likely indicative of an error.

Indeed. :-)  The issue comes from guix/transformations.scm 
(options->transformation):

--8<---------------cut here---------------start------------->8---
              (let ((new (transform obj)))
                (when (eq? new obj)
                  (warning (G_ "transformation '~a' had no effect on ~a~%")
                           name
                           (if (package? obj)
                               (package-full-name obj)
                               obj)))
                new)
--8<---------------cut here---------------end--------------->8---

and the warning is not reach because guix/packages.scm (package-mapping):

--8<---------------cut here---------------start------------->8---
            (else
             ;; Return a variant of P with PROC applied to P and its explicit
             ;; dependencies, recursively.  Memoize the transformations.  
Failing
             ;; to do that, we would build a huge object graph with lots of
             ;; duplicates, which in turns prevents us from benefiting from
             ;; memoization in 'package-derivation'.
             (let ((p (proc p)))
               (package
                …
--8<---------------cut here---------------end--------------->8---

In the case of “guix build hello --with-latest=foo”, then ’proc’ has no
effect, i.e., ’p’ is identical to ’(proc p)’ but a new package is
allocated which leads that ’new’ and ’obj’ are not ’eq?’.

Well, I have tried the attached patch.  But then ’tests/packages.scm’
fails.  Hum, I need an input because I do not know if I miss something
or if the test is also inaccurate.

--8<---------------cut here---------------start------------->8---
(test-assert "package-input-rewriting/spec"
  (let* ((dep     (dummy-package "chbouib"
                    (native-inputs `(("x" ,grep)))))
         (p0      (dummy-package "example"
                    (inputs `(("foo" ,coreutils)
                              ("bar" ,grep)
                              ("baz" ,dep)))))
         (rewrite (package-input-rewriting/spec
                   `(("coreutils" . ,(const sed))
                     ("grep" . ,(const findutils)))
                   #:deep? #f))
         (p1      (rewrite p0))
         (p2      (rewrite p0)))
    (and (not (eq? p1 p0))
         (eq? p1 p2)                              ;memoization
--8<---------------cut here---------------end--------------->8---

I miss why ’p1’ should not be the same as ’p0’.

> What do you think?

Maybe, it could be worth to open a report for that.  Feel free. ;-)


Cheers,
simon

>From e1cd54f4cccad37f7134b342c8dee9da9fa28588 Mon Sep 17 00:00:00 2001
From: zimoun <zimon.toutoune@gmail.com>
Date: Wed, 25 Aug 2021 17:32:58 +0200
Subject: [PATCH 1/1] packages: 'package-mapping' does not allocate unwritten
 package.

Reported by Ryan Prior <rprior@protonmail.com>.

* guix/packages.scm (package-mapping): Do not allocate a new package if the
procedure has no effect.
---
 guix/packages.scm | 31 +++++++++++++++++--------------
 1 file changed, 17 insertions(+), 14 deletions(-)

diff --git a/guix/packages.scm b/guix/packages.scm
index c825f427d8..15aa67fe0a 100644
--- a/guix/packages.scm
+++ b/guix/packages.scm
@@ -6,6 +6,7 @@
 ;;; Copyright © 2017, 2019, 2020 Efraim Flashner <efraim@flashner.co.il>
 ;;; Copyright © 2019 Marius Bakke <mbakke@fastmail.com>
 ;;; Copyright © 2021 Chris Marusich <cmmarusich@gmail.com>
+;;; Copyright © 2021 Simon Tournier <zimon.toutoune@gmail.com>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -1058,20 +1059,22 @@ applied to implicit inputs as well."
              ;; to do that, we would build a huge object graph with lots of
              ;; duplicates, which in turns prevents us from benefiting from
              ;; memoization in 'package-derivation'.
-             (let ((p (proc p)))
-               (package
-                 (inherit p)
-                 (location (package-location p))
-                 (build-system (if deep?
-                                   (build-system-with-package-mapping
-                                    (package-build-system p) rewrite)
-                                   (package-build-system p)))
-                 (inputs (map rewrite (package-inputs p)))
-                 (native-inputs (map rewrite (package-native-inputs p)))
-                 (propagated-inputs (map rewrite (package-propagated-inputs 
p)))
-                 (replacement (and=> (package-replacement p) replace))
-                 (properties `((,mapping-property . #t)
-                               ,@(package-properties p)))))))))
+             (let ((new (proc p)))
+               (if (eq? new p)
+                   p
+                   (package
+                     (inherit new)
+                     (location (package-location new))
+                     (build-system (if deep?
+                                       (build-system-with-package-mapping
+                                        (package-build-system new) rewrite)
+                                       (package-build-system new)))
+                     (inputs (map rewrite (package-inputs new)))
+                     (native-inputs (map rewrite (package-native-inputs new)))
+                     (propagated-inputs (map rewrite 
(package-propagated-inputs new)))
+                     (replacement (and=> (package-replacement new) replace))
+                     (properties `((,mapping-property . #t)
+                                   ,@(package-properties new))))))))))
 
   replace)
 
-- 
2.32.0