[bug#50515] [PATCH 2/2] website: Add 'computed-origin-method' packages to 'sources.json'.

[zimoun]

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