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

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

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?

> >   or a fix
> > +has already been accepted upstream, patches are the preferred way
> > of
> > +eliminating said bug
> > +Refer to the @code{origin} record documentation
> > +(particularly the fields @code{snippet} and @code{modules}), for
> > more
> > +information (@pxref{origin Reference}).
> > +
> 
> The "Refer to the ... documentation for more information" occurred in
> the old version of (guix)Snippets versus Phases. However, back then,
> I did not find more information on how to decide between snippets,
> patches and phases, and neither do I now.
> 
> Maybe:
> 
> +Refer to the @code{origin} record documentation
> +(@pxref{origin Reference}) (particularly the fields @code{snippet}
> and @code{modules})
> +for more information on how to use snippets
> 
> , to avoid a reader's assumption that that section contains
> information on deciding between snippets, phases and patches.
Yeah, this was meant for "how to use".

> > + Furthermore, as with patches, modifying the snippets causes two
> > derivations to be built.
> 
> This is true, but I don't think reviewers and package authors have to
> worry about that.
It does make a difference to the author when debugging their package. 
Starting with a phase and then moving it to a snippet can save good
time.

> > Such changes include, but are not limited to fixes of the
> > +build script(s) or embeddings of store paths (e.g. replacement of
> > +@file{/bin/sh} with @code{(search-input-file inputs "bin/sh")}).
> Include what? I think you need to close the subsentence here:
> 
> > +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.

> > +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.
> 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.

> > +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.

Cheers