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

[Maxime Devos <maximedevos@telenet.be>]

[-- Attachment #1.1.1.1: Type: text/plain, Size: 18302 bytes --]

On 07-09-2022 10:09, Liliana Marie Prikler wrote:

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

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?

>>>> +@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".
How does ignoring a test fix the technical issue identified by the test
(sometimes, the technical issue being a bug in the test itself)?
>>> 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.

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.

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

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?

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

>>>> +@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."
It loses some information (that patches are preferred) and
(after re-adding the conclusion) is more verbose, which appears
to be considered very important.
>>> [...]
>>> 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.
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?

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

Do you have anything in particular in mind?

>> 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.
What particular things do you have in mind, and where do you
think they should better be documented?  I can move things
around a bit and add cross-references.
>> 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.

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

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

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

  * ‘To make things more concrete and to resolve conflicts between the
    principles, a _few_ cases have been worked out:’

    (Emphasis on _few_ added)
  * ‘When there is more than one way to do something, choose whichever
    method
    is the simplest’

    (It says ‘simplest’ instead of ‘most convenient’, but whatever.)

>    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. E.g.,

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

  * ‘For phases, the problem is that phases do not influence the result of
    @command{guix build --source}.’

> 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.  And as tools
for deciding between the three for not-yet-documented cases, there are
the guiding principles.  As a demonstration on how to use these guiding
principles, various cases have been worked out based on the guiding
principles.

Summarised, it gives both the tools _and_ the solutions.

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.

Greetings,
Maxime

[-- Attachment #1.1.1.2: Type: text/html, Size: 26582 bytes --]

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 929 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]