[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