[bug#50620] [PATCH 1/2] guix: packages: Document 'computed-origin-method'.

[Liliana Marie Prikler]

Hi,

Am Mittwoch, den 29.09.2021, 10:32 +0200 schrieb zimoun:
> Hi,
> 
> On Tue, 28 Sept 2021 at 19:24, Liliana Marie Prikler
> <liliana.prikler@gmail.com> wrote:
> 
> > > Ok.  I do not find it better than (guix packages) where 'origin'
> > > is
> > > defined but anyway.
> > > I will send a v2 considering this and the rename you proposed.
> > 
> > By that logic all of (guix git-download), (guix svn-download), etc.
> > could be inlined there as well.  Obviously that's a bad idea, but
> > *why* is it a bad idea?  I'd argue it's because we have a clear
> > separation of the record descriptor for an origin and the ways it
> > can be computed (the former in (guix packages), the latter
> > elsewhere) and that it's good to keep those concerns separate.
> > 
> > I also personally find the name "computed-origin" to be somewhat
> > weird naming choice.  I could just as well write the entire source
> > code for some given package in the snippet part of an origin,
> > perhaps applying some weird tricks in the category of Kolmogorov
> > code golf – would that origin not be computed?
> 
> Are we bikeshedding here? ;-)
> 
> Again, the aim of this patch it not to fix the
> 'computed-origin-method'.  The aim of this patch is to improve the
> readibility of the patch#50515 [1] which allows linux-libre and
> icecat to be ingested by SWH from 'guix.gnu.org/sources.json'.
> 
> Maybe there is an original issue with 'computed-origin-method', as
> Mark explained [0].  But that's another story than the SWH and
> sources.json one!
Perhaps we're bikeshedding, but you started to shed the bike when you
chose to move this procedure to (guix packages) rather than
implementing one of Mark's suggestions in [0].  I do think we should
allow for bikeshedding comments from both sides if that is the case.

> [...]
> 
> > > > it, but my main issue is that we still need to hide it!  This
> > > > will
> > > > cause other channels to refer to it using @@ or roll their own
> > > > implementations.
> > > 
> > > This patch is not about discussing if this method should be
> > > public or
> > > not.  It is private.  Please discuss that elsewhere.
> > > 
> > > Mark commented in [0]:
> > > 
> > > --8<---------------cut here---------------start------------->8---
> > > 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.
> > > --8<---------------cut here---------------end--------------->8---
> > > 
> > > and this patch is about improving the situation (by removing the
> > > code
> > > duplication).  That's all.  The aim of this improvement is
> > > related to
> > > saving these IceCat and Linux Libre packages by Software Heritage
> > > [1].
> > 
> > I don't think delaying this review is a good idea, though.  When
> > you're removing code duplication, you ought to do it in a way that
> > all duplicated code can indeed be removed, at least in my
> > opinion.  As-is this patch just invites practises otherwise
> > discouraged by Guix.
> 
> If you go this road, please push patch#50515 [1] as-is.  It perfectly
> works and fixes the issue with 'guix.gnu.org/sources.json' and SWH.
> This current patch#50620 is a way to improve the readibility of
> patch#50515 but then reading all this discussion I miss why
> patch#50620 is thus a blocker for patch#50515.  Because I feel we are
> entering into the famous "Bigger problem" from xkcd. ;-)
I don't think #50515 is "perfect as-is", though.  Mark's (1) suggestion
was to put computed-origin-method into its own module, which is the
same as my long-term position.  Mark suggested a short-term fix (2) of
still comparing by eq?, which I believe you dismissed for the wrong
reasons.  Yes, you would still need to check the promise, but you
wouldn't invert said check, i.e. you would still first look for the
method and then assert that the uri makes sense.  I think it is safer
to err on the side of conservatism here rather than claim that this
code will work for future computed-origin-esque methods.

Instead of doing either (1) or (2), you opted for your own solution
(3), which is to put this method into (guix packages).  In my personal
opinion, this is a half-baked solution, because it puts computed-
origin-method into the (guix ...) without addressing any of the more
fundamental issues raised by Mark.  If you really, really can't put
this into its own module, then I'd at least suggest (3a), which is to
put it into (gnu packages) instead.  That way, the definition is at
least closer to where it's used and it's clearer that it's a hack to
package certain things, not a core part of Guix.  Perhaps you can even
make use of it without needing to rebuild the guix package in [2/2],
but don't quote me on that.

> Patch#50515 is the first part of a concrete answer to
> <https://lists.gnu.org/archive/html/guix-devel/2021-09/msg00106.html>
> and <
> https://lists.gnu.org/archive/html/guix-devel/2021-09/msg00290.html>;
> .
> It is discussed to use SWH for such situations but today our
> linux-libre is not ingested by SWH.  Therefore, let first ingest
> it--which does patch#50515.
> 
> All the best,
> simon
> 
> PS: I disagree with your last statement.  Because I am in favour for
> incremental improvements and not "The Right Thing or
> nothing".  That's out of scope of the patch at hand. ;-)
Perhaps you are right in that we can't wait for a lengthy discussion of
whether computed-origin-method can be public (guix) API or not.  Either
way, it does not look as though this thread attracts enough attention
to that issue, which is why we can ignore Mark's option (1) for now.

For the right amount of incremental change, I'd suggest the following:
Try to see whether you can do with computed-origin-method in (gnu
packages) and without rebuilding the guix package, and if so, submit a
v2 to #50515, which implements that.  Otherwise, submit a v2 to #50515
that implements Mark's option (2).

If you are also interested in the more long-term discussion of allowing
computed origins into the (guix) tree, I suggest sending a v2 patch to
this thread, which creates a new module, adds documentation, and so on,
and so forth, and also link to it on guix-devel.  For the time being,
this v2 should also refrain from touching anything that uses the
current computed-origin-method, as that can be addressed in rather
simple follow-up commits, particularly if we also implement a #50515-v2 
before.  Then we can fully use this to bikeshed about making it a verb
and what not.

WDYT?