From: "Ludovic Courtès" <ludo@gnu.org>
To: zimoun <zimon.toutoune@gmail.com>
Cc: "guix-devel@gnu.org" <guix-devel@gnu.org>
Subject: Re: Transform options should error on nonexistant targets
Date: Wed, 08 Sep 2021 22:55:30 +0200 [thread overview]
Message-ID: <8735qe23el.fsf@gnu.org> (raw)
In-Reply-To: <CAJ3okZ2bctROohMHpHovrcwrZpBnOCqEE7XT-5+fhZuGXSqk=w@mail.gmail.com> (zimoun's message of "Thu, 2 Sep 2021 12:50:02 +0200")
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’.
next prev parent reply other threads:[~2021-09-08 20:55 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-08-18 3:57 Transform options should error on nonexistant targets Ryan Prior
2021-08-25 16:16 ` zimoun
2021-09-02 10:06 ` Ludovic Courtès
2021-09-02 10:50 ` zimoun
2021-09-08 20:55 ` Ludovic Courtès [this message]
2021-09-08 22:29 ` Transform options: check if applied or not zimoun
2021-09-09 10:32 ` Maxime Devos
2021-09-11 12:09 ` Transform: walk through packages zimoun
2021-09-17 8:57 ` Transform options should error on nonexistant targets zimoun
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: https://guix.gnu.org/
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=8735qe23el.fsf@gnu.org \
--to=ludo@gnu.org \
--cc=guix-devel@gnu.org \
--cc=zimon.toutoune@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/guix.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).