Transform options should error on nonexistant targets

Transform options should error on nonexistant targets

[Ryan Prior]

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

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.

What do you think?

[-- Attachment #2: Type: text/html, Size: 512 bytes --]

Re: Transform options should error on nonexistant targets

[zimoun]

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

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


[-- Attachment #2: patch.patch --]
[-- Type: text/x-diff, Size: 3037 bytes --]

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

Re: Transform options should error on nonexistant targets

[Ludovic Courtès]

Hi,

zimoun <zimon.toutoune@gmail.com> skribis:

> 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):
>
>               (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)
>
>
> and the warning is not reach because guix/packages.scm (package-mapping):
>
>             (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
>                 …
>
>
> 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?’.

Indeed.

[...]

> +             (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))))))))))

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.)

Does that make sense?

Thanks,
Ludo’.

Re: Transform options should error on nonexistant targets

[zimoun]

[-- 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

Re: Transform options should error on nonexistant targets

[Ludovic Courtès]

Hello!

zimoun <zimon.toutoune@gmail.com> skribis:

> 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.

Right.

> 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. :-)

Hmm the ‘mapping-property’ is not enough.  I think you pretty much have
to compute the derivations of the new and old packages and compare them.

> 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.

Right, I guess it could work.  It’s the same complexity anyway, but at
least it would lower the constant costs.

> 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.

[...]

> -    (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)))))

The goal here is to catch mistakes like ‘--with-debug-info=foo=bar’,
right?  Is that a plausible typo?  :-)

>  Each symbol names a transformation and the corresponding string is an argument
>  to that transformation."
> +  (define (package-name? value)

Rather ‘assert-package-specification’, since it’s used for control
effects (exception raised by ‘specification->package’).

> +    ;; Return an error if value does not correspond to a package.
> +    (match (string-tokenize value %not-equal)
> +      ((name _ ...)
> +       (specification->package name))))

The problem I see is that it prevents rewrites where the package to be
rewritten is not public.  Maybe that’s an acceptable tradeoff though,
I’m not sure.

Thoughts?

Thanks,
Ludo’.

Transform options: check if applied or not

[zimoun]

Hi Ludo,

This thread is initially about 2 issues:

 1) apply a transformation to a non-existent package,
 2) check if the transformation makes sense or not.

Here, I will comment only about #2.

On Wed, 08 Sep 2021 at 22:55, Ludovic Courtès <ludo@gnu.org> wrote:

>> 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. :-)
>
> Hmm the ‘mapping-property’ is not enough.  I think you pretty much have
> to compute the derivations of the new and old packages and compare them.

From my understanding, the transformation can replace something and then
return a new package but this new package can be the same as the old
one.  I agree that the detection of such cases requires to compute the
both derivation––which is probably too expensive / complex, IMHO.

However, the transformation might replace nothing and so it is sure that
the new and old packages are the same. :-)

Other said,

  a) there is no guarantee that if the new graph contains a least one
replacement then the new package is different than the old one.

But,

  b) there is a guarantee that if the new graph does not contains any
replacement, then for sure the new package is the same than the old one.

For instance,

 a) guix build hello@2.10 --with-source=hello=https://ftpmirror.gnu.org/gnu/hello/hello-2.10.tar.gz
 b) guix build hello@2.10 --with-latest=python-scipy

and it would be nice to warn for the case b-.  After digging again in
the code, the ’mapping-property’ is probably not the right way, even if
a ’properties’ could be added when the rewrite happens and this
’properties’ can be checked at the end of the traversal.  WDYT?


Cheers,
simon

Well well, I miss a point:

--8<---------------cut here---------------start------------->8---
$ guix build hello@2.10 --with-source=hello=https://ftpmirror.gnu.org/gnu/hello/hello-2.10.tar.gz
/gnu/store/zfwhbbknkhxi3yqmp0qgh1l1crljgbm6-hello-2.10

$ guix build hello@2.10 
/gnu/store/a462kby1q51ndvxdv3b6p0rsixxrgx1h-hello-2.10
--8<---------------cut here---------------end--------------->8---

Where https://ftpmirror.gnu.org/gnu/hello/hello-2.10.tar.gz is an
official mirror.  It was expecting the same store item so why not a
different one.

It is because the derivations are different.  The first call contains
the source tarball as an “input”; that’s fine.  The second call contains
the source tarball as a derivation; again that’s fine.  The corresponding
first and second derivations are:

--8<---------------cut here---------------start------------->8---
Derive
([("out","/gnu/store/zfwhbbknkhxi3yqmp0qgh1l1crljgbm6-hello-2.10","","")]
 ,[("/gnu/store/101ny738l311p4fm3cas7jgnkzagjv6a-module-import-compiled.drv",["out"])

[...]

   ,("/gnu/store/z4s27gwacbw8f38andfsh21r8v330dag-xz-5.2.4.drv",["out"])]
 ,["/gnu/store/8a0wry8cvr405ha8d8bpjyzj5dzghigd-module-import",
 "/gnu/store/chariqd6k0sli3s7vcl4q3al0crirz5v-hello-2.10.tar.gz",
 "/gnu/store/rndq9g8877l29ha41dvsl3aj1z0gw0ng-hello-2.10-guile-builder"]

[...]

--8<---------------cut here---------------end--------------->8---

and

--8<---------------cut here---------------start------------->8---
Derive
([("out","/gnu/store/a462kby1q51ndvxdv3b6p0rsixxrgx1h-hello-2.10","","")]
 ,[("/gnu/store/101ny738l311p4fm3cas7jgnkzagjv6a-module-import-compiled.drv",["out"])
   ,("/gnu/store/1a7xfcqcxj0pqi4f81x1agcxa46v2bbm-hello-2.10.tar.gz.drv",["out"])

[...]

   ,("/gnu/store/z4s27gwacbw8f38andfsh21r8v330dag-xz-5.2.4.drv",["out"])]
 ,["/gnu/store/8a0wry8cvr405ha8d8bpjyzj5dzghigd-module-import",
 "/gnu/store/kql8b2hbsabcmany4m3hfm3wzdiymliy-hello-2.10-guile-builder"]

[...]

--8<---------------cut here---------------end--------------->8---

Therefore, I understand why the two store items have different hashes.


What puzzled me are the corresponding ’guile-builder’s:

--8<---------------cut here---------------start------------->8---
  (define %build-inputs
    (quote
     (("source" . "/gnu/store/chariqd6k0sli3s7vcl4q3al0crirz5v-hello-2.10.tar.gz")
--8<---------------cut here---------------end--------------->8---

and

--8<---------------cut here---------------start------------->8---
  (define %build-inputs
    (quote
     (("source" . "/gnu/store/hbdalsf5lpf01x4dcknwx6xbn6n5km6k-hello-2.10.tar.gz")
--8<---------------cut here---------------end--------------->8---

but these 2 items have the same hash:

--8<---------------cut here---------------start------------->8---
$ guix hash /gnu/store/chariqd6k0sli3s7vcl4q3al0crirz5v-hello-2.10.tar.gz
0ssi1wpaf7plaswqqjwigppsg5fyh99vdlb9kzl7c9lng89ndq1i

$ guix hash /gnu/store/hbdalsf5lpf01x4dcknwx6xbn6n5km6k-hello-2.10.tar.gz
0ssi1wpaf7plaswqqjwigppsg5fyh99vdlb9kzl7c9lng89ndq1i
--8<---------------cut here---------------end--------------->8---

Why?  It is fixed-output so I was expecting the same thing.  And the
same as:

--8<---------------cut here---------------start------------->8---
$ guix download https://ftpmirror.gnu.org/gnu/hello/hello-2.10.tar.gz

Starting download of /tmp/guix-file.qkhxkp
From https://ftpmirror.gnu.org/gnu/hello/hello-2.10.tar.gz...
following redirection to `https://mirrors.sarata.com/gnu/hello/hello-2.10.tar.gz'...
 …10.tar.gz  709KiB                                           549KiB/s 00:01 [##################] 100.0%
/gnu/store/hbdalsf5lpf01x4dcknwx6xbn6n5km6k-hello-2.10.tar.gz
0ssi1wpaf7plaswqqjwigppsg5fyh99vdlb9kzl7c9lng89ndq1i
--8<---------------cut here---------------end--------------->8---

Why the store item of source tarball is it different when applying the
transformation?

Re: Transform options: check if applied or not

[Maxime Devos]

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

zimoun schreef op do 09-09-2021 om 00:29 [+0200]:
> Why?  It is fixed-output so I was expecting the same thing.  And the
> same as:
> 
> --8<---------------cut here---------------start------------->8---
> $ guix download https://ftpmirror.gnu.org/gnu/hello/hello-2.10.tar.gz
> 
> Starting download of /tmp/guix-file.qkhxkp
> From https://ftpmirror.gnu.org/gnu/hello/hello-2.10.tar.gz...
> following redirection to `https://mirrors.sarata.com/gnu/hello/hello-2.10.tar.gz'...
>  …10.tar.gz  709KiB                                           549KiB/s 00:01 [##################] 100.0%
> /gnu/store/hbdalsf5lpf01x4dcknwx6xbn6n5km6k-hello-2.10.tar.gz
> 0ssi1wpaf7plaswqqjwigppsg5fyh99vdlb9kzl7c9lng89ndq1i
> --8<---------------cut here---------------end--------------->8---
> 
> Why the store item of source tarball is it different when applying the
> transformation?

There is one situation I know of where this can happen, though that's
for 'local-file' and not 'upstream-source-compiler': one has #:recursive? #t
and the other has #:recursive? #f.

‘download-to-store’ (used indirectly by upstream-source-compiler) sets #:recursive? #false
by default.  built-in-download (used by url-fetch) sets #:recursive? #false too, unless
the file is expected to be executale.

This doesn't explain what's going on here (as #:recursive? is #f in both cases),
but maybe it gives an idea where to look?

Greetings,
Maxime.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

Transform: walk through packages

[zimoun]

Hi,

The initial message [1] leads to various questions about
transformations:

  1) optimize the walk
  2) check if the transformation makes sense or not.
  3) apply a transformation to a non-existent package,

The #2 is commented in this subthread [2].  The #3 is addressed with the
draft patch attached [3].  Here, let comment about #1.

1: <https://lists.gnu.org/archive/html/guix-devel/2021-08/msg00072.html>
2: <https://lists.gnu.org/archive/html/guix-devel/2021-09/msg00110.html>
3: <https://lists.gnu.org/archive/html/guix-devel/2021-09/msg00022.html>

On Wed, 08 Sep 2021 at 22:55, Ludovic Courtès <ludo@gnu.org> wrote:
> zimoun <zimon.toutoune@gmail.com> skribis:

>> 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.
>
> Right, I guess it could work.  It’s the same complexity anyway, but at
> least it would lower the constant costs.

Yeah, for sure.  But for example, if you go to 5 shops, buy stuff, put
in your bag, you will not first go to each and just pay then second go
again to each and bring the stuff.  It is the same complexity in term of
number of shops, though. ;-)

I do not know the Guile internals so maybe I miss a point.  From my
understanding, this pattern is odd:

   (map f (map g xs))

and instead it seems better:

  (map (compose f g) xs)

The “visible” effect of the win of the factor 2 in the constant costs
depends on the constant costs of f and g vs the constant cost to walk
the list.  But still, there is a factor 2 win. :-)

For instance, GHC has a rewrite rule [4] pass for automatic optimization
when compiling. :-)

4: <https://downloads.haskell.org/~ghc/6.12.2/docs/html/users_guide/rewrite-rules.html>


Anyway.  From the previous example, it appears to me easy to group the
transform ’--with-latest’ and applies a list of 2 elements.  It is
somehow a list arrangement in ’applicable’ and then each transform
already accepts a list of specs.  I will try to come up with a patch if
no one beats me. :-)

However, I do not know beforehand if all the transformations
(e.g., --with-latest and --with-input) will compose well.

Well, it is an investigation of this comment. ;-)

                     (transform
                      ;; XXX: We used to pass TRANSFORM a list of several
                      ;; arguments, but we now pass only one, assuming that
                      ;; transform composes well.

Cheers,
simon

Re: Transform options should error on nonexistant targets

[zimoun]

Hi,

On Wed, 8 Sept 2021 at 22:55, Ludovic Courtès <ludo@gnu.org> wrote:

> >  Each symbol names a transformation and the corresponding string is an argument
> >  to that transformation."
> > +  (define (package-name? value)
>
> Rather ‘assert-package-specification’, since it’s used for control
> effects (exception raised by ‘specification->package’).

Thanks.  Indeed, it is better.

> > +    ;; Return an error if value does not correspond to a package.
> > +    (match (string-tokenize value %not-equal)
> > +      ((name _ ...)
> > +       (specification->package name))))
>
> The problem I see is that it prevents rewrites where the package to be
> rewritten is not public.  Maybe that’s an acceptable tradeoff though,
> I’m not sure.

Yes.  It is already used by 'evaluate-replacement-specs' and
'transform-package-toolchain'.  That's why I used it.  And it appeared
to me the simplest. :-)


All the best,
simon