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

[Liliana Marie Prikler]

Am Dienstag, dem 09.08.2022 um 22:30 +0200 schrieb Maxime Devos:
> 
> On 09-08-2022 21:08, Liliana Marie Prikler wrote:
> > [I]t doesn't take multiple lines to embed a store path, the change
> > usually happens on a single line
> I was thinking of files that contain multiple instances of "foo"
> (usually on multiple lines) to be replaced by
> "/gnu/store/.../bin/foo", not files were a "foo
>  bar" was broken over multiple lines and it needs to be replaced by
> "/gnu/store/.../bin/foo
>  bar".
That's a single-line change imho, even if the single line gets repeated
9000 times.

Cheers

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

[Maxime Devos]

On 09-08-2022 21:08, Liliana Marie Prikler wrote:

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.


I am not reading an answer to my question.

I don't think I've mentioned patch-and-repack (at least not by name, 
which you seem to be referring to?).


I would not recommend "source = upstream source", as the more general 
meaning is used in (guix)Introduction and 
 and elsewhere, otherwise 
terminology would become inconsistent, which can lead to misinterpretations.


I don't think there's such a thing as 'overly precise'.

My suggestion is the same as your suggestion:


You could s/source derivation/the result of
@code{guix build -S}/, but I don't think that's much better.

Greetings,
Maxime.



OpenPGP_0x49E3EE22191725EE.asc
Description: OpenPGP public key


OpenPGP_signature
Description: OpenPGP digital signature

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

[Maxime Devos]

On 09-08-2022 21:08, Liliana Marie Prikler wrote:

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.


I was thinking of files that contain multiple instances of "foo" 
(usually on multiple lines) to be replaced by "/gnu/store/.../bin/foo", 
not files were a "foo
bar" was broken over multiple lines and it needs to be replaced by 
"/gnu/store/.../bin/foo

bar".

At least for the former, I don't think an intermediate "@foo@" is useful 
(with some exceptions, when a pattern match would catch too much).


The latter could in principle happen, but it doesn't seem to happen in 
practice.


Greetings,
Maxime.



OpenPGP_0x49E3EE22191725EE.asc
Description: OpenPGP public key


OpenPGP_signature
Description: OpenPGP digital signature

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

[Liliana Marie Prikler]

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

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

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

+ 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.
Hm, maybe, I guess I often work on 'small' packages where it doesn't 
matter much.


On 09-08-2022 19:05, Liliana Marie Prikler wrote:

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



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"



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


Greetings,
Maxime.



OpenPGP_0x49E3EE22191725EE.asc
Description: OpenPGP public key


OpenPGP_signature
Description: OpenPGP digital signature

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

[Liliana Marie Prikler]

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

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

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

  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.



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



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. [...])



[...]


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



+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'?


Greetings,
Maxime.



OpenPGP_0x49E3EE22191725EE.asc
Description: OpenPGP public key


OpenPGP_signature
Description: OpenPGP digital signature

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

[Liliana Marie Prikler]

* doc/contributing.texi ("Snippets versus Phases"): Replaced with...
("Modifying Sources"): ... this.  List more use cases and some principles.
---
 doc/contributing.texi | 85 ---
 1 file changed, 72 insertions(+), 13 deletions(-)

diff --git a/doc/contributing.texi b/doc/contributing.texi
index 02c7c5ae59..7a03715abf 100644
--- a/doc/contributing.texi
+++ b/doc/contributing.texi
@@ -441,7 +441,7 @@ needed is to review and apply the patch.
 * Package Naming::  What's in a name?
 * Version Numbers:: When the name is not enough.
 * Synopses and Descriptions::   Helping users find the right package.
-* Snippets versus Phases::  Whether to use a snippet, or a build phase.
+* Modifying Sources::   When to use patches, snippets, or build phases.
 * Emacs Packages::  Your Elisp fix.
 * Python Modules::  A touch of British comedy.
 * Perl Modules::Little pearls.
@@ -698,20 +698,79 @@ Gettext}):
 for the X11 resize-and-rotate (RandR) extension. @dots{}")
 @end lisp
 
-@node Snippets versus Phases
-@subsection Snippets versus Phases
+@node Modifying Sources
+@subsection Modifying Sources
 
+@cindex patches, when to use
 @cindex snippets, when to use
-The boundary between using an origin snippet versus a build phase to
-modify the sources of a package can be elusive.  Origin snippets are
-typically used to remove unwanted files such as bundled libraries,
-nonfree sources, or to apply simple substitutions.  The source derived
-from an origin should produce a source that can be used to build the
-package on any system that the upstream package supports (i.e., act as
-the corresponding source).  In particular, origin snippets must not
-embed store items in the sources; such patching should rather be done
-using build phases.  Refer to the @code{origin} record documentation for
-more information (@pxref{origin Reference}).
+
+Guix has three main ways of modifying the source code of a package,
+that you as a packager may use.  Each one has its strengths and
+drawbacks, along with intended and historically derived use cases.
+These are
+
+@table @asis
+
+@item patches
+If your package has a bug that takes multiple lines to fix, 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{patches}, @code{patch-inputs}, and
+@code{patch-flags}) for more information (@pxref{origin Reference}).
+When adding a patch, do not forget to also list it in
+@code{dist_patch_DATA} of @file{gnu/local.mk}
+
+Patches are limited in that they lack the expressiveness of Guile.
+If all changes are constrained to single lines, a patch might be much
+larger than the equivalent @code{substitute*}.  Furthermore, building
+a package with an additional patch causes two new derivations to be
+built: First the source, then the package.
+
+@item snippets
+If your package contains non-free sources, these need to be removed
+through a snippet.  This snippet should not only remove the sources in
+question, but also references to the removed sources in build scripts,
+documentation, and so on. @ref{Software Freedom}
+
+If your package bundles external libraries, snippets are the preferred
+way of removing said them.  Unlike with non-free sources, it is not a
+requirement to remove @emph{all} bundled libraries, although doing so
+is very much preferred.  Bundled libraries that are kept should be
+clearly indicated, preferrably with a reason as to why the bundled copy
+remains.  As with non-free sources, references to the removed libraries
+should also be updated in the snippet.
+
+Refer to the @code{origin} record documentation
+(particularly the fields @code{snippet} and @code{modules}), for more
+information (@pxref{origin Reference}).
+
+Snippets are limited in that @code{substitute*} can not deal with
+multi-line changes well.  Furthermore, as with patches, modifying the
+snippets causes two derivations to be built.
+
+@item build phases
+For modifications that retain the intended functionality of the
+package, build phases (usually between @code{unpack} and
+@code{configure}, sometimes between @code{configure} and @code{build})
+can be used.  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")}).
+
+If you need to embed a store path, do so only inside a build phase.
+A workaround for patches that span multiple lines, is to use a variable
+such as @code{@@store_path@@} inside the patch and substitute the actual
+store path at build time via @code{substitute*}.
+
+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