From: zimoun <zimon.toutoune@gmail.com>
To: Mark H Weaver <mhw@netris.org>
Cc: 50515@debbugs.gnu.org
Subject: [bug#50515] [PATCH 2/2] website: Add 'computed-origin-method' packages to 'sources.json'.
Date: Mon, 13 Sep 2021 09:01:45 +0200 [thread overview]
Message-ID: <865yv5vu06.fsf@gmail.com> (raw)
In-Reply-To: <87a6kid34a.fsf@netris.org>
Hi Mark,
Thanks for looking at the patch and for your inputs.
On Sat, 11 Sep 2021 at 20:54, Mark H Weaver <mhw@netris.org> wrote:
>> @@ -106,53 +109,67 @@
>> (append-map (cut maybe-expand-mirrors <> %mirrors)
>> (map string->uri urls))))
>>
>> - `((type . ,(cond ((or (eq? url-fetch method)
>> - (eq? url-fetch/tarbomb method)
>> - (eq? url-fetch/zipbomb method)) 'url)
>> - ((eq? git-fetch method) 'git)
>> - ((or (eq? svn-fetch method)
>> - (eq? svn-multi-fetch method)) 'svn)
>> - ((eq? hg-fetch method) 'hg)
>> - (else #nil)))
>> - ,@(cond ((or (eq? url-fetch method)
>> + (match uri
>> + ((? promise? promise) ;computed-origin-method
>> + (match (force promise)
>
> Here, you're implicitly assuming that 'computed-origin-method' is the
> only origin method that puts a promise in the 'uri' field. That may be
> true today, but it will not necessarily be true tomorrow, and therefore
> it seems suboptimal to make that assumption in the code.
Yes, I agree. My initial draft contained something as your wrote below:
(or (eq? method (@@ (gnu packages gnuzilla) computed-origin-method))
(eq? method (@@ (gnu packages linux) computed-origin-method)))
but then, I thought it was a redundant test because then the promise
check is necessary to unwrap the values of embedded origins. And
currently, all the 'computed-origin-method's use a promise.
> Instead, I would suggest checking for "computed origins" in the same way
> that is done for the other cases: using 'eq?'. It's not ideal, but it's
> more future-proof than checking for a promise in the 'url' field, and
> anyway it's the way things are currently being done.
I cannot predict the future but the check about the method is as
suboptimal as mine. :-) If another package uses computed-origin-method,
then it should be added here. However, from my understanding, there is
an higher probability that this hypothetical packages would use a
promise.
> However, there's a difficulty, and I suspect you're already aware of it
> and that it's why you used the suboptimal approach above:
>
> At present, 'computed-origin-method' is not exported by any Guix module,
> nor is there even a unique definition of it. Instead, there are two
> copies of it, one in gnuzilla.scm and one in linux.scm.
Yes. :-)
> The reason 'computed-origin-method' is not exported is because it never
> went through the review process that such a radical new capability in
> Guix should go through before becoming part of it's public API.
>
> At the time that I added 'computed-origin-method', I was under time
> pressure to push security updates for IceCat, and my previous method of
> cherry picking dozens of upsteam patches and applying them to the most
> recent IceCat release suddenly became impractical due to comprehensive
> code reformatting done upstream.
>
> I've always viewed 'computed-origin-method' as a temporary hack to work
> around limitations in the 'snippet' mechanism. Most importantly, last I
> checked, it was not possible for a 'snippet' to produce a tarball with a
> different base name than the original downloaded source. I consider it
> a *requirement* for the 'icecat' source tarball and it's unpacked
> directory to be named "icecat-…" and not "firefox-…", and similarly for
> 'linux-libre'.
Thanks for explaining.
> Anyway, regarding your proposed patch: for now, I would suggest the
> following options:
>
> (1) In a separate preceding commit, move 'computed-origin-method' to its
> own module, export it, use the exported one in gnuzilla.scm and
> linux.scm, and use 'eq?' to test for it in the code above. There
> should probably also be a comment next to the definition of
> 'computed-origin-method' pointing out that it's a temporary hack,
> hopefully to be superceded by snippets when they have gained the
> required functionality.
I think it is the better approach. Move the ’computed-origin-method’
procedure to (guix packages) and export it; add a comment about it.
However, I would not like that the sources.json situation stays blocked
by the computed-origin-method situation when sources.json is produced by
the website independently of Guix, somehow. :-)
Therefore, there is an option (3). Move the ’computed-origin-method’
procedure to (guix packages) and add a comment about it; use it for
icecat and linux with (@@ (guix packages) computed-origin-method).
WDYT about this (3)? It simplifies this patch and let the time to
discuss the ’computed-origin-method’ case without exposing it to the
public API.
> (2) Alternatively, for now, use 'eq?' against the two private copies
> (accessed using @@, see below), along with a "FIXME" comment.
>
> ___ (or (eq? method (@@ (gnu packages gnuzilla) computed-origin-method))
> _______ (eq? method (@@ (gnu packages linux) computed-origin-method)))
I commented above why I am not convinced that is better than directly
check the promise. I do agree with the FIXME comment; the commit
message is not enough here.
Cheers,
simon
next prev parent reply other threads:[~2021-09-13 7:19 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-09-11 0:14 [bug#50515] (guix-artwork)[PATCH 0/2] List linux origins in 'sources.json' zimoun
2021-09-11 0:26 ` [bug#50515] [PATCH 1/2] website: Tweak 'GUIX_WEB_SITE_LOCAL' zimoun
2021-09-11 0:26 ` [bug#50515] [PATCH 2/2] website: Add 'computed-origin-method' packages to 'sources.json' zimoun
2021-10-01 14:16 ` zimoun
2021-10-04 7:53 ` Ludovic Courtès
2021-10-05 14:09 ` zimoun
2021-09-12 0:54 ` Mark H Weaver
2021-09-13 7:01 ` zimoun [this message]
2021-09-16 0:07 ` Mark H Weaver
2021-09-16 11:48 ` zimoun
2021-10-05 14:09 ` [bug#50515] [PATCH v2 1/2] website: Tweak 'GUIX_WEB_SITE_LOCAL' zimoun
2021-10-05 14:09 ` [bug#50515] [PATCH v2 2/2] website: Add 'computed-origin-method' packages to 'sources.json' zimoun
2021-10-18 12:23 ` [bug#50515] (guix-artwork)[PATCH 0/2] List linux origins in 'sources.json' Ludovic Courtès
2021-10-21 9:42 ` zimoun
2021-10-21 9:41 ` [bug#50515] [PATCH v3 1/2] website: Tweak 'GUIX_WEB_SITE_LOCAL' zimoun
2021-10-21 9:41 ` [bug#50515] [PATCH v3 2/2] website: Add 'computed-origin-method' packages to 'sources.json' zimoun
2021-10-21 20:58 ` bug#50515: (guix-artwork)[PATCH 0/2] List linux origins in 'sources.json' Ludovic Courtès
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=865yv5vu06.fsf@gmail.com \
--to=zimon.toutoune@gmail.com \
--cc=50515@debbugs.gnu.org \
--cc=mhw@netris.org \
/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).