Hi Maxime,

Adding some basic grammatical corrections below:

August 5, 2022 1:59 PM, "Maxime Devos" <maximedevos@telenet.be> wrote:


Technical grammatical correction: the software that Guix "has" is that in the monorepo,
but it "distributes" many packages. Thus:
--8<---------------cut here---------------start------------->8---
* In principle, Guix only distributes free software; when the upstream source contains some
non-free software, it should be removed such that ‘guix build --source’ returns the "freed"
source code rather than the unmodified upstream source (see: 28.4.1 Software Freedom).
--8<---------------cut here---------------end--------------->8---

I consider the difference between referring to external source code by including a (snippet-sanitised) copy or downloading it from an URL + snippet-sanitising to be immaterial, except for space and I/O savings, so I consider "has" to include "distributes".

While "distributes" is more specific, I really meant "has" here -- gnu/packages/patches/... and gnu/packages/*.scm must be free too, even if it is was not a distributed package (more concretely, see the bits about patches failing to remove non-freeness).

[...]

* The source of the package needs to correspond to what is actually built (i.e., act as the
corresponding source), to fulfill our ethical and legal obligations.

The [i.e.] addendum above is redundant, its better worded as:
--8<---------------cut here---------------start------------->8---
* The source of a package must correspond to what is actually built (i.e., there must be
an explicit relation between source code and the result of its build for all builds),
to fulfill our ethical and legal obligations.
--8<---------------cut here---------------end--------------->8---

You write that the addendum is redundant, but then change the addendum by replacing a word in the addendum by a possible definition. I'm not following how that reduces redundancy, and it also appears to be contrary to the lack of verbosity that Andreas would like.

* It is convenient for the source derived from an origin to build on any system that the upstream
package supports.
* The source needs to actually work, not only on your Guix system but also for other systems; this
requires some care for substitutions involving store items and other architecture-specific changes.

* Sometimes, there is more than one way to do it. Let's go for the simplest one. Sometimes, which
tool is the simplest, is subjective, that's fine too.

I think would be more clearly worded as:
--8<---------------cut here---------------start------------->8---
* When presented with a variety of strategies for defining a package, choose whichever is simplest.
Sometimes this is subjective, which is also fine. What matters is that you prefer techniques that
are common within the community (i.e. patterns that appear throughout gnu/packages/...) and
are thus clearly legible for reviewers.
--8<---------------cut here---------------end--------------->8---

To be clear, this is to replace the third point (Sometimes, there's more than one way to do it, etc.), without removing the previous two?

I would not use combinations like 'presented with a variety of strategies' however, I don't think that leads to clarity. Also, the preferences of 'you' are not all that important here, it's what they choose for that's important even if it is not their preference. (Though the two coinciding would be ideal of course!) Maybe:

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 gnu/packages/...) and
are thus clearly legible for reviewers.

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

To a newcomer (the target audience), the above may lead to confusion as to what wasn't already 
concrete in the above descriptions, or what principles above come into conflict. There is a mild,
latent assumption that they are familiar with the Guix workflow, which should be avoided. Thus I 
suggest:
--8<---------------cut here---------------start------------->8---
For the purpose of clarifying preferred practices and reducing friction in the review process
introduced by subjective variation, a few guidelines have been worked out:
--8<---------------cut here---------------end--------------->8---

I don't see how a fancy wording amounting to essentially the same thing would reduce confusion or avoid latent assumptions.

Anyway, I'm not seeing any assumption that they are already familiar with the Guix workflow -- the point of such sections is to explain the workflow, people already familiar aren't expected to read it (at least, not often). I don't see how it would lead to confusion, because it doesn't allude to any principles in particular, because the text isn't some kind of analysis on principles and their interactions but a howto of sorts, and because it points to the cases below.

20.4.5.1 Removing non-free software. Non-free software has to be removed in a snippet; the reason is
that a patch or phase will not work.

Well, it might work on their machine, but not for community standards. To reduce confusion:
--8<---------------cut here---------------start------------->8---
20.4.5.1 Removing non-free software.
Non-free software should be removed using snippets; when removing non-free software, a patch or phase
will not be accepted.
--8<---------------cut here---------------end--------------->8---

The purpose of the hypothetical patch or phase is to remove the non-freeness. As such, if the non-freeness wasn't removed, the patch or phase did not work, for the local meaning of "working".  This is machine-independent (so no "it might work on their machine"), unless there is some kind of non-determinism, but I haven't ever noticed that happening for non-freeness removal code.

For a patch, the problem is that a patch removing a non-free file automatically contains the
non-free file (^), and we do not want anything non-free to appear in Guix even if only in its
patches.

Is better as:
--8<---------------cut here---------------start------------->8---
For patches, the issue is that a patch that removes a non-free file automatically contains the
non-free file (^), and we do not want any non-free software to be distributed with Guix, even 
if it only appears within the context of a patch.

Concerning phases, the problem is that they do not influence the result of ‘guix build --source’.
--8<---------------cut here---------------end--------------->8---

Why the change:

• singular->plural (for: patch) -- is this to reduce the wordcount (avoid an 'a'), or for consistency, or ...?
  
• passive->active (for: removing -> that removes) -- it becomes more wordy, so seems harder to follow for me
• appear->be distributed -- more wordy, also things parts of Guix itself  (and hence not distributed, except for "guix build guix" and "guix pull") should be free so I don't think being more specific makes things more precise.
• "only in its patches" -> "if it only appears within the context of a patch" -- more wordy, and more indirection
• "For a phases" -> "Concerning phases"  -- slightly less direct

20.4.5.2 Removing bundled libraries.

Bundled libraries should not be removed with a patch, because then the patch would contain the full
bundled library, which can be large. They can be removed either in a snippet or a phase, often
using the procedure 'delete-file-recursively'. There are a few benefits for snippets here:

When using snippets, the bundled library does not occur in the source returned by ‘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 ‘guix build --source’, especially if the
bundled library is not actually used on Guix systems. (*)

As such, snippets are recommended here.

(*) This is _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.

20.4.5.3 Fixing technical issues (compilation errors, test failures, other bugs ...)

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 '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 choosing between a patch or a snippet is a matter of
convenience. However, there are two things to keep in mind:

First, when the fix is not Guix-specific, it is strongly desired to upstream the fix to avoid the
additional maintenance cost to Guix. As upstreams cannot accept a snippet, writing a patch can be a
more efficient use of time. Secondly, if the fix of a technical issue embeds a store file name,
then it has to be a phase. Otherwise, if a store file name was embedded in the source, the result
of 'guix build --source' would be unusable on non-Guix systems and likely also unusable on Guix
systems of another architecture.

And some basic last corrections here:
--8<---------------cut here---------------start------------->8---
First, when the fix is not Guix-specific, it is strongly desired by upstream that the fix avoids any
additional maintenance costs for Guix.

While I would like upstreams to care about additional maintenance costs downstream, I do not think we have proof of that at the moment, so I would stick with the meaning of the original sentence -- I used 'upstream' as a verb here, not as a noun.

However, I could look into reformulating the sentence a bit to avoid reading it as a noun and hence avoid backtracing when parsing that sentence?

 As upstream cannot accept a snippet, writing a patch can be a

more efficient use of time. 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 to be embedded in the source,

Right, I already introduces a store file name, and that "otherwise" refers to the same store file name.

"the store file name" is singular, so ^W^W -- nevermind, that's a subjunctive? I would go for the simpler "i fthe store file name were embedded in the source", dropping the 'to be' indirection.

Question: do you know how to decide between "if X were to be embedded"/"if X were embedded" and "if X was embedded"?

 the
result of 'guix build --source' would be unusable on non-Guix systems, and also likely unusable on
Guix systems of another architecture.
--8<---------------cut here---------------end--------------->8---

The Oxford comma or lack thereof are acceptable stylistic conventions, but not a grammatical error. There are only two conjucts here though, so I don't think the Oxford comma applies here. Additionally, the two conjucts are about mostly the same thing, but just different (similar!) subcases that merely happen to need different qualifications (unusable on non-Guix vs. likely unusable on different-arch Guix), so I don't think that 'also' brings something here.