Re: [PATCH] doc: Update contribution guidelines on patches, etc.

[Liliana Marie Prikler <liliana.prikler@gmail.com>]

Am Dienstag, dem 09.08.2022 um 20:19 +0200 schrieb Maxime Devos:
> > Am Dienstag, dem 09.08.2022 um 18:45 +0200 schrieb Maxime Devos:
> > 
> > > On 06-08-2022 08:55, Liliana Marie Prikler wrote:
> > > 
> > > 
> > > > +If your package has a bug that takes multiple lines to fix,
> > > I don't think this is true for replacing all instances of "foo"
> > > by 
> > > "/gnu/store/.../bin/foo" in a file.
> > Should it?
> I don't think so. Directly substituting all the instances instead of
> first writing a patch that does "foo" -> "@foo@" or such seems
> simpler to me.  This might be a bit too nit-picky though, maybe it's
> clear from context that this is not the kind of fix meant by that
> line.
I'm struggling to see the issue here.  For starters, it doesn't take
multiple lines to embed a store path, the change usually happens on a
single line.  Of course, translating this into a substitute*, you
expand this single line into multiple ones, but that's not what is
meant here.

However, there are exceptions to this rule.  For an example that
requires indirection, see webkitgtk.

> [...]
> > 
> > > > +Such changes include, but are not limited to, fixes of the
> > > > build
> > > > +script(s) or embeddings of store paths (e.g. [...])
> > > > 
> > > [...]
> > Is that how to English comma? Sorry, I'm not a native speaker so I
> > get
> > somewhat weirded out by the when to skip/not to skip rules.
> > 
> Neither am I. English doesn't seem to do "rules" much. I do think,
> however, that adding a comma after "to" makes things a bit simpler to
> read here, and it doesn't appear to be ungrammatical -- at least, in
> licenses "but is/are not limited to" is often used that way.
Fair enough, will add the comma.

> > Derivations are a rather low-level concept, could they be avoided
> > in the origin and phases documentation?
> > I don't quite see how. You could s/source derivation/the result of
> > @code{guix build -S}/, but I don't think that's much better.
> > 
> To be clear, do you mean you:
>  * think it's not better, maybe even worse
>  * think it's not _much_ better (but still _slightly_ better)
>  * are undecided
>  * or something else
> ?
> Also, "guix build -S" returns the source code (after snippet / patch,
> if any), not its derivation. For the latter: "guix build -S -d"
FWIW I don't think mentioning patch-and-repack is too helpful here
either.  Also, I'd like to use consistent wording at least within this
section, so here "source" means "upstream source" whereas "source
derivation" is a shorthand for the stuff Guix builds.  Yes, the
derivation is not the same thing as the output, but I again fail to see
how being overly precise is helpful.  That being said, I'm open to
suggestions.

> > > > +Build phases are limited in that they do not modify the source
> > > > +derivation.  Thus, they are inadequate for changes that are to
> > > > be
> > > > +reflected in the source code.  On the other hand, they only
> > > > cause
> > > > a
> > > > +single rebuild and are thus slightly easier to debug than
> > > > phases
> > > > and
> > > > +snippets.
> > > See Andreas' comment on phase->snippet.
> > > 
> > > Also, do I understand correctly that the argument here is that
> > > 'single rebuild -> less compilation time -> easier to debug'?
> > Easier to debug for the package author currently fiddling with the
> > phase/snippet. Not really a statement in any direction otherwise.
> I don't see how "slightly easier to debug than phases" follows from
> "they cause only a single rebuild". My guess was that the
> intermediate step was lower compilation time, but apparently this was
> not the argument. As such, I'm not following.
I think this follows from your "I only deal with small packages"
experience earlier.  In my experience, the sources that are more likely
to require patches or snippets to fix their... issues... are also the
ones that take larger time to patch and repack.  After all, you don't
have that much overhead unpacking a 7 gig blob that doesn't exist.

Cheers