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

[Maxime Devos]

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.

OpenPGP_0x49E3EE22191725EE.asc
Description: OpenPGP public key

OpenPGP_signature
Description: OpenPGP digital signature