Hello! zimoun <zimon.touto...@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.touto...@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’.
