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

From: Liliana Marie Prikler <[email-masked]>
Date: Fri, 09 Sep 2022 22:09:18 +0200

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/[email-masked]/
> > 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