[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: Transform options should error on nonexistant targets
From: |
Ludovic Courtès |
Subject: |
Re: Transform options should error on nonexistant targets |
Date: |
Wed, 08 Sep 2021 22:55:30 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/27.2 (gnu/linux) |
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’.