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