bug#65665: [PATCH] Really get all the implicit inputs.
Hi Maxim, On Thu, 05 Oct 2023 at 22:36, Maxim Cournoyer wrote: > I've reviewed it, and it makes sense to me. I'd like to apply it to the > core-updates branch. Core team, what say you? Well, I am not sure to deeply understand some details to get all the implications here, so I have nothing relevant to say. For instance, from my understanding, the core change is: --8<---cut here---start->8--- + (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))) --8<---cut here---end--->8--- --8<---cut here---start->8--- + (arguments + (match p + (($ _ _ _ _ args-proc) +;; If we let ARGS-PROC be passed its original package, +;; we somehow end up in an infinite (or maybe just +;; exponential? Never seen it end...) loop. Should +;; probably figure out what's causing that at some +;; point. +(let ((args (args-proc this-package))) + (if deep? + (map rewrite-argument args) + args) --8<---cut here---end--->8--- and I do not feel enough skilled here for getting the implications. Equally for this kind of changes: --8<---cut here---start->8--- - (arguments (strip-keyword-arguments private-keywords arguments) + (arguments + (substitute-keyword-arguments + (strip-keyword-arguments private-keywords arguments) +((#:guile _ #t) guile)) --8<---cut here---end--->8--- Therefore, I trust other opinions or I need some time for diving and filling some gaps. Cheers, simon
bug#65665: [PATCH] Really get all the implicit inputs.
Hi! Maxim Cournoyer skribis: > Ulf Herrman writes: > >> 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. > > Nice series! > > I've reviewed it, and it makes sense to me. I'd like to apply it to the > core-updates branch. Core team, what say you? Sorry for the delay, I’ll provide feedback within a couple of days. Ludo’.
bug#65665: [PATCH] Really get all the implicit inputs.
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.
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, applies PROC to all the packages @@
bug#65665: [PATCH] Really get all the implicit inputs.
Hello Ulf! Ulf Herrman writes: > 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. Nice series! I've reviewed it, and it makes sense to me. I'd like to apply it to the core-updates branch. Core team, what say you? -- Thanks, Maxim
bug#65665: [PATCH] Really get all the implicit inputs.
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) ,@(package-properties p))) @@ -1500,11
