[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