Re: v2: A proposal of a consistent set of clear rules and guidelines involving snippets, phases and patches.

[Maxime Devos <maximedevos@telenet.be>]

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


On 05-08-2022 18:59, blake@reproduciblemedia.com wrote:
> Hi Maxime,
>
> Adding some basic grammatical corrections below:
I have read several proposed corrections, but most of them don't appear 
to be grammatical corrections but rather stylistic or about 
(non-grammar) clarity or contents.
> 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
There's more than one upstream as Guix has multiple packages, so the 
plural "upstreams" seems more correct to me, though I wouldn't consider 
the singular to be incorrect.  I have however, in some other places used 
singular for something of which there exist multiple instances, so I'll 
look at making the wording consistent one way or the other.
> 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.

Greetings,
Maxime.

[-- Attachment #1.1.1.2: Type: text/html, Size: 16241 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 --]