Re: [PATCH] doc: Update contribution guidelines on patches, etc.

[Liliana Marie Prikler]

Am Dienstag, dem 09.08.2022 um 22:30 +0200 schrieb Maxime Devos:
> 
> On 09-08-2022 21:08, Liliana Marie Prikler wrote:
> > [I]t doesn't take multiple lines to embed a store path, the change
> > usually happens on a single line
> I was thinking of files that contain multiple instances of "foo"
> (usually on multiple lines) to be replaced by
> "/gnu/store/.../bin/foo", not files were a "foo
>  bar" was broken over multiple lines and it needs to be replaced by
> "/gnu/store/.../bin/foo
>  bar".
That's a single-line change imho, even if the single line gets repeated
9000 times.

Cheers

Re: [PATCH] doc: Update contribution guidelines on patches, etc.

[Maxime Devos]

On 09-08-2022 21:08, Liliana Marie Prikler wrote:

To be clear, do you mean you:
  * think it's not better, maybe even worse
  * think it's not_much_  better (but still_slightly_  better)
  * are undecided
  * or something else
?
Also, "guix build -S" returns the source code (after snippet / patch,
if any), not its derivation. For the latter: "guix build -S -d"

FWIW I don't think mentioning patch-and-repack is too helpful here
either.  Also, I'd like to use consistent wording at least within this
section, so here "source" means "upstream source" whereas "source
derivation" is a shorthand for the stuff Guix builds.  Yes, the
derivation is not the same thing as the output, but I again fail to see
how being overly precise is helpful.  That being said, I'm open to
suggestions.


I am not reading an answer to my question.

I don't think I've mentioned patch-and-repack (at least not by name, 
which you seem to be referring to?).


I would not recommend "source = upstream source", as the more general 
meaning is used in (guix)Introduction and 
 and elsewhere, otherwise 
terminology would become inconsistent, which can lead to misinterpretations.


I don't think there's such a thing as 'overly precise'.

My suggestion is the same as your suggestion:


You could s/source derivation/the result of
@code{guix build -S}/, but I don't think that's much better.

Greetings,
Maxime.



OpenPGP_0x49E3EE22191725EE.asc
Description: OpenPGP public key


OpenPGP_signature
Description: OpenPGP digital signature

Re: [PATCH] doc: Update contribution guidelines on patches, etc.

[Maxime Devos]

On 09-08-2022 21:08, Liliana Marie Prikler wrote:

On 06-08-2022 08:55, Liliana Marie Prikler wrote:



+If your package has a bug that takes multiple lines to fix,

I don't think this is true for replacing all instances of "foo"
by
"/gnu/store/.../bin/foo" in a file.

Should it?

I don't think so. Directly substituting all the instances instead of
first writing a patch that does "foo" -> "@foo@" or such seems
simpler to me.  This might be a bit too nit-picky though, maybe it's
clear from context that this is not the kind of fix meant by that
line.

I'm struggling to see the issue here.  For starters, it doesn't take
multiple lines to embed a store path, the change usually happens on a
single line.  Of course, translating this into a substitute*, you
expand this single line into multiple ones, but that's not what is
meant here.


I was thinking of files that contain multiple instances of "foo" 
(usually on multiple lines) to be replaced by "/gnu/store/.../bin/foo", 
not files were a "foo
bar" was broken over multiple lines and it needs to be replaced by 
"/gnu/store/.../bin/foo

bar".

At least for the former, I don't think an intermediate "@foo@" is useful 
(with some exceptions, when a pattern match would catch too much).


The latter could in principle happen, but it doesn't seem to happen in 
practice.


Greetings,
Maxime.



OpenPGP_0x49E3EE22191725EE.asc
Description: OpenPGP public key


OpenPGP_signature
Description: OpenPGP digital signature

Re: [PATCH] doc: Update contribution guidelines on patches, etc.

[Liliana Marie Prikler]

Am Dienstag, dem 09.08.2022 um 20:19 +0200 schrieb Maxime Devos:
> > Am Dienstag, dem 09.08.2022 um 18:45 +0200 schrieb Maxime Devos:
> > 
> > > On 06-08-2022 08:55, Liliana Marie Prikler wrote:
> > > 
> > > 
> > > > +If your package has a bug that takes multiple lines to fix,
> > > I don't think this is true for replacing all instances of "foo"
> > > by 
> > > "/gnu/store/.../bin/foo" in a file.
> > Should it?
> I don't think so. Directly substituting all the instances instead of
> first writing a patch that does "foo" -> "@foo@" or such seems
> simpler to me.  This might be a bit too nit-picky though, maybe it's
> clear from context that this is not the kind of fix meant by that
> line.
I'm struggling to see the issue here.  For starters, it doesn't take
multiple lines to embed a store path, the change usually happens on a
single line.  Of course, translating this into a substitute*, you
expand this single line into multiple ones, but that's not what is
meant here.

However, there are exceptions to this rule.  For an example that
requires indirection, see webkitgtk.

> [...]
> > 
> > > > +Such changes include, but are not limited to, fixes of the
> > > > build
> > > > +script(s) or embeddings of store paths (e.g. [...])
> > > > 
> > > [...]
> > Is that how to English comma? Sorry, I'm not a native speaker so I
> > get
> > somewhat weirded out by the when to skip/not to skip rules.
> > 
> Neither am I. English doesn't seem to do "rules" much. I do think,
> however, that adding a comma after "to" makes things a bit simpler to
> read here, and it doesn't appear to be ungrammatical -- at least, in
> licenses "but is/are not limited to" is often used that way.
Fair enough, will add the comma.

> > Derivations are a rather low-level concept, could they be avoided
> > in the origin and phases documentation?
> > I don't quite see how. You could s/source derivation/the result of
> > @code{guix build -S}/, but I don't think that's much better.
> > 
> To be clear, do you mean you:
>  * think it's not better, maybe even worse
>  * think it's not _much_ better (but still _slightly_ better)
>  * are undecided
>  * or something else
> ?
> Also, "guix build -S" returns the source code (after snippet / patch,
> if any), not its derivation. For the latter: "guix build -S -d"
FWIW I don't think mentioning patch-and-repack is too helpful here
either.  Also, I'd like to use consistent wording at least within this
section, so here "source" means "upstream source" whereas "source
derivation" is a shorthand for the stuff Guix builds.  Yes, the
derivation is not the same thing as the output, but I again fail to see
how being overly precise is helpful.  That being said, I'm open to
suggestions.

> > > > +Build phases are limited in that they do not modify the source
> > > > +derivation.  Thus, they are inadequate for changes that are to
> > > > be
> > > > +reflected in the source code.  On the other hand, they only
> > > > cause
> > > > a
> > > > +single rebuild and are thus slightly easier to debug than
> > > > phases
> > > > and
> > > > +snippets.
> > > See Andreas' comment on phase->snippet.
> > > 
> > > Also, do I understand correctly that the argument here is that
> > > 'single rebuild -> less compilation time -> easier to debug'?
> > Easier to debug for the package author currently fiddling with the
> > phase/snippet. Not really a statement in any direction otherwise.
> I don't see how "slightly easier to debug than phases" follows from
> "they cause only a single rebuild". My guess was that the
> intermediate step was lower compilation time, but apparently this was
> not the argument. As such, I'm not following.
I think this follows from your "I only deal with small packages"
experience earlier.  In my experience, the sources that are more likely
to require patches or snippets to fix their... issues... are also the
ones that take larger time to patch and repack.  After all, you don't
have that much overhead unpacking a 7 gig blob that doesn't exist.

Cheers

Re: [PATCH] doc: Update contribution guidelines on patches, etc.

[Maxime Devos]

Am Dienstag, dem 09.08.2022 um 18:45 +0200 schrieb Maxime Devos:

On 06-08-2022 08:55, Liliana Marie Prikler wrote:


+If your package has a bug that takes multiple lines to fix,

I don't think this is true for replacing all instances of "foo" by
"/gnu/store/.../bin/foo" in a file.

Should it?
I don't think so. Directly substituting all the instances instead of 
first writing a patch that does "foo" -> "@foo@" or such seems simpler 
to me.  This might be a bit too nit-picky though, maybe it's clear from 
context that this is not the kind of fix meant by that line.

+ Furthermore, as with patches, modifying the snippets causes two
derivations to be built.

This is true, but I don't think reviewers and package authors have to
worry about that.

It does make a difference to the author when debugging their package.
Starting with a phase and then moving it to a snippet can save good
time.
Hm, maybe, I guess I often work on 'small' packages where it doesn't 
matter much.


On 09-08-2022 19:05, Liliana Marie Prikler wrote:

+Such changes include, but are not limited to, fixes of the build
+script(s) or embeddings of store paths (e.g. [...])


[...]

Is that how to English comma?  Sorry, I'm not a native speaker so I get
somewhat weirded out by the when to skip/not to skip rules.

Neither am I. English doesn't seem to do "rules" much. I do think, 
however, that adding a comma after "to" makes things a bit simpler to 
read here, and it doesn't appear to be ungrammatical -- at least, in 
licenses "but is/are not limited to" is often used that way.



Derivations are a rather low-level concept, could they be avoided in
the origin and phases documentation?
I don't quite see how.  You could s/source derivation/the result of
@code{guix build -S}/, but I don't think that's much better.


To be clear, do you mean you:

 * think it's not better, maybe even worse
 * think it's not _much_ better (but still _slightly_ better)
 * are undecided
 * or something else

?

Also, "guix build -S" returns the source code (after snippet / patch, if 
any), not its derivation. For the latter: "guix build -S -d"



+Build phases are limited in that they do not modify the source
+derivation.  Thus, they are inadequate for changes that are to be
+reflected in the source code.  On the other hand, they only cause
a
+single rebuild and are thus slightly easier to debug than phases
and
+snippets.

See Andreas' comment on phase->snippet.

Also, do I understand correctly that the argument here is that
'single rebuild -> less compilation time -> easier to debug'?

Easier to debug for the package author currently fiddling with the
phase/snippet.  Not really a statement in any direction otherwise.
I don't see how "slightly easier to debug than phases" follows from 
"they cause only a single rebuild". My guess was that the intermediate 
step was lower compilation time, but apparently this was not the 
argument. As such, I'm not following.


Greetings,
Maxime.



OpenPGP_0x49E3EE22191725EE.asc
Description: OpenPGP public key


OpenPGP_signature
Description: OpenPGP digital signature

Re: [PATCH] doc: Update contribution guidelines on patches, etc.

[Liliana Marie Prikler]

Am Dienstag, dem 09.08.2022 um 18:45 +0200 schrieb Maxime Devos:
> On 06-08-2022 08:55, Liliana Marie Prikler wrote:
> 
> > +If your package has a bug that takes multiple lines to fix,
> I don't think this is true for replacing all instances of "foo" by 
> "/gnu/store/.../bin/foo" in a file.
Should it?

> >   or a fix
> > +has already been accepted upstream, patches are the preferred way
> > of
> > +eliminating said bug
> > +Refer to the @code{origin} record documentation
> > +(particularly the fields @code{snippet} and @code{modules}), for
> > more
> > +information (@pxref{origin Reference}).
> > +
> 
> The "Refer to the ... documentation for more information" occurred in
> the old version of (guix)Snippets versus Phases. However, back then,
> I did not find more information on how to decide between snippets,
> patches and phases, and neither do I now.
> 
> Maybe:
> 
> +Refer to the @code{origin} record documentation
> +(@pxref{origin Reference}) (particularly the fields @code{snippet}
> and @code{modules})
> +for more information on how to use snippets
> 
> , to avoid a reader's assumption that that section contains
> information on deciding between snippets, phases and patches.
Yeah, this was meant for "how to use".

> > + Furthermore, as with patches, modifying the snippets causes two
> > derivations to be built.
> 
> This is true, but I don't think reviewers and package authors have to
> worry about that.
It does make a difference to the author when debugging their package. 
Starting with a phase and then moving it to a snippet can save good
time.

> > Such changes include, but are not limited to fixes of the
> > +build script(s) or embeddings of store paths (e.g. replacement of
> > +@file{/bin/sh} with @code{(search-input-file inputs "bin/sh")}).
> Include what? I think you need to close the subsentence here:
> 
> > +Such changes include, but are not limited to, fixes of the build
> > +script(s) or embeddings of store paths (e.g. [...])
> > 
> 
> [...]
Is that how to English comma?  Sorry, I'm not a native speaker so I get
somewhat weirded out by the when to skip/not to skip rules.

> > +Build phases are limited in that they do not modify the source
> > +derivation.  Thus, they are inadequate for changes that are to be
> > +reflected in the source code.  On the other hand, they only cause
> > a
> > +single rebuild and are thus slightly easier to debug than phases
> > and
> > +snippets.
> Derivations are a rather low-level concept, could they be avoided in
> the origin and phases documentation?
I don't quite see how.  You could s/source derivation/the result of
@code{guix build -S}/, but I don't think that's much better.

> > +Build phases are limited in that they do not modify the source
> > +derivation.  Thus, they are inadequate for changes that are to be
> > +reflected in the source code.  On the other hand, they only cause
> > a
> > +single rebuild and are thus slightly easier to debug than phases
> > and
> > +snippets.
> See Andreas' comment on phase->snippet.
> 
> Also, do I understand correctly that the argument here is that
> 'single rebuild -> less compilation time -> easier to debug'?
Easier to debug for the package author currently fiddling with the
phase/snippet.  Not really a statement in any direction otherwise.

Cheers

Re: [PATCH] doc: Update contribution guidelines on patches, etc.

[Maxime Devos]

On 06-08-2022 08:55, Liliana Marie Prikler wrote:


+If your package has a bug that takes multiple lines to fix,
I don't think this is true for replacing all instances of "foo" by 
"/gnu/store/.../bin/foo" in a file.

  or a fix
+has already been accepted upstream, patches are the preferred way of
+eliminating said bug
+Refer to the @code{origin} record documentation
+(particularly the fields @code{snippet} and @code{modules}), for more
+information (@pxref{origin Reference}).
+


The "Refer to the ... documentation for more information" occurred in 
the old version of (guix)Snippets versus Phases. However, back then, I 
did not find more information on how to decide between snippets, patches 
and phases, and neither do I now.


Maybe:

+Refer to the @code{origin} record documentation
+(@pxref{origin Reference}) (particularly the fields @code{snippet} and 
@code{modules})
+for more information on how to use snippets

, to avoid a reader's assumption that that section contains information 
on deciding between snippets, phases and patches.



+ Furthermore, as with patches, modifying the snippets causes two derivations 
to be built.


This is true, but I don't think reviewers and package authors have to 
worry about that.



Such changes include, but are not limited to fixes of the
+build script(s) or embeddings of store paths (e.g. replacement of
+@file{/bin/sh} with @code{(search-input-file inputs "bin/sh")}).

Include what? I think you need to close the subsentence here:


+Such changes include, but are not limited to, fixes of the build
+script(s) or embeddings of store paths (e.g. [...])



[...]


+Build phases are limited in that they do not modify the source
+derivation.  Thus, they are inadequate for changes that are to be
+reflected in the source code.  On the other hand, they only cause a
+single rebuild and are thus slightly easier to debug than phases and
+snippets.
Derivations are a rather low-level concept, could they be avoided in the 
origin and phases documentation?



+Build phases are limited in that they do not modify the source
+derivation.  Thus, they are inadequate for changes that are to be
+reflected in the source code.  On the other hand, they only cause a
+single rebuild and are thus slightly easier to debug than phases and
+snippets.

See Andreas' comment on phase->snippet.

Also, do I understand correctly that the argument here is that 'single 
rebuild -> less compilation time -> easier to debug'?


Greetings,
Maxime.



OpenPGP_0x49E3EE22191725EE.asc
Description: OpenPGP public key


OpenPGP_signature
Description: OpenPGP digital signature