Re: Transform options should error on nonexistant targets
Hi, On Wed, 8 Sept 2021 at 22:55, Ludovic Courtès 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
Re: Transform options should error on nonexistant targets
Hello! zimoun skribis: > However, from my opinion, it is easy to check if the package-target is > a package or not, i.e. > > $ guix build foo --=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 > 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’.
Re: Transform options should error on nonexistant targets
Hi Ludo, On Thu, 2 Sept 2021 at 12:06, Ludovic Courtès 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 --=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 From c0fa86d316c91044630b85c9e789f9a455fd29f4 Mon Sep 17 00:00:00 2001 From: zimoun 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 +;;; Copyright © 2021 Simon Tournier ;;; ;;; 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
Re: Transform options should error on nonexistant targets
Hi, zimoun skribis: > On Wed, 18 Aug 2021 at 03:57, Ryan Prior 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
Hi Ryan,
On Wed, 18 Aug 2021 at 03:57, Ryan Prior wrote:
> I learned today that Guix will chug happily along applying a transform to a
> nonexistent package.
>
> For example, I can run:
> guix environment --with-latest=not-exist --ad-hoc which
>
> This shows no warning or errors. I think it would be beneficial to show an
> error & bail if the target of a transformation is a package that doesn't
> exist, as this is likely indicative of an error.
Indeed. :-) The issue comes from guix/transformations.scm
(options->transformation):
--8<---cut here---start->8---
(let ((new (transform obj)))
(when (eq? new obj)
(warning (G_ "transformation '~a' had no effect on ~a~%")
name
(if (package? obj)
(package-full-name obj)
obj)))
new)
--8<---cut here---end--->8---
and the warning is not reach because guix/packages.scm (package-mapping):
--8<---cut here---start->8---
(else
;; Return a variant of P with PROC applied to P and its explicit
;; dependencies, recursively. Memoize the transformations.
Failing
;; to do that, we would build a huge object graph with lots of
;; duplicates, which in turns prevents us from benefiting from
;; memoization in 'package-derivation'.
(let ((p (proc p)))
(package
…
--8<---cut here---end--->8---
In the case of “guix build hello --with-latest=foo”, then ’proc’ has no
effect, i.e., ’p’ is identical to ’(proc p)’ but a new package is
allocated which leads that ’new’ and ’obj’ are not ’eq?’.
Well, I have tried the attached patch. But then ’tests/packages.scm’
fails. Hum, I need an input because I do not know if I miss something
or if the test is also inaccurate.
--8<---cut here---start->8---
(test-assert "package-input-rewriting/spec"
(let* ((dep (dummy-package "chbouib"
(native-inputs `(("x" ,grep)
(p0 (dummy-package "example"
(inputs `(("foo" ,coreutils)
("bar" ,grep)
("baz" ,dep)
(rewrite (package-input-rewriting/spec
`(("coreutils" . ,(const sed))
("grep" . ,(const findutils)))
#:deep? #f))
(p1 (rewrite p0))
(p2 (rewrite p0)))
(and (not (eq? p1 p0))
(eq? p1 p2) ;memoization
--8<---cut here---end--->8---
I miss why ’p1’ should not be the same as ’p0’.
> What do you think?
Maybe, it could be worth to open a report for that. Feel free. ;-)
Cheers,
simon
>From e1cd54f4cccad37f7134b342c8dee9da9fa28588 Mon Sep 17 00:00:00 2001
From: zimoun
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 .
* 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
;;; Copyright © 2019 Marius Bakke
;;; Copyright © 2021 Chris Marusich
+;;; Copyright © 2021 Simon Tournier
;;;
;;; 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
+
