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

[Maxime Devos]

On 09-09-2022 10:04, Liliana Marie Prikler wrote:
> Am Donnerstag, dem 08.09.2022 um 13:12 +0200 schrieb Maxime Devos:
> > On 07-09-2022 10:09, Liliana Marie Prikler wrote:
> > > Am Dienstag, dem 06.09.2022 um 22:21 +0200 schrieb Maxime Devos:
> > > > 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".
> > What I also meant is, that to my knowledge there is no soft rule
> > either.  Again, why should we avoid this, what's the point of that?
> 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, whichisy

Sentence might have been truncated? 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.

> > 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.

> > > 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 [...])
      [...]))


> > > 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.

 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 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,
> > This has nothing to do with length and remembering, but rather with
> > explaining why a phase must be used -- to explain that, I state which
> > principle applies (as mentioned previously). If I removed the
> > explanations, I would just be stating how to do things, without
> > giving a logical reasoning on the 'why'.
> 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 [...]’.

> > > 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.
> > I consider it more natural to have the 'guiding principles' _before_
> > the concrete cases, as they are meant to be 'guiding' and
> > 'principles'.
> > It's like 'starting from first principles', there introducing the
> > first principles as you go is ad-hoc.
> > The guiding principles also need to be outside the examples, in case
> >   one of the examples doesn't apply to the packager's use case, such
> >   that they can fall-back to the guiding principles.
> > Also, in your patch you are dividing things in subsubsections as
> > well, just under a different name and different representation (table
> > entries in a subsection), as mentioned previously.
> A table entry is not a subsection, as much as you want it to be that.

It indeed is not, rather they are equivalent here in terms of structure, 
nesting and problematicness.

> 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.

> 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:

* 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 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'.

> > > 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.
> > The "only free software" is mentioned elsewhere, yes, but it is one
> > of the principles for deciding between snippets, phases and patches.
> > While you call it a no-brainer, it is sometimes neglected, so it
> > sounds important to me to explicitly list it.
> > Merging the 3th and 4th @item, I count 4 principles, so it fits with
> > an enumeration.
> > Also, I'm not following your point here -- your complaint was that
> > they aren't guiding principles (based on the number of them), but
> > your response is that they might not form an enumeration?  They are
> > named the guiding principles, not the guiding enumeration.  What have
> > enumerations to do with anything?
> 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.

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.

> 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.

> > > > 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.
> > (define cached-guiding-principles
> >    (delay (list (guiding-principle-0)
> >                      [...]
> >                      (guiding-principle-24))))
> > Caching the guiding principles does not reduce the length.
> > I don't see the problem with calling back to earlier information.
> > Also, it isn't earlier information, there is no nice list of guiding
> > principles anywhere else.
> 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'.

> > > > > 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.
> > 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.

> > > > 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.
> > 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/c4c0a071-2e03-d4d6-6718-05424d21d146@telenet.be/
* 
https://yhetil.org/guix-devel/84e13ef7d437062df5cca51a12e6da54929e0176.camel@telenet.be/

Not solving the problem defeats the whole point, as the purpose is to 
solve that problem.

> > > 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.
> > Also already the case.
> > > 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".
> > A problem->solution structure is useful for that too?
> > And it already lists all the solutions (snippets, phases and patches)
> > and information to decide whether the solution fits their problem
> > (the guiding principles, and the worked-out cases).
> Again, I believe you're overselling the guiding principles.

I never claimed they were super great, just that they are convenient and 
solved a number of problems.  I'm not seeing any overselling here.

> > > 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 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?)

> 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.

> > > 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.

> > 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’.

> 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
> 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.

> 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?

Greetings,
Maxime

OpenPGP_0x49E3EE22191725EE.asc
Description: OpenPGP public key

OpenPGP_signature
Description: OpenPGP digital signature