bug#67221: Package identity transformation produces different derivation

[Ulf Herrman]

$ guix build --no-grafts --derivations -e \
 '(begin
   (use-modules (guix packages) (gnu packages gtk))
   ((package-mapping identity) cairo-sans-poppler))' \
   -e '(@ (gnu packages gtk) cairo-sans-poppler)'
=>
/gnu/store/51hwf2mc2ig76a3nm86msv4z9az3s0j3-cairo-1.16.0.drv
/gnu/store/q78ziqcg2dvi2lzj18hzdld85bcydzwk-cairo-1.16.0.drv

Some interesting notes:

1. (package-mapping identity #:deep? #t) *does* produce the same
   derivation.
2. bag->derivation's `delete-duplicates' call uses `input=?', but its
   input objects in the second tuple position are all s
   produced by `expand-input'.  Consequently, `input=?' will fall back
   to using `equal?' instead of `derivation=?'.  I don't know to what
   extent this has to do with the observed bug.

- ulfvonbelow


signature.asc
Description: PGP signature

bug#65665: package-mapping with #:deep? #t doesn't get all the implicit inputs

[Ulf Herrman]

Ludovic Courtès  writes:

> I don’t know, should we start by having a proper bug report for this and
> study how this happen?

Does that mean opening a new issue, or what?  The bug you've described
is the one this issue was initially opened for.

> Again I’m sorry if I’m slow to understand, but I’d like to make sure we
> have a good understanding of the problem before we start discussing
> solutions.

Okay, I suppose I should have given a concrete example of the behavior.
The qgit example can fill that role:

$ cat $(./pre-inst-env guix build qgit -n --derivations --no-grafts 
--with-latest=qtbase)


The way this happens is that qt-build-system's bag-builder
(e.g. qt-build, qt-cross-build)) introduces a reference to the qtbase
from its #:qtbase argument into the gexp it creates a derivation from,
completely independently of any inputs.  qgit doesn't explicitly specify
#:qtbase, and qt-build-system's 'lower' procedure doesn't add one, so
the default value for that keyword argument, (default-qtbase), is used
in qt-build.

This default value can only be overridden by explicitly passing #:qtbase
as a package or bag argument.  This requirement doesn't map well to a
generic package transformation interface at all - it requires that every
transformer (e.g. package-mapping) check if the package it's
transforming is using qt-build-system, and if so explicitly supply a
#:qtbase that is the result of transforming the original implicit or
explicit value, after somehow figuring out what that may be (currently
only doable by manually reading guix/build-system/qt.scm).

This behavior is also currently exhibited by all build systems' handling
of #:guile.  Here's a concrete example of that, taken from another
mailing I sent to this issue (https://issues.guix.gnu.org/65665#15):

-
(use-modules (guix packages)
 (guix profiles)
 (gnu packages base))

(define guile-named-lyle
  (package
(inherit (default-guile))
(name "lyle")))

;; contrived example that only replaces hello's immediate dependencies
(define hello-transformer
  (package-mapping (lambda (p0)
 (if (eq? p0 (default-guile))
 guile-named-lyle
 p0))
   (lambda (p)
 (not (eq? p hello)))
   #:deep? #t))

(define hello-with-lyle
  (hello-transformer hello))

(packages->manifest (list hello hello-with-lyle))

;; $ guix build --derivations --manifest=THISFILE
;; Expectation: build for hello-with-lyle is done with guile-named-lyle.
;; Reality: derivation for hello-with-lyle is the same as hello.
-

Hopefully that makes it clear why this is happening.

As for solutions, some options that come to mind:

1. guile and qtbase, instead of being passed to bag builders as a
   separate argument, are passed as a bag input that is looked up by
   magic input label, e.g. "guile-for-build", "qtbase-for-build", etc.
   Seems fragile, but requires no changes to package-mapping, etc.

2. Modify the build systems so that these kinds of implicit arguments
   that are packages are always present in bags, and the defaults for
   those keyword arguments in e.g. qt-build are never used.  This is the
   approach I've taken with
   https://issues.guix.gnu.org/issue/65665/attachment/4/0/3.  This still
   requires that bag arguments that are packages are transformed in
   addition to inputs.

I hope that counts as a proper bug report.

- Ulf


signature.asc
Description: PGP signature

bug#65665: package-mapping with #:deep? #t doesn't get all the implicit inputs

[Ulf Herrman]

Ludovic Courtès  writes:

> Hello,
>
> Ulf Herrman  skribis:
>
>> That and a growing thirst for a nuclear option for package rewriting
>> brought about by trying to debug deep transformations while
>> simultaneously experimenting with rewriting a manifest of around 270
>> packages.
>
> On that topic of a catch-all option for rewriting: there’s an unused
> procedure called ‘map-derivation’, which performs rewriting at the level
> of derivations.  It’s harder to use, more expensive, but who knows,
> perhaps we’ll find a motivating use case…  (Allowing for graph rewriting
> has been a major goal for me since the beginning.)
>
>> There are really several distinct issues at play here:
>> 1. The case of #:qtbase and #:guile being invisible to package-mapping.
>>This is what I first noticed, and cannot be fixed without modifying
>>the build systems.  This is what prompted looking for packages in
>>package and bag arguments, and recursing into lists therein (just in
>>case an argument took a list of packages).
>
> How are #:qtbase and #:guile invisible to package mapping?
>
> They appear in the bag inputs and thus are definitely visible to
> ‘package-mapping’, as a I showed with the CMake and Python examples in
> this thread.

"Invisible to package-mapping" was perhaps not the clearest phrasing;
"not completely replaced by package-mapping" might be better.  qtbase
does indeed go in the bag inputs, but replacing the bag inputs has no
effect on the bag arguments, and even if we replaced it in the bag
arguments as well, it might not even be in there to begin with, in which
case the bag-builder would introduce a default that is only visible at
the level of derivations.

>
> For good measure :-) here’s an example with #:qtbase:
>
> $ ./pre-inst-env guix build qgit -n
> substitute: updating substitutes from 'http://192.168.1.48:8123'... 100.0%
> substitute: updating substitutes from 'https://ci.guix.gnu.org'... 100.0%
> 0.4 MB would be downloaded:
>   /gnu/store/7b20q17yg90b62404chgbnwgvd6ry1qf-qgit-2.10
> $ ./pre-inst-env guix build qgit -n --with-latest=qtbase
> following redirection to 
> `https://mirrors.ocf.berkeley.edu/qt/official_releases/qt/'...
> following redirection to 
> `https://mirrors.ocf.berkeley.edu/qt/official_releases/qt/6.6/'...
> guix build: warning: cannot authenticate source of 'qtbase', version 6.6.0
>
> Starting download of /tmp/guix-file.CTehnY
> From 
> https://mirrors.ocf.berkeley.edu/qt/official_releases/qt/6.6/6.6.0/submodules/qtbase-everywhere-src-6.6.0.tar.xz...
>  …-src-6.6.0.tar.xz  46.1MiB  
>   
> 12.9MiB/s 00:04 ▕██▏ 100.0%
> substitute: updating substitutes from 'http://192.168.1.48:8123'... 100.0%
> substitute: updating substitutes from 'https://ci.guix.gnu.org'... 100.0%
> substitute: updating substitutes from 'https://bordeaux.guix.gnu.org'... 
> 100.0%
> substitute: updating substitutes from 'https://guix.bordeaux.inria.fr'... 
> 100.0%
> The following derivations would be built:
>   /gnu/store/paixxkdaakv55bffggxx4l9hiknl8i5r-qgit-2.10.drv
>   /gnu/store/f9fdjk1g1s1aqmlmi4clla2kqns7283v-qtbase-6.6.0.drv
> 0.4 MB would be downloaded:
>   /gnu/store/nl9dadzfmjm9wg7v3r31jkx773dl683x-module-import-compiled
>   /gnu/store/6zryxmypw0wygayc9pvhyxkx47w0lyci-gperf-3.1
>   /gnu/store/a57n7wy8mdi7l52pr4zg07132blgj5xp-qgit-2.10-checkout

And if you do a quick 'cat
/gnu/store/paixxkdaakv55bffggxx4l9hiknl8i5r-qgit-2.10.drv'...

--
Derive
([("out","/gnu/store/z8qrhfmicylxy2mwvcvh9sizfhd3x4d3-qgit-2.10","","")]
 ,[("/gnu/store/0jk33gpzyicyppbnh458213007v0qjhs-mesa-23.1.4.drv",["out"])
   ,("/gnu/store/0njvvgjlb52abhqnmb4rx22sfkxm2h9c-gcc-11.3.0.drv",["out"])
   ,("/gnu/store/0p1sns81qbgr8ayiv02fv4rm5drcycd7-libxdamage-1.1.5.drv",["out"])
   ,("/gnu/store/14a2ban238fng3c8632lrfkmz54y7m2c-binutils-2.38.drv",["out"])
   ,("/gnu/store/1n39zcbr528b7rh9bf1pwfrm7mv8nr8m-bzip2-1.0.8.drv",["out"])
   
,("/gnu/store/2pv3mjjiwh37b0m3m1hijxifnchrw76i-libpciaccess-0.16.drv",["out"])
   ,("/gnu/store/3fy3bf7wysi1n1qz9jz8xzx11sgy8m6d-git-2.41.0.drv",["out"])
   ,("/gnu/store/3r0r8j76l0qvxasmb7rgn7lvpikjdyn1-libxdmcp-1.1.3.drv",["out"])
   ,("/gnu/store/4jfy1ca1d5772z15jcyk1v8wdwdcllbi-gzip-1.12.drv",["out"])
   ,("/gnu/store/5nvwagz2hphvlax2bnj93smr1rgrzr8l-libx11-1.8.1.drv",["out"])
   ,("/gnu/store/64vwaah2spd7q66hji6sm1j2fl6pd1rn-diffutils-3.8.drv",["ou

bug#66510: `this-package' references reintroduce pre-transformation packages.

[Ulf Herrman]

Suppose you have a package that is using a gexp in its argument list
and, like a good citizen of the gexp world, it uses this-package-input
to refer to its own input packages.  In fact, let's suppose that it's
the model citizen depicted in
https://guix.gnu.org/en/blog/2021/the-big-change/ under the
"G-expressions and self-referential records" heading:

(define hello
  (package
(name "hello")
;; …
(arguments
 (list #:configure-flags
   #~(list (string-append "--with-gawk="
  #$(this-package-input "gawk")
(inputs `(("gawk" ,gawk)

If we define a variant like so:

(define hello-variant
  (package
(inherit hello)
(name "hello-variant")
(inputs `(("gawk" ,gawk-4.0)

it will work just fine.  But if we define a variant like SO:

(define hello-variant
  (package
(inherit hello)
(name "hello-variant")
(inputs `(("gawk" ,gawk-4.0)))
(arguments
 (substitute-keyword-arguments (package-arguments hello)
   ((#:configure-flags flags #~'())
#~(cons "--with-hospitality=ice-cream"
#$flags))

it will NOT work just fine.  When (package-arguments hello) is
evaluated, it will execute the field definition for `hello' with
`this-package' bound to `hello', rather than `hello-variant'.
Consequently, `this-package-input' will return gawk rather than
gawk-4.0.  We need a way to access the "parent" package's fields while
keeping `this-package' bound to its current value.  The most general
form of this would look something like this:

(define (package-arguments-with-package p0 p)
  (match p0
(($  _ _ _ _ arguments-proc)
 (arguments-proc p

Then hello-variant could be changed to use
(package-arguments-with-package hello this-package)
instead of (package-arguments hello).  This may be needlessly general,
though; the problem could also be solved with an interface more along
the lines of

(parent-package-arguments hello)

which expands into the aforementioned package-arguments-with-package
call.

Another option would be to, yet again, extend the record syntax.  It's
always bugged me a bit to have to explicitly reference the original
record in more than one place when using derived fields, so this might
be generally useful as well:

(define hello-variant
  (package
(inherit hello
 (arguments hello-arguments))
(name "hello-variant")
(inputs `(("gawk" ,gawk-4.0)))
(arguments
 (substitute-keyword-arguments hello-arguments
   ((#:configure-flags flags #~'())
#~(cons "--with-hospitality=ice-cream"
#$flags))

This would create a macro named `hello-arguments' within the scope of
the (package ...) form which expands into something equivalent to a
`parent-package-arguments' call.  Adjust syntax to taste.

Thoughts?

- Ulf


signature.asc
Description: PGP signature

bug#65665: package-mapping with #:deep? #t doesn't get all the implicit inputs

[Ulf Herrman]

Ludovic Courtès  writes:

> Ulf Herrman  skribis:
>
>> -(define (build-system-with-package-mapping bs rewrite)
>> +(define (build-system-with-package-mapping bs rewrite-input 
>> rewrite-argument)
>>"Return a variant of BS, a build system, that rewrites a bag's inputs by
>>  passing them through REWRITE, a procedure that takes an input tuplet and
>>  returns a \"rewritten\" input tuplet."
>> @@ -1442,9 +1442,10 @@ (define (build-system-with-package-mapping bs rewrite)
>>  (let ((lowered (apply lower args)))
>>(bag
>>  (inherit lowered)
>> -(build-inputs (map rewrite (bag-build-inputs lowered)))
>> -(host-inputs (map rewrite (bag-host-inputs lowered)))
>> -(target-inputs (map rewrite (bag-target-inputs lowered))
>> +(build-inputs (map rewrite-input (bag-build-inputs lowered)))
>> +(host-inputs (map rewrite-input (bag-host-inputs lowered)))
>> +(target-inputs (map rewrite-input (bag-target-inputs lowered)))
>> +(arguments (map rewrite-argument (bag-arguments lowered))
>
> Aah, now I understand.  :-)
>
> It’s indeed the case that arguments can capture references to packages
> that won’t be caught by ‘package-mapping’.  For instance, if you write:
>
>   (package
> …
> (arguments (list … #~(whatever #$coreutils
>
> … then ‘coreutils’ here cannot be replaced.
>
> To address this, the recommendation is to always add dependencies to
> input fields and to use self-references within arguments.  The example
> above should be written like this:
>
>   (package
> …
> (arguments
>  (list … #~(whatever #$(this-package-input "coreutils")
>
> It’s just a recommendation and one can perfectly ignore it, and I
> suppose that was the impetus for this patch.

That and a growing thirst for a nuclear option for package rewriting
brought about by trying to debug deep transformations while
simultaneously experimenting with rewriting a manifest of around 270
packages.

There are really several distinct issues at play here:
1. The case of #:qtbase and #:guile being invisible to package-mapping.
   This is what I first noticed, and cannot be fixed without modifying
   the build systems.  This is what prompted looking for packages in
   package and bag arguments, and recursing into lists therein (just in
   case an argument took a list of packages).
2. The (perceived) case of packages hiding inside arguments.  In
   hindsight, this was probably actually (3) causing this, though it's
   hard to tell because I discovered it last.  I attempted to resolve
   this by recursing through s and s.
3. `this-package' referring to the inherited package in thunked fields,
   rather than the package inheriting them.  This is what prompted the
   use of package-{inputs,arguments,etc}-with-package.

(1) could be resolved in several ways.  I'm partial to looking for
arguments whose values are packages and transforming them (when #:deep?
#t is specified), and adjusting the build systems to make that work
consistently, which is what I've done.

(2) is probably not an issue, though it occurs to me that the technique
of recursively searching through arguments looking for packages could be
used to implement a sort of automated "transformability" check, which
could help a lot when trying to debug transformations.

(3) is a major issue; the entire strategy of using `this-package-input'
to enable transformations breaks because of it.  My fix works for me at
least, though it requires exposing additional low-level procedures and
transformation authors using them.  I'll open another bug about it, as
requested.

> This is one of the things discussed while designing this change:
>
>   https://guix.gnu.org/en/blog/2021/the-big-change/
>   (search for “self-referential records”)
>
>   https://issues.guix.gnu.org/49169
>
> My take was and still is that it’s an acceptable limitation.  Packagers
> need to follow the guideline above if they want proper support for
> rewriting, ‘guix graph’, and other tools.
>
> WDYT?

I think it's probably reasonable, though I would like it if there were
tooling in place to detect cases where said guidelines are not
followed, so as to aid in debugging and verifying that a transformation
worked as intended.

- Ulf


signature.asc
Description: PGP signature

bug#65665: package-mapping with #:deep? #t doesn't get all the implicit inputs

[Ulf Herrman]

Maxim Cournoyer  writes:

> Any build system accepting package as arguments are subject to this
> problem, if I understand correctly.

It's not quite everywhere a package is accepted as an argument: there
are many cases where a package is passed as a package argument but
doesn't end up in the arguments list of the corresponding bag.  This is
usually the case because that argument is only overriding a default
package that is just added as an input to the bag, with no special
treatment aside from being an implicit input.  For example, #:cmake in
cmake-build-system works this way.

This is however not the case for #:qtbase in qt-build-system, and, much
more prominently, for #:guile.  Neither qtbase nor guile are added to
bag inputs implicitly, but they do end up in the final builder, even if
not specified.

Concretely, this looks like this:
-
(use-modules (guix packages)
 (guix profiles)
 (gnu packages base))

(define guile-named-lyle
  (package
(inherit (default-guile))
(name "lyle")))

;; contrived example that only replaces hello's immediate dependencies
(define hello-transformer
  (package-mapping (lambda (p0)
 (if (eq? p0 (default-guile))
 guile-named-lyle
 p0))
   (lambda (p)
 (not (eq? p hello)))
   #:deep? #t))

(define hello-with-lyle
  (hello-transformer hello))

(packages->manifest (list hello hello-with-lyle))

;; $ guix build --derivations --manifest=THISFILE
;; Expectation: build for hello-with-lyle is done with guile-named-lyle.
;; Reality: derivation for hello-with-lyle is the same as hello.
-
(and I have confirmed that the above results in guile-named-lyle being
used with the proposed patches applied)

A similar problem manifests when one tries replacing (default-qtbase) in
a package using qt-build-system.  Both it and guile are in bag arguments
and not inputs because it's not enough that they be included as inputs,
the builder must also know which one is considered "used by the build
system" if there are multiple.  One could argue that this ambiguity also
exists with other build systems - "which cmake should be used, which gcc
should be used", etc - but they choose to resolve it with "eh, whatever
shows up first in PATH matching the required name is probably fine".
It's not unimaginable that there could be cases where explicitly
specifying which package should be used for a particular role would be
necessary, though.

- Ulf


signature.asc
Description: PGP signature

bug#65665: [PATCH] Really get all the implicit inputs.

[Ulf Herrman]

Ulf Herrman  writes:

> Anyway, I've since forgotten exactly what happened and why (I didn't
> actually write the commit message until quite some time later), but I
> do know that this patch fixed it.
>
> I might try to rediscover what the problem was later, but thought it
> would be good to make you aware of this so as to hopefully consolidate
> world rebuilds.

I've since done some more checking to recall what the problem actually
was, and it indeed manifested as a combination of an icecat and mesa
problem: the wrap-program phase of icecat-minimal uses
`this-package-input' to get the mesa to point LD_LIBRARY_PATH to.
However, it then stuffs the resulting package inside a 
object, which we don't currently recurse through, so it ends up
compiling with one mesa and using another at runtime, which somehow
causes a segmentation fault.

Having looked at it again, I'm not sure that rebinding `this-package' is
the best solution - it's certainly not a general solution, since any old
package could be shoved into a  object (or really any
declarative file-like object) and thereby be hidden from
transformations.  My understanding is that packaging guidelines already
discourage directly substituting top-level package references,
preferring instead tools like `this-package-input' so as to work nicely
with transformations.  If those guidelines are adhered to, the
aforementioned patch should fix issues of this nature.  I'm not sure
what the best way of handling objects like  and such is,
but as long as package references go through `this-package', it should
only matter for implicit inputs, and I don't think any of them use
declarative file-like objects other than .

After thinking about it some more, I think it would be good if we had a
way of testing to make sure that every package is "transformable": that
is, if you apply a deep transformation to it, and lower the result to a
derivation, at no point within the dynamic extent of that lowering is a
derivation from an untransformed package introduced.  This would allow
for testing for transformability en masse, and could be added to 'guix
lint'.

- Ulf


signature.asc
Description: PGP signature

bug#65665: [PATCH] Really get all the implicit inputs.

[Ulf Herrman]

I've also been using this patch, which rebinds `this-package' within
package variants to refer instead to the variant rather than the
original.  When I tried applying a deep transformation to icecat, it
failed to reach icecat-minimal or something like that.  Or maybe it was
some input of icecat - might have been my local version of mesa that had
breakage.  Anyway, I've since forgotten exactly what happened and why (I
didn't actually write the commit message until quite some time later),
but I do know that this patch fixed it.

I might try to rediscover what the problem was later, but thought it
would be good to make you aware of this so as to hopefully consolidate
world rebuilds.

- Ulf

From 06245c37fc0ee21144279682223dff01f3e6dbf4 Mon Sep 17 00:00:00 2001
Message-Id: <06245c37fc0ee21144279682223dff01f3e6dbf4.1696574074.git.strin...@tilde.club>
From: Ulf Herrman 
Date: Wed, 4 Oct 2023 03:17:25 -0500
Subject: [PATCH] guix: packages: rebind `this-package' for package variants.

Background: there is a `this-package' identifier that can be used within any
of the thunked fields of .  This is implemented by passing the
package in question as an argument to the "thunk" that is called to get the
field value.  `this-package' is used to implement macros like
`this-package-input'.  Consequently, there is a subtle difference between a
package that inherits another package's thunked field and one that wraps it:
in the former, the `this-package' of the thunked field will refer to the
inheriting package, while in the latter case it will refer to the original
package.

Use of `this-package' tends to cause lots of subtle and hard-to-debug breakage
when package transformations are used.  In `package-mapping', it makes more
sense to have `this-package' refer to the transformed package (that is, the
package after being transformed by the user-provided procedure and having all
of its applicable inputs transformed, recursively), since otherwise
untransformed inputs may be reintroduced via `this-package'.

There's still one place where `this-package' can cause problems, though, and
that's in the user-specified transformation procedure itself.  Whether it's
appropriate to have the thunks of the original refer to the original package
or the transformed package is only really known by the author of the
transformation.  We therefore expose procedures for invoking the field thunks
with arbitrary packages.

In my experience, this has led to far more consistent and predictable
package-transforming.

If we really need the ability to refer to the lexically enclosing package
instead of "whichever package ends up using this field definition" - which
seems to be unnecessary at present, since the overwhelming majority of uses of
'this-package' are in the context of 'this-package-input', etc - we could
introduce a 'this-literal-package' identifier.  Or we could rename all current
uses of 'this-package' to use 'this-package-variant' or something like that,
though that sounds like more work.

* guix/packages.scm (package-inputs-with-package,
  package-native-inputs-with-package, package-propagated-inputs-with-package,
  package-arguments-with-package):
  new procedures.
  (package-mapping): use them.
---
 guix/packages.scm | 61 ---
 1 file changed, 47 insertions(+), 14 deletions(-)

diff --git a/guix/packages.scm b/guix/packages.scm
index 1334def142..288867b911 100644
--- a/guix/packages.scm
+++ b/guix/packages.scm
@@ -138,6 +138,11 @@ (define-module (guix packages)
 package-transitive-propagated-inputs
 package-transitive-native-search-paths
 package-transitive-supported-systems
+package-inputs-with-package
+package-native-inputs-with-package
+package-propagated-inputs-with-package
+package-arguments-with-package
+
 package-mapping
 package-input-rewriting
 package-input-rewriting/spec
@@ -1451,6 +1456,31 @@ (define (build-system-with-package-mapping bs rewrite-input rewrite-argument)
 (inherit bs)
 (lower lower*)))
 
+(define (package-inputs-with-package p0 p)
+  (match p0
+(($  _ _ _ _ args-proc
+   inputs-proc
+   propagated-inputs-proc
+   native-inputs-proc)
+ (inputs-proc p
+
+(define (package-native-inputs-with-package p0 p)
+  (match p0
+(($  _ _ _ _ _ _ _
+   native-inputs-proc)
+ (native-inputs-proc p
+
+(define (package-propagated-inputs-with-package p0 p)
+  (match p0
+(($  _ _ _ _ _ _
+   propagated-inputs-proc)
+ (propagated-inputs-proc p
+
+(define (package-arguments-with-package p0 p)
+  (match p0
+(($  _ _ _ _ arguments-proc)
+ (arguments-proc p
+
 (define* (package-mapping proc #:optional (cut? (const #f))
   #:key deep?)
   "Return a procedure that, given a package, a

bug#65665: [PATCH] Really get all the implicit inputs.

[Ulf Herrman]

This patch series causes package-mapping to recurse into package and bag
arguments when #:deep? #t is given.  It also recurses into gexps and
gexp inputs in search of packages to devour.  It also ensures that build
systems leave all of their implicit package arguments in the bag
arguments, ready to be found by package-mapping.  It also fixes a couple
build system errors I came across while modifying 40+ build systems and
testing a deep package transformation.

- ulfvonbelow

From 73b2dfe98591073104fd069622ede2acab0c7655 Mon Sep 17 00:00:00 2001
Message-Id: <73b2dfe98591073104fd069622ede2acab0c7655.1694806866.git.strin...@tilde.club>
From: Ulf Herrman 
Date: Fri, 15 Sep 2023 07:38:24 -0500
Subject: [PATCH 1/5] guix: packages: rewrite arguments with `package-mapping'.

* guix/gexp.scm (make-gexp, gexp-references, gexp-self-modules,
  gexp-self-extensions, gexp-proc, %gexp-location): export.
* guix/packages.scm (build-system-with-package-mapping): replace `rewrite'
  argument with `rewrite-input' and `rewrite-argument' arguments.  Use
  `rewrite-argument' to rewrite bag arguments.
  (package-mapping): rewrite package arguments when `deep?' is true.
---
 guix/gexp.scm |  7 ++
 guix/packages.scm | 56 ++-
 2 files changed, 53 insertions(+), 10 deletions(-)

diff --git a/guix/gexp.scm b/guix/gexp.scm
index 0fe4f1c98a..6acd3116d4 100644
--- a/guix/gexp.scm
+++ b/guix/gexp.scm
@@ -39,6 +39,13 @@ (define-module (guix gexp)
   #:use-module (ice-9 match)
   #:export (gexp
 gexp?
+make-gexp
+gexp-references
+gexp-self-modules
+gexp-self-extensions
+gexp-proc
+%gexp-location
+
 sexp->gexp
 with-imported-modules
 with-extensions
diff --git a/guix/packages.scm b/guix/packages.scm
index ba98bb0fb4..1334def142 100644
--- a/guix/packages.scm
+++ b/guix/packages.scm
@@ -1431,7 +1431,7 @@ (define* (package-closure packages #:key (system (%current-system)))
(vhash-consq package #t visited)
(fold set-insert closure dependencies
 
-(define (build-system-with-package-mapping bs rewrite)
+(define (build-system-with-package-mapping bs rewrite-input rewrite-argument)
   "Return a variant of BS, a build system, that rewrites a bag's inputs by
 passing them through REWRITE, a procedure that takes an input tuplet and
 returns a \"rewritten\" input tuplet."
@@ -1442,9 +1442,10 @@ (define (build-system-with-package-mapping bs rewrite)
 (let ((lowered (apply lower args)))
   (bag
 (inherit lowered)
-(build-inputs (map rewrite (bag-build-inputs lowered)))
-(host-inputs (map rewrite (bag-host-inputs lowered)))
-(target-inputs (map rewrite (bag-target-inputs lowered))
+(build-inputs (map rewrite-input (bag-build-inputs lowered)))
+(host-inputs (map rewrite-input (bag-host-inputs lowered)))
+(target-inputs (map rewrite-input (bag-target-inputs lowered)))
+(arguments (map rewrite-argument (bag-arguments lowered))
 
   (build-system
 (inherit bs)
@@ -1456,13 +1457,32 @@ (define* (package-mapping proc #:optional (cut? (const #f))
 depended on and returns the resulting package.  The procedure stops recursion
 when CUT? returns true for a given package.  When DEEP? is true, PROC is
 applied to implicit inputs as well."
-  (define (rewrite input)
+  (define (rewrite-input input)
 (match input
   ((label (? package? package) outputs ...)
(cons* label (replace package) outputs))
   (_
input)))
 
+  (define (rewrite-argument arg)
+(match arg
+  ((? package? p)
+   (replace p))
+  ((? gexp-input? gi)
+   (gexp-input (rewrite-argument (gexp-input-thing gi))
+   (gexp-input-output gi)
+   #:native? (gexp-input-native? gi)))
+  ((? gexp? gxp)
+   (make-gexp (map rewrite-argument (gexp-references gxp))
+  (gexp-self-modules gxp)
+  (gexp-self-extensions gxp)
+  (gexp-proc gxp)
+  (%gexp-location gxp)))
+  ((lst ...)
+   (map rewrite-argument lst))
+  (_
+   arg)))
+
   (define mapping-property
 ;; Property indicating whether the package has already been processed.
 (gensym " package-mapping-done"))
@@ -1484,7 +1504,8 @@ (define* (package-mapping proc #:optional (cut? (const #f))
  (inherit p)
  (location (package-location p))
  (replacement (package-replacement p))
- (propagated-inputs (map rewrite (package-propagated-inputs p)))
+ (propagated-inputs (map rewrite-input
+ (package-propagated-inputs p)))
  (properties `((,mapping-property . #t)

bug#65665: package-mapping with #:deep? #t doesn't get all the implicit inputs

[Ulf Herrman]

#:deep? #t currently works by interposing a dummy build system that
lowers the package to a bag using the original build system, then
applies the supplied transformation to all of the bag's inputs, then
returns a new bag with the new inputs.

The problem with this approach is that it doesn't affect the bag
arguments.  This means that packages passed in as arguments may still
end up being used without being transformed.  Worse still, packages
*not* passed in as arguments may end up being used *unless one is
explicitly passed in as an argument*, as is the case with
qt-build-system's #:qtbase argument.

In short, the current approach of having the build-system lower
procedure leave the arguments mostly unchanged and letting the
bag->derivation procedure (the "bag builder") fill in lots of defaults
means that there are implicit inputs that cannot be touched at the
package level without adding special logic for every single build system
that does something like this.

I propose that we have the build system lower procedure (that is, the
one that converts from package to bag) completely fill in the argument
list with all defaults, and we modify build-system-with-package-mapping
to transform all arguments that look like a package or a list of
packages (or maybe even a tree containing packages).

Many large-scale package transformations have their purpose defeated if
even a single package slips through and has to be built or downloaded
separately (e.g. "I wanted to use a more minimal version of P, and now I
have both the original version of P *and* a more minimal version of P
*and* a duplicate copy of a large portion of the package graph...").  In
fact, in some situations, this could cause exponential growth of the
number of packages, e.g. a transformation is applied to qtbase and its
dependencies, but not to the implicit version and its dependencies, so
now all the dependencies of qtbase are duplicated, and if this happens
again at a higher level with something that depends on a package with
qt-build-system, now there are four duplicate subgraphs, and so on.

What do you think?

- Ulf


signature.asc
Description: PGP signature

bug#63319: [PATCH 3/3] profiles: remove `parent' field.

[Ulf Herrman]

This field was only present for consumption by (guix ui) when reporting
propagation chains that lead to profile collision errors, but it is only valid
in general with respect to a single manifest.  (guix ui) now derives parent
information by itself with respect to an explicit manifest, so this field is
no longer needed.

* guix/profiles.scm (manifest-entry-parent): remove field.
  (package->manifest-entry, sexp->manifest): do not populate it.
  (manifest->gexp): adjust match specifications to account for its absence.
* guix/inferior.scm (inferior-package->manifest-entry): do not populate
  nonexistent parent field.
---
 guix/inferior.scm |  36 ++
 guix/profiles.scm | 123 +++---
 2 files changed, 67 insertions(+), 92 deletions(-)

diff --git a/guix/inferior.scm b/guix/inferior.scm
index 5dfd30a6c8..4030640f6d 100644
--- a/guix/inferior.scm
+++ b/guix/inferior.scm
@@ -819,27 +819,23 @@ (define-syntax-rule (memoized package output exp)
 result
 
   (let loop ((package package)
- (output  output)
- (parent  (delay #f)))
+ (output  output))
 (memoized package output
-  ;; For each dependency, keep a promise pointing to its "parent" entry.
-  (letrec* ((deps  (map (match-lambda
-  ((label package)
-   (loop package "out" (delay entry)))
-  ((label package output)
-   (loop package output (delay entry
-(inferior-package-propagated-inputs package)))
-(entry (manifest-entry
- (name (inferior-package-name package))
- (version (inferior-package-version package))
- (output output)
- (item package)
- (dependencies (delete-duplicates deps))
- (search-paths
-  (inferior-package-transitive-native-search-paths 
package))
- (parent parent)
- (properties properties
-entry
+  (let ((deps  (map (match-lambda
+  ((label package)
+   (loop package "out"))
+  ((label package output)
+   (loop package output)))
+(inferior-package-propagated-inputs package
+(manifest-entry
+  (name (inferior-package-name package))
+  (version (inferior-package-version package))
+  (output output)
+  (item package)
+  (dependencies (delete-duplicates deps))
+  (search-paths
+   (inferior-package-transitive-native-search-paths package))
+  (properties properties))
 
 
 ;;;
diff --git a/guix/profiles.scm b/guix/profiles.scm
index b812a6f7d9..0d22667362 100644
--- a/guix/profiles.scm
+++ b/guix/profiles.scm
@@ -90,7 +90,6 @@ (define-module (guix profiles)
 manifest-entry-item
 manifest-entry-dependencies
 manifest-entry-search-paths
-manifest-entry-parent
 manifest-entry-properties
 lower-manifest-entry
 
@@ -229,8 +228,6 @@ (define-record-type*  manifest-entry
 (default '()))
   (search-paths manifest-entry-search-paths   ; search-path-specification*
 (default '()))
-  (parent   manifest-entry-parent; promise (#f | )
-(default (delay #f)))
   (properties   manifest-entry-properties ; list of symbol/value pairs
 (default '(
 
@@ -416,29 +413,23 @@ (define (default-properties package)
 (transformations `((transformations . ,transformations)
 
 (define* (package->manifest-entry package #:optional (output "out")
-  #:key (parent (delay #f))
   (properties (default-properties package)))
   "Return a manifest entry for the OUTPUT of package PACKAGE."
-  ;; For each dependency, keep a promise pointing to its "parent" entry.
-  (letrec* ((deps  (map (match-lambda
-  ((label package)
-   (package->manifest-entry package
-#:parent (delay entry)))
-  ((label package output)
-   (package->manifest-entry package output
-#:parent (delay entry
-(package-propagated-inputs package)))
-(entry (manifest-entry
- (name (package-name package))
- (version (package-version package))
- (output output)
- (item 

bug#63319: [PATCH 2/3] ui: derive parents of profile collision entries from manifest.

[Ulf Herrman]

This fixes .

* guix/ui.scm (display-collision-resolution-hint, call-with-error-handling):
  use manifest-entry->parents and the manifest included with the collision to
  get the parents.
---
 guix/ui.scm | 30 +-
 1 file changed, 25 insertions(+), 5 deletions(-)

diff --git a/guix/ui.scm b/guix/ui.scm
index 5d2ae23c25..71fe5caa72 100644
--- a/guix/ui.scm
+++ b/guix/ui.scm
@@ -674,9 +674,19 @@ (define unit
 
 (define (display-collision-resolution-hint collision)
   "Display hints on how to resolve COLLISION, a "
+  (define manifest (profile-collision-error-manifest collision))
+
+  (define entry->parents
+(manifest-entry->parents manifest))
+
+  (define (manifest-entry-parent entry)
+(match (entry->parents entry)
+  (() #f)
+  ((x . _) x)))
+
   (define (top-most-entry entry)
 (let loop ((entry entry))
-  (match (force (manifest-entry-parent entry))
+  (match (manifest-entry-parent entry)
 (#f entry)
 (parent (loop parent)
 
@@ -751,9 +761,19 @@ (define (port-filename* port)
  (output output)
  ((profile-collision-error? c)
   (let ((entry(profile-collision-error-entry-lowered c))
-(conflict (profile-collision-error-conflict-lowered c)))
+(conflict (profile-collision-error-conflict-lowered c))
+(manifest (profile-collision-error-manifest c)))
+
+(define entry->parents
+  (manifest-entry->parents manifest))
+
+(define (manifest-entry-parent entry)
+  (match (entry->parents entry)
+(() #f)
+((x . rest) x)))
+
 (define (report-parent-entries entry)
-  (let ((parent (force (manifest-entry-parent entry
+  (let ((parent (manifest-entry-parent entry)))
 (when (manifest-entry? parent)
   (report-error (G_ "   ... propagated from ~a@~a~%")
 (manifest-entry-name parent)
@@ -773,13 +793,13 @@ (define (manifest-entry-output* entry)
   (manifest-entry-version entry)
   (manifest-entry-output* entry)
   (manifest-entry-item entry))
-(report-parent-entries entry)
+(report-parent-entries (profile-collision-error-entry c))
 (report-error (G_ "  second entry: ~a@~a~a ~a~%")
   (manifest-entry-name conflict)
   (manifest-entry-version conflict)
   (manifest-entry-output* conflict)
   (manifest-entry-item conflict))
-(report-parent-entries conflict)
+(report-parent-entries (profile-collision-error-conflict c))
 (display-collision-resolution-hint c)
 (exit 1)))
  ((nar-error? c)
-- 
2.39.1

bug#63319: [PATCH 0/3]

[Ulf Herrman]

This patch series fixes issue #63319, making propagation chains leading to
profile collisions be reported properly.  It does this by having
'check-for-collisions' fill in the  with the manifest
and the lowered versions of the entries in question, and (guix ui) derive
parent information from the manifest.  It then removes the now-unused 'parent'
field of .

Ulf Herrman (3):
  profiles: include non-lowered entries and manifest in collision error.
  ui: derive parents of profile collision entries from manifest.
  profiles: remove `parent' field.

 guix/inferior.scm |  36 -
 guix/profiles.scm | 183 +-
 guix/ui.scm   |  32 ++--
 3 files changed, 142 insertions(+), 109 deletions(-)

-- 
2.39.1

bug#63319: [PATCH 1/3] profiles: include non-lowered entries and manifest in collision error.

[Ulf Herrman]

This provides the necessary information for (guix ui) to accurately determine
the actual entries causing the collision.  The entries alone aren't enough,
since they inherit their parent (singular!) field from whatever it happened to
be before any manifest transaction was applied.  The lowered variants are
included because (guix ui) needs them for reporting store paths, and the
non-lowered variants are included so that the proper parents can be derived
from the included manifest, which must contain them.

We also add and export a convenience procedure for finding the parents of menu
entries in a particular manifest.

* guix/profiles.scm (profile-collision-error-entry-lowered,
  profile-collision-error-conflict-lowered, profile-collision-error-manifest):
  new fields.
  (check-for-collisions): populate them.
  (manifest-entry->parents): new procedure.
* guix/ui.scm (call-with-error-handling): use lowered entries.
---
 guix/profiles.scm | 60 ++-
 guix/ui.scm   |  4 ++--
 2 files changed, 51 insertions(+), 13 deletions(-)

diff --git a/guix/profiles.scm b/guix/profiles.scm
index 0785f9..b812a6f7d9 100644
--- a/guix/profiles.scm
+++ b/guix/profiles.scm
@@ -64,7 +64,10 @@ (define-module (guix profiles)
 
 profile-collision-error?
 profile-collision-error-entry
+profile-collision-error-entry-lowered
 profile-collision-error-conflict
+profile-collision-error-conflict-lowered
+profile-collision-error-manifest
 
 missing-generation-error?
 missing-generation-error-generation
@@ -107,6 +110,7 @@ (define-module (guix profiles)
 manifest-installed?
 manifest-matching-entries
 manifest-search-paths
+manifest-entry->parents
 check-for-collisions
 
 manifest->code
@@ -186,7 +190,10 @@ (define-condition-type  

 (define-condition-type  
   profile-collision-error?
   (entryprofile-collision-error-entry);
-  (conflict profile-collision-error-conflict));
+  (conflict profile-collision-error-conflict) ;
+  (entry-lowered profile-collision-error-entry-lowered)   ;
+  (conflict-lowered profile-collision-error-conflict-lowered) ;
+  (manifest profile-collision-error-manifest));
 
 (define-condition-type  
   unmatched-pattern-error?
@@ -329,6 +336,34 @@ (define (recurse entry)
 (item (derivation->output-path drv output))
 (dependencies dependencies)))
 
+(define (manifest-entry->parents manifest)
+  "Return a procedure that maps each  in MANIFEST to the list
+of s in MANIFEST or their dependencies, recursively, that
+have the entry in question as a direct dependency."
+  (define (visit-entries entries mapping visited?)
+(match entries
+  (((and entry ($  _ _ _ _ dependencies)) . rest)
+   (if (vhash-assq entry visited?)
+   (visit-entries rest mapping visited?)
+   (call-with-values
+   (lambda ()
+ (visit-entries dependencies
+(fold (lambda (dependency mapping)
+(vhash-consq dependency entry mapping))
+  mapping
+  dependencies)
+(vhash-consq entry #t visited?)))
+ (lambda (mapping visited?)
+   (visit-entries rest mapping visited?)
+  (()
+   (values mapping visited?
+
+  (define mapping
+(visit-entries (manifest-entries manifest) vlist-null vlist-null))
+
+  (lambda (entry)
+(vhash-foldq* cons '() entry mapping)))
+
 (define* (check-for-collisions manifest system #:key target)
   "Check whether the entries of MANIFEST conflict with one another; raise a
 '' when a conflict is encountered."
@@ -348,25 +383,28 @@ (define candidates
   (define lower-pair
 (match-lambda
   ((first second)
-   (mlet %store-monad ((first  (lower-manifest-entry first system
- #:target target))
-   (second (lower-manifest-entry second system
- #:target target)))
- (return (list first second))
+   (mlet %store-monad ((first-low  (lower-manifest-entry first system
+ #:target target))
+   (second-low (lower-manifest-entry second system
+ #:target target)))
+ (return (list first first-low second second-low))
 
   ;; Start by lowering CANDIDATES "in parallel".
-  (mlet %store-monad ((lst (mapm/accumulate-builds lower-pair candidates)))
+  (mlet* %store-monad ((lst (mapm/accumulate-builds lower-pair candidates)))
 (foldm %store-monad
(lambda 

bug#63319: Incorrect propagation chain reporting on profile collision

[Ulf Herrman]

A  has a `parent' field that contains a promise that
returns either #f or another .  This is somewhat
incomplete information - a given entry can be a propagated-input to
multiple entries, so there can actually be multiple parents.  Given that
this is only used for tracing a propagation chain to display to the
user, though, that shouldn't be a problem.  What is a problem, though,
is that this field is determined when the manifest is read in and
remains static thereafter.  To demonstrate this, consider the following
example:

User installs package A1, which propagates B1.  User also installs
package C, which also propagates B1.  Later the package definition for A
is updated with a newer version, A2, and this propagates a newer version
of B, B2.  User runs 'guix pull', and subsequently runs 'guix package
-u'.  Note that C is unchanged and still propagates B1.  Guix sees that
the proposed new manifest contains A2, C, B1, and B2, and properly
detects that B1 and B2 represent a collision.  It traces B2 back to A2
correctly, and then goes to hunt down what propagated B1.  What happens
next depends somewhat on the arbitrary ordering of entries in the
manifest, but it is entirely possible that the parent recorded for B1 is
not C, but A1, even though the proposed new manifest doesn't even
contain A1.

So you end up with bizarre error messages like this:

-
$ guix package -u certbot
The following package will be upgraded:
   certbot 1.28.0 → 2.3.0

guix package: error: profile contains conflicting entries for 
python-cryptography
guix package: error:   first entry: python-cryptography@40.0.1 
/gnu/store/bcixgv8vg9bz6gyvijavqiha77h17icv-python-cryptography-40.0.1
guix package: error:... propagated from python-josepy@1.13.0
guix package: error:... propagated from python-acme@2.3.0
guix package: error:... propagated from certbot@2.3.0
guix package: error:   second entry: python-cryptography@3.4.8 
/gnu/store/2kz701xhwvcswdp4dj3fd7v86v2ra70x-python-cryptography-3.4.8
guix package: error:... propagated from python-josepy@1.13.0
guix package: error:... propagated from python-acme@1.28.0
guix package: error:... propagated from certbot@1.28.0
hint: You cannot have two different versions or variants of `certbot' in the 
same
profile.
-

This happens because the collision check is performed on the new
manifest, and the parent fields are leftover from the old manifest.

Ultimately, the parent field is just an optimization - it can always be
re-derived from a manifest, and indeed, when a  is
shared across multiple manifests, *must* be re-derived with respect to a
given manifest in order for it to make sense.  I propose that we remove
the 'parent' field entirely and modify  so that
it also reports the manifest the collision occurred in.  (guix ui) can
then use this to provide more complete (and also accurate) error
messages.  Note that at present check-for-collisions actually can report
manifest entries that aren't "part of" *any* manifest, since they're
freshly created by lower-manifest-entry.



Tangentially related: it's worth noting that only showing two
propagation chains tends to lead to frustrating "whack-a-mole"-style
package transactions, since a decision about which package to keep,
remove, or upgrade has to be made before the next collision source can
be found.  We should probably add a profile mode to 'guix graph' that
shows the propagation graph for a proposed new profile manifest, or at
least let 'guix package' show all collisions when invoked with higher
verbosity.


signature.asc
Description: PGP signature