[bug#57598] [alternative PATCH] doc: Update contribution guidelines on patches, etc.

[bug#57598] [alternative PATCH] doc: Update contribution guidelines on patches, etc.

[Liliana Marie Prikler]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 6867 bytes --]

* doc/contributing.texi ("Snippets versus Phases"): Replaced with...
("Modifying Sources"): ... this.  List more use cases and some principles.
---
Hi Maxime,

here is an update of my guidelines patch, incorporating the changes Ludo’
requested and some of your bits, as well as fixing some omissions.

Cheers

 doc/contributing.texi | 115 +++++++++++++++++++++++++++++++++++++-----
 1 file changed, 102 insertions(+), 13 deletions(-)

diff --git a/doc/contributing.texi b/doc/contributing.texi
index 17a54f94cc..4894982259 100644
--- a/doc/contributing.texi
+++ b/doc/contributing.texi
@@ -451,7 +451,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.
@@ -708,20 +708,109 @@ 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 @b
+
+@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.@footnote{If you patch a bug that has hitherto
+not been reported or fixed upstream, consider also contacting upstream
+unless the bug and its fix are specific to Guix.}
+Refer to the @code{origin} record documentation
+(particularly the fields @code{patches}, @code{patch-inputs}, and
+@code{patch-flags}) for more information on how to use patches
+(@pxref{origin Reference}).
+When adding a patch, do not forget to also list it in
+@code{dist_patch_DATA} of @file{gnu/local.mk}
+
+As patches are applied to the origin of a package, they become part
+of the corresponding source.  You can retrieve this source by
+invoking @code{guix build -S YOUR_PACKAGE}.  This also means that
+modifying the patch causes two rebuilds: one for the source and one
+for the package built from it.
+
+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*}.  It is further bad form
+to use a single patch to address multiple unrelated issues, whereas
+snippets can take ``multiple jobs''.
+
+@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 on how to use snippets (@pxref{origin Reference}).
+
+While snippets have all of Guile's core as well as extra @code{modules}
+available, their most useful procedure for @emph{editing} sources
+(rather than removing them), is @code{substitute*}, which can not deal
+with multi-line changes that well.  Like patches, snippets become part
+of the corresponding source.
+
+@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.  @ref{Build Phases}.
+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*}.
+
+Unlike patches and snippets, build phases do @b{not} become part of
+the corresponding source of a package, and should thus be avoided for
+changes that result in observably different runtime behaviour.
+On the other hand, the reduced overhead of unpacking, repacking and
+unpacking again might make for a slightly more pleasant debugging
+experience.
+
+@end table
+
+If your change does not neatly fit in any of the categories above, it
+is usually a matter of preference or convenience.
+
+@cindex auxiliary files
+
+Sometimes, you may need to add a new file, e.g. a source file or
+configuration file, to your package.  As a matter of principle
+@b{auxiliary files} should be preferred over an equivalent
+@code{display} or @code{format} when creating non-trivial files, as that
+makes them easier to edit.  The exact threshold for a non-trivial file
+might be subjective, though it should lie somewhere between
+10--20 lines.
+
+Auxiliary files are stored in the @file{gnu/packages/aux-files}
+directory and can be retrieved in a snippet or build phase via
+@code{search-auxiliary-file}.
+When adding an auxiliary file, do not forget to also list it in
+@code{AUX_FILES} of @file{Makefile.am}.
 
 @node Emacs Packages
 @subsection Emacs Packages
-- 
2.37.2

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

[Maxime Devos]

* doc/contributing.texi (Modifying Sources): Replaced with ...
("Modifying Sources"): ... this.  List more use cases and some principles.

This patch incorporates some tet written by Liliana Marie Prikler.  The
structure is based on a proposal by Julien Lepiller.  The text has been
revised based on reviews by David Larsson and blake.
---
 doc/contributing.texi | 141 +++++++++++++++++++++++++++++++++++++-----
 doc/guix.texi         |   2 +-
 2 files changed, 127 insertions(+), 16 deletions(-)

diff --git a/doc/contributing.texi b/doc/contributing.texi
index 02c7c5ae59..f6bdb8a441 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::           Whether to use a snippet, or a build phase.
 * Emacs Packages::              Your Elisp fix.
 * Python Modules::              A touch of British comedy.
 * Perl Modules::                Little pearls.
@@ -698,20 +698,131 @@ Gettext}):
 for the X11 resize-and-rotate (RandR) extension. @dots{}")
 @end lisp
 
-@node Snippets versus Phases
-@subsection Snippets versus Phases
-
-@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}).
+@node Modifying Sources
+@subsection Modifying Sources
+
+Guix has tree main ways of modifying the source code of a package, that
+you as a packager may use.  These are patches, snippets and phases.
+Each one has its strengths and drawbacks.  To decide between the tree,
+there are a few guiding principles that to satisfy simultanuously where
+possible:
+
+@itemize
+@item
+In principle, Guix only has free software; when the upstream source
+contains some non-free software, it has to be removed such that
+@command{guix build --source} returns the ‘freed’ source code rather than
+the unmodified upstream source (@pxref{Software Freedom}).
+@item
+The source of the package needs to correspond to what is actually built
+(i.e., act as the corresponding source), to fulfill our ethical and
+legal obligations.
+@item
+It is convenient for the source derived from an origin to build on any
+system that the upstream package supports.
+@item
+The source needs to actually work, not only on your Guix system but also
+for other systems; this requires some care for substitutions involving
+store items and other architecture-specific changes.
+@item
+When there is more than one way to do something, choose whichever method
+is the simplest.  Sometimes this is subjective, which is also fine.
+What matters is that you use techniques that are common within the
+community (i.e., patterns that appear throughout @code{gnu/packages/...})
+and are thus clearly legible for reviewers.
+@end itemize
+
+To make things more concrete and to resolve conflicts between the
+principles, a few cases have been worked out:
+
+@subsubsection Removing non-free software
+Non-free software has to be removed in snippets; the reason is that
+patches or phases will not work.
+
+For patches, the problem is that a patch removing a non-free file
+automatically contains the non-free file@footnote{It has been noted that
+git patches support removing files without including the file in the
+patch in
+@url{https://yhetil.org/guix/8b13e899-eb60-490b-a268-639249698c81@@www.fastmail.com/}. If
+it is verified that the @command{patch} utility supports such patches,
+this method can be used and this policy adjusted appropriately.}, and we
+do not want anything non-free in Guix even if only in its patches.
+
+For phases, the problem is that phases do not influence the result of
+@command{guix build --source}.
+
+@subsubsection Removing bundled libraries
+Bundled libraries should not be removed with patches, because then the
+patch would contain the full bundled library, which can be large. They
+can be removed either in snippets or phases, often using the procedure
+@code{delete-file-recursively}. There are a few benefits for snippets here:
+
+When using snippets, the bundled library does not occur in the source
+returned by @code{guix build --source}, so users and reviewers do not
+have to worry about whether the bundled library contains malware,
+whether it is non-free, if it contains pre-compiled binaries ... There
+are also less licensing concerns: if the bundled libraries are removed,
+it becomes less likely that the licensing conditions apply to people
+sharing the source returned by @command{guix build --source}, especially if
+the bundled library is not actually used on Guix systems.@footnote{This
+is @emph{not} a claim that you can simply ignore the licenses of
+libraries when they are unbundled and replaced by Guix packages -- there
+are less concerns, not none.}
+
+As such, snippets are recommended here.
+
+@subsubsection Fixing technical issues (compilation errors, test failures, other bugs ...)
+Usually, a bug fix comes in the form of a patch copied from upstream or
+another distribution.  In that case, simply adding the patch to the
+@code{patches} field is the most convenient and usually does not cause
+any problems; there is no need to rewrite it as a snippet or a phase.
+
+If no ready-made patch already exists, then choosing between a patch or
+a snippet is a matter of convenience. However, there are two things to
+keep in mind:
+
+First, when the fix is not Guix-specific, upstreaming the fix is
+strongly desired to avoid the additional maintenance cost to Guix.  As
+upstreams cannot accept snippets, writing a patch can be a more
+efficient use of time.  Secondly, if the fix of a technical issue embeds
+a store file name, then it has to be a phase.  Otherwise, if the store
+file name were embedded in the source, the result of @command{guix build
+--source} would be unusable on non-Guix systems and also likely unusable
+on Guix systems of another architecture.
+
+@subsubsection Adding new functionality
+To add new functionality, a patch is almost always the most convenient
+choice of the three -- patches are usually multi-line changes, which are
+convenient to do with patches and inconvenient to do with phases or
+snippets.  This choice is usually also compatible with all the guiding
+principles.  As such, patches are preferred.  However, as with bug
+fixes, upstreaming the new functionality is desired.
+
+@subsection How to add patches
+Refer to the @code{origin} record documentation (particularly the fields
+@code{patches}, @code{patch-inputs}, and @code{patch-flags}) for
+information on how to use patches (@pxref{origin Reference}).  When
+adding a patch, do not forget to also list it in @code{dist_patch_DATA}
+of @file{gnu/local.mk}.
+
+@subsection How to add files
+New source files can be added in phases or snippets, by using
+@b{auxiliarry files}.  Auxiliary files are stored in the
+@file{gnu/packages/aux-files} directory and can be retrieved (in a
+snippet or a phase) with @code{search-auxiliary-file}.  When adding an
+auxiliary file, do not forget to also list it in @code{AUX_FILES} of
+@file{Makefile.am}.
+
+Another option for adding new files, is to use procedures such as
+@code{display}, @code{format} and @code{call-with-output-file}.  As a
+matter of principle, auxiliary files ought to be preferred over an
+equivalent @code{call-with-output-file} when creating non-trivil files,
+as that makes them easier to edit.  The exact threshold for a
+non-trivial file might be subjective, though it should lie somewhere
+between 10--20 lines.
+
+Currently, there is no policy on deciding between phase and snippets for
+adding new files, except for the guiding principles.
 
 @node Emacs Packages
 @subsection Emacs Packages
diff --git a/doc/guix.texi b/doc/guix.texi
index 7bce8a567c..042ab3bd8a 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -70,7 +70,7 @@ Copyright @copyright{} 2019 Jakob L. Kreuze@*
 Copyright @copyright{} 2019 Kyle Andrews@*
 Copyright @copyright{} 2019 Alex Griffin@*
 Copyright @copyright{} 2019, 2020, 2021, 2022 Guillaume Le Vaillant@*
-Copyright @copyright{} 2020 Liliana Marie Prikler@*
+Copyright @copyright{} 2020, 2022 Liliana Marie Prikler@*
 Copyright @copyright{} 2019, 2020, 2021, 2022 Simon Tournier@*
 Copyright @copyright{} 2020 Wiktor Żelazny@*
 Copyright @copyright{} 2020 Damien Cassou@*

base-commit: 57f8f69562e942557e3331bb81c7e4acd973d189
prerequisite-patch-id: 78c06b38a189109a5108a157d39ffe7eab8be109
prerequisite-patch-id: aaf0731113d36df901ed2186975e3bb872ec22c0
prerequisite-patch-id: 28e8223cfd59adf84007db9ceefd8a78c41fd10d
prerequisite-patch-id: fb73228d99c36f50e2959c2303c7c707460fd147
prerequisite-patch-id: 7626f1464f4926416fb13daf3d846176aa93f51b
prerequisite-patch-id: 445c6f624e99627959f2e54a6ee97337c44d9ea6
prerequisite-patch-id: 7a16c500faec9d58700a2b50b26bded079e9c3ac
prerequisite-patch-id: f7d406c61e069c04c3b7da453192f51c04763db1
prerequisite-patch-id: 4674bf40052d97215f837c9dfd4e7e1ae999492d
prerequisite-patch-id: 6259468375bfa157277521b17fdd97d6ab0748b7
prerequisite-patch-id: 9d14b38a33b68883c43d6b26dcdbdf7c28e417e7
prerequisite-patch-id: f0e3faffe768e9c660b0a9340042acfa0f790308
prerequisite-patch-id: 550485506255a67c0a1cb9ede7778d4d538b6e2a
prerequisite-patch-id: 9282e95ff076cc2c742be8d2fede83ac14006f6f
prerequisite-patch-id: 1503aa5c698f72ee47b7a987a95c0919efb203c4
prerequisite-patch-id: 24297940086a3780fd7e2e7fa345f262b12efb6f
prerequisite-patch-id: c5f647b5472465666328b123f0f314a6138d6293
prerequisite-patch-id: 56386c4df9130221cad664ec161d1ad9713f4dc3
prerequisite-patch-id: f09ccfb7e53bc7934326af603a197344f4ef53f3
prerequisite-patch-id: 0c18c83d1f2da4639b43861103a028706a147022
prerequisite-patch-id: 066bfca8bf0c3d3bc57a14b48aa1e241555c1e86
prerequisite-patch-id: 13d9ac7b0fadc92b9351409df26b41443497a964
prerequisite-patch-id: ad831a04543475288aba1c938078dcc5ad05870e
prerequisite-patch-id: ed9ec2d0bea23c2c2dbfa4c62290893f1a938f7f
prerequisite-patch-id: 335ce9dbbb2b36b960203a79fdc8f6033ebda2fb
prerequisite-patch-id: f2ca362056369913d0b8319187a8f46ea78b6dc7
prerequisite-patch-id: c02a17479ad4e01837fc307cf6defe0ae92e2435
prerequisite-patch-id: 3a794307f3bbd3641023d978f4b359eb2f5a46e4
prerequisite-patch-id: c545007535db365a29dc3a86e10866f5eef7b7d5
prerequisite-patch-id: 8b8fd18762282129e2d034f7cdceb368f53295d6
prerequisite-patch-id: d435d42bafa65f14049740a3d6cf1a163f855f97
prerequisite-patch-id: 27da1b857019b217b25b6795b84577fdc992a84c
prerequisite-patch-id: aef95e76144ae5e92a41f81b11e84ddc7ececd91
prerequisite-patch-id: 95aa6b45d93026e4375b53a471f0f96e2016914e
prerequisite-patch-id: 715d2bb93fe711e72388458846a0afde6babdc97
prerequisite-patch-id: 63422b710539c3eeac249bf0201107914a215d16
prerequisite-patch-id: 53c9d2236538f9a9009b5b7b2455ef4875d0e31a
prerequisite-patch-id: 37df0e9658435e0a5e2b49fe54a69b91385fb596
prerequisite-patch-id: d2ecaf3439d153de1d53f608e7f5815c73d7b93c
-- 
2.37.2

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

[Maxime Devos]

[-- Attachment #1.1.1: Type: text/plain, Size: 285 bytes --]

See the thread 
<https://yhetil.org/guix/03a28262-316d-20fc-ea91-5379c74362b1@telenet.be/>.

There is also another patch 
<https://yhetil.org/guix/1dcca575580624ddcc208883ee62d8c75f908139.camel@gmail.com/>, 
which documents the same thing, but differently.

Greetings,
Maxime

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 929 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

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

[Liliana Marie Prikler]

Am Montag, dem 05.09.2022 um 18:00 +0200 schrieb Maxime Devos:
> +Guix has tree main ways of modifying the source code of a package,
> that
> +you as a packager may use.  These are patches, snippets and phases.
> +Each one has its strengths and drawbacks.  To decide between the tree,
three
> +there are a few guiding principles that to satisfy simultanuously
> where
> +possible:
"there are a few guiding principles to satisfy simultaneously",
"there are some guiding principles, of which as many as possible should
be satisfied".
> +
> +@itemize
> +@item
> +In principle, Guix only has free software; when the upstream source
> +contains some non-free software, it has to be removed such that
> +@command{guix build --source} returns the ‘freed’ source code rather
> than
> +the unmodified upstream source (@pxref{Software Freedom}).
If the latter wording above is chosen, this is not a "guiding
principle", because it is non-negotiable.
> +@item
> +The source of the package needs to correspond to what is actually
> built
> +(i.e., act as the corresponding source), to fulfill our ethical and
> +legal obligations.
> +@item
> +It is convenient for the source derived from an origin to build on any
> +system that the upstream package supports.
> +@item
> +The source needs to actually work, not only on your Guix system but
> also
> +for other systems; this requires some care for substitutions involving
> +store items and other architecture-specific changes.
If you embed store items, it won't even work on Guix System 😛️

Also, this appears to be a rather convenient rewording of the previous
item, does it not?
> +@item
> +When there is more than one way to do something, choose whichever
> method
> +is the simplest.  Sometimes this is subjective, which is also fine.
> +What matters is that you use techniques that are common within the
> +community (i.e., patterns that appear throughout
> @code{gnu/packages/...})
> +and are thus clearly legible for reviewers.
> +@end itemize
> +
> +To make things more concrete and to resolve conflicts between the
> +principles, a few cases have been worked out:
> +
> +@subsubsection Removing non-free software
> +Non-free software has to be removed in snippets; the reason is that
> +patches or phases will not work.
> +
> +For patches, the problem is that a patch removing a non-free file
> +automatically contains the non-free file@footnote{It has been noted
> that
> +git patches support removing files without including the file in the
> +patch in
> +@url{
> https://yhetil.org/guix/8b13e899-eb60-490b-a268-639249698c81@@www.fastmail.com/
> }. If
> +it is verified that the @command{patch} utility supports such patches,
> +this method can be used and this policy adjusted appropriately.}, and
> we
> +do not want anything non-free in Guix even if only in its patches.
We also avoid spelling out the non-free filename where possible,
preferring keep lists over remove lists, which this kind of patches
would be.

> +For phases, the problem is that phases do not influence the result of
> +@command{guix build --source}.
> +
> +@subsubsection Removing bundled libraries
> +Bundled libraries should not be removed with patches, because then the
> +patch would contain the full bundled library, which can be large. They
> +can be removed either in snippets or phases, often using the procedure
> +@code{delete-file-recursively}. There are a few benefits for snippets
> here:
> +
> +When using snippets, the bundled library does not occur in the source
> +returned by @code{guix build --source}, so users and reviewers do not
> +have to worry about whether the bundled library contains malware,
> +whether it is non-free, if it contains pre-compiled binaries ... There
> +are also less licensing concerns: if the bundled libraries are
> removed,
> +it becomes less likely that the licensing conditions apply to people
> +sharing the source returned by @command{guix build --source},
> especially if
> +the bundled library is not actually used on Guix
> systems.@footnote{This
> +is @emph{not} a claim that you can simply ignore the licenses of
> +libraries when they are unbundled and replaced by Guix packages --
> there
> +are less concerns, not none.}
> +
> +As such, snippets are recommended here.
> +
> +@subsubsection Fixing technical issues (compilation errors, test
> failures, other bugs ...)
> +Usually, a bug fix comes in the form of a patch copied from upstream
> or
> +another distribution.  In that case, simply adding the patch to the
> +@code{patches} field is the most convenient and usually does not cause
> +any problems; there is no need to rewrite it as a snippet or a phase.
> +
> +If no ready-made patch already exists, then choosing between a patch
> or
> +a snippet is a matter of convenience. However, there are two things to
> +keep in mind:
> +
> +First, when the fix is not Guix-specific, upstreaming the fix is
> +strongly desired to avoid the additional maintenance cost to Guix.  As
> +upstreams cannot accept snippets, writing a patch can be a more
> +efficient use of time.  Secondly, if the fix of a technical issue
> embeds
> +a store file name, then it has to be a phase.  
I am pretty sure that most of these are *not* done in snippets, but
rather phases, if they only affect Guix.  In particular, grep for
failing-tests and you will find a few phases disabling them.  In fact,
as far as files that will not be installed are concerned, I think
phases ought to be preferred, because they're easier to take away if an
actual fix is made.

For the store path embedding, that's a rather roundabout way of saying
that contributers *ought to* embed store paths of certaing things, such
as commands invoked via exec et al.

> Otherwise, if the store
> +file name were embedded in the source, the result of @command{guix
> build
> +--source} would be unusable on non-Guix systems and also likely
> unusable
> +on Guix systems of another architecture.
Why are you repeating a guiding principle?

> +@subsubsection Adding new functionality
> +To add new functionality, a patch is almost always the most convenient
> +choice of the three -- patches are usually multi-line changes, which
> are
> +convenient to do with patches and inconvenient to do with phases or
> +snippets.
Uhm, what?  Patches are the preferred form of patches?

>   This choice is usually also compatible with all the guiding
> +principles.  As such, patches are preferred.  However, as with bug
> +fixes, upstreaming the new functionality is desired.
> +
> +@subsection How to add patches
> +Refer to the @code{origin} record documentation (particularly the
> fields
> +@code{patches}, @code{patch-inputs}, and @code{patch-flags}) for
> +information on how to use patches (@pxref{origin Reference}).  When
> +adding a patch, do not forget to also list it in
> @code{dist_patch_DATA}
> +of @file{gnu/local.mk}.
I don't think this should be a subsection.

> +@subsection How to add files
> +New source files can be added in phases or snippets, by using
> +@b{auxiliarry files}.  Auxiliary files are stored in the
> +@file{gnu/packages/aux-files} directory and can be retrieved (in a
> +snippet or a phase) with @code{search-auxiliary-file}.  When adding an
> +auxiliary file, do not forget to also list it in @code{AUX_FILES} of
> +@file{Makefile.am}.
> +
> +Another option for adding new files, is to use procedures such as
> +@code{display}, @code{format} and @code{call-with-output-file}.  As a
> +matter of principle, auxiliary files ought to be preferred over an
> +equivalent @code{call-with-output-file} when creating non-trivil
> files,
> +as that makes them easier to edit.  The exact threshold for a
> +non-trivial file might be subjective, though it should lie somewhere
> +between 10--20 lines.
> +
> +Currently, there is no policy on deciding between phase and snippets
> for
> +adding new files, except for the guiding principles.
This should probably be a subsubsection after "Adding new
functionality" or explained within "Adding new functionality".

Overall, I'm not convinced that we have enough guiding principles to
call them that, which (along with its sheer length) is my main
complaint with the way you've phrased things.  Going down to
subsubsections just to find out where patches are appropriate, is imho
overkill.

Cheers

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

[Maxime Devos]

[-- Attachment #1.1.1: Type: text/plain, Size: 14781 bytes --]

On 06-09-2022 13:33, Liliana Marie Prikler wrote:

> Am Montag, dem 05.09.2022 um 18:00 +0200 schrieb Maxime Devos:
>> +Guix has tree main ways of modifying the source code of a package,
>> that
>> +you as a packager may use.  These are patches, snippets and phases.
>> +Each one has its strengths and drawbacks.  To decide between the tree,
> three
Right.
>> +there are a few guiding principles that to satisfy simultanuously
>> where
>> +possible:
> "there are a few guiding principles to satisfy simultaneously",
> "there are some guiding principles, of which as many as possible should
> be satisfied".
>> +
>> +@itemize
>> +@item
>> +In principle, Guix only has free software; when the upstream source
>> +contains some non-free software, it has to be removed such that
>> +@command{guix build --source} returns the ‘freed’ source code rather
>> than
>> +the unmodified upstream source (@pxref{Software Freedom}).
> If the latter wording above is chosen, this is not a "guiding
> principle", because it is non-negotiable.
I'll go for first option, "there are a few guiding principles to satisfy
simultaneously where possible", dropping the misplaced "to".
>> +@item
>> +The source of the package needs to correspond to what is actually
>> built
>> +(i.e., act as the corresponding source), to fulfill our ethical and
>> +legal obligations.
>> +@item
>> +It is convenient for the source derived from an origin to build on any
>> +system that the upstream package supports.
>> +@item
>> +The source needs to actually work, not only on your Guix system but
>> also
>> +for other systems; this requires some care for substitutions involving
>> +store items and other architecture-specific changes.
> If you embed store items, it won't even work on Guix System 😛️

It does, though? Conditional on no --system and no --target. Though given
the third @item, doesn't matter.

> Also, this appears to be a rather convenient rewording of the previous
> item, does it not?
I think that with the first, I referred to systems in the sense of
--system / --target, and that with the second I meant distributions,
though yes. I'll look into unifying the two.
>> +@item
>> +When there is more than one way to do something, choose whichever
>> method
>> +is the simplest.  Sometimes this is subjective, which is also fine.
>> +What matters is that you use techniques that are common within the
>> +community (i.e., patterns that appear throughout
>> @code{gnu/packages/...})
>> +and are thus clearly legible for reviewers.
>> +@end itemize
>> +
>> +To make things more concrete and to resolve conflicts between the
>> +principles, a few cases have been worked out:
>> +
>> +@subsubsection Removing non-free software
>> +Non-free software has to be removed in snippets; the reason is that
>> +patches or phases will not work.
>> +
>> +For patches, the problem is that a patch removing a non-free file
>> +automatically contains the non-free file@footnote{It has been noted
>> that
>> +git patches support removing files without including the file in the
>> +patch in
>> +@url{
>> https://yhetil.org/guix/8b13e899-eb60-490b-a268-639249698c81@@www.fastmail.com/
>> }. If
>> +it is verified that the @command{patch} utility supports such patches,
>> +this method can be used and this policy adjusted appropriately.}, and
>> we
>> +do not want anything non-free in Guix even if only in its patches.
> We also avoid spelling out the non-free filename where possible,
> preferring keep lists over remove lists, which this kind of patches
> would be.
Should we? I'm not seeing the point of that. I have not experienced any
such avoidance myself, see e.g. 'tennix', 'neverball' and 'shogun'. It is,
to my knowledge, not forbidden to mention non-free software by name
in code, as long as its not a recommendation (explicit or implied).
>> +For phases, the problem is that phases do not influence the result of
>> +@command{guix build --source}.
>> +
>> +@subsubsection Removing bundled libraries
>> +Bundled libraries should not be removed with patches, because then the
>> +patch would contain the full bundled library, which can be large. They
>> +can be removed either in snippets or phases, often using the procedure
>> +@code{delete-file-recursively}. There are a few benefits for snippets
>> here:
>> +
>> +When using snippets, the bundled library does not occur in the source
>> +returned by @code{guix build --source}, so users and reviewers do not
>> +have to worry about whether the bundled library contains malware,
>> +whether it is non-free, if it contains pre-compiled binaries ... There
>> +are also less licensing concerns: if the bundled libraries are
>> removed,
>> +it becomes less likely that the licensing conditions apply to people
>> +sharing the source returned by @command{guix build --source},
>> especially if
>> +the bundled library is not actually used on Guix
>> systems.@footnote{This
>> +is @emph{not} a claim that you can simply ignore the licenses of
>> +libraries when they are unbundled and replaced by Guix packages --
>> there
>> +are less concerns, not none.}
>> +
>> +As such, snippets are recommended here.
>> +
>> +@subsubsection Fixing technical issues (compilation errors, test
>> failures, other bugs ...)
>> +Usually, a bug fix comes in the form of a patch copied from upstream
>> or
>> +another distribution.  In that case, simply adding the patch to the
>> +@code{patches} field is the most convenient and usually does not cause
>> +any problems; there is no need to rewrite it as a snippet or a phase.
>> +
>> +If no ready-made patch already exists, then choosing between a patch
>> or
>> +a snippet is a matter of convenience. However, there are two things to
>> +keep in mind:
>> +
>> +First, when the fix is not Guix-specific, upstreaming the fix is
>> +strongly desired to avoid the additional maintenance cost to Guix.  As
>> +upstreams cannot accept snippets, writing a patch can be a more
>> +efficient use of time.  Secondly, if the fix of a technical issue
>> embeds
>> +a store file name, then it has to be a phase.
> I am pretty sure that most of these are *not* done in snippets, but
> rather phases, if they only affect Guix.  In particular, grep for
> failing-tests and you will find a few phases disabling them.
I do not think that ignoring a test counts as a bug fix.  I'll add it to 
this
subsubsection, at cost of some additional length.
>    In fact,
> as far as files that will not be installed are concerned, I think
> phases ought to be preferred, because they're easier to take away if an
> actual fix is made.
I do not see a difference in hardness/easyness between removing a
phase and removing a snippet (both are just a matter of opening
an editor, pointing it at gnu/packages/... and removing a few lines),
though I do consider removing a patch to be slightly harder (because
gnu/local.mk is easy to forget).
> For the store path embedding, that's a rather roundabout way of saying
> that contributers *ought to* embed store paths of certaing things, such
> as commands invoked via exec et al.

It's not? It's kind of implied, yes, but the purpose isn't being a 'you
should embed store paths' (subsub)section, but rather, 'if you go 
embedding store
paths (at least for fixing a technical issue), do it in a phase'.

I'm not following what the complaint is, I suppose a section could be added
somewhere to properly document the 'embedding store file names' practice,
and insert a cross-reference, but that wasn't the purpose of the patch and
going by later responses, you seem opposed to making things longer.

The alternative would be to remove this information, but then valuable
information would be lost (there had been some cases where store file names
were embedded in origin).

>> Otherwise, if the store
>> +file name were embedded in the source, the result of @command{guix
>> build
>> +--source} would be unusable on non-Guix systems and also likely
>> unusable
>> +on Guix systems of another architecture.
> Why are you repeating a guiding principle?
I'm showing why, in this case, a phase must be used, by noting that
not doing so would be contrary to one of the principles.

If not repeating the principle is desired, I could perhaps number them, and
refer to the principles by number instead of restating them? Would reduce
the length a little.

>> +@subsubsection Adding new functionality
>> +To add new functionality, a patch is almost always the most convenient
>> +choice of the three -- patches are usually multi-line changes, which
>> are
>> +convenient to do with patches and inconvenient to do with phases or
>> +snippets.
> Uhm, what?  Patches are the preferred form of patches?

No, I meant that patches are (usually) the preferred method for adding
new functionality, and that multi-line changes are convenient to do with
patches.  ‘which’ refers to the ‘multi-line changes’ here, not ‘patches’.

>>    This choice is usually also compatible with all the guiding
>> +principles.  As such, patches are preferred.  However, as with bug
>> +fixes, upstreaming the new functionality is desired.
>> +
>> +@subsection How to add patches
>> +Refer to the @code{origin} record documentation (particularly the
>> fields
>> +@code{patches}, @code{patch-inputs}, and @code{patch-flags}) for
>> +information on how to use patches (@pxref{origin Reference}).  When
>> +adding a patch, do not forget to also list it in
>> @code{dist_patch_DATA}
>> +of @file{gnu/local.mk}.
> I don't think this should be a subsection.
Right, should have been a subsubsection.
>> +@subsection How to add files
>> +New source files can be added in phases or snippets, by using
>> +@b{auxiliarry files}.  Auxiliary files are stored in the
>> +@file{gnu/packages/aux-files} directory and can be retrieved (in a
>> +snippet or a phase) with @code{search-auxiliary-file}.  When adding an
>> +auxiliary file, do not forget to also list it in @code{AUX_FILES} of
>> +@file{Makefile.am}.
>> +
>> +Another option for adding new files, is to use procedures such as
>> +@code{display}, @code{format} and @code{call-with-output-file}.  As a
>> +matter of principle, auxiliary files ought to be preferred over an
>> +equivalent @code{call-with-output-file} when creating non-trivil
>> files,
>> +as that makes them easier to edit.  The exact threshold for a
>> +non-trivial file might be subjective, though it should lie somewhere
>> +between 10--20 lines.
>> +
>> +Currently, there is no policy on deciding between phase and snippets
>> for
>> +adding new files, except for the guiding principles.
> This should probably be a subsubsection after "Adding new
> functionality" or explained within "Adding new functionality".
>
> Overall, I'm not convinced that we have enough guiding principles to
> call them that,

I don't think there's any lower limit on how many guiding principles
to have, except for perhaps 2 (because otherwise it should have been
singular or there aren't any).  At how few guiding principles stop
the guiding principles from being guiding principles for you, and why?

For example, on <https://www.gnu.org/philosophy/free-sw.html>, four 
guiding principles are mentioned (which are named 'essential freedoms' 
there), and
<https://en.wikipedia.org/wiki/Guiding_Principles> has 5 ‘Guiding 
Principles’.
> which (along with its sheer length) is my main
> complaint with the way you've phrased things.

(I'm assuming "its = the patch as a whole" here)

I could remove another section of the manual to compensate for the
additional length, but I doubt that's what you intended.  I do not see the
problem with the sheer length -- we have a bit of a documentation problem
in Guix, there is lots of useful information that is currently undocumented.
I do not think there have been any complaints about the manual being too 
long,
if anything, it's too short.

I've written some documentation, it was originally a bit hard to follow so
in a next version I've restructured it a bit and explained more, this
restructuring and explanation entailed some additional length.

There had been some proposals for additional cases to document, so they
were added, increasing the length.  You have added new information is your
patch, it was considered useful so I've integrated some of it in my patch,
increasing the length.  (I didn't integrate all of the new parts, if I 
did, it would
increase even further.  (If desired, in can integrate the rest, at cost 
of some
time.)).

I do not see what the problem is with additional length as long as this
additional length comes with additional useful information and the manual
is well-structured (e.g. with (sub)(sub)sections, chapters and indices) 
-- we
do not have a page limit.

At worst, perhaps the same information could perhaps be encoded with
fewer words? I could compare the two patches to see which one formulates
certain information in the fewest words, and choose the least verbose of the
two for each piece of information that is present in both?

Also, comparing the two patches, my patch has 40 more lines, but about
25 of them are for noting the guiding principles (which are absent in 
your patch).
Compensating for that, the patches are about the same length, so I do 
not think
that 'sheer length' is accurate here.

> Going down to
> subsubsections just to find out where patches are appropriate, is imho
> overkill.

The 'going down to subsubsection' is the case for your patch too, though?
In my case, it's a subsubsection, in your case it's a table entry inside a
subsection, both are the same level of nesting.

Also, it's a matter of different structure -- in my v2 and v3 patch, I 
have a
'problem -> solution' structure -- the idea is that the packages has a 
problem,
they look at the section, they read the subsubsection names, select the
subsubsection that matches their problem and read the solution -- in short,
the idea is to provide a solution to the problem.

Your structure is the other way around -- for solutions (patches, snippets,
phases), it gives the permitted problems to apply it to.

So yes, your patch is more convenient for finding out where patches are
appropriate.  I do not see the benefit of that though -- a new contributor
packaging a thing wouldn't know in advance which solutions could be
appropriate for them (your 'solution -> problem' patch?), rather, they start
with a problem and are searching for an appropriate solution (my
problem->solution patch).

Greetings,
Maxime.

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 929 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

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

[Liliana Marie Prikler]

Am Dienstag, dem 06.09.2022 um 22:21 +0200 schrieb Maxime Devos:
> 
> > We also avoid spelling out the non-free filename where possible,
> > preferring keep lists over remove lists, which this kind of patches
> > would be.
> Should we? I'm not seeing the point of that. I have not experienced
> any such avoidance myself, see e.g. 'tennix', 'neverball' and
> 'shogun'.  It is, to my knowledge, not forbidden to mention non-free
> software by name in code, as long as its not a recommendation
> (explicit or implied).
Indeed,  there is no hard rule, hence "avoid" rather than "forbid".

> > 
> > > +@subsubsection Fixing technical issues (compilation errors, test
> > > failures, other bugs ...)
> > > [...]
> > I am pretty sure that most of these are *not* done in snippets, but
> > rather phases, if they only affect Guix.  In particular, grep for
> > failing-tests and you will find a few phases disabling them.
> I do not think that ignoring a test counts as a bug fix.  I'll add it
> to this subsubsection, at cost of some additional length.
I do think it counts as "fixing technical issues such as test
failures".

> > In fact, as far as files that will not be installed are concerned,
> > I think phases ought to be preferred, because they're easier to
> > take away if an actual fix is made.
> I do not see a difference in hardness/easyness between removing a
> phase and removing a snippet (both are just a matter of opening
> an editor, pointing it at gnu/packages/... and removing a few lines),
> though I do consider removing a patch to be slightly harder (because
> gnu/local.mk is easy to forget).
There still is the difference that phases are clearly delimited while
snippets are a block of code that shouldn't get too large.

> > For the store path embedding, that's a rather roundabout way of
> > saying that contributers *ought to* embed store paths of certaing
> > things, such as commands invoked via exec et al.
> 
> It's not? It's kind of implied, yes, but the purpose isn't being a
> 'you should embed store paths' (subsub)section, but rather, 'if you
> go embedding store paths (at least for fixing a technical issue), do
> it in a phase'.
> 
> I'm not following what the complaint is, I suppose a section could be
> added somewhere to properly document the 'embedding store file names'
> practice, and insert a cross-reference, but that wasn't the purpose
> of the patch and going by later responses, you seem opposed to making
> things longer.
> 
> The alternative would be to remove this information, but then
> valuable information would be lost (there had been some cases where
> store file names were embedded in origin).
I think my version at least hinted at this practice in a more concise
way, so it's not impossible to mention.  Not embedding store paths *is*
a technical issue, because it'll cause the installed program to fail
and most people new to Guix will then just go "oh, let's propagate gcc-
toolchain".

> > > Otherwise, if the store
> > > +file name were embedded in the source, the result of
> > > @command{guix build
> > > +--source} would be unusable on non-Guix systems and also likely
> > > unusable
> > > +on Guix systems of another architecture.
> > Why are you repeating a guiding principle?
> I'm showing why, in this case, a phase must be used, by noting that
> not doing so would be contrary to one of the principles.
> 
> If not repeating the principle is desired, I could perhaps number
> them, and refer to the principles by number instead of restating
> them? Would reduce the length a little.
I think calling back to a guiding principle in and of itself shows that
the section has grown too long to remember it by the point you come to
this example, and I think that's more problematic than merely the
callback.  If you didn't need to divide this into subsubsections, you
could introduce the guiding principles in a way that feels more
natural.

> > > +@subsubsection Adding new functionality
> > > +To add new functionality, a patch is almost always the most
> > > convenient
> > > +choice of the three -- patches are usually multi-line changes,
> > > which
> > > are
> > > +convenient to do with patches and inconvenient to do with phases
> > > or
> > > +snippets.
> > Uhm, what?  Patches are the preferred form of patches?
> 
> No, I meant that patches are (usually) the preferred method for
> adding new functionality, and that multi-line changes are convenient
> to do with patches.  ‘which’ refers to the ‘multi-line changes’ here,
> not ‘patches’.
I still find this wording very confusing.  Perhaps "To add new
functionality, a patch is almost always the best choice.  For one, it
is likely that the new functionality requires changing multiple lines
of source code, which is more convenient to do with a patch than with a
snippet.  Further, patches can be taken from and submitted to upstreams
more easily.  If your patch has not been submitted to upstream,
consider doing so."

> > 
> > [...]
> > Overall, I'm not convinced that we have enough guiding principles
> > to call them that,
> 
> I don't think there's any lower limit on how many guiding principles
> to have, except for perhaps 2 (because otherwise it should have been
> singular or there aren't any).  At how few guiding principles stop
> the guiding principles from being guiding principles for you, and
> why?
> 
> For example, on <https://www.gnu.org/philosophy/free-sw.html>, four 
> guiding principles are mentioned (which are named 'essential
> freedoms' there), and
> <https://en.wikipedia.org/wiki/Guiding_Principles> has 5 ‘Guiding 
> Principles’.
An enumeration ought to have at least three elements (otherwise it's
just a pair), and I think if we do proper counting and omit no-
brainers, such as the "only free software" part that has already been
mentioned, we come very close to skirting that line.

> > which (along with its sheer length) is my main
> > complaint with the way you've phrased things.
> 
> (I'm assuming "its = the patch as a whole" here)
> 
> I could remove another section of the manual to compensate for the
> additional length, but I doubt that's what you intended.  I do not
> see the problem with the sheer length -- we have a bit of a
> documentation problem in Guix, there is lots of useful information
> that is currently undocumented.
> I do not think there have been any complaints about the manual being
> too long, if anything, it's too short.
I personally tend towards "less verbose", hence my complaint of
describing something with many words that could be described with
fewer.  A section can still be too long while the chapter around it is
too short.

> I've written some documentation, it was originally a bit hard to
> follow so in a next version I've restructured it a bit and explained
> more, this restructuring and explanation entailed some additional
> length.
> 
> There had been some proposals for additional cases to document, so
> they were added, increasing the length.  You have added new
> information is your patch, it was considered useful so I've
> integrated some of it in my patch, increasing the length.  (I didn't
> integrate all of the new parts, if I did, it would increase even
> further.  (If desired, in can integrate the rest, at cost  of some
> time.)).
My patch did not just state some things you missed, it also omitted
things that I think are either not necessary or probably better
documented elsewhere.

> I do not see what the problem is with additional length as long as
> this additional length comes with additional useful information and
> the manual is well-structured (e.g. with (sub)(sub)sections, chapters
> and indices) -- we do not have a page limit.
> 
> At worst, perhaps the same information could perhaps be encoded with
> fewer words? I could compare the two patches to see which one
> formulates certain information in the fewest words, and choose the
> least verbose of the two for each piece of information that is
> present in both?
> 
> Also, comparing the two patches, my patch has 40 more lines, but
> about 25 of them are for noting the guiding principles (which are
> absent in your patch).
> Compensating for that, the patches are about the same length, so I do
> not think that 'sheer length' is accurate here.
25 lines calling back to earlier information are, imho, an indicator
that the section is too long.  Imagine you'd have twenty-five function
calls to guiding_principles(n) in your program – at some point, you'd
try to cache those.

> > Going down to subsubsections just to find out where patches are
> > appropriate, is imho overkill.
> 
> The 'going down to subsubsection' is the case for your patch too,
> though?  In my case, it's a subsubsection, in your case it's a table
> entry inside a subsection, both are the same level of nesting.
These are still two very different kinds of nesting.  A table fits onto
a (virtual) page more easily than several subsections.

> Also, it's a matter of different structure -- in my v2 and v3 patch,
> I have a 'problem -> solution' structure -- the idea is that the
> packages has a problem, they look at the section, they read the
> subsubsection names, select the
> subsubsection that matches their problem and read the solution -- in
> short, the idea is to provide a solution to the problem.
> 
> Your structure is the other way around -- for solutions (patches,
> snippets, phases), it gives the permitted problems to apply it to.
> 
> So yes, your patch is more convenient for finding out where patches
> are appropriate.  I do not see the benefit of that though -- a new
> contributor packaging a thing wouldn't know in advance which
> solutions could be appropriate for them (your 'solution -> problem'
> patch?), rather, they start with a problem and are searching for an
> appropriate solution (my problem->solution patch).
I think this idea can be debunked pretty easily.  If I give you a
hammer and I tell you "this is a hammer, you can use it to put nails
into a wall", and you have a nail and you want to put it into a wall,
you won't go "oh no, however will I put this nail into a wall?" – you
will simply use the hammer to do so.  Of course, for this to work I
also have to tell you *how* to use a hammer to put nails into a wall,
but that's exactly what I did in my patch by inserting the right notes
into the Guix manual.

My solution->problem approach has the benefit, that folks can just go
over all the solutions, check if their problem fits, and apply the one
that says "here, use this".  And if they don't find anything, they see
the handy little line at the bottom saying "use whatever you think is
convenient".  I also expand a little on the benefits and drawbacks of
these approaches as you would when describing design patterns.

Your problem->solution approach instead leaves people wondering when
their particular use case has not been described.  It gives them a
solution rather than the tools to build solutions with.

Cheers

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

[Maxime Devos]

[-- Attachment #1.1.1.1: Type: text/plain, Size: 18302 bytes --]

On 07-09-2022 10:09, Liliana Marie Prikler wrote:

> Am Dienstag, dem 06.09.2022 um 22:21 +0200 schrieb Maxime Devos:
>>> We also avoid spelling out the non-free filename where possible,
>>> preferring keep lists over remove lists, which this kind of patches
>>> would be.
>> Should we? I'm not seeing the point of that. I have not experienced
>> any such avoidance myself, see e.g. 'tennix', 'neverball' and
>> 'shogun'.  It is, to my knowledge, not forbidden to mention non-free
>> software by name in code, as long as its not a recommendation
>> (explicit or implied).
> Indeed,  there is no hard rule, hence "avoid" rather than "forbid".

What I also meant is, that to my knowledge there is no soft rule either.

Again, why should we avoid this, what's the point of that?

>>>> +@subsubsection Fixing technical issues (compilation errors, test
>>>> failures, other bugs ...)
>>>> [...]
>>> I am pretty sure that most of these are *not* done in snippets, but
>>> rather phases, if they only affect Guix.  In particular, grep for
>>> failing-tests and you will find a few phases disabling them.
>> I do not think that ignoring a test counts as a bug fix.  I'll add it
>> to this subsubsection, at cost of some additional length.
> I do think it counts as "fixing technical issues such as test
> failures".
How does ignoring a test fix the technical issue identified by the test
(sometimes, the technical issue being a bug in the test itself)?
>>> In fact, as far as files that will not be installed are concerned,
>>> I think phases ought to be preferred, because they're easier to
>>> take away if an actual fix is made.
>> I do not see a difference in hardness/easyness between removing a
>> phase and removing a snippet (both are just a matter of opening
>> an editor, pointing it at gnu/packages/... and removing a few lines),
>> though I do consider removing a patch to be slightly harder (because
>> gnu/local.mk is easy to forget).
> There still is the difference that phases are clearly delimited while
> snippets are a block of code that shouldn't get too large.

Snippets are delimited clearly as well, though, with the 'snippet' field?

And the limitations of snippet length and phases length are the same
-- no limits, though conciseness is appreciate as always.

>>> For the store path embedding, that's a rather roundabout way of
>>> saying that contributers *ought to* embed store paths of certaing
>>> things, such as commands invoked via exec et al.
>> It's not? It's kind of implied, yes, but the purpose isn't being a
>> 'you should embed store paths' (subsub)section, but rather, 'if you
>> go embedding store paths (at least for fixing a technical issue), do
>> it in a phase'.
>>
>> I'm not following what the complaint is, I suppose a section could be
>> added somewhere to properly document the 'embedding store file names'
>> practice, and insert a cross-reference, but that wasn't the purpose
>> of the patch and going by later responses, you seem opposed to making
>> things longer.
>>
>> The alternative would be to remove this information, but then
>> valuable information would be lost (there had been some cases where
>> store file names were embedded in origin).
> I think my version at least hinted at this practice in a more concise
> way, so it's not impossible to mention. [...]

I agree it's possible -- as I replied previously:

> I suppose a section could be added somewhere to properly document the
> 'embedding store file names' practice, and insert a cross-reference,
I don't think documenting the how of the practice should be done
in this section, properly explaining 'search-input-file' / 
'search-input-directory',
'inputs / native-inputs', 'bash' being an implicit input but you still
have to add it to 'inputs' in some cases because of cross-compilation,
this-package-input and this-package-native-input ... would make the
subsubsection a bit too long I think, distracting from other situations,
hence the proposal for a cross-reference.

How about leaving the 'how to embed store file names' for a separate
documentation patch and section, adding a cross-reference later?

>>>> Otherwise, if the store
>>>> +file name were embedded in the source, the result of
>>>> @command{guix build
>>>> +--source} would be unusable on non-Guix systems and also likely
>>>> unusable
>>>> +on Guix systems of another architecture.
>>> Why are you repeating a guiding principle?
>> I'm showing why, in this case, a phase must be used, by noting that
>> not doing so would be contrary to one of the principles.
>>
>> If not repeating the principle is desired, I could perhaps number
>> them, and refer to the principles by number instead of restating
>> them? Would reduce the length a little.
> I think calling back to a guiding principle in and of itself shows that
> the section has grown too long to remember it by the point you come to
> this example,
This has nothing to do with length and remembering, but rather with
explaining why a phase must be used -- to explain that, I state which
principle applies (as mentioned previously). If I removed the
explanations, I would just be stating how to do things, without giving
a logical reasoning on the 'why'.
> and I think that's more problematic than merely the
> callback.  If you didn't need to divide this into subsubsections, you
> could introduce the guiding principles in a way that feels more
> natural.
I consider it more natural to have the 'guiding principles' _before_ the
concrete cases, as they are meant to be 'guiding' and 'principles'. It's
like 'starting from first principles', there introducing the first 
principles
as you go is ad-hoc.

The guiding principles also need to be outside the examples, in case
one of the examples doesn't apply to the packager's use case, such
that they can fall-back to the guiding principles.

Also, in your patch you are dividing things in subsubsections as well,
just under a different name and different representation (table entries
in a subsection), as mentioned previously.

>>>> +@subsubsection Adding new functionality
>>>> +To add new functionality, a patch is almost always the most
>>>> convenient
>>>> +choice of the three -- patches are usually multi-line changes,
>>>> which
>>>> are
>>>> +convenient to do with patches and inconvenient to do with phases
>>>> or
>>>> +snippets.
>>> Uhm, what?  Patches are the preferred form of patches?
>> No, I meant that patches are (usually) the preferred method for
>> adding new functionality, and that multi-line changes are convenient
>> to do with patches.  ‘which’ refers to the ‘multi-line changes’ here,
>> not ‘patches’.
> I still find this wording very confusing.  Perhaps "To add new
> functionality, a patch is almost always the best choice.  For one, it
> is likely that the new functionality requires changing multiple lines
> of source code, which is more convenient to do with a patch than with a
> snippet.  Further, patches can be taken from and submitted to upstreams
> more easily.  If your patch has not been submitted to upstream,
> consider doing so."
It loses some information (that patches are preferred) and
(after re-adding the conclusion) is more verbose, which appears
to be considered very important.
>>> [...]
>>> Overall, I'm not convinced that we have enough guiding principles
>>> to call them that,
>> I don't think there's any lower limit on how many guiding principles
>> to have, except for perhaps 2 (because otherwise it should have been
>> singular or there aren't any).  At how few guiding principles stop
>> the guiding principles from being guiding principles for you, and
>> why?
>>
>> For example, on<https://www.gnu.org/philosophy/free-sw.html>, four
>> guiding principles are mentioned (which are named 'essential
>> freedoms' there), and
>> <https://en.wikipedia.org/wiki/Guiding_Principles>  has 5 ‘Guiding
>> Principles’.
> An enumeration ought to have at least three elements (otherwise it's
> just a pair), and I think if we do proper counting and omit no-
> brainers, such as the "only free software" part that has already been
> mentioned, we come very close to skirting that line.
The "only free software" is mentioned elsewhere, yes, but it is one
of the principles for deciding between snippets, phases and patches.
While you call it a no-brainer, it is sometimes neglected, so it sounds
important to me to explicitly list it.

Merging the 3th and 4th @item, I count 4 principles, so it fits with
an enumeration.

Also, I'm not following your point here -- your complaint was that they
aren't guiding principles (based on the number of them), but your
response is that they might not form an enumeration?  They are named
the guiding principles, not the guiding enumeration.  What have
enumerations to do with anything?

>>> which (along with its sheer length) is my main
>>> complaint with the way you've phrased things.
>> (I'm assuming "its = the patch as a whole" here)
>>
>> I could remove another section of the manual to compensate for the
>> additional length, but I doubt that's what you intended.  I do not
>> see the problem with the sheer length -- we have a bit of a
>> documentation problem in Guix, there is lots of useful information
>> that is currently undocumented.
>> I do not think there have been any complaints about the manual being
>> too long, if anything, it's too short.
> I personally tend towards "less verbose", hence my complaint of
> describing something with many words that could be described with
> fewer.  A section can still be too long while the chapter around it is
> too short.

Do you have anything in particular in mind?

>> I've written some documentation, it was originally a bit hard to
>> follow so in a next version I've restructured it a bit and explained
>> more, this restructuring and explanation entailed some additional
>> length.
>>
>> There had been some proposals for additional cases to document, so
>> they were added, increasing the length.  You have added new
>> information is your patch, it was considered useful so I've
>> integrated some of it in my patch, increasing the length.  (I didn't
>> integrate all of the new parts, if I did, it would increase even
>> further.  (If desired, in can integrate the rest, at cost  of some
>> time.)).
> My patch did not just state some things you missed, it also omitted
> things that I think are either not necessary or probably better
> documented elsewhere.
What particular things do you have in mind, and where do you
think they should better be documented?  I can move things
around a bit and add cross-references.
>> I do not see what the problem is with additional length as long as
>> this additional length comes with additional useful information and
>> the manual is well-structured (e.g. with (sub)(sub)sections, chapters
>> and indices) -- we do not have a page limit.
>>
>> At worst, perhaps the same information could perhaps be encoded with
>> fewer words? I could compare the two patches to see which one
>> formulates certain information in the fewest words, and choose the
>> least verbose of the two for each piece of information that is
>> present in both?
>>
>> Also, comparing the two patches, my patch has 40 more lines, but
>> about 25 of them are for noting the guiding principles (which are
>> absent in your patch).
>> Compensating for that, the patches are about the same length, so I do
>> not think that 'sheer length' is accurate here.
> 25 lines calling back to earlier information are, imho, an indicator
> that the section is too long.  Imagine you'd have twenty-five function
> calls to guiding_principles(n) in your program – at some point, you'd
> try to cache those.
(define cached-guiding-principles
   (delay (list (guiding-principle-0)
                     [...]
                     (guiding-principle-24))))

Caching the guiding principles does not reduce the length.

I don't see the problem with calling back to earlier information.
Also, it isn't earlier information, there is no nice list of guiding
principles anywhere else.

>>> Going down to subsubsections just to find out where patches are
>>> appropriate, is imho overkill.
>> The 'going down to subsubsection' is the case for your patch too,
>> though?  In my case, it's a subsubsection, in your case it's a table
>> entry inside a subsection, both are the same level of nesting.
> These are still two very different kinds of nesting.  A table fits onto
> a (virtual) page more easily than several subsections.
I suppose table items might take two less line or so less than
subsubsections, but I don't think that's sufficient reason to step
away from a nice section structure.
>> Also, it's a matter of different structure -- in my v2 and v3 patch,
>> I have a 'problem -> solution' structure -- the idea is that the
>> packages has a problem, they look at the section, they read the
>> subsubsection names, select the
>> subsubsection that matches their problem and read the solution -- in
>> short, the idea is to provide a solution to the problem.
>>
>> Your structure is the other way around -- for solutions (patches,
>> snippets, phases), it gives the permitted problems to apply it to.
>>
>> So yes, your patch is more convenient for finding out where patches
>> are appropriate.  I do not see the benefit of that though -- a new
>> contributor packaging a thing wouldn't know in advance which
>> solutions could be appropriate for them (your 'solution -> problem'
>> patch?), rather, they start with a problem and are searching for an
>> appropriate solution (my problem->solution patch).
> I think this idea can be debunked pretty easily.  If I give you a
> hammer and I tell you "this is a hammer, you can use it to put nails
> into a wall", and you have a nail and you want to put it into a wall,
> you won't go "oh no, however will I put this nail into a wall?" – you
> will simply use the hammer to do so.
The patch does this, currently.  It already proposes a number of hammers
(patches, snippets and phases) and purposes (adding new functionality,
fixing technical issues, unbundling, ...).

Also, the scenario "oh no, however will I put this nail into a wall"
actually happened -- see the Shepherd discussion, where there was
a lot of disagreement on how nails (= small work-around in the Makefile)
should be put in walls (= patches, snippet, phase?).   It was the whole
reason to start writing a documentation patch.

> Of course, for this to work I
> also have to tell you *how* to use a hammer to put nails into a wall,
> but that's exactly what I did in my patch by inserting the right notes
> into the Guix manual.
Also already the case.
> My solution->problem approach has the benefit, that folks can just go
> over all the solutions, check if their problem fits, and apply the one
> that says "here, use this".

A problem->solution structure is useful for that too?

And it already lists all the solutions (snippets, phases and patches)
and information to decide whether the solution fits their problem
(the guiding principles, and the worked-out cases).

> And if they don't find anything, they see
> the handy little line at the bottom saying "use whatever you think is
> convenient".
Nowhere did the patch imply that the listed cases were all cases. In fact,
in two places in the introduction it is implied that the examples are not
exhaustive, and that they can choose according to convenience:

  * ‘To make things more concrete and to resolve conflicts between the
    principles, a _few_ cases have been worked out:’

    (Emphasis on _few_ added)
  * ‘When there is more than one way to do something, choose whichever
    method
    is the simplest’

    (It says ‘simplest’ instead of ‘most convenient’, but whatever.)

>    I also expand a little on the benefits and drawbacks of
> these approaches as you would when describing design patterns.

This is also done in my patch. E.g.,

  * ‘There are a few benefits for snippets here:

    When using snippets, the bundled library does not occur in the source

    returned by @code{guix build --source}, so users and reviewers do not

    have to worry about whether the bundled library contains malware,

    whether it is non-free, if it contains pre-compiled binaries ... There

    are also less licensing concerns: if the bundled libraries are removed,

    it becomes less likely that the licensing conditions apply to people

    sharing the source returned by @command{guix build --source}, especially if

    the bundled library is not actually used on Guix systems.@footnote{This

    is @emph{not} a claim that you can simply ignore the licenses of

    libraries when they are unbundled and replaced by Guix packages -- there

    are less concerns, not none.}

    As such, snippets are recommended here.’

  * ‘For phases, the problem is that phases do not influence the result of
    @command{guix build --source}.’

> Your problem->solution approach instead leaves people wondering when
> their particular use case has not been described.
See my reply to ‘And if they don't find anything, they see the handy little
line at the bottom saying "use whatever you think is convenient’.
> It gives them a solution rather than the tools to build solutions with.

It does give the tools: snippets, patches and phases.  And as tools
for deciding between the three for not-yet-documented cases, there are
the guiding principles.  As a demonstration on how to use these guiding
principles, various cases have been worked out based on the guiding
principles.

Summarised, it gives both the tools _and_ the solutions.

Also, "giving the tools to build solutions with" does not help with the
problem that I aimed to solve -- there was disagreement on what the
appropriate tools are (see: Shepherd), so it not just needs to give the
tools, but also the solutions.

Greetings,
Maxime

[-- Attachment #1.1.1.2: Type: text/html, Size: 26582 bytes --]

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 929 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

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

[Liliana Marie Prikler]

Am Donnerstag, dem 08.09.2022 um 13:12 +0200 schrieb Maxime Devos:
> On 07-09-2022 10:09, Liliana Marie Prikler wrote:
> > Am Dienstag, dem 06.09.2022 um 22:21 +0200 schrieb Maxime Devos:
> > > It is, to my knowledge, not forbidden to mention non-free
> > > software by name in code, as long as its not a recommendation
> > > (explicit or implied).
> > Indeed, there is no hard rule, hence "avoid" rather than "forbid".
> What I also meant is, that to my knowledge there is no soft rule
> either.  Again, why should we avoid this, what's the point of that?
In descriptions, it is wise to do so because it helps software stand on
its own merits rather than just "being a clone of this thing you aren't
allowed to have" (this is real criticism pointed at us from the
proprietary software embracers).  See for instance minetest, whichisy

> How does ignoring a test fix the technical issue identified by the
> test (sometimes, the technical issue being a bug in the test itself)?
It fixes the technical issue that an otherwise functional package
(w.r.t. the N tests that don't fail) builds.  This is a particularly
useful distinction with tests that require a network connection and
thus have to fail in a build container, or are known flaky upstream and
thus cause reproducibility issues.

> > There still is the difference that phases are clearly delimited
> > while snippets are a block of code that shouldn't get too large.
> Snippets are delimited clearly as well, though, with the 'snippet'
> field?
> And the limitations of snippet length and phases length are the same
>  -- no limits, though conciseness is appreciate as always. 
True, but phases can be made to do just one thing, whereas snippets
have to fix everything that's wrong in more or less one (begin ...). 
This is a noticable distinction.

> 
> > I think my version at least hinted at this practice in a more
> > concise way, so it's not impossible to mention. [...]
> I agree it's possible -- as I replied previously:
> > I suppose a section could be added somewhere to properly document
> > the 'embedding store file names' practice, and insert a
> > cross-reference,
> I don't think documenting the how of the practice should be done
> in this section, properly explaining 'search-input-file' / 'search-
> input-directory', 'inputs / native-inputs', 'bash' being an implicit
> input but you still have to add it to 'inputs' in some cases because
> of cross-compilation, this-package-input and this-package-native-
> input ... would make the
> subsubsection a bit too long I think, distracting from other
> situations, hence the proposal for a cross-reference.
> How about leaving the 'how to embed store file names' for a separate
>  documentation patch and section, adding a cross-reference later?
See above, "I suppose a section could be added..." means I'm somewhat
indifferent to whether it's done now or later; I would however very
much prefer a wording that points people toward this practice existing,
even if they have to go look in the code for examples.  Alternatively,
a footnote for the most common case (search-input-file inputs
"bin/command") ought to suffice for the time being.

> 
> 
> > I think calling back to a guiding principle in and of itself shows
> > that the section has grown too long to remember it by the point you
> > come to this example,
> This has nothing to do with length and remembering, but rather with
> explaining why a phase must be used -- to explain that, I state which
> principle applies (as mentioned previously). If I removed the
> explanations, I would just be stating how to do things, without
> giving a logical reasoning on the 'why'.
IMHO, I think a reader who remembers the guiding principles should see
that it applies.

> > and I think that's more problematic than merely the
> > callback. If you didn't need to divide this into subsubsections,
> > you could introduce the guiding principles in a way that feels more
> > natural.
> I consider it more natural to have the 'guiding principles' _before_
> the concrete cases, as they are meant to be 'guiding' and
> 'principles'. 
> It's like 'starting from first principles', there introducing the
> first principles as you go is ad-hoc.
> The guiding principles also need to be outside the examples, in case
>  one of the examples doesn't apply to the packager's use case, such
>  that they can fall-back to the guiding principles.
> Also, in your patch you are dividing things in subsubsections as
> well, just under a different name and different representation (table
> entries in a subsection), as mentioned previously.
A table entry is not a subsection, as much as you want it to be that.

Also, your guiding principles are (with one exception) really just
invariants that ought to hold for the source field of a package.  
As such, I think it would be easier to state "A package's source should
be the smallest corresponding source in terms of uncompressed file
size.  This corresponding source must consist only of free software
(note Free Software) and should build on all platforms supported by
upstream."  Note how smallest naturally implies unbundling bundled
sources.

> > I still find this wording very confusing. Perhaps "To add new
> > functionality, a patch is almost always the best choice. For one,
> > it is likely that the new functionality requires changing multiple
> > lines of source code, which is more convenient to do with a patch
> > than with a snippet. Further, patches can be taken from and
> > submitted to upstreams more easily. If your patch has not been
> > submitted to upstream, consider doing so."
> It loses some information (that patches are preferred) and
> (after re-adding the conclusion) is more verbose, which appears
> to be considered very important.
Which conclusion is there to re-add?  The conclusion is already stated
in the beginning: patches are almost always the best choice.  Then two
reasons as for why.  The part w.r.t. upstreaming changes has also been
addressed.

> > An enumeration ought to have at least three elements (otherwise
> > it's just a pair), and I think if we do proper counting and omit
> > no-brainers, such as the "only free software" part that has already
> > been mentioned, we come very close to skirting that line.
> The "only free software" is mentioned elsewhere, yes, but it is one
> of the principles for deciding between snippets, phases and patches.
> While you call it a no-brainer, it is sometimes neglected, so it
> sounds important to me to explicitly list it. 
> Merging the 3th and 4th @item, I count 4 principles, so it fits with
> an enumeration.
> Also, I'm not following your point here -- your complaint was that
> they aren't guiding principles (based on the number of them), but
> your response is that they might not form an enumeration?  They are
> named the guiding principles, not the guiding enumeration.  What have
> enumerations to do with anything?
I'm using enumeration as a super term here, which can be
((sub)sub)sections, chapters, list elements, whatever, and my claim is
that we barely have enough principles to allow the use of a plural. 
Adding to that, now that I think of it, I also doubt their usefulness
in guiding.  "Use whatever feels convenient, but note that that might
be subjective" is more useful at the end of the section when a user
didn't find what they were looking for than at the start.

> > > > which (along with its sheer length) is my main
> > > > complaint with the way you've phrased things.
> > > (I'm assuming "its = the patch as a whole" here)
> > > 
> > > I could remove another section of the manual to compensate for
> > > the additional length, but I doubt that's what you intended.  I
> > > do not see the problem with the sheer length -- we have a bit of
> > > a documentation problem in Guix, there is lots of useful
> > > information that is currently undocumented.
> > > I do not think there have been any complaints about the manual
> > > being too long, if anything, it's too short.
> > I personally tend towards "less verbose", hence my complaint of
> > describing something with many words that could be described with
> > fewer. A section can still be too long while the chapter around it
> > is too short.
> Do you have anything in particular in mind?
This is more of a broad statement that applies to the patch as a whole
than to any of its constituent parts in particular.  However, in some
cases where I think it's particularly noticable, I'll try to point out
shorter formulations.

> > > I've written some documentation, it was originally a bit hard to
> > > follow so in a next version I've restructured it a bit and
> > > explained more, this restructuring and explanation entailed some
> > > additional length.
> > > 
> > > There had been some proposals for additional cases to document,
> > > so they were added, increasing the length.  You have added new
> > > information is your patch, it was considered useful so I've
> > > integrated some of it in my patch, increasing the length.  (I
> > > didn't integrate all of the new parts, if I did, it would
> > > increase even further.  (If desired, in can integrate the rest,
> > > at cost of some time.)).
> > My patch did not just state some things you missed, it also omitted
> > things that I think are either not necessary or probably better
> > documented elsewhere.
> What particular things do you have in mind, and where do you
> think they should better be documented?  I can move things
> around a bit and add cross-references. 
> > 
> > > I do not see what the problem is with additional length as long
> > > as this additional length comes with additional useful
> > > information and the manual is well-structured (e.g. with
> > > (sub)(sub)sections, chapters and indices) -- we do not have a
> > > page limit.
> > > 
> > > At worst, perhaps the same information could perhaps be encoded
> > > with fewer words? I could compare the two patches to see which
> > > one formulates certain information in the fewest words, and
> > > choose the least verbose of the two for each piece of information
> > > that is present in both?
> > > 
> > > Also, comparing the two patches, my patch has 40 more lines, but
> > > about 25 of them are for noting the guiding principles (which are
> > > absent in your patch).
> > > Compensating for that, the patches are about the same length, so
> > > I do not think that 'sheer length' is accurate here.
> > 25 lines calling back to earlier information are, imho, an
> > indicator that the section is too long. Imagine you'd have twenty-
> > five function calls to guiding_principles(n) in your program – at
> > some point, you'd try to cache those.
> (define cached-guiding-principles
>   (delay (list (guiding-principle-0)
>                     [...]
>                     (guiding-principle-24)))) 
> Caching the guiding principles does not reduce the length.
> I don't see the problem with calling back to earlier information.
> Also, it isn't earlier information, there is no nice list of guiding
> principles anywhere else.
At the risk of responding jokingly to what was meant serious: I didn't
know we suddenly gained 20 guiding principles.

> 
> > > > Going down to subsubsections just to find out where patches are
> > > > appropriate, is imho overkill.
> > > The 'going down to subsubsection' is the case for your patch too,
> > > though? In my case, it's a subsubsection, in your case it's a
> > > table
> > > entry inside a subsection, both are the same level of nesting.
> > These are still two very different kinds of nesting. A table fits
> > onto a (virtual) page more easily than several subsections.
> I suppose table items might take two less line or so less than
> subsubsections, but I don't think that's sufficient reason to step
> away from a nice section structure.
Another reason is that you can end a table in the middle of a section,
which you can't do with subsections.  Hence why I'm able to put a
remark at the bottom, which you have to clumsily fit into the top.

> > > Also, it's a matter of different structure -- in my v2 and v3
> > > patch, I have a 'problem -> solution' structure -- the idea is
> > > that the packages has a problem, they look at the section, they
> > > read the subsubsection names, select the
> > > subsubsection that matches their problem and read the solution --
> > > in short, the idea is to provide a solution to the problem.
> > > 
> > > Your structure is the other way around -- for solutions (patches,
> > > snippets, phases), it gives the permitted problems to apply it
> > > to.
> > > 
> > > So yes, your patch is more convenient for finding out where
> > > patches are appropriate.  I do not see the benefit of that though
> > > -- a new contributor packaging a thing wouldn't know in advance
> > > which solutions could be appropriate for them (your 'solution ->
> > > problem' patch?), rather, they start with a problem and are
> > > searching for an appropriate solution (my problem->solution
> > > patch).
> > I think this idea can be debunked pretty easily. If I give you a
> > hammer and I tell you "this is a hammer, you can use it to put
> > nails into a wall", and you have a nail and you want to put it into
> > a wall, you won't go "oh no, however will I put this nail into a
> > wall?" – you will simply use the hammer to do so.
> The patch does this, currently.  It already proposes a number of
> hammers (patches, snippets and phases) and purposes (adding new
> functionality, fixing technical issues, unbundling, ...). 
> Also, the scenario "oh no, however will I put this nail into a wall"
> actually happened -- see the Shepherd discussion, where there was
> a lot of disagreement on how nails (= small work-around in the
> Makefile) should be put in walls (= patches, snippet, phase?).   It
> was the whole reason to start writing a documentation patch.
You might want to add a link here if it supports your argument, but
without looking at the discussion this rather sounds like "oh no, I
have three hammers, which one do I pick?" – which, fair enough, is
still a problem, but one that neither of our patches would cause imho.

> > Of course, for this to work I also have to tell you *how* to use a
> > hammer to put nails into a wall, but that's exactly what I did in
> > my patch by inserting the right notes into the Guix manual.
> Also already the case. 
> > My solution->problem approach has the benefit, that folks can just
> > go over all the solutions, check if their problem fits, and apply
> > the one that says "here, use this".
> A problem->solution structure is useful for that too?
> And it already lists all the solutions (snippets, phases and patches)
> and information to decide whether the solution fits their problem
> (the guiding principles, and the worked-out cases).
Again, I believe you're overselling the guiding principles.

> > And if they don't find anything, they see the handy little line at
> > the bottom saying "use whatever you think is convenient".
> Nowhere did the patch imply that the listed cases were all cases. In
> fact, in two places in the introduction it is implied that the
> examples are not exhaustive, and that they can choose according to
> convenience  [...]
Emphasis on handy little line rather than needing to be told twice
(particularly if people have no idea what to expect due to not having
looked at the worked-out cases yet).

> >  I also expand a little on the benefits and drawbacks of
> > these approaches as you would when describing design patterns.
> This is also done in my patch. [...]
This is not about the contained information, but the structure of the
contained information.

My solution->problem style follows roughly this style:
1. solution
2. problems it is known to solve
3. how to use
4. properties, caveats, etc.

Your problem->solution style roughly has the following:
1. problem
2. (set of) solution(s)
3. if more than one solution, maybe a help to select

This makes it so that people might have to go to a different subsection
than the one they read for their solution to find out about potential
caveats, e.g. not embedding store paths in a snippet.

> > Your problem->solution approach instead leaves people wondering
> > when their particular use case has not been described.
> See my reply to ‘And if they don't find anything, they see the handy
> little line at the bottom saying "use whatever you think is
> convenient’. 
> > It gives them a solution rather than the tools to build solutions
> > with.
> It does give the tools: snippets, patches and phases.
As far as I read, it describes none of those.  It puts out guiding
principles and some already worked-out cases.

> Also, "giving the tools to build solutions with" does not help with
> the problem that I aimed to solve -- there was disagreement on what
> the appropriate tools are (see: Shepherd), so it not just needs to
> give the tools, but also the solutions.
I don't see how my patch lacks this information however.  In
particular, for review purposes, mine should be easier to work with. 
For instance, the reviewer sees a hash embedded in a snippet, sees in
the snippet entry that you shouldn't do that, and can thus say "nope,
don't do a snippet here".

Regardless of which patch we pick, it should not mandate a particular
solution in potentially contentious cases, and also point towards the
right solution.  See our discussions on the individual solutions on
points in which I believe you've errored.

Cheers

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

[Maxime Devos]

[-- Attachment #1.1.1: Type: text/plain, Size: 26078 bytes --]



On 09-09-2022 10:04, Liliana Marie Prikler wrote:
> Am Donnerstag, dem 08.09.2022 um 13:12 +0200 schrieb Maxime Devos:
>> On 07-09-2022 10:09, Liliana Marie Prikler wrote:
>>> Am Dienstag, dem 06.09.2022 um 22:21 +0200 schrieb Maxime Devos:
>>>> It is, to my knowledge, not forbidden to mention non-free
>>>> software by name in code, as long as its not a recommendation
>>>> (explicit or implied).
>>> Indeed, there is no hard rule, hence "avoid" rather than "forbid".
>> What I also meant is, that to my knowledge there is no soft rule
>> either.  Again, why should we avoid this, what's the point of that?
> In descriptions, it is wise to do so because it helps software stand on
> its own merits rather than just "being a clone of this thing you aren't
> allowed to have" (this is real criticism pointed at us from the
> proprietary software embracers).  See for instance minetest, whichisy

Sentence might have been truncated? Also, this is the package source 
field, not the description, I don't see how the "helps software stand on 
its own merits" applies to snippets of the package source.

> 
>> How does ignoring a test fix the technical issue identified by the
>> test (sometimes, the technical issue being a bug in the test itself)?
> It fixes the technical issue that an otherwise functional package
> (w.r.t. the N tests that don't fail) builds.  This is a particularly
> useful distinction with tests that require a network connection and
> thus have to fail in a build container, or are known flaky upstream and
> thus cause reproducibility issues.

Network test: right (though preferably those would support a 
--no-network-tests test option or such).

For flaky tests: those sound like bugs to me, ignoring them doesn't 
remove the flakyness.

>>> There still is the difference that phases are clearly delimited
>>> while snippets are a block of code that shouldn't get too large.
>> Snippets are delimited clearly as well, though, with the 'snippet'
>> field?
>> And the limitations of snippet length and phases length are the same
>>   -- no limits, though conciseness is appreciate as always.
> True, but phases can be made to do just one thing, whereas snippets
> have to fix everything that's wrong in more or less one (begin ...).
> This is a noticable distinction.> 

You can do that in snippets too, with comments:

(snippet
   #~(begin
       ;; Do the foo thing
       (foo)
       (foo-2 [...])
       [...]
       ;; Do the bar thing
       (bar)
       (bar-2 [...])
       [...]))


>>
>>> I think my version at least hinted at this practice in a more
>>> concise way, so it's not impossible to mention. [...]
>> I agree it's possible -- as I replied previously:
>>> I suppose a section could be added somewhere to properly document
>>> the 'embedding store file names' practice, and insert a
>>> cross-reference,
>> I don't think documenting the how of the practice should be done
>> in this section, properly explaining 'search-input-file' / 'search-
>> input-directory', 'inputs / native-inputs', 'bash' being an implicit
>> input but you still have to add it to 'inputs' in some cases because
>> of cross-compilation, this-package-input and this-package-native-
>> input ... would make the
>> subsubsection a bit too long I think, distracting from other
>> situations, hence the proposal for a cross-reference.
>> How about leaving the 'how to embed store file names' for a separate
>>   documentation patch and section, adding a cross-reference later?
> See above, "I suppose a section could be added..." means I'm somewhat
> indifferent to whether it's done now or later;

Nitpick: you are quoting some text I wrote, so 'I' refers to me here, 
not you.

  I would however very
> much prefer a wording that points people toward this practice existing,
> even if they have to go look in the code for examples.  Alternatively,
> a footnote for the most common case (search-input-file inputs
> "bin/command") ought to suffice for the time being.

Aside from the typo's and a few rephrasings, I'm done with the 
documentation.  If someone wants to extend the section with such 
information, they can always do so later.

>>
>>> I think calling back to a guiding principle in and of itself shows
>>> that the section has grown too long to remember it by the point you
>>> come to this example,
>> This has nothing to do with length and remembering, but rather with
>> explaining why a phase must be used -- to explain that, I state which
>> principle applies (as mentioned previously). If I removed the
>> explanations, I would just be stating how to do things, without
>> giving a logical reasoning on the 'why'.
> IMHO, I think a reader who remembers the guiding principles should see
> that it applies.

Likely. But removing the explicit mention of the guiding principle still 
makes the logical reasoning incomplete, remembering has nothing to do 
with it.  As I've written previously: ‘This has nothing do do with [...] 
and remembering, but rather with [...]’.

>>> and I think that's more problematic than merely the
>>> callback. If you didn't need to divide this into subsubsections,
>>> you could introduce the guiding principles in a way that feels more
>>> natural.
>> I consider it more natural to have the 'guiding principles' _before_
>> the concrete cases, as they are meant to be 'guiding' and
>> 'principles'.
>> It's like 'starting from first principles', there introducing the
>> first principles as you go is ad-hoc.
>> The guiding principles also need to be outside the examples, in case
>>   one of the examples doesn't apply to the packager's use case, such
>>   that they can fall-back to the guiding principles.
>> Also, in your patch you are dividing things in subsubsections as
>> well, just under a different name and different representation (table
>> entries in a subsection), as mentioned previously.
> A table entry is not a subsection, as much as you want it to be that.

It indeed is not, rather they are equivalent here in terms of structure, 
nesting and problematicness.

> 
> Also, your guiding principles are (with one exception) really just
> invariants that ought to hold for the source field of a package.

I don't know about the exceptions (I haven't counted them), but yes, 
indeed.  I do not see the problem of this.

> As such, I think it would be easier to state "A package's source should
> be the smallest corresponding source in terms of uncompressed file
> size.  This corresponding source must consist only of free software
> (note Free Software) and should build on all platforms supported by
> upstream."  Note how smallest naturally implies unbundling bundled
> sources.

This criterium on overly small sources.  Often, a package's source 
contains things that is not used for the build or outputs and hence
is not part of the corresponding source.  Examples:

* the source contains documentation that could be built and installed,
   but Guix doesn't do so yet.  --> should be kept (unless non-free)
* documentation that isn't meant to be built or installed
   (e.g. HACKING, PACKAGERS, ...) --> useful, shouldn't be removed.
* .gitignore, .github, ... --> nothing wrong with removing those,
   but pointless, let's not waste our time with looking for those
   and removing them, even though doing so would make it smaller.
* source files for platforms the upstream does not support yet/anymore
   (but with some volunteer effort (e.g. bugfixes), it might become
   a supported system again)
   --> removing them (e.g. as part of an overly-broad 
(delete-file-recursively 
"this-dir-has-bundling-albeit-not-all-of-it-is-bundling"))
       can be OK-ish, but removing them for 'minimality' is pointless.


>>> I still find this wording very confusing. Perhaps "To add new
>>> functionality, a patch is almost always the best choice. For one,
>>> it is likely that the new functionality requires changing multiple
>>> lines of source code, which is more convenient to do with a patch
>>> than with a snippet. Further, patches can be taken from and
>>> submitted to upstreams more easily. If your patch has not been
>>> submitted to upstream, consider doing so."
>> It loses some information (that patches are preferred) and
>> (after re-adding the conclusion) is more verbose, which appears
>> to be considered very important.
> Which conclusion is there to re-add?

"patches are preferred"

   The conclusion is already stated
> in the beginning: patches are almost always the best choice.  Then two
> reasons as for why.  The part w.r.t. upstreaming changes has also been
> addressed.

While I consider that policies should have "best choices" coinciding 
with "preferred" and that not doing so would be illogical, people, 
projects, decisions ... are far from always logical.

Because of this, people cannot assume that the 'best choices' are 
'preferred', so it needs to be mentioned explicitly that these 'best 
choices' are actually 'preferred'.

>>> An enumeration ought to have at least three elements (otherwise
>>> it's just a pair), and I think if we do proper counting and omit
>>> no-brainers, such as the "only free software" part that has already
>>> been mentioned, we come very close to skirting that line.
>> The "only free software" is mentioned elsewhere, yes, but it is one
>> of the principles for deciding between snippets, phases and patches.
>> While you call it a no-brainer, it is sometimes neglected, so it
>> sounds important to me to explicitly list it.
>> Merging the 3th and 4th @item, I count 4 principles, so it fits with
>> an enumeration.
>> Also, I'm not following your point here -- your complaint was that
>> they aren't guiding principles (based on the number of them), but
>> your response is that they might not form an enumeration?  They are
>> named the guiding principles, not the guiding enumeration.  What have
>> enumerations to do with anything?
> I'm using enumeration as a super term here, which can be
> ((sub)sub)sections, chapters, list elements, whatever, and my claim is
> that we barely have enough principles to allow the use of a plural.

In English, things are either plural or singular.  Everything >= 2 is 
plural.  There number of principles, however we count them, is, at 
least, 2.  Consequently, the principles are plural, not singular. 
Treating the principles as singular is simply grammatically incorrect.

Maybe it is barely allowed to be plural, but English grammar doesn't 
care about that -- it is definitively disallowed to be singular, only 
plural remains.

> Adding to that, now that I think of it, I also doubt their usefulness
> in guiding.  "Use whatever feels convenient, but note that that might
> be subjective" is more useful at the end of the section when a user
> didn't find what they were looking for than at the start.

The introduction has a set of guiding principles, from with concrete 
cases are built.  By adding another principle at the end, the cases
cannot make use of the "use [...] convenient" principle.

Additionally, now the user has to look at _two_ places to find the 
guiding principles -- at the beginning, and at the end.  Worse,
the beginning does not have a cross-reference to the end, so since
the set at the beginning is presented as exhaustive, the user might
not know there is another guiding principle.

And even if they did read through the whole section (even though they 
should only have to read the introduction and the relevant worked-out 
case), assuming they read through it in a linear fashion, due to the new 
guiding principle that wasn't alluded to at the beginning, they have
to redo their mental model of "Modifying Sources" as this principle
could invalidate things.

>>>> I do not see what the problem is with additional length as long
>>>> as this additional length comes with additional useful
>>>> information and the manual is well-structured (e.g. with
>>>> (sub)(sub)sections, chapters and indices) -- we do not have a
>>>> page limit.
>>>>
>>>> At worst, perhaps the same information could perhaps be encoded
>>>> with fewer words? I could compare the two patches to see which
>>>> one formulates certain information in the fewest words, and
>>>> choose the least verbose of the two for each piece of information
>>>> that is present in both?
>>>>
>>>> Also, comparing the two patches, my patch has 40 more lines, but
>>>> about 25 of them are for noting the guiding principles (which are
>>>> absent in your patch).
>>>> Compensating for that, the patches are about the same length, so
>>>> I do not think that 'sheer length' is accurate here.
>>> 25 lines calling back to earlier information are, imho, an
>>> indicator that the section is too long. Imagine you'd have twenty-
>>> five function calls to guiding_principles(n) in your program – at
>>> some point, you'd try to cache those.
>> (define cached-guiding-principles
>>    (delay (list (guiding-principle-0)
>>                      [...]
>>                      (guiding-principle-24))))
>> Caching the guiding principles does not reduce the length.
>> I don't see the problem with calling back to earlier information.
>> Also, it isn't earlier information, there is no nice list of guiding
>> principles anywhere else.
> At the risk of responding jokingly to what was meant serious: I didn't
> know we suddenly gained 20 guiding principles.

25 lines are for the guiding principles (sometimes referring to a 
principle of elsewhere in Guix, sometimes a new principle for "Modifying 
Sources").  You proposed to 'cache' them somehow.  In Guile, to cache 
something, you can use 'delay/force'.  But this increases the amount of 
code (due to the additional use of 'delay'), instead of decreasing.

The documentation equivalent (whatever that might be, I am not seeing 
one myself) would then also be at least as long, maybe even a little 
longer due to the use of 'delay'.

>>>>> Going down to subsubsections just to find out where patches are
>>>>> appropriate, is imho overkill.
>>>> The 'going down to subsubsection' is the case for your patch too,
>>>> though? In my case, it's a subsubsection, in your case it's a
>>>> table
>>>> entry inside a subsection, both are the same level of nesting.
>>> These are still two very different kinds of nesting. A table fits
>>> onto a (virtual) page more easily than several subsections.
>> I suppose table items might take two less line or so less than
>> subsubsections, but I don't think that's sufficient reason to step
>> away from a nice section structure.
> Another reason is that you can end a table in the middle of a section,
> which you can't do with subsections.  Hence why I'm able to put a
> remark at the bottom, which you have to clumsily fit into the top.

I can, indeed, not put the 'convenience principle' at the bottom, except 
perhaps by adding a new subsubsection, and for tables adding such a 
remark at the bottom is indeed more convenient.

However, I don't see the 'have to clumsily' here -- as explained 
elsewhere, I believe that the 'convenience principle' shouldn't be 
separated from the other principles and that it fits nicely next to the 
the principles --- there is no 'have to', because I choose for this, and 
I am not seeing any 'clumsiness' here.

>>>> Also, it's a matter of different structure -- in my v2 and v3
>>>> patch, I have a 'problem -> solution' structure -- the idea is
>>>> that the packages has a problem, they look at the section, they
>>>> read the subsubsection names, select the
>>>> subsubsection that matches their problem and read the solution --
>>>> in short, the idea is to provide a solution to the problem.
>>>>
>>>> Your structure is the other way around -- for solutions (patches,
>>>> snippets, phases), it gives the permitted problems to apply it
>>>> to.
>>>>
>>>> So yes, your patch is more convenient for finding out where
>>>> patches are appropriate.  I do not see the benefit of that though
>>>> -- a new contributor packaging a thing wouldn't know in advance
>>>> which solutions could be appropriate for them (your 'solution ->
>>>> problem' patch?), rather, they start with a problem and are
>>>> searching for an appropriate solution (my problem->solution
>>>> patch).
>>> I think this idea can be debunked pretty easily. If I give you a
>>> hammer and I tell you "this is a hammer, you can use it to put
>>> nails into a wall", and you have a nail and you want to put it into
>>> a wall, you won't go "oh no, however will I put this nail into a
>>> wall?" – you will simply use the hammer to do so.
>> The patch does this, currently.  It already proposes a number of
>> hammers (patches, snippets and phases) and purposes (adding new
>> functionality, fixing technical issues, unbundling, ...).
>> Also, the scenario "oh no, however will I put this nail into a wall"
>> actually happened -- see the Shepherd discussion, where there was
>> a lot of disagreement on how nails (= small work-around in the
>> Makefile) should be put in walls (= patches, snippet, phase?).   It
>> was the whole reason to start writing a documentation patch.
> You might want to add a link here if it supports your argument, but
> without looking at the discussion this rather sounds like "oh no, I
> have three hammers, which one do I pick?" – which, fair enough, is
> still a problem, but one that neither of our patches would cause imho.

As I think I've written previously, the whole point was to solve that 
problem. For the discussion, see:

* https://issues.guix.gnu.org/54216
* https://yhetil.org/guix/c4c0a071-2e03-d4d6-6718-05424d21d146@telenet.be/
* 
https://yhetil.org/guix-devel/84e13ef7d437062df5cca51a12e6da54929e0176.camel@telenet.be/

Not solving the problem defeats the whole point, as the purpose is to 
solve that problem.

>>> Of course, for this to work I also have to tell you *how* to use a
>>> hammer to put nails into a wall, but that's exactly what I did in
>>> my patch by inserting the right notes into the Guix manual.
>> Also already the case.
>>> My solution->problem approach has the benefit, that folks can just
>>> go over all the solutions, check if their problem fits, and apply
>>> the one that says "here, use this".
>> A problem->solution structure is useful for that too?
>> And it already lists all the solutions (snippets, phases and patches)
>> and information to decide whether the solution fits their problem
>> (the guiding principles, and the worked-out cases).
> Again, I believe you're overselling the guiding principles.

I never claimed they were super great, just that they are convenient and 
solved a number of problems.  I'm not seeing any overselling here.

>>> And if they don't find anything, they see the handy little line at
>>> the bottom saying "use whatever you think is convenient".
>> Nowhere did the patch imply that the listed cases were all cases. In
>> fact, in two places in the introduction it is implied that the
>> examples are not exhaustive, and that they can choose according to
>> convenience  [...]
> Emphasis on handy little line rather than needing to be told twice
> (particularly if people have no idea what to expect due to not having
> looked at the worked-out cases yet).

They don't need to be told twice.  Also, my patch also has that handy 
little line (albeit differently worded), see the 'guiding principles'.

Also, on the 'no idea what to expect' -- this is exactly the same for 
your patch too?  In both patches, if the user only reads the 
introduction and conclusion (if any) and doesn't read the actual 
(relevant examples)/(explanation of patches, snippets, phases), then 
that's their problem.

>>>   I also expand a little on the benefits and drawbacks of
>>> these approaches as you would when describing design patterns.
>> This is also done in my patch. [...]
> This is not about the contained information, but the structure of the
> contained information.
> 
> My solution->problem style follows roughly this style:
> 1. solution
> 2. problems it is known to solve
> 3. how to use
> 4. properties, caveats, etc.
> 
> Your problem->solution style roughly has the following:
> 1. problem
> 2. (set of) solution(s)
> 3. if more than one solution, maybe a help to select

Also, in no particular order

   4.: how to use
   5.: explanation why it is preferred (properties, caveats?)

> 
> This makes it so that people might have to go to a different subsection
> than the one they read for their solution to find out about potential
> caveats, e.g. not embedding store paths in a snippet.

I assume their problem was "X doesn't find Y".  This being a technical 
issue, they go to "Fixing technical issues".  In that subsubsection, 
there are the words:

> Secondly, if the fix of a technical issue embeds a store file name, then
> it has to be a phase.  Otherwise, if the store file name were embedded in
> the source, the result of @command{guix build --source} would be unusable
> on non-Guix systems and also likely unusable on Guix systems of another
> architecture.

so they actually do know of the caveat 'don't embed store paths in a 
snippet, do it in a phase instead', and they did not need to go to a 
different subsubsection.

>>> Your problem->solution approach instead leaves people wondering
>>> when their particular use case has not been described.
>> See my reply to ‘And if they don't find anything, they see the handy
>> little line at the bottom saying "use whatever you think is
>> convenient’.
>>> It gives them a solution rather than the tools to build solutions
>>> with.
>> It does give the tools: snippets, patches and phases.
> As far as I read, it describes none of those.  It puts out guiding
> principles and some already worked-out cases.
Here are the descriptions:

> Guix has tree main ways of modifying the source code of a package, that
> you as a packager may use.  These are patches, snippets and phases

Once the user knows _which_ of the three they should use (by consulting
the following subsubsections), they can then search for 'patch', 
'snippet' and 'phases' in the manual for more information, no need to 
duplicate that information.

>> Also, "giving the tools to build solutions with" does not help with
>> the problem that I aimed to solve -- there was disagreement on what
>> the appropriate tools are (see: Shepherd), so it not just needs to
>> give the tools, but also the solutions.
> I don't see how my patch lacks this information however.

IIUC, the raw information should usually be indeed all there, but the 
user has to consult _all_ of the sections to determine which option 
(patch, snippet, phase) is appropriate, having to assemble all the 
information is (a) a waste of time and (b) can lead to different 
interpretations and conclusions (see: Shepherd).

More concretely, I cannot use your patch to decide between phases, 
snippets and patches for the Shepherd issue:

* none of three appear to be forbidden by your documentation
* there is no recommendation for 'patches' (IIRC it wasn't accepted 
upstream and there was no intent to submit it upstream, it being really 
a Guile bug, not a Shepherd bug)
* there is no recommendation for snippets (it's all free, no bundling)
* build phases are 'to be avoided' (but not forbidden), as it resulted
   in observably different runtime behaviour (being a bug fix)

-- three (or at best, two, if build phases are discounted) options 
remain.  Myself, I would then consider 'snippets' to be the most 
convenient, but some others (see: Shepherd, IIRC) would really want a 
patch instead, because 'patches can be reverted' or something like that.

As such, you are giving the tools (snippets / patches / phases, some 
downsides and upsides), but different people can construct different 
solutions from those tools even in the same situation, leading to conflicts.

If I use my patch instead, I go to "fixing technical issues". This 
section tells me to use a patch or a snippet.  As the fix is not 
Guix-specific, it recommends a patch to save time when upstreaming. 
Conclusion: write a patch.  It was a Guile bug, so the fix was a patch 
to Guile.  But that would take time and upstream took the responsibility 
for a fix, so the 'efficient time thing' doesn't really apply and a 
small work-around (setting optimisation flags) suffices.

In this situation, the subsubsection doesn't care at all if you go for a 
snippet, so apply the last guiding principle: go for the simplest thing:
a snippet. (Unless you already have a patch, then a patch is simplest.)
Does someone else have a different idea on what's simplest?  Doesn't
really matter, ‘Sometimes this is subjective, which is also fine’.

> In
> particular, for review purposes, mine should be easier to work with.
> For instance, the reviewer sees a hash embedded in a snippet, sees in
> the snippet entry that you shouldn't do that, and can thus say "nope,
> don't do a snippet here".

I think we should optimise for packagers before reviewers
> 
> Regardless of which patch we pick, it should not mandate a particular
> solution in potentially contentious cases,

Actually the whole point was to mandate a particular solution for the 
contentious cases, see Shepherd.

> and also point towards the
> right solution.  See our discussions on the individual solutions on
> points in which I believe you've errored.

These are:

* the typo's
* including "skipping tests indicating failure under ‘Fixing technical 
issues’"
* "don't mention file names of non-free things in patches"

Did I miss any?

Greetings,
Maxime

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 929 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

[bug#57598] [PATCH v2] doc: Update contribution guidelines on patches, etc.

[Maxime Devos]

* doc/contributing.texi (Modifying Sources): Replaced with ...
("Modifying Sources"): ... this.  List more use cases and some principles.

This patch incorporates some text written by Liliana Marie Prikler.  The
structure is based on a proposal by Julien Lepiller.  The text has been
revised based on reviews by David Larsson and blake.

(! remove following text before committing !)

Unless someone finds some more typo's or such, this is the final version for
me, as it has become a time sink with diminishing returns.

Changes since previous version:

- typofix: tree -> three
- third and fourth principle are merged
- corrected @subsection -> @subsubsection

Not changed:
- no additional information on bugfixes even though initially proposed, I'm
  done with the patch, if you'd like such information please write a follow-up
  patch.
- I didn't add 'no naming the file names of non-free things in the snippets’,
  as I don't think this is currently policy and it seems rather inconvenient
  to work with (e.g.: then we wouldn't be able to keep records of what parts
  still need to be replaced with something free in the comments)
- going from ‘problem -> solution’ to ‘solution -> problem’ -- would partially
  defeats the point of the patch.
- going through the two different patches and take the most concise phrasing
  (other people in 57598@debbugs.gnu.org didn't agree to my proposal, also,
  I'm about done with the patch
- ‘subsubsections -> item / table’: I haven't read a convincing explanation
  on why item / table is better here.
---
 doc/contributing.texi | 140 +++++++++++++++++++++++++++++++++++++-----
 doc/guix.texi         |   2 +-
 2 files changed, 126 insertions(+), 16 deletions(-)

diff --git a/doc/contributing.texi b/doc/contributing.texi
index 02c7c5ae59..bd2ae8d9b6 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::           Whether to use a snippet, or a build phase.
 * Emacs Packages::              Your Elisp fix.
 * Python Modules::              A touch of British comedy.
 * Perl Modules::                Little pearls.
@@ -698,20 +698,130 @@ Gettext}):
 for the X11 resize-and-rotate (RandR) extension. @dots{}")
 @end lisp
 
-@node Snippets versus Phases
-@subsection Snippets versus Phases
-
-@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}).
+@node Modifying Sources
+@subsection Modifying Sources
+
+Guix has tree main ways of modifying the source code of a package, that
+you as a packager may use.  These are patches, snippets and phases.
+Each one has its strengths and drawbacks.  To decide between the three,
+there are a few guiding principles to satisfy simultanuously where
+possible:
+
+@itemize
+@item
+In principle, Guix only has free software; when the upstream source
+contains some non-free software, it has to be removed such that
+@command{guix build --source} returns the ‘freed’ source code rather than
+the unmodified upstream source (@pxref{Software Freedom}).
+@item
+The source of the package needs to correspond to what is actually built
+(i.e., act as the corresponding source), to fulfill our ethical and
+legal obligations.
+@item
+The source needs to actually work, not only on your Guix system but also
+on other Guix systems.  It preferably should also work on non-Guix
+systems if supported by upstream.  This requires some care for
+substitutions involving store items and other architecture-specific
+changes.
+@item
+When there is more than one way to do something, choose whichever method
+is the simplest.  Sometimes this is subjective, which is also fine.
+What matters is that you use techniques that are common within the
+community (i.e., patterns that appear throughout @code{gnu/packages/...})
+and are thus clearly legible for reviewers.
+@end itemize
+
+To make things more concrete and to resolve conflicts between the
+principles, a few cases have been worked out:
+
+@subsubsection Removing non-free software
+Non-free software has to be removed in snippets; the reason is that
+patches or phases will not work.
+
+For patches, the problem is that a patch removing a non-free file
+automatically contains the non-free file@footnote{It has been noted that
+git patches support removing files without including the file in the
+patch in
+@url{https://yhetil.org/guix/8b13e899-eb60-490b-a268-639249698c81@@www.fastmail.com/}. If
+it is verified that the @command{patch} utility supports such patches,
+this method can be used and this policy adjusted appropriately.}, and we
+do not want anything non-free in Guix even if only in its patches.
+
+For phases, the problem is that phases do not influence the result of
+@command{guix build --source}.
+
+@subsubsection Removing bundled libraries
+Bundled libraries should not be removed with patches, because then the
+patch would contain the full bundled library, which can be large. They
+can be removed either in snippets or phases, often using the procedure
+@code{delete-file-recursively}. There are a few benefits for snippets here:
+
+When using snippets, the bundled library does not occur in the source
+returned by @code{guix build --source}, so users and reviewers do not
+have to worry about whether the bundled library contains malware,
+whether it is non-free, if it contains pre-compiled binaries ... There
+are also less licensing concerns: if the bundled libraries are removed,
+it becomes less likely that the licensing conditions apply to people
+sharing the source returned by @command{guix build --source}, especially if
+the bundled library is not actually used on Guix systems.@footnote{This
+is @emph{not} a claim that you can simply ignore the licenses of
+libraries when they are unbundled and replaced by Guix packages -- there
+are less concerns, not none.}
+
+As such, snippets are recommended here.
+
+@subsubsection Fixing technical issues (compilation errors, test failures, other bugs ...)
+Usually, a bug fix comes in the form of a patch copied from upstream or
+another distribution.  In that case, simply adding the patch to the
+@code{patches} field is the most convenient and usually does not cause
+any problems; there is no need to rewrite it as a snippet or a phase.
+
+If no ready-made patch already exists, then choosing between a patch or
+a snippet is a matter of convenience. However, there are two things to
+keep in mind:
+
+First, when the fix is not Guix-specific, upstreaming the fix is
+strongly desired to avoid the additional maintenance cost to Guix.  As
+upstreams cannot accept snippets, writing a patch can be a more
+efficient use of time.  Secondly, if the fix of a technical issue embeds
+a store file name, then it has to be a phase.  Otherwise, if the store
+file name were embedded in the source, the result of @command{guix build
+--source} would be unusable on non-Guix systems and also likely unusable
+on Guix systems of another architecture.
+
+@subsubsection Adding new functionality
+To add new functionality, a patch is almost always the most convenient
+choice of the three -- patches are usually multi-line changes, which are
+convenient to do with patches and inconvenient to do with phases or
+snippets.  This choice is usually also compatible with all the guiding
+principles.  As such, patches are preferred.  However, as with bug
+fixes, upstreaming the new functionality is desired.
+
+@subsubsection How to add patches
+Refer to the @code{origin} record documentation (particularly the fields
+@code{patches}, @code{patch-inputs}, and @code{patch-flags}) for
+information on how to use patches (@pxref{origin Reference}).  When
+adding a patch, do not forget to also list it in @code{dist_patch_DATA}
+of @file{gnu/local.mk}.
+
+@subsubsection How to add files
+New source files can be added in phases or snippets, by using
+@b{auxiliarry files}.  Auxiliary files are stored in the
+@file{gnu/packages/aux-files} directory and can be retrieved (in a
+snippet or a phase) with @code{search-auxiliary-file}.  When adding an
+auxiliary file, do not forget to also list it in @code{AUX_FILES} of
+@file{Makefile.am}.
+
+Another option for adding new files, is to use procedures such as
+@code{display}, @code{format} and @code{call-with-output-file}.  As a
+matter of principle, auxiliary files ought to be preferred over an
+equivalent @code{call-with-output-file} when creating non-trivil files,
+as that makes them easier to edit.  The exact threshold for a
+non-trivial file might be subjective, though it should lie somewhere
+between 10--20 lines.
+
+Currently, there is no policy on deciding between phase and snippets for
+adding new files, except for the guiding principles.
 
 @node Emacs Packages
 @subsection Emacs Packages
diff --git a/doc/guix.texi b/doc/guix.texi
index 7bce8a567c..042ab3bd8a 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -70,7 +70,7 @@ Copyright @copyright{} 2019 Jakob L. Kreuze@*
 Copyright @copyright{} 2019 Kyle Andrews@*
 Copyright @copyright{} 2019 Alex Griffin@*
 Copyright @copyright{} 2019, 2020, 2021, 2022 Guillaume Le Vaillant@*
-Copyright @copyright{} 2020 Liliana Marie Prikler@*
+Copyright @copyright{} 2020, 2022 Liliana Marie Prikler@*
 Copyright @copyright{} 2019, 2020, 2021, 2022 Simon Tournier@*
 Copyright @copyright{} 2020 Wiktor Żelazny@*
 Copyright @copyright{} 2020 Damien Cassou@*

base-commit: a44e08337d15b3f254a35d0311663c2bbd501852
prerequisite-patch-id: 0caac311875ee39cb48573657ebb960e90da6dfb
prerequisite-patch-id: 418285493d89ebf102175902d9b09a0174e88190
prerequisite-patch-id: 3c39eb839d9d3ff3fca6cd98621a5d5c411b7af4
prerequisite-patch-id: 8d5662e874c469f5ee496ef5181cf2d0a30ad1d8
prerequisite-patch-id: 26513c3b3b86963df718ee41d14a25d1cc6a8f3f
prerequisite-patch-id: 2b2497e2edec0afc48ebadd6f09f0c661c466127
prerequisite-patch-id: 2712efb97bf33985fd0658e4dd8e936dc08be5fe
prerequisite-patch-id: 9d2409b480a8bff0fef029b4b095922d4957e06f
-- 
2.37.2

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

[Liliana Marie Prikler]

Am Freitag, dem 09.09.2022 um 13:14 +0200 schrieb Maxime Devos:
> 
> > In descriptions, it is wise to do so because it helps software
> > stand on its own merits rather than just "being a clone of this
> > thing you aren't allowed to have" (this is real criticism pointed
> > at us from the proprietary software embracers).  See for instance
> > minetest, which is described in terms of its features rather than
> > "being a Minecraft clone".
> 
> Sentence might have been truncated? 
Corrected above.

> Also, this is the package source field, not the description, I don't
> see how the "helps software stand on its own merits" applies to
> snippets of the package source.
Point taken, but imho it still makes sense to prefer keep lists over
remove lists.  The GNU project encourages people to read the sources
after all.


> > > How does ignoring a test fix the technical issue identified by
> > > the test (sometimes, the technical issue being a bug in the test
> > > itself)?
> > It fixes the technical issue that an otherwise functional package
> > (w.r.t. the N tests that don't fail) builds.  This is a
> > particularly
> > useful distinction with tests that require a network connection and
> > thus have to fail in a build container, or are known flaky upstream
> > and thus cause reproducibility issues.
> 
> Network test: right (though preferably those would support a 
> --no-network-tests test option or such).
> 
> For flaky tests: those sound like bugs to me, ignoring them doesn't 
> remove the flakyness.
Call it a bug or whatever, it is a technical issue that we deal with at
build time and not in the origin.

> > > 
> > > > There still is the difference that phases are clearly delimited
> > > > while snippets are a block of code that shouldn't get too
> > > > large.
> > > Snippets are delimited clearly as well, though, with the
> > > 'snippet' field?
> > > And the limitations of snippet length and phases length are the
> > > same -- no limits, though conciseness is appreciate as always.
> > True, but phases can be made to do just one thing, whereas snippets
> > have to fix everything that's wrong in more or less one (begin
> > ...).
> > This is a noticable distinction.> 
> 
> You can do that in snippets too, with comments:
> 
> (snippet
>    #~(begin
>        ;; Do the foo thing
>        (foo)
>        (foo-2 [...])
>        [...]
>        ;; Do the bar thing
>        (bar)
>        (bar-2 [...])
>        [...]))
You can, but it's still wiser to just keep the snippet short enough so
you don't have to.

> > 
> > > > I think my version at least hinted at this practice in a more
> > > > concise way, so it's not impossible to mention. [...]
> > > I agree it's possible -- as I replied previously:
> > > > I suppose a section could be added somewhere to properly
> > > > document
> > > > the 'embedding store file names' practice, and insert a
> > > > cross-reference,
> > > I don't think documenting the how of the practice should be done
> > > in this section, properly explaining 'search-input-file' /
> > > 'search-
> > > input-directory', 'inputs / native-inputs', 'bash' being an
> > > implicit
> > > input but you still have to add it to 'inputs' in some cases
> > > because
> > > of cross-compilation, this-package-input and this-package-native-
> > > input ... would make the
> > > subsubsection a bit too long I think, distracting from other
> > > situations, hence the proposal for a cross-reference.
> > > How about leaving the 'how to embed store file names' for a
> > > separate
> > >   documentation patch and section, adding a cross-reference
> > > later?
> > See above, "I suppose a section could be added..." means I'm
> > somewhat indifferent to whether it's done now or later;
> 
> Nitpick: you are quoting some text I wrote, so 'I' refers to me here,
> not you.
Ahh, my bad, the nesting level confused me.

>   I would however very
> > much prefer a wording that points people toward this practice
> > existing,
> > even if they have to go look in the code for examples. 
> > Alternatively,
> > a footnote for the most common case (search-input-file inputs
> > "bin/command") ought to suffice for the time being.
> 
> Aside from the typo's and a few rephrasings, I'm done with the 
> documentation.  If someone wants to extend the section with such 
> information, they can always do so later.
I haven't seen a v2 though.  Am I correct in assuming that none of the
points we discussed in the last few mails are going to be addressed?

> > 
> > IMHO, I think a reader who remembers the guiding principles should
> > see that it applies.
> 
> Likely. But removing the explicit mention of the guiding principle
> still makes the logical reasoning incomplete, remembering has nothing
> to do with it.  As I've written previously: ‘This has nothing do do
> with [...] and remembering, but rather with [...]’.
In that case, let me rephrase my criticism: "It passes the guidelines"
should not be part of the logical reasoning here.  Otherwise, why not
have a guideline checklist at the end of each subsection (which would
be silly, obviously, but that's the point).

> > > 
> > Also, your guiding principles are (with one exception) really just
> > invariants that ought to hold for the source field of a package.
> 
> I don't know about the exceptions (I haven't counted them), but yes, 
> indeed.  I do not see the problem of this.
For the record, the exception is the "most convenient" bit you've
copied from my patch :)

> > As such, I think it would be easier to state "A package's source
> > should
> > be the smallest corresponding source in terms of uncompressed file
> > size.  This corresponding source must consist only of free software
> > (note Free Software) and should build on all platforms supported by
> > upstream."  Note how smallest naturally implies unbundling bundled
> > sources.
> 
> This criterium on overly small sources.  Often, a package's source 
> contains things that is not used for the build or outputs and hence
> is not part of the corresponding source.  Examples:
Note "should" rather than "shall" or "must", meaning that slightly
larger tarballs are still acceptable.

> * the source contains documentation that could be built and
> installed,
>    but Guix doesn't do so yet.  --> should be kept (unless non-free)
> * documentation that isn't meant to be built or installed
>    (e.g. HACKING, PACKAGERS, ...) --> useful, shouldn't be removed.
> * .gitignore, .github, ... --> nothing wrong with removing those,
>    but pointless, let's not waste our time with looking for those
>    and removing them, even though doing so would make it smaller.
> * source files for platforms the upstream does not support
> yet/anymore
>    (but with some volunteer effort (e.g. bugfixes), it might become
>    a supported system again)
>    --> removing them (e.g. as part of an overly-broad 
> (delete-file-recursively 
> "this-dir-has-bundling-albeit-not-all-of-it-is-bundling"))
>        can be OK-ish, but removing them for 'minimality' is
> pointless.
I see you're nitpicking for the sake of the argument, but none of the
examples you provide (safe for the bundling one) add much cost to
either packing or unpacking.  Thus, I'm pretty sure we can ignore them
for practical purposes.  That being said, if you have a better
formulation, please do tell.

> 
> > > > I still find this wording very confusing. Perhaps "To add new
> > > > functionality, a patch is almost always the best choice. For
> > > > one,
> > > > it is likely that the new functionality requires changing
> > > > multiple
> > > > lines of source code, which is more convenient to do with a
> > > > patch
> > > > than with a snippet. Further, patches can be taken from and
> > > > submitted to upstreams more easily. If your patch has not been
> > > > submitted to upstream, consider doing so."
> > > It loses some information (that patches are preferred) and
> > > (after re-adding the conclusion) is more verbose, which appears
> > > to be considered very important.
> > Which conclusion is there to re-add?
> 
> "patches are preferred"
> 
>    The conclusion is already stated
> > in the beginning: patches are almost always the best choice.  Then
> > two
> > reasons as for why.  The part w.r.t. upstreaming changes has also
> > been
> > addressed.
> 
> While I consider that policies should have "best choices" coinciding 
> with "preferred" and that not doing so would be illogical, people, 
> projects, decisions ... are far from always logical.
> 
> Because of this, people cannot assume that the 'best choices' are 
> 'preferred', so it needs to be mentioned explicitly that these 'best 
> choices' are actually 'preferred'.
In that case, simply write preferred choice?

> > I'm using enumeration as a super term here, which can be
> > ((sub)sub)sections, chapters, list elements, whatever, and my claim
> > is that we barely have enough principles to allow the use of a
> > plural.
> 
> In English, things are either plural or singular.  Everything >= 2 is
> plural.  There number of principles, however we count them, is, at 
> least, 2.  Consequently, the principles are plural, not singular. 
> Treating the principles as singular is simply grammatically
> incorrect.
Correction: Everything that isn't exactly 1 is plural in English,
including 0, with perhaps the exception of -1 also using singular.

> Maybe it is barely allowed to be plural, but English grammar doesn't 
> care about that -- it is definitively disallowed to be singular, only
> plural remains.
Note that my argument isn't about English grammar, it uses English
grammar.

> > Adding to that, now that I think of it, I also doubt their
> > usefulness in guiding.  "Use whatever feels convenient, but note
> > that that might be subjective" is more useful at the end of the
> > section when a user didn't find what they were looking for than at
> > the start.
> 
> The introduction has a set of guiding principles, from with concrete 
> cases are built.  By adding another principle at the end, the cases
> cannot make use of the "use [...] convenient" principle.
> 
> Additionally, now the user has to look at _two_ places to find the 
> guiding principles -- at the beginning, and at the end.  Worse,
> the beginning does not have a cross-reference to the end, so since
> the set at the beginning is presented as exhaustive, the user might
> not know there is another guiding principle.
> 
> And even if they did read through the whole section (even though they
> should only have to read the introduction and the relevant worked-out
> case), assuming they read through it in a linear fashion, due to the
> new guiding principle that wasn't alluded to at the beginning, they
> have to redo their mental model of "Modifying Sources" as this
> principle could invalidate things.
This shows both a problem with your structure, but more importantly, a
failure to understand what I meant when I wrote "use whatever is
convenient" in my own patch.  This principle *can* only work for cases
in which there is *no clearly established practice*, thus putting it at
the end *after having enumerated all practices* is the logical choice.
With your structure, you have to bend it sideways to shoehorn it in
with the other principles.

> > At the risk of responding jokingly to what was meant serious: I
> > didn't know we suddenly gained 20 guiding principles.
> 
> 25 lines are for the guiding principles (sometimes referring to a 
> principle of elsewhere in Guix, sometimes a new principle for
> "Modifying Sources").  You proposed to 'cache' them somehow.  In
> Guile, to cache something, you can use 'delay/force'.  But this
> increases the amount of code (due to the additional use of 'delay'),
> instead of decreasing.
> 
> The documentation equivalent (whatever that might be, I am not seeing
> one myself) would then also be at least as long, maybe even a little 
> longer due to the use of 'delay'.
Okay, so I did misunderstand those 25 lines as 25 lines within the text
calling back to those guiding principles rather than 25 lines for the
guiding principles themselves.  Still, 25 lines are a little much,
especially given that the bulk of it explains the semantics of the
packages' source field.
> > 

> > > I suppose table items might take two less line or so less than
> > > subsubsections, but I don't think that's sufficient reason to
> > > step away from a nice section structure.
> > Another reason is that you can end a table in the middle of a
> > section, which you can't do with subsections.  Hence why I'm able
> > to put a remark at the bottom, which you have to clumsily fit into
> > the top.
> 
> I can, indeed, not put the 'convenience principle' at the bottom,
> except perhaps by adding a new subsubsection, and for tables adding
> such a remark at the bottom is indeed more convenient.
> 
> However, I don't see the 'have to clumsily' here -- as explained 
> elsewhere, I believe that the 'convenience principle' shouldn't be 
> separated from the other principles and that it fits nicely next to
> the the principles --- there is no 'have to', because I choose for
> this, and I am not seeing any 'clumsiness' here.
I think we'd have to debate choice vs. force here which would be a
rather long aside, but to make my argument short, your choice of how to
structure this section (table vs subsections) directly influences your
"choice" where to place particular information in a way that might 
inhibit natural information flow.
> > 
> > 
> > 

> > > The patch does this, currently.  It already proposes a number of
> > > hammers (patches, snippets and phases) and purposes (adding new
> > > functionality, fixing technical issues, unbundling, ...).
> > > Also, the scenario "oh no, however will I put this nail into a
> > > wall"
> > > actually happened -- see the Shepherd discussion, where there was
> > > a lot of disagreement on how nails (= small work-around in the
> > > Makefile) should be put in walls (= patches, snippet, phase?).  
> > > It
> > > was the whole reason to start writing a documentation patch.
> > You might want to add a link here if it supports your argument, but
> > without looking at the discussion this rather sounds like "oh no, I
> > have three hammers, which one do I pick?" – which, fair enough, is
> > still a problem, but one that neither of our patches would cause
> > imho.
> 
> As I think I've written previously, the whole point was to solve that
> problem. For the discussion, see:
> 
> * https://issues.guix.gnu.org/54216
> *
> https://yhetil.org/guix/c4c0a071-2e03-d4d6-6718-05424d21d146@telenet.be/
> * 
> https://yhetil.org/guix-devel/84e13ef7d437062df5cca51a12e6da54929e0176.camel@telenet.be/
> 
> Not solving the problem defeats the whole point, as the purpose is to
> solve that problem.
I think we might be disagreeing about what constitutes "solving the
problem" here.  Correct me if I'm wrong, but to me it seems any
scenario that is not covered by whatever patch is applied counts as
"not having solved the problem".  I personally find this line of
reasoning questionable, as there are perfectly valid reasons why
multiple tools could apply.

Take the problem of embedding a store path.  You could for instance
replace all occurences of "sh" with /gnu/store/.../bin/sh, or you could
first replace them in a patch with "@sh@" and then replace that with
/gnu/store/.../bin/sh.  Of course, you rarely have to do the latter and
the former is simpler, hence why you will often see the former, but
that doesn't mean the latter is invalid.
> 

> 
> > > 
> > > > And if they don't find anything, they see the handy little line
> > > > at the bottom saying "use whatever you think is convenient".
> > > Nowhere did the patch imply that the listed cases were all cases.
> > > In fact, in two places in the introduction it is implied that the
> > > examples are not exhaustive, and that they can choose according
> > > to convenience  [...]
> > Emphasis on handy little line rather than needing to be told twice
> > (particularly if people have no idea what to expect due to not
> > having looked at the worked-out cases yet).
> 
> They don't need to be told twice.  Also, my patch also has that handy
> little line (albeit differently worded), see the 'guiding
> principles'.
> 
> Also, on the 'no idea what to expect' -- this is exactly the same for
> your patch too?  In both patches, if the user only reads the 
> introduction and conclusion (if any) and doesn't read the actual 
> (relevant examples)/(explanation of patches, snippets, phases), then 
> that's their problem.
I do think a table in the middle of the section is hard to ignore, but
suppose for the sake of argument that they do, then in my case they
will have learned exactly nothing.  In your case, they will know the
"guiding principles" and then interpret them to mean whatever they
think it means.  I'm not convinced mine is worse here.

> > > 
> > > > I also expand a little on the benefits and drawbacks of
> > > > these approaches as you would when describing design patterns.
> > > This is also done in my patch. [...]
> > This is not about the contained information, but the structure of
> > the contained information.
> > 
> > My solution->problem style follows roughly this style:
> > 1. solution
> > 2. problems it is known to solve
> > 3. how to use
> > 4. properties, caveats, etc.
> > 
> > Your problem->solution style roughly has the following:
> > 1. problem
> > 2. (set of) solution(s)
> > 3. if more than one solution, maybe a help to select
> 
> Also, in no particular order
> 
>    4.: how to use
>    5.: explanation why it is preferred (properties, caveats?)
No particular order is problematic, but more importantly if a
technology appears more than once (guaranteed by the pigeonhole
principle), then either its properties get repeated (not good for
length) or scattered (not good for the flow you want).  Again, a
structural problem.

> > This makes it so that people might have to go to a different
> > subsection than the one they read for their solution to find out
> > about potential caveats, e.g. not embedding store paths in a
> > snippet.
> 
> I assume their problem was "X doesn't find Y".  This being a
> technical issue, they go to "Fixing technical issues".  In that
> subsubsection,  there are the words:
> 
> > Secondly, if the fix of a technical issue embeds a store file name,
> > then it has to be a phase.  Otherwise, if the store file name were
> > embedded in the source, the result of @command{guix build --source}
> > would be unusable on non-Guix systems and also likely unusable on
> > Guix systems of another architecture.
> 
> so they actually do know of the caveat 'don't embed store paths in a 
> snippet, do it in a phase instead', and they did not need to go to a 
> different subsubsection.
Typical happy path.

> > > 
> > > > Your problem->solution approach instead leaves people wondering
> > > > when their particular use case has not been described.
> > > See my reply to ‘And if they don't find anything, they see the
> > > handy little line at the bottom saying "use whatever you think is
> > > convenient’.
> > > > It gives them a solution rather than the tools to build
> > > > solutions with.
> > > It does give the tools: snippets, patches and phases.
> > As far as I read, it describes none of those.  It puts out guiding
> > principles and some already worked-out cases.
> Here are the descriptions:
> 
> > Guix has tree main ways of modifying the source code of a package,
> > that you as a packager may use.  These are patches, snippets and
> > phases
> 
> Once the user knows _which_ of the three they should use (by
> consulting the following subsubsections), they can then search for
> 'patch', 'snippet' and 'phases' in the manual for more information,
> no need to duplicate that information.
Point taken, but imho cross-references are nice for those who just
learned "okay, I now know I need to solve my technical problem with a
phase, how do I do that?"  
> 
> > > Also, "giving the tools to build solutions with" does not help
> > > with the problem that I aimed to solve -- there was disagreement
> > > on what the appropriate tools are (see: Shepherd), so it not just
> > > needs to give the tools, but also the solutions.
> > I don't see how my patch lacks this information however.
> 
> IIUC, the raw information should usually be indeed all there, but the
> user has to consult _all_ of the sections to determine which option 
> (patch, snippet, phase) is appropriate, having to assemble all the 
> information is (a) a waste of time and (b) can lead to different 
> interpretations and conclusions (see: Shepherd).
> 
> More concretely, I cannot use your patch to decide between phases, 
> snippets and patches for the Shepherd issue:
> 
> * none of three appear to be forbidden by your documentation
> * there is no recommendation for 'patches' (IIRC it wasn't accepted 
> upstream and there was no intent to submit it upstream, it being
> really a Guile bug, not a Shepherd bug)
> * there is no recommendation for snippets (it's all free, no
> bundling)
> * build phases are 'to be avoided' (but not forbidden), as it
> resulted
>    in observably different runtime behaviour (being a bug fix)
> 
> -- three (or at best, two, if build phases are discounted) options 
> remain.  Myself, I would then consider 'snippets' to be the most 
> convenient, but some others (see: Shepherd, IIRC) would really want a
> patch instead, because 'patches can be reverted' or something like
> that.
> 
> As such, you are giving the tools (snippets / patches / phases, some 
> downsides and upsides), but different people can construct different 
> solutions from those tools even in the same situation, leading to
> conflicts.
> 
> If I use my patch instead, I go to "fixing technical issues". This 
> section tells me to use a patch or a snippet.  As the fix is not 
> Guix-specific, it recommends a patch to save time when upstreaming. 
> Conclusion: write a patch.  It was a Guile bug, so the fix was a
> patch to Guile.  But that would take time and upstream took the
> responsibility for a fix, so the 'efficient time thing' doesn't
> really apply and a small work-around (setting optimisation flags)
> suffices.
> 
> In this situation, the subsubsection doesn't care at all if you go
> for a snippet, so apply the last guiding principle: go for the
> simplest thing: a snippet. (Unless you already have a patch, then a
> patch is simplest.)
> Does someone else have a different idea on what's simplest?  Doesn't
> really matter, ‘Sometimes this is subjective, which is also fine’.
I do get the impression that this patch simply codifies what you feel
is right based on the shepherd situation rather than thinking about the
general case, which is why my patch makes weaker statements here.  If
this issue could have been avoided with a #:make-flag, we would have
used that.

More importantly, I fail to see the superiority of your patch here. 
IIUC neither patch offers a unique solution to the shepherd thing, so
the room for debate remains.  You could claim that my patch offers more
room, which fair enough, it does, but on the same token it allows
people to see that the guidelines have been followed and the patch can
be applied.

> > In particular, for review purposes, mine should be easier to work
> > with.  For instance, the reviewer sees a hash embedded in a
> > snippet, sees in the snippet entry that you shouldn't do that, and
> > can thus say "nope, don't do a snippet here".
> 
> I think we should optimise for packagers before reviewers
Code is more often read than written, so optimising for the reader is
optimizing for the packager as well.

> > Regardless of which patch we pick, it should not mandate a
> > particular solution in potentially contentious cases,
> 
> Actually the whole point was to mandate a particular solution for the
> contentious cases, see Shepherd.
But I disagree.

Memes aside, please refrain from the "I'm right, do this" style (which
is another problem with the "problem-centric" approach), but rather
focus on writing a patch that makes reviewers not discard a patch due
to formalities.  If the shepherd thing had needed an upstream patch,
even with a snippet that patch could have easily been created by
converting that snippet to a sed, so more time was wasted debating than
overworking.

> > and also point towards the right solution.  See our discussions on
> > the individual solutions on points in which I believe you've
> > errored.
> 
> These are:
> 
> * the typo's
> * including "skipping tests indicating failure under ‘Fixing
> technical issues’"
> * "don't mention file names of non-free things in patches"
> 
> Did I miss any?
I'm pretty sure there's also a discussion on store path embeddings at
the very least.  Forgive me if I too missed something.

Cheers

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

[Maxime Devos]

[-- Attachment #1.1.1: Type: text/plain, Size: 33217 bytes --]



On 09-09-2022 15:32, Liliana Marie Prikler wrote:
>> Also, this is the package source field, not the description, I don't
>> see how the "helps software stand on its own merits" applies to
>> snippets of the package source.
> Point taken, but imho it still makes sense to prefer keep lists over
> remove lists.  The GNU project encourages people to read the sources
> after all.

I do not see how the former follows from the latter, I would expect the 
opposite.  GNU project encourages reading source code, we put useful 
information in the source code (e.g. records on what non-free parts
haven't been replaced yet).

>>>>
>>>>> There still is the difference that phases are clearly delimited
>>>>> while snippets are a block of code that shouldn't get too
>>>>> large.
>>>> Snippets are delimited clearly as well, though, with the
>>>> 'snippet' field?
>>>> And the limitations of snippet length and phases length are the
>>>> same -- no limits, though conciseness is appreciate as always.
>>> True, but phases can be made to do just one thing, whereas snippets
>>> have to fix everything that's wrong in more or less one (begin
>>> ...).
>>> This is a noticable distinction.>
>>
>> You can do that in snippets too, with comments:
>>
>> (snippet
>>     #~(begin
>>         ;; Do the foo thing
>>         (foo)
>>         (foo-2 [...])
>>         [...]
>>         ;; Do the bar thing
>>         (bar)
>>         (bar-2 [...])
>>         [...]))
> You can, but it's still wiser to just keep the snippet short enough so
> you don't have to.

That's also the case for phases, though?  Conciseness is both good for 
phases and snippets.
>>    I would however very
>>> much prefer a wording that points people toward this practice
>>> existing,
>>> even if they have to go look in the code for examples.
>>> Alternatively,
>>> a footnote for the most common case (search-input-file inputs
>>> "bin/command") ought to suffice for the time being.
>>
>> Aside from the typo's and a few rephrasings, I'm done with the
>> documentation.  If someone wants to extend the section with such
>> information, they can always do so later.
> I haven't seen a v2 though.  Am I correct in assuming that none of the
> points we discussed in the last few mails are going to be addressed?

I did sent a v2: <https://issues.guix.gnu.org/57598#8>.
And most were addressed, even if only as me not considering it an issue.

>>>
>>> IMHO, I think a reader who remembers the guiding principles should
>>> see that it applies.
>>
>> Likely. But removing the explicit mention of the guiding principle
>> still makes the logical reasoning incomplete, remembering has nothing
>> to do with it.  As I've written previously: ‘This has nothing do do
>> with [...] and remembering, but rather with [...]’.
> In that case, let me rephrase my criticism: "It passes the guidelines"
> should not be part of the logical reasoning here.  Otherwise, why not
> have a guideline checklist at the end of each subsection (which would
> be silly, obviously, but that's the point).

Because, as you note, that's a silly structure, putting the premises (= 
guiding principles) after the conclusions (= worked-out cases) is an 
illogical structure, whereas putting the premises _before_ the 
conclusions is logical

>>>>
>>> Also, your guiding principles are (with one exception) really just
>>> invariants that ought to hold for the source field of a package.
>>
>> I don't know about the exceptions (I haven't counted them), but yes,
>> indeed.  I do not see the problem of this.
> For the record, the exception is the "most convenient" bit you've
> copied from my patch :)

That guideline actually predates your patch, it's part of the original 
proposal (of which the wording changed later):

 > https://yhetil.org/guix/c4c0a071-2e03-d4d6-6718-05424d21d146@telenet.be/
> Sometimes, there remains more than one acceptable way to accomplish the 
> goal. In that case, choose whatever appears to be most convenient.




>>> As such, I think it would be easier to state "A package's source
>>> should
>>> be the smallest corresponding source in terms of uncompressed file
>>> size.  This corresponding source must consist only of free software
>>> (note Free Software) and should build on all platforms supported by
>>> upstream."  Note how smallest naturally implies unbundling bundled
>>> sources.
>>
>> This criterium on overly small sources.  Often, a package's source
>> contains things that is not used for the build or outputs and hence
>> is not part of the corresponding source.  Examples:
> Note "should" rather than "shall" or "must", meaning that slightly
> larger tarballs are still acceptable.
OK.  The criterium remains overly tight though -- by that criterium, 
slightly larger tarballs are considered undesirable, even when they have 
useful (and hence, desired) things:

> 
>> * the source contains documentation that could be built and
>> installed,
>>     but Guix doesn't do so yet.  --> should be kept (unless non-free)
>> * documentation that isn't meant to be built or installed
>>     (e.g. HACKING, PACKAGERS, ...) --> useful, shouldn't be removed.
>> * .gitignore, .github, ... --> nothing wrong with removing those,
>>     but pointless, let's not waste our time with looking for those
>>     and removing them, even though doing so would make it smaller.
>> * source files for platforms the upstream does not support
>> yet/anymore
>>     (but with some volunteer effort (e.g. bugfixes), it might become
>>     a supported system again)
>>     --> removing them (e.g. as part of an overly-broad
>> (delete-file-recursively
>> "this-dir-has-bundling-albeit-not-all-of-it-is-bundling"))
>>         can be OK-ish, but removing them for 'minimality' is
>> pointless.

> I see you're nitpicking for the sake of the argument,

I'm not.  It's nitpicking for the sake of the four * examples above.

> but none of the
> examples you provide (safe for the bundling one) add much cost to
> either packing or unpacking.

Not the point, the point is that having to treat those examples 
seriously  is a waste of time and even somewhat harmful (removing useful 
stuff).

> Thus, I'm pretty sure we can ignore them
> for practical purposes.

As I've explained, this guideline is inconsistent with what is actually 
recommended.  Here are some practical purposes:

* minimal dissonance between 'appreciated by policy' and 'appreciated by
   reviewers, packagers, users'.
* not having to review and accept pointless patches to Guix for deleting
   .github, .gitignore, uninstalled documentation that were written
   for the sake of minimality
* not having to (in the sense of: it would be appreciated if you'd do
   that) write such patches

   That being said, if you have a better
> formulation, please do tell.

The four guiding principles.

> 
>>
>>>>> I still find this wording very confusing. Perhaps "To add new
>>>>> functionality, a patch is almost always the best choice. For
>>>>> one,
>>>>> it is likely that the new functionality requires changing
>>>>> multiple
>>>>> lines of source code, which is more convenient to do with a
>>>>> patch
>>>>> than with a snippet. Further, patches can be taken from and
>>>>> submitted to upstreams more easily. If your patch has not been
>>>>> submitted to upstream, consider doing so."
>>>> It loses some information (that patches are preferred) and
>>>> (after re-adding the conclusion) is more verbose, which appears
>>>> to be considered very important.
>>> Which conclusion is there to re-add?
>>
>> "patches are preferred"
>>
>>     The conclusion is already stated
>>> in the beginning: patches are almost always the best choice.  Then
>>> two
>>> reasons as for why.  The part w.r.t. upstreaming changes has also
>>> been
>>> addressed.
>>
>> While I consider that policies should have "best choices" coinciding
>> with "preferred" and that not doing so would be illogical, people,
>> projects, decisions ... are far from always logical.
>>
>> Because of this, people cannot assume that the 'best choices' are
>> 'preferred', so it needs to be mentioned explicitly that these 'best
>> choices' are actually 'preferred'.
> In that case, simply write preferred choice?

It is already mentioned that they are preferred:

> ‘As such, patches are preferred’

By removing the information that 'patches are the most convenient 
choice' (by replacing it with ‘patches are the preferred choice’), the 
logical structure becomes weaker; a part of the explanation on the ‘why’ 
becomes missing.

> 
>>> I'm using enumeration as a super term here, which can be
>>> ((sub)sub)sections, chapters, list elements, whatever, and my claim
>>> is that we barely have enough principles to allow the use of a
>>> plural.
>>
>> In English, things are either plural or singular.  Everything >= 2 is
>> plural.  There number of principles, however we count them, is, at
>> least, 2.  Consequently, the principles are plural, not singular.
>> Treating the principles as singular is simply grammatically
>> incorrect.
> Correction: Everything that isn't exactly 1 is plural in English,
> including 0, with perhaps the exception of -1 also using singular.
> 
>> Maybe it is barely allowed to be plural, but English grammar doesn't
>> care about that -- it is definitively disallowed to be singular, only
>> plural remains.
> Note that my argument isn't about English grammar, it uses English
> grammar.

Your argument is that ‘there aren't a sufficient amount of principles to 
allow the use of a plural’.  'Plural' is a concept of English grammar.
As such, you have written an argument about English grammar.

You appear to have meant some different argument, but all the arguments 
on ‘are they guiding principles or not’ I've read are based on the 
number of principles or on what 'guiding principle' means, which are 
issues of grammar and vocabulary respectively.

I'm still not seeing how they aren't guiding principles.  Maybe you have 
a different meaning of ‘guiding principles’ in mind?  Or maybe you would
like to name them 'guidelines' or 'general recommendations' instead.

>>> Adding to that, now that I think of it, I also doubt their
>>> usefulness in guiding.  "Use whatever feels convenient, but note
>>> that that might be subjective" is more useful at the end of the
>>> section when a user didn't find what they were looking for than at
>>> the start.
>>
>> The introduction has a set of guiding principles, from with concrete
>> cases are built.  By adding another principle at the end, the cases
>> cannot make use of the "use [...] convenient" principle.
>>
>> Additionally, now the user has to look at _two_ places to find the
>> guiding principles -- at the beginning, and at the end.  Worse,
>> the beginning does not have a cross-reference to the end, so since
>> the set at the beginning is presented as exhaustive, the user might
>> not know there is another guiding principle.
>>
>> And even if they did read through the whole section (even though they
>> should only have to read the introduction and the relevant worked-out
>> case), assuming they read through it in a linear fashion, due to the
>> new guiding principle that wasn't alluded to at the beginning, they
>> have to redo their mental model of "Modifying Sources" as this
>> principle could invalidate things.
> This shows both a problem with your structure,

I'm not seeing the problem.  How does this show a problem?

> but more importantly, a
> failure to understand what I meant when I wrote "use whatever is
> convenient" in my own patch.  This principle *can* only work for cases
> in which there is *no clearly established practice*, thus putting it at
> the end *after having enumerated all practices* is the logical choice.
> With your structure, you have to bend it sideways to shoehorn it in
> with the other principles.

You appear to have meant it as a kind of ‘fallback principle’, in which 
case putting it at the end does indeed make sense.

In my patch, the principle works differently, it is also used for 
explaining already established practices, for the cases where there are 
multiple technically allowed principles but still often a single 
_recommended_ principle.  E.g.:

> Usually, a bug fix comes in the form of a patch copied from upstream or
> another distribution.  In that case, simply adding the patch to the
> @code{patches} field is the most convenient and usually does not cause
> any problems; there is no need to rewrite it as a snippet or a phase.
> 
> If no ready-made patch already exists, then ...]

and

> To add new functionality, a patch is almost always the most convenient
> choice of the three 

.

To go from the principle as used in your patch to the principle as used 
in my patch, perhaps in the process it is bent sideways and shoehorned.
But after all the bending and shoehorning, it seems to fit well now in 
the beginning (and not well at all the end), so I'm not seeing any 
problems here.

> 
>>> At the risk of responding jokingly to what was meant serious: I
>>> didn't know we suddenly gained 20 guiding principles.
>>
>> 25 lines are for the guiding principles (sometimes referring to a
>> principle of elsewhere in Guix, sometimes a new principle for
>> "Modifying Sources").  You proposed to 'cache' them somehow.  In
>> Guile, to cache something, you can use 'delay/force'.  But this
>> increases the amount of code (due to the additional use of 'delay'),
>> instead of decreasing.
>>
>> The documentation equivalent (whatever that might be, I am not seeing
>> one myself) would then also be at least as long, maybe even a little
>> longer due to the use of 'delay'.
> Okay, so I did misunderstand those 25 lines as 25 lines within the text
> calling back to those guiding principles rather than 25 lines for the
> guiding principles themselves.  Still, 25 lines are a little much,
> especially given that the bulk of it explains the semantics of the
> packages' source field.

I think we'll have to agree to disagree on that.

> 
>>>> I suppose table items might take two less line or so less than
>>>> subsubsections, but I don't think that's sufficient reason to
>>>> step away from a nice section structure.
>>> Another reason is that you can end a table in the middle of a
>>> section, which you can't do with subsections.  Hence why I'm able
>>> to put a remark at the bottom, which you have to clumsily fit into
>>> the top.
>>
>> I can, indeed, not put the 'convenience principle' at the bottom,
>> except perhaps by adding a new subsubsection, and for tables adding
>> such a remark at the bottom is indeed more convenient.
>>
>> However, I don't see the 'have to clumsily' here -- as explained
>> elsewhere, I believe that the 'convenience principle' shouldn't be
>> separated from the other principles and that it fits nicely next to
>> the the principles --- there is no 'have to', because I choose for
>> this, and I am not seeing any 'clumsiness' here.
> I think we'd have to debate choice vs. force here which would be a
> rather long aside, but to make my argument short, your choice of how to
> structure this section (table vs subsections) directly influences your
> "choice" where to place particular information

Sure.

> in a way that might
> inhibit natural information flow.

I'm not seeing it.  As I've explained previously, putting the premises 
(= guiding principles) before the conclusions (= worked out cases / 
solutions) is a logical (hence, natural) information flow, and 
introducing them on-the-way or implicitly is ad-hoc (unnatural).
>>>> The patch does this, currently.  It already proposes a number of
>>>> hammers (patches, snippets and phases) and purposes (adding new
>>>> functionality, fixing technical issues, unbundling, ...).
>>>> Also, the scenario "oh no, however will I put this nail into a
>>>> wall"
>>>> actually happened -- see the Shepherd discussion, where there was
>>>> a lot of disagreement on how nails (= small work-around in the
>>>> Makefile) should be put in walls (= patches, snippet, phase?).
>>>> It
>>>> was the whole reason to start writing a documentation patch.
>>> You might want to add a link here if it supports your argument, but
>>> without looking at the discussion this rather sounds like "oh no, I
>>> have three hammers, which one do I pick?" – which, fair enough, is
>>> still a problem, but one that neither of our patches would cause
>>> imho.
>>
>> As I think I've written previously, the whole point was to solve that
>> problem. For the discussion, see:
>>
>> * https://issues.guix.gnu.org/54216
>> *
>> https://yhetil.org/guix/c4c0a071-2e03-d4d6-6718-05424d21d146@telenet.be/
>> *
>> https://yhetil.org/guix-devel/84e13ef7d437062df5cca51a12e6da54929e0176.camel@telenet.be/
>>
>> Not solving the problem defeats the whole point, as the purpose is to
>> solve that problem.
> I think we might be disagreeing about what constitutes "solving the
> problem" here.  Correct me if I'm wrong, but to me it seems any
> scenario that is not covered by whatever patch is applied counts as
> "not having solved the problem".

It doesn't -- I consider the Shepherd problem to be solved by my patch 
(albeit rather weakly, see later), and the wider problem of unclear 
guidelines and policy on snippet/phase/patch to be, perhaps not really 
solved, but still much improved (this time not weakly).

> I personally find this line of
> reasoning questionable, as there are perfectly valid reasons why
> multiple tools could apply.

See previous paragraph.

> Take the problem of embedding a store path.  You could for instance
> replace all occurences of "sh" with /gnu/store/.../bin/sh, or you could
> first replace them in a patch with "@sh@" and then replace that with
> /gnu/store/.../bin/sh.  Of course, you rarely have to do the latter and
> the former is simpler, hence why you will often see the former, but
> that doesn't mean the latter is invalid.

It's contrary to 'It preferably should also work on non-Guix systems’ -- 
not invalid, but still usually (*) against (proposed) policy because a 
direct substitute* is at the same time more convenient and follows all 
the the principles.

(*) sometimes, in case of shell scripts, a substitute* is fragile 
(substituting too much), in which case a "sh" -> "@sh@" becomes more 
convenient and hence the violation of the 'non-Guix system' rules 
becomes acceptable.

>>>>> And if they don't find anything, they see the handy little line
>>>>> at the bottom saying "use whatever you think is convenient".
>>>> Nowhere did the patch imply that the listed cases were all cases.
>>>> In fact, in two places in the introduction it is implied that the
>>>> examples are not exhaustive, and that they can choose according
>>>> to convenience  [...]
>>> Emphasis on handy little line rather than needing to be told twice
>>> (particularly if people have no idea what to expect due to not
>>> having looked at the worked-out cases yet).
>>
>> They don't need to be told twice.  Also, my patch also has that handy
>> little line (albeit differently worded), see the 'guiding
>> principles'.
>>
>> Also, on the 'no idea what to expect' -- this is exactly the same for
>> your patch too?  In both patches, if the user only reads the
>> introduction and conclusion (if any) and doesn't read the actual
>> (relevant examples)/(explanation of patches, snippets, phases), then
>> that's their problem.
> I do think a table in the middle of the section is hard to ignore,

I think so too, hence my previous response.

> but suppose for the sake of argument that they do, [...]

Given this response, I assume I've misinterpreted what you meant with 
‘not having looked at the worked-out cases yet’.

>>>>
>>>>> I also expand a little on the benefits and drawbacks of
>>>>> these approaches as you would when describing design patterns.
>>>> This is also done in my patch. [...]
>>> This is not about the contained information, but the structure of
>>> the contained information.
>>>
>>> My solution->problem style follows roughly this style:
>>> 1. solution
>>> 2. problems it is known to solve
>>> 3. how to use
>>> 4. properties, caveats, etc.
>>>
>>> Your problem->solution style roughly has the following:
>>> 1. problem
>>> 2. (set of) solution(s)
>>> 3. if more than one solution, maybe a help to select
>>
>> Also, in no particular order
>>
>>     4.: how to use
>>     5.: explanation why it is preferred (properties, caveats?)
> No particular order is problematic,

I'm not seeing why.

  but more importantly if a
> technology appears more than once (guaranteed by the pigeonhole
> principle), then either its properties get repeated (not good for
> length) or scattered (not good for the flow you want).  Again, a
> structural problem.

That's equally applies to your patch too, though?  (Switching 
"technology" with "problem".)

Also, I've re-read my patch, and I didn't find any repeated properties 
or scattering, except for a single property (patches are upstreamable):

 > First, when the fix is not Guix-specific, upstreaming the fix is
 > strongly desired to avoid the additional maintenance cost to Guix.
 > [...]
> As such, patches are preferred.  However, as with bug
> fixes, upstreaming the new functionality is desired.

As such, I'm not following your point here.

>>> This makes it so that people might have to go to a different
>>> subsection than the one they read for their solution to find out
>>> about potential caveats, e.g. not embedding store paths in a
>>> snippet.
>>
>> I assume their problem was "X doesn't find Y".  This being a
>> technical issue, they go to "Fixing technical issues".  In that
>> subsubsection,  there are the words:
>>
>>> Secondly, if the fix of a technical issue embeds a store file name,
>>> then it has to be a phase.  Otherwise, if the store file name were
>>> embedded in the source, the result of @command{guix build --source}
>>> would be unusable on non-Guix systems and also likely unusable on
>>> Guix systems of another architecture.
>>
>> so they actually do know of the caveat 'don't embed store paths in a
>> snippet, do it in a phase instead', and they did not need to go to a
>> different subsubsection.
> Typical happy path.

Indeed, it's a happy path!

>>>>
>>>>> Your problem->solution approach instead leaves people wondering
>>>>> when their particular use case has not been described.
>>>> See my reply to ‘And if they don't find anything, they see the
>>>> handy little line at the bottom saying "use whatever you think is
>>>> convenient’.
>>>>> It gives them a solution rather than the tools to build
>>>>> solutions with.
>>>> It does give the tools: snippets, patches and phases.
>>> As far as I read, it describes none of those.  It puts out guiding
>>> principles and some already worked-out cases.
>> Here are the descriptions:
>>
>>> Guix has tree main ways of modifying the source code of a package,
>>> that you as a packager may use.  These are patches, snippets and
>>> phases
>>
>> Once the user knows _which_ of the three they should use (by
>> consulting the following subsubsections), they can then search for
>> 'patch', 'snippet' and 'phases' in the manual for more information,
>> no need to duplicate that information.
> Point taken, but imho cross-references are nice for those who just
> learned "okay, I now know I need to solve my technical problem with a
> phase, how do I do that?"

They are indeed.  But I'm not following what you mean with "But" here.
I didn't claim anything about cross-references there?

Do you want me to add a few cross-references (for 'patch', 'snippet' and 
'phases')?

>>>> Also, "giving the tools to build solutions with" does not help
>>>> with the problem that I aimed to solve -- there was disagreement
>>>> on what the appropriate tools are (see: Shepherd), so it not just
>>>> needs to give the tools, but also the solutions.
>>> I don't see how my patch lacks this information however.
>>
>> IIUC, the raw information should usually be indeed all there, but the
>> user has to consult _all_ of the sections to determine which option
>> (patch, snippet, phase) is appropriate, having to assemble all the
>> information is (a) a waste of time and (b) can lead to different
>> interpretations and conclusions (see: Shepherd).
>>
>> More concretely, I cannot use your patch to decide between phases,
>> snippets and patches for the Shepherd issue:
>>
>> * none of three appear to be forbidden by your documentation
>> * there is no recommendation for 'patches' (IIRC it wasn't accepted
>> upstream and there was no intent to submit it upstream, it being
>> really a Guile bug, not a Shepherd bug)
>> * there is no recommendation for snippets (it's all free, no
>> bundling)
>> * build phases are 'to be avoided' (but not forbidden), as it
>> resulted
>>     in observably different runtime behaviour (being a bug fix)
>>
>> -- three (or at best, two, if build phases are discounted) options
>> remain.  Myself, I would then consider 'snippets' to be the most
>> convenient, but some others (see: Shepherd, IIRC) would really want a
>> patch instead, because 'patches can be reverted' or something like
>> that.
>>
>> As such, you are giving the tools (snippets / patches / phases, some
>> downsides and upsides), but different people can construct different
>> solutions from those tools even in the same situation, leading to
>> conflicts.
>>
>> If I use my patch instead, I go to "fixing technical issues". This
>> section tells me to use a patch or a snippet.  As the fix is not
>> Guix-specific, it recommends a patch to save time when upstreaming.
>> Conclusion: write a patch.  It was a Guile bug, so the fix was a
>> patch to Guile.  But that would take time and upstream took the
>> responsibility for a fix, so the 'efficient time thing' doesn't
>> really apply and a small work-around (setting optimisation flags)
>> suffices.
>>
>> In this situation, the subsubsection doesn't care at all if you go
>> for a snippet, so apply the last guiding principle: go for the
>> simplest thing: a snippet. (Unless you already have a patch, then a
>> patch is simplest.)
>> Does someone else have a different idea on what's simplest?  Doesn't
>> really matter, ‘Sometimes this is subjective, which is also fine’.
> I do get the impression that this patch simply codifies what you feel
> is right based on the shepherd situation

True, except for the 'simply', as I also considered several other 
situations (Removing bundled libraries, Removing non-free software, 
Adding new functionality).  I do not see the problem with this -- the 
proposed policy was considered workable even if some would like it to be 
a little bit different, and if others feel strongly about they could, I 
don't, maybe set up a vote system or such.

> rather than thinking about the general case,

See: several other cases that should cover most situations encountered 
in practice, and for the parts of the general case that aren't 
worked-out yet, there are the guiding principles.

> which is why my patch makes weaker statements here.  If
> this issue could have been avoided with a #:make-flag, we would have
> used that.
> 
> More importantly, I fail to see the superiority of your patch here > IIUC neither patch offers a unique solution to the shepherd thing, so
> the room for debate remains

In my patch, multiple options remain, but there is no real room for 
debate.  More concretely:

> When there is more than one way to do something, choose whichever method
> is the simplest.  Sometimes this is subjective, which is also fine.
> What matters is that you use techniques that are common within the
> community (i.e., patterns that appear throughout @code{gnu/packages/...})
> and are thus clearly legible for reviewers.

it is considered subjective, having multiple options if fine!  To 
correct a packaging following the other option (to help with following 
the policies), you don't have to debate on what's the proper option to 
get a fix in, as both are considered acceptable.  In many cases, no need 
for debate, just pick one and move on.  (Also the case for your patch IIUC.)

Things are also phrased reconciliatory, e;g.:

> To make things more concrete and to resolve conflicts between the
> principles, a few cases have been worked out:

However, in your patch, there is more opportunity for conflicts. E.g.:

> Each one has its strengths and drawbacks, along with intended andhistorically derived use cases.

By this line, arguments like 'X was historically the intended purpose of 
Y, using Y for Z is incorrect' become reasonable, which makes 
disagreements more difficult to resolve (as you know need to consult 
history and because history is fixed, so no improvements would be 
possible anymore in areas where there is a historically intended use case).

> You could claim that my patch offers more
> room, which fair enough, it does, but on the same token it allows
> people to see that the guidelines have been followed and the patch can
> be applied.
> 
>>> In particular, for review purposes, mine should be easier to work
>>> with.  For instance, the reviewer sees a hash embedded in a
>>> snippet, sees in the snippet entry that you shouldn't do that, and
>>> can thus say "nope, don't do a snippet here".

That is also the case for problem->solution?  A "sh" -> #$(file-append 
bash-minimal "/bin/sh") substitution can be recognised on sight as not 
possibly being anything else than a technical fix, and then the 
subsubsection on technical fixes mentions that those must be done in phases.

Or, another explanation: reviewers usually have seen a lot of more 
packages than a new packager.  Because of that, and because they have 
familiarised theirselves with policy, they have over time automatically 
built a mental 'reverse index' of solutions->problems.
>> I think we should optimise for packagers before reviewers
> Code is more often read than written, so optimising for the reader is
> optimizing for the packager as well.

While all reviewers are readers, not all readers are reviewers, and 
reviewing usually happens only once, maybe twice per patch.

>>> Regardless of which patch we pick, it should not mandate a
>>> particular solution in potentially contentious cases,
>>
>> Actually the whole point was to mandate a particular solution for the
>> contentious cases, see Shepherd.
> But I disagree.
> 
> Memes aside, please refrain from the "I'm right, do this" style (which
> is another problem with the "problem-centric" approach),

Is this referring to the style of the documentation patch?  If so: it 
does explain why the "do this" is right, so I don't see the problem.  Do 
you have a particular point of the documentation in mind you consider 
wrong (*)?

(*) points I know of:
* keep lists / remove lists (for the footnote about not-yet-policy on 
removing things in patches)
* the guiding principles aren't guiding principles

> but rather
> focus on writing a patch that makes reviewers not discard a patch due
> to formalities.

I think the patch (v1 and v2) satisfies that.

> If the shepherd thing had needed an upstream patch,
> even with a snippet that patch could have easily been created by
> converting that snippet to a sed, so more time was wasted debating than
> overworking.

I do not see your point here -- #57598 is about documentating/making 
policy, not 'should we spent time on correcting the shepherd package to 
match policy / convincing the other party that the status quo matches 
policy'.

>>> and also point towards the right solution.  See our discussions on
>>> the individual solutions on points in which I believe you've
>>> errored.
>>
>> These are:
>>
>> * the typo's
>> * including "skipping tests indicating failure under ‘Fixing
>> technical issues’"
>> * "don't mention file names of non-free things in patches"
>>
>> Did I miss any?
> I'm pretty sure there's also a discussion on store path embeddings at
> the very least.  Forgive me if I too missed something.
There was a discussion on store file name embeddings.  There was no 
mention of any errors in the documentation of store file names.  IIRC, 
we agree on how store file name embeddings must be done.  There were, 
however, proposals to extend the documentation to cover ‘how to embed’ 
in more detail (substitute* + search-input-file ...), but no errors.

Greetings,
Maxime.

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 929 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

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

[Liliana Marie Prikler]

Am Freitag, dem 09.09.2022 um 20:44 +0200 schrieb Maxime Devos:
> GNU project encourages reading source code, we put useful 
> information in the source code (e.g. records on what non-free parts
> haven't been replaced yet).
You mean bundled parts not unbundled, right?  IIUC, there should be no
non-free parts in a free software distribution.

> > You can, but it's still wiser to just keep the snippet short enough
> > so you don't have to.
> 
> That's also the case for phases, though?  Conciseness is both good
> for phases and snippets.
Intentionally or otherwise, you're talking past me.

> > >    I would however very
> > > > much prefer a wording that points people toward this practice
> > > > existing,
> > > > even if they have to go look in the code for examples.
> > > > Alternatively,
> > > > a footnote for the most common case (search-input-file inputs
> > > > "bin/command") ought to suffice for the time being.
> > > 
> > > Aside from the typo's and a few rephrasings, I'm done with the
> > > documentation.  If someone wants to extend the section with such
> > > information, they can always do so later.
> > I haven't seen a v2 though.  Am I correct in assuming that none of
> > the
> > points we discussed in the last few mails are going to be
> > addressed?
> 
> I did sent a v2: <https://issues.guix.gnu.org/57598#8>.
> And most were addressed, even if only as me not considering it an
> issue.
Hmm, that was while I was still composing that message.

> > In that case, let me rephrase my criticism: "It passes the
> > guidelines" should not be part of the logical reasoning here. 
> > Otherwise, why not have a guideline checklist at the end of each
> > subsection (which would be silly, obviously, but that's the point).
> 
> Because, as you note, that's a silly structure, putting the premises
> (= guiding principles) after the conclusions (= worked-out cases) is
> an illogical structure, whereas putting the premises _before_ the 
> conclusions is logical
And yet you're putting a premise after a conclusion here.

> > For the record, the exception is the "most convenient" bit you've
> > copied from my patch :)
> 
> That guideline actually predates your patch, it's part of the
> original proposal (of which the wording changed later):
> 
>  >
> https://yhetil.org/guix/c4c0a071-2e03-d4d6-6718-05424d21d146@telenet.be/
> > Sometimes, there remains more than one acceptable way to accomplish
> > the goal. In that case, choose whatever appears to be most
> > convenient.
Fair enough.
> 

> > Note "should" rather than "shall" or "must", meaning that slightly
> > larger tarballs are still acceptable.
> OK.  The criterium remains overly tight though -- by that criterium, 
> slightly larger tarballs are considered undesirable, even when they
> have useful (and hence, desired) things:
Point taken, but...

> > 
> > > * the source contains documentation that could be built and
> > > installed,
> > >     but Guix doesn't do so yet.  --> should be kept (unless non-
> > > free)
> > > * documentation that isn't meant to be built or installed
> > >     (e.g. HACKING, PACKAGERS, ...) --> useful, shouldn't be
> > > removed.
> > > * .gitignore, .github, ... --> nothing wrong with removing those,
> > >     but pointless, let's not waste our time with looking for
> > > those
> > >     and removing them, even though doing so would make it
> > > smaller.
> > > * source files for platforms the upstream does not support
> > > yet/anymore
> > >     (but with some volunteer effort (e.g. bugfixes), it might
> > > become
> > >     a supported system again)
> > >     --> removing them (e.g. as part of an overly-broad
> > > (delete-file-recursively
> > > "this-dir-has-bundling-albeit-not-all-of-it-is-bundling"))
> > >         can be OK-ish, but removing them for 'minimality' is
> > > pointless.
> 
> > I see you're nitpicking for the sake of the argument,
> 
> I'm not.  It's nitpicking for the sake of the four * examples above.
> 
> > but none of the
> > examples you provide (safe for the bundling one) add much cost to
> > either packing or unpacking.
> 
> Not the point, the point is that having to treat those examples 
> seriously  is a waste of time and even somewhat harmful (removing
> useful stuff).
> 
> > Thus, I'm pretty sure we can ignore them
> > for practical purposes.
> 
> As I've explained, this guideline is inconsistent with what is
> actually recommended.  Here are some practical purposes:
> 
> * minimal dissonance between 'appreciated by policy' and 'appreciated
> by
>    reviewers, packagers, users'.
> * not having to review and accept pointless patches to Guix for
> deleting
>    .github, .gitignore, uninstalled documentation that were written
>    for the sake of minimality
> * not having to (in the sense of: it would be appreciated if you'd do
>    that) write such patches
> 
>    That being said, if you have a better
> > formulation, please do tell.
> 
> The four guiding principles.
That's an awful lot to write rather than "the package's source should
be the corresponding source minus the bits we don't like".  Ludo’
already commented on the length of my patch (and yes, I'm aware that
both of our patches blow up the section by a factor of like 3 at
minimum), so I don't think you're doing anyone a service here by
expanding something into four guidelines that would take, like, a
simple sentence instead if carefully worded.  Let's take the time to
write the shorter letter, shall we?


> By removing the information that 'patches are the most convenient 
> choice' (by replacing it with ‘patches are the preferred choice’),
> the logical structure becomes weaker; a part of the explanation on
> the ‘why’ becomes missing.
I'm going to sound like a broken record, but conciseness is key:
"Patches are preferred when adding functionality to a package.  They
can more easily be shared with upstreams and are easier to work with
when making multi-line changes.  Next item."

> > > > [W]e barely have enough principles to allow the use of a
> > > > plural.
> Your argument is that ‘there aren't a sufficient amount of principles
> to allow the use of a plural’.  
Don't put words into my mouth.

> 'Plural' is a concept of English grammar.  
The concept of a plural is not unique to English grammar.

> As such, you have written an argument about English grammar.
You're failing to read between the lines and it's getting rather
exhausting.

> You appear to have meant some different argument, but all the
> arguments on ‘are they guiding principles or not’ I've read are based
> on the number of principles or on what 'guiding principle' means,
> which are issues of grammar and vocabulary respectively.
> 
> I'm still not seeing how they aren't guiding principles.  Maybe you
> have a different meaning of ‘guiding principles’ in mind?  Or maybe
> you would like to name them 'guidelines' or 'general recommendations'
> instead.
Call it whatever you want, my point is twofold:
1. By proper counting and simplification, you will have at most two.
2. At least one of those two doesn't feel very "guiding" to me.
Further, I think you're doing a bad job of splitting mandatory and
optional "recommendations" by lumping them together.

> > > 
> > > The introduction has a set of guiding principles, from with
> > > concrete cases are built.  By adding another principle at the
> > > end, the cases cannot make use of the "use [...] convenient"
> > > principle.
> > > 
> > > Additionally, now the user has to look at _two_ places to find
> > > the guiding principles -- at the beginning, and at the end. 
> > > Worse, the beginning does not have a cross-reference to the end,
> > > so since the set at the beginning is presented as exhaustive, the
> > > user might not know there is another guiding principle.
> > > 
> > > And even if they did read through the whole section (even though
> > > they should only have to read the introduction and the relevant
> > > worked-out case), assuming they read through it in a linear
> > > fashion, due to the new guiding principle that wasn't alluded to
> > > at the beginning, they have to redo their mental model of
> > > "Modifying Sources" as this principle could invalidate things.
> > This shows both a problem with your structure,
> 
> I'm not seeing the problem.  How does this show a problem?
You're writing three paragraphs to lament how necessary the principle
of "use whatever is convenient" – which mind you I believe should never
be used outside of resolving conflicts between otherwise equally
applicable choices – is to your reasoning.

> > but more importantly, a failure to understand what I meant when I
> > wrote "use whatever is convenient" in my own patch.  This principle
> > *can* only work for cases in which there is *no clearly established
> > practice*, thus putting it at the end *after having enumerated all
> > practices* is the logical choice.
> > With your structure, you have to bend it sideways to shoehorn it in
> > with the other principles.
> 
> You appear to have meant it as a kind of ‘fallback principle’, in
> which case putting it at the end does indeed make sense.
> 
> In my patch, the principle works differently, it is also used for 
> explaining already established practices, for the cases where there
> are multiple technically allowed [technique] but still often a single
> _recommended_ [technique].  E.g.:
> 
> > Usually, a bug fix comes in the form of a patch copied from
> > upstream or another distribution.  In that case, simply adding the
> > patch to the @code{patches} field is the most convenient and
> > usually does not cause any problems; there is no need to rewrite it
> > as a snippet or a phase.
> > 
> > If no ready-made patch already exists, then ...]
> 
> and
> 
> > To add new functionality, a patch is almost always the most
> > convenient choice of the three 
> 
> .
> 
> To go from the principle as used in your patch to the principle as
> used  in my patch, perhaps in the process it is bent sideways and
> shoehorned.
> But after all the bending and shoehorning, it seems to fit well now
> in the beginning (and not well at all the end), so I'm not seeing any
> problems here.
And here I must disagree with the nature of your patch, because
elevating this into a guiding principle makes what feels most
convenient to you (or any other writer at the time of editing the
manual) most convenient to everyone.  My patch allows for deliberation
by both the packager and reviewer, which might lead to some conflicts
of opinion, but unless either solution can be shown clearly superior by
appeal to *another reason*, which mind you is lacking here, both
solutions should be accepted.
> 

> > in a way that might
> > inhibit natural information flow.
> 
> I'm not seeing it.  As I've explained previously, putting the
> premises (= guiding principles) before the conclusions (= worked out
> cases / solutions) is a logical (hence, natural) information flow,
> and introducing them on-the-way or implicitly is ad-hoc (unnatural).


> > I think we might be disagreeing about what constitutes "solving the
> > problem" here.  Correct me if I'm wrong, but to me it seems any
> > scenario that is not covered by whatever patch is applied counts as
> > "not having solved the problem".
> 
> It doesn't -- I consider the Shepherd problem to be solved by my
> patch (albeit rather weakly, see later), and the wider problem of
> unclear guidelines and policy on snippet/phase/patch to be, perhaps
> not really solved, but still much improved (this time not weakly).
> 
> > I personally find this line of reasoning questionable, as there are
> > perfectly valid reasons why multiple tools could apply.
> 
> See previous paragraph.


> > Take the problem of embedding a store path.  You could for instance
> > replace all occurences of "sh" with /gnu/store/.../bin/sh, or you
> > could first replace them in a patch with "@sh@" and then replace
> > that with /gnu/store/.../bin/sh.  Of course, you rarely have to do
> > the latter and the former is simpler, hence why you will often see
> > the former, but that doesn't mean the latter is invalid.
> 
> It's contrary to 'It preferably should also work on non-Guix systems’
> -- 
> not invalid, but still usually (*) against (proposed) policy because
> a direct substitute* is at the same time more convenient and follows
> all the the principles.
> 
> (*) sometimes, in case of shell scripts, a substitute* is fragile 
> (substituting too much), in which case a "sh" -> "@sh@" becomes more 
> convenient and hence the violation of the 'non-Guix system' rules 
> becomes acceptable.
Fair enough, guess I'll have to update configure.ac in the same patch
to honour the guiding principles.

> > > > > 
> > > In both patches, if the user only reads the
> > > introduction and conclusion (if any) and doesn't read the actual
> > > (relevant examples)/(explanation of patches, snippets, phases),
> > > then that's their problem.
> > I do think a table in the middle of the section is hard to ignore,
> 
> I think so too, hence my previous response.
> 
> > but suppose for the sake of argument that they do, [...]
> 
> Given this response, I assume I've misinterpreted what you meant with
> ‘not having looked at the worked-out cases yet’.
I think you understood it elsewhere, see the fallback guiding
principle.

> > No particular order is problematic,
> 
> I'm not seeing why.
Why have a structure, when you won't have a structure?  Sounds a little
counterintuitive, does it not?

>   but more importantly if a
> > technology appears more than once (guaranteed by the pigeonhole
> > principle), then either its properties get repeated (not good for
> > length) or scattered (not good for the flow you want).  Again, a
> > structural problem.
> 
> That's equally applies to your patch too, though?  (Switching 
> "technology" with "problem".)
Not really, since most problems that are to be uniquely solved with a
given tool can be uniquely named under that tool.  I do expand a little
on using patches and phases in combination for a more complicated job,
but that use case is notably missing from your patch.  I also repeat
shared characteristics of patches and snippets, which I could condense
to save some space, but in comparison to your patch, space is no issue.

> Also, I've re-read my patch, and I didn't find any repeated
> properties or scattering, except for a single property (patches are
> upstreamable):
> 
>  > First, when the fix is not Guix-specific, upstreaming the fix is
>  > strongly desired to avoid the additional maintenance cost to Guix.
>  > [...]
> > As such, patches are preferred.  However, as with bug
> > fixes, upstreaming the new functionality is desired.
> 
> As such, I'm not following your point here.
The trend of you not seeing continues.  Look at your first two
subsubsections (both v1 and v2).
> 

> > Point taken, but imho cross-references are nice for those who just
> > learned "okay, I now know I need to solve my technical problem with
> > a phase, how do I do that?"
> 
> They are indeed.  But I'm not following what you mean with "But"
> here.  I didn't claim anything about cross-references there?
> 
> Do you want me to add a few cross-references (for 'patch', 'snippet'
> and 'phases')?
I don't think you have a useful location to put them though, owing once
again to structure :)

> 
> > I do get the impression that this patch simply codifies what you
> > feel is right based on the shepherd situation
> 
> True, except for the 'simply'
Okay, I'll correct myself: This patch complicatedly codifies what you
feel is right based on the shepherd situation.

Jokes aside,

> I also considered several other situations (Removing bundled
> libraries, Removing non-free software, Adding new functionality).  I
> do not see the problem with this -- the proposed policy was
> considered workable even if some would like it to be a little bit
> different, and if others feel strongly about they could, I don't,
> maybe set up a vote system or such.
I think this is not the place for the kind of democracy where you put
everything barely the half of people can agree is the worst tradeoff
into "law".  Even worse if we had an anonymous online vote, because
those aren't susceptible to manipulation at all.

I think we should rather exercise caution and document what if not the
entire community then at least the reviewers can agree on.

> > rather than thinking about the general case,
> 
> See: several other cases that should cover most situations
> encountered in practice, and for the parts of the general case that
> aren't worked-out yet, there are the guiding principles.
"guiding" principle"s"

> > which is why my patch makes weaker statements here.  If
> > this issue could have been avoided with a #:make-flag, we would
> > have used that.
> > 
> > More importantly, I fail to see the superiority of your patch here
> > > IIUC neither patch offers a unique solution to the shepherd
> > thing, so
> > the room for debate remains
> 
> In my patch, multiple options remain, but there is no real room for 
> debate.  More concretely:
> 
> > When there is more than one way to do something, choose whichever
> > method is the simplest.  Sometimes this is subjective, which is
> > also fine.
> > What matters is that you use techniques that are common within the
> > community (i.e., patterns that appear throughout
> > @code{gnu/packages/...})
> > and are thus clearly legible for reviewers.
> 
> it is considered subjective, having multiple options if fine!  To 
> correct a packaging following the other option (to help with
> following the policies), you don't have to debate on what's the
> proper option to get a fix in, as both are considered acceptable.  In
> many cases, no need for debate, just pick one and move on.  (Also the
> case for your patch IIUC.)
I do think elevating this into a guiding principle somewhat undermines
what you're stating here though.

> Things are also phrased reconciliatory, e;g.:
> 
> > To make things more concrete and to resolve conflicts between the
> > principles, a few cases have been worked out:
> 
> However, in your patch, there is more opportunity for conflicts.
> E.g.:
> 
> > Each one has its strengths and drawbacks, along with intended
> > andhistorically derived use cases.
> 
> By this line, arguments like 'X was historically the intended purpose
> of Y, using Y for Z is incorrect' become reasonable, which makes 
> disagreements more difficult to resolve (as you know need to consult 
> history and because history is fixed, so no improvements would be 
> possible anymore in areas where there is a historically intended use
> case).
The intended and historically derived use cases are those that ought to
be explicitly named in the following.  If it's not documented, there is
no historic precedent.

> > > > In particular, for review purposes, mine should be easier to
> > > > work with.  For instance, the reviewer sees a hash embedded in
> > > > a snippet, sees in the snippet entry that you shouldn't do
> > > > that, and can thus say "nope, don't do a snippet here".
> 
> That is also the case for problem->solution?  A "sh" -> #$(file-
> append bash-minimal "/bin/sh") substitution can be recognised on
> sight as not possibly being anything else than a technical fix, and
> then the subsubsection on technical fixes mentions that those must be
> done in phases.
> 
> Or, another explanation: reviewers usually have seen a lot of more 
> packages than a new packager.  Because of that, and because they have
> familiarised theirselves with policy, they have over time
> automatically built a mental 'reverse index' of solutions->problems.
I wouldn't put too much value in this experience.  It can also blind
you to the point of producing old-style inputs, for example.

> > 
> > > I think we should optimise for packagers before reviewers
> > Code is more often read than written, so optimising for the reader
> > is optimizing for the packager as well.
> 
> While all reviewers are readers, not all readers are reviewers, and 
> reviewing usually happens only once, maybe twice per patch.
That's not a problem, though? You can take my guidelines, apply it to
any random package in the Guix source and see whether things (still)
fit.  That's more troublesome with the problem->solution style.  In
other words, that the solution passes is apparent from the written form
rather than requiring the process of writing it.

> > > > Regardless of which patch we pick, it should not mandate a
> > > > particular solution in potentially contentious cases,
> > > 
> > > Actually the whole point was to mandate a particular solution for
> > > the
> > > contentious cases, see Shepherd.
> > But I disagree.
> > 
> > Memes aside, please refrain from the "I'm right, do this" style
> > (which is another problem with the "problem-centric" approach),
> 
> Is this referring to the style of the documentation patch?  If so: it
> does explain why the "do this" is right, so I don't see the problem. 
> Do you have a particular point of the documentation in mind you
> consider wrong (*)?
> 
> (*) points I know of:
> * keep lists / remove lists (for the footnote about not-yet-policy on
> removing things in patches)
> * the guiding principles aren't guiding principles
For one, you seriously erred on the test cases thing.  I also wonder if
we're drawing the wrong conclusions for Makefiles from the Shepherd
incident.  Historically, patching build files in a phase has been very
fine.

> > but rather focus on writing a patch that makes reviewers not
> > discard a patch due to formalities.
> 
> I think the patch (v1 and v2) satisfies that.
I'm getting slightly different vibes, but sure.

Cheers

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

[Ludovic Courtès]

Hi!

Thanks for this welcome addition!  Modulo the cosmetic suggestions
below, I think it’s fine.

Maintainers, if you have something to say on the guidelines, now’s the
time!

  https://issues.guix.gnu.org/57598

Maxime Devos <maximedevos@telenet.be> skribis:

> * doc/contributing.texi (Modifying Sources): Replaced with ...
> ("Modifying Sources"): ... this.  List more use cases and some principles.
>
> This patch incorporates some text written by Liliana Marie Prikler.  The
> structure is based on a proposal by Julien Lepiller.  The text has been
> revised based on reviews by David Larsson and blake.
>
> (! remove following text before committing !)

You can write comments below the --- and diffstats.  That way, ‘git am’
will ignore them when applying the patch.

> +Guix has tree main ways of modifying the source code of a package, that
> +you as a packager may use.  These are patches, snippets and phases.
                            ^
“may use: patches, snippets, and build phases.”

> +Each one has its strengths and drawbacks.  To decide between the three,

s/decide between the three/choose among these/

> +there are a few guiding principles to satisfy simultanuously where
> +possible:
> +
> +@itemize
> +@item
> +In principle, Guix only has free software; when the upstream source

s/In principle, Guix only has free software/Every package in Guix is
free software/g

> +community (i.e., patterns that appear throughout @code{gnu/packages/...})

Normally such parenthetical expressions go between em dashes:

  community---i.e., patterns that appear throughout
  @file{gnu/packages}---and …

> +To make things more concrete and to resolve conflicts between the
> +principles, a few cases have been worked out:
> +
> +@subsubsection Removing non-free software
> +Non-free software has to be removed in snippets; the reason is that
> +patches or phases will not work.

You can’t have a colon between the section heading.  Instead, you can
write “a few cases have been worked out and will be illustrated in the
following sections.”

Section titles should be capitalized.

Leave a blank line after the section title.

(Same comment for the sections that come after.)

Instead of “will not work”, which is vague, I’d suggest more explicit
wording: “would not be appropriate because they would expose the
offending code.”

> +For patches, the problem is that a patch removing a non-free file
> +automatically contains the non-free file@footnote{It has been noted that
> +git patches support removing files without including the file in the
> +patch in
> +@url{https://yhetil.org/guix/8b13e899-eb60-490b-a268-639249698c81@@www.fastmail.com/}. If
> +it is verified that the @command{patch} utility supports such patches,
> +this method can be used and this policy adjusted appropriately.}, and we
> +do not want anything non-free in Guix even if only in its patches.

I’d drop the footnote, it’s already dense enough.

> +@code{delete-file-recursively}. There are a few benefits for snippets here:
> +
> +When using snippets, the bundled library does not occur in the source

s/snippets here:/snippets here./
s/When using snippets/First, when using snippets/

> +As such, snippets are recommended here.

s/are recommended here/are the recommended way to delete non-free material/

> +@subsubsection Fixing technical issues (compilation errors, test failures, other bugs ...)

In addition to capitalizing, please remove the parenthetical bit.

> +@subsubsection Adding new functionality
> +To add new functionality, a patch is almost always the most convenient
> +choice of the three -- patches are usually multi-line changes, which are

s/three -- patches/three---patches/

That’s all I have to say!

I think what the text says is fine.  It’s dense and rather long though,
which means that people may overlook things.  OTOH it’s structured so
it’s easier to skim through it, and I wouldn’t know what to remove.

Could you send an updated version?

Thanks,
Ludo’.

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

[Maxime Devos]

[-- Attachment #1.1.1: Type: text/plain, Size: 1118 bytes --]



On 24-09-2022 15:03, Ludovic Courtès wrote:
> Hi!
> 
> Thanks for this welcome addition!  Modulo the cosmetic suggestions
> below, I think it’s fine.
> 
> Maintainers, if you have something to say on the guidelines, now’s the
> time!
> 
>    https://issues.guix.gnu.org/57598
> 
> Maxime Devos <maximedevos@telenet.be> skribis:
> 
>> * doc/contributing.texi (Modifying Sources): Replaced with ...
>> ("Modifying Sources"): ... this.  List more use cases and some principles.
>>
>> This patch incorporates some text written by Liliana Marie Prikler.  The
>> structure is based on a proposal by Julien Lepiller.  The text has been
>> revised based on reviews by David Larsson and blake.
>>
>> (! remove following text before committing !)
> 
> You can write comments below the --- and diffstats.  That way, ‘git am’
> will ignore them when applying the patch.

I know of '---', but AFAICT this is undocumented behaviour, so I'm 
hesitant to rely on that.

 > [...]
> Could you send an updated version?

I intend to do so after bringing antiox to 100%.

Greetings,
Maxime.

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 929 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

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

[Kyle Meyer]

Maxime Devos writes:

> On 24-09-2022 15:03, Ludovic Courtès wrote:
[...]
>> You can write comments below the --- and diffstats.  That way, ‘git am’
>> will ignore them when applying the patch.
>
> I know of '---', but AFAICT this is undocumented behaviour, so I'm 
> hesitant to rely on that.

I don't think you should be worried about Git changing that behavior.
It's used heavily by the Git project itself as well as many other
projects.

Here are some mentions in git.git (4fd6c5e444):

,----[ Documentation/SubmittingPatches ]
| You often want to add additional explanation about the patch,
| other than the commit message itself.  Place such "cover letter"
| material between the three-dash line and the diffstat.  For
| patches requiring multiple iterations of review and discussion,
| an explanation of changes between each iteration can be kept in
| Git-notes and inserted automatically following the three-dash
| line via `git format-patch --notes`.
`----

,----[ Documentation/git-notes.txt ]
| Notes can also be added to patches prepared with `git format-patch` by
| using the `--notes` option. Such notes are added as a patch commentary
| after a three dash separator line.
`----

,----[ Documentation/user-manual.txt ]
| `git format-patch` can include an initial "cover letter". You can insert
| commentary on individual patches after the three dash line which
| `format-patch` places after the commit message but before the patch
| itself.  If you use `git notes` to track your cover letter material,
| `git format-patch --notes` will include the commit's notes in a similar
| manner.
`----

,----[ Documentation/git-format-patch.txt ]
| Typically it will be placed in a MUA's drafts folder, edited to add
| timely commentary that should not go in the changelog after the three
| dashes, and then sent as a message whose body, in our example, starts
| with "arch/arm config files were...".  On the receiving end, readers [...]
`----

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

[Maxim Cournoyer]

Hi,

Ludovic Courtès <ludo@gnu.org> writes:

> Hi!
>
> Thanks for this welcome addition!  Modulo the cosmetic suggestions
> below, I think it’s fine.
>
> Maintainers, if you have something to say on the guidelines, now’s the
> time!
>
>   https://issues.guix.gnu.org/57598

A couple things, on top of things you spot yourself and discussed by
Liliana (although I haven't read the full thread, which is getting
rather long):

1. s/tree/three/

2. I think it'd be nice to mention that that the work of freeing
software should not be done in Guix, but in upstream or if that fails as
a second upstream project.  That's easier to maintain longer term (more
interested parties can share the burden), and friendlier to other FSDG
distributions.  Also, software containing nonfree software is risky: we
have no reasons to trust they won't add more nonfree things to it,
putting our users at risk, so deciding to include such software should
be made carefully.  It may be preferable to simply not include it, if
the risks outweighs the benefits.

3. "guiding principles" sounds a bit too serious/strong to my taste.  I
think it's more guidelines we're discussing.

4.

    The source of the package needs to correspond to what is actually built
    (i.e., act as the corresponding source), to fulfill our ethical and
    legal obligations.

This also seems worded too strongly -- I don't think Guix is bound to
the concept of the "corresponding source", although it is a nice
property.

I find the new text rather long but it does add value, and I wouldn't
know how to structure or improve it much, so I'm in favor of including
it.

Thanks,

Maxim

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

[zimoun]

Hi,

Some work and energy had been put in these patches #57598 [1,2] and they
appear to me a nice improvement.

So what is missing?  Some rewording here or there?  Merge some wording
from one to the other?


1: <https://issues.guix.gnu.org/issue/57598#msgid-1ba11b761cf81336b9788d5f0650b3742eed9d7f>
2: <https://issues.guix.gnu.org/issue/57598#msgid-7b0e5632669defb4d951b1cec54515bacc1c0a89>


Cheers,
simon

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

[Maxime Devos]

[-- Attachment #1.1.1: Type: text/plain, Size: 5128 bytes --]

>> +Guix has tree main ways of modifying the source code of a package, that
>> +you as a packager may use.  These are patches, snippets and phases.
>                             ^
> “may use: patches, snippets, and build phases.”

OK, for the next version of the patch I won't write.

>> +Each one has its strengths and drawbacks.  To decide between the three,
> 
> s/decide between the three/choose among these/

Why?

> Toggle quote (7 lines)
>> +there are a few guiding principles to satisfy simultanuously where
>> +possible:
>> +
>> +@itemize
>> +@item
>> +In principle, Guix only has free software; when the upstream source
> 
> s/In principle, Guix only has free software/Every package in Guix is
> free software/g

This new sentence is most likely false.  There have been packages in 
Guix that were non-free and have been non-free, quite likely there will 
be non-free packages in Guix in the future.  I would rather have that 
Guix doesn't lie about itself.

>> +community (i.e., patterns that appear throughout @code{gnu/packages/...})
> 
> Normally such parenthetical expressions go between em dashes:
> 
>   community---i.e., patterns that appear throughout
>   @file{gnu/packages}---and …

Err, source?  Normally parenthetical expressions go between parentheses. 
  Also, normality is irrelevant.

>> +To make things more concrete and to resolve conflicts between the
>> +principles, a few cases have been worked out:
>> +
>> +@subsubsection Removing non-free software
>> +Non-free software has to be removed in snippets; the reason is that
>> +patches or phases will not work.
> 
> You can’t have a colon between the section heading.  Instead, you can
> write “a few cases have been worked out and will be illustrated in the
> following sections.”

You definitely can have a colon between the section heading, I just did 
so.  Also, that new sentence has a different meaning.

 > Section titles should be capitalized.

The letter R is capitalized.  I disagree that non-first words should be 
capitalized, but I suppose that's a style change that, if done, should 
be separate from this patch.

> Leave a blank line after the section title.

Why?

> Instead of “will not work”, which is vague, I’d suggest more explicit
> wording: “would not be appropriate because they would expose the
> offending code.”

To be more precise:

s/would not be appropriate/would be inappropriate

‘expose code’ and ‘offending code’ seems rather vague to me.  ‘will not 
work’ might be slightly vague, but it's clear from context (i.e., the 
words before the semicolon).  Also, didn't you have complaints about 
denseness?

>> +For patches, the problem is that a patch removing a non-free file
>> +automatically contains the non-free file@footnote{It has been noted that
>> +git patches support removing files without including the file in the
>> +patch in
>> +@url{https://yhetil.org/guix/8b13e899-eb60-490b-a268-639249698c81@@www.fastmail.com/}. If
>> +it is verified that the @command{patch} utility supports such patches,
>> +this method can be used and this policy adjusted appropriately.}, and we
>> +do not want anything non-free in Guix even if only in its patches.
> 
> I’d drop the footnote, it’s already dense enough.

I would rather keep the footnote -- there are no file size concerns for 
info files, it contains useful information, and it's a _footnote_.  One 
of the main benefits of footnotes is that they are out of the way and 
you can just skip them if you don't want to read them?

> s/snippets here:/snippets here./

Err, no? Directly after this sentence I'm saying which the benefits are, 
so this should be ':', not '.' -- ':' is strictly better here.

> s/When using snippets/First, when using snippets/

The next sentences don't do ‘Second, ...’, ‘Third, ...’, so no.

> Toggle quote (2 lines)
>> +As such, snippets are recommended here.
> 
> s/are recommended here/are the recommended way to delete non-free material/

No.  Please see see the subsubsection title:

 > +@subsubsection Removing bundled libraries

It's not merely about non-free material in in bundled libraries, it is 
about all bundled libraries.  It is not about all non-free material, but 
only about material in bundled libraries (unbundled non-free material 
are treated in the previous subsubsection).

As such, the proposed new sentence does not fit the subsubsection.

> Toggle quote (2 lines)
>> +@subsubsection Fixing technical issues (compilation errors, test failures, other bugs ...)
> 
> In addition to capitalizing, please remove the parenthetical bit.

No.  ‘Technical issues’ is vague, the parenthetical bit clarifies 
things.  Besides, in another comment for something else you wanted 
things to be less vague by adding additional text.

 > Could you send an updated version?

No.  The continued misunderstanding elsewhere in the thread and the 
inconsistency and occasional vagueness in arguments is tiresome.

Best regards,
Maxime Devos.

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 929 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

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

[Maxime Devos]

[-- Attachment #1.1.1: Type: text/plain, Size: 1090 bytes --]

> 
>     The source of the package needs to correspond to what is actually built
>     (i.e., act as the corresponding source), to fulfill our ethical and
>     legal obligations.

(Note: this may need to be changed a bit, in the sense of "guix build 
--source=transitive,all" instead  of "guix build --source".)

> This also seems worded too strongly -- I don't think Guix is bound to
> the concept of the "corresponding source", although it is a nice
> property.

As long as copyright law exists in something sufficiently close to its 
current form, as long as Guix contains GPL software and as long as Guix 
intends to be legal, Guix is bound to the concept of corresponding source.

If Guix decides it isn't bound by the concept of corresponding source, 
there is some software in Guix I am a copyright holder of and the GPL 
has a section ‘8.  Termination’.  IIRC, last time I checked, "guix build 
--sources=transitive,all scheme-gnunet guile-fibers" still works, but if 
that functionality is removed without a replacement ...

Best regards,
Maxime Devos

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 929 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]