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

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

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/[email-masked]/
> * 
> https://yhetil.org/guix-devel/[email-masked]/
> 
> 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