Re: Guix Days: Patch flow discussion

Re: Guix Days: Patch flow discussion

[Clément Lassieur]

On Tue, Feb 06 2024, Suhail wrote:

> Steve George <steve@futurile.net> writes:
>
>> elsewhere in the thread someone mentions some tags we could use
>> consistently so maintainers can find patches that have been reviewed
>> easily.
>
> It seems on the [dev manual] we already have "reviewed-looks-good"
> documented.  Thus, I'd like to propose the below *mutually exclusive*
> Debbugs tag set:
>
> - "not-yet-reviewed" :: automatically set for all submissions
> - "reviewed-needs-fix" :: set explicitly by the reviewer
> - "needs-another-review" :: automatically set if there's a revised
>   patch, unless "not-yet-reviewed" (in which case no change)
> - "reviewed-looks-good" :: set explicitly by the reviewer

Would it makes sense to have a "does-not-apply" tag too?

I believe that would help sorting old those old patches that don't apply
anymore.

> In addition to the above, it might also help for there to be an
> additional tag of "might-not-need-review" (or simpler,
> "review-not-needed") which gets automatically set, provided we implement
> a way to label some changes (for some packages) as being "trivial enough
> that they're okay as long as build succeeds".
>
> On a related note, is it possible for a reviewer who isn't a committer
> to set debbugs tags?
>
> [dev manual]: <https://guix.gnu.org/en/manual/devel/en/html_node/Debbugs-Usertags.html>
>
>> It would be great to agree those - try them for a bit - and document
>> them in a 'howto' so that everyone uses the same process.
>
> In addition to documenting the tags in the "Debbugs Usertags" section of
> the manual, it would help for there to be a "howto" which focuses more
> on the transition between the tags (i.e., the contribution workflow).

Re: Guix Days: Patch flow discussion

[Andreas Enge]

Am Fri, Feb 16, 2024 at 11:56:50AM +0100 schrieb Clément Lassieur:
> Would it makes sense to have a "does-not-apply" tag too?

Should this not appear in the QA page, assuming that once all the new
issues are closed, older ones will bubble to the top and be treated by QA?
(I am not sure if just looking at the n latests issues is how QA works,
but I think so.)

Andreas

Re: Guix Days: Patch flow discussion

[Clément Lassieur]

On Fri, Feb 16 2024, Andreas Enge wrote:

> Am Fri, Feb 16, 2024 at 11:56:50AM +0100 schrieb Clément Lassieur:
>> Would it makes sense to have a "does-not-apply" tag too?
>
> Should this not appear in the QA page, assuming that once all the new
> issues are closed, older ones will bubble to the top and be treated by QA?
> (I am not sure if just looking at the n latests issues is how QA works,
> but I think so.)

I think it would be useful (at least for me) if I browse a list of
patches that I want to review, to document why a review can't be done.

Also, so that other people won't try to apply it.

It would be great that QA does that job, I imagine when it does it it
can also add that tag.

Re: Guix Days: Patch flow discussion

[Christopher Baines]

[-- Attachment #1: Type: text/plain, Size: 1112 bytes --]


Clément Lassieur <clement@lassieur.org> writes:

> On Fri, Feb 16 2024, Andreas Enge wrote:
>
>> Am Fri, Feb 16, 2024 at 11:56:50AM +0100 schrieb Clément Lassieur:
>>> Would it makes sense to have a "does-not-apply" tag too?
>>
>> Should this not appear in the QA page, assuming that once all the new
>> issues are closed, older ones will bubble to the top and be treated by QA?
>> (I am not sure if just looking at the n latests issues is how QA works,
>> but I think so.)
>
> I think it would be useful (at least for me) if I browse a list of
> patches that I want to review, to document why a review can't be done.
>
> Also, so that other people won't try to apply it.
>
> It would be great that QA does that job, I imagine when it does it it
> can also add that tag.

There's the filter issues form which allows finding patches which can't
be applied:

  https://qa.guix.gnu.org/patches?failed-to-apply-patches=on

I think QA tagging issues might be useful for people to benefit from
that data in other places, and should be possible, it just needs QA to
send emails I think.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 987 bytes --]

Re: Guix Days: Patch flow discussion

[Suhail]

Clément Lassieur <clement@lassieur.org> writes:

>> It seems on the [dev manual] we already have "reviewed-looks-good"
>> documented.  Thus, I'd like to propose the below *mutually exclusive*
>> Debbugs tag set:
>>
>> - "not-yet-reviewed" :: automatically set for all submissions
>> - "reviewed-needs-fix" :: set explicitly by the reviewer
>> - "needs-another-review" :: automatically set if there's a revised
>>   patch, unless "not-yet-reviewed" (in which case no change)
>> - "reviewed-looks-good" :: set explicitly by the reviewer
>
> Would it makes sense to have a "does-not-apply" tag too?
>
> I believe that would help sorting old those old patches that don't apply
> anymore.

Agreed.  A "does-not-apply" tag would be helpful.  This tag would be
explicitly applied, either by the QA mechanism or a reviewer.

Assuming there aren't any objections to these tags, what are the next
steps?  If my understanding of Debbugs Usertags is correct it seems we
can simply start using them?  Though noting the above in the manual
would be helpful.  However, that still leaves open the issue of how the
automated setting of tags is accomplished.

Thoughts?

-- 
Suhail

Re: Guix Days: Patch flow discussion

[Suhail]

Steve George <steve@futurile.net> writes:

> The general opinion seemed to be, that it was better to fix small
> issues and commit the change for new users, so they had the
> satisfaction of their contribution making it into the repository. One
> proposal was to do the 'fix', and to then reply back to the bug with a
> diff - showing what was done.

I see.  Thank you for sharing.  I believe being more didactic with "new
users" would be good for the community.

-- 
Suhail

Re: Guix Days: Patch flow discussion

[Suhail]

Steve George <steve@futurile.net> writes:

> elsewhere in the thread someone mentions some tags we could use
> consistently so maintainers can find patches that have been reviewed
> easily.

It seems on the [dev manual] we already have "reviewed-looks-good"
documented.  Thus, I'd like to propose the below *mutually exclusive*
Debbugs tag set:

- "not-yet-reviewed" :: automatically set for all submissions
- "reviewed-needs-fix" :: set explicitly by the reviewer
- "needs-another-review" :: automatically set if there's a revised
  patch, unless "not-yet-reviewed" (in which case no change)
- "reviewed-looks-good" :: set explicitly by the reviewer

In addition to the above, it might also help for there to be an
additional tag of "might-not-need-review" (or simpler,
"review-not-needed") which gets automatically set, provided we implement
a way to label some changes (for some packages) as being "trivial enough
that they're okay as long as build succeeds".

On a related note, is it possible for a reviewer who isn't a committer
to set debbugs tags?

[dev manual]: <https://guix.gnu.org/en/manual/devel/en/html_node/Debbugs-Usertags.html>

> It would be great to agree those - try them for a bit - and document
> them in a 'howto' so that everyone uses the same process.

In addition to documenting the tags in the "Debbugs Usertags" section of
the manual, it would help for there to be a "howto" which focuses more
on the transition between the tags (i.e., the contribution workflow).

-- 
Suhail

Re: Guix Days: Patch flow discussion

[Felix Lechner via]

Hi Suhail,

On Mon, Feb 05 2024, suhail@bayesians.ca wrote:

> Felix Lechner via <help-guix@gnu.org> writes:
>
> Is your position that

First off, I'm sorry I write so much today. For a project the size of
Guix, it's not good for one person to belabor a point repeatedly. I am
responding to your request for a clarification.

> 1. this never happens today and thus, should happen some times when
>    warranted.  Or that,
>
> 2. it happens far too rarely today, and should happen more often. Or
>    that,
>
> 3. committers should never ask for revisions?

None of the above. I believe committers should commit only changes they
are comfortable with. Otherwise, they should leave a bug to folks with
more experience, perhaps tagging it along the way.

I think there should be a hundred people who do nothing but add new
packages. They would free up valuable capacity for the people
experienced with build systems, Guix services and their configuration,
the user interface, the daemon, cross-compilation and other more
difficult speciality areas.

Kind regards
Felix

Re: Guix Days: Patch flow discussion

[Suhail]

Felix Lechner via <help-guix@gnu.org> writes:

> Another is that committers should commit what they think is right
> rather than ask for revised patches.

I could be mistaken, but I believe this does happen today at least some
of the time.  Is your position that

1. this never happens today and thus, should happen some times when
   warranted.  Or that,

2. it happens far too rarely today, and should happen more often. Or
   that,

3. committers should never ask for revisions?

-- 
Suhail

Re: Guix Days: Patch flow discussion

[Steve George]

Hi Suhail,

On  5 Feb, Suhail wrote:
> Felix Lechner via <help-guix@gnu.org> writes:
> 
> > Another is that committers should commit what they think is right
> > rather than ask for revised patches.
> 
> I could be mistaken, but I believe this does happen today at least some
> of the time.  Is your position that
> 
> 1. this never happens today and thus, should happen some times when
>    warranted.  Or that,
> 
> 2. it happens far too rarely today, and should happen more often. Or
>    that,
> 
> 3. committers should never ask for revisions?
(...)

Just to reply to this part, there were a variety of views on what the correct 
balance was between teaching someone and fixing issues so that a contributors 
patch was applied without them having to make further efforts. The general 
opinion seemed to be, that it was better to fix small issues and commit the 
change for new users, so they had the satisfaction of their contribution making 
it into the repository. One proposal was to do the 'fix', and to then reply 
back to the bug with a diff - showing what was done.

Steve

Guix Days: Patch flow discussion

[Steve George]

Hi,

Our goal for the discussion:

	How do we double the number of patches that are *reviewed* and
	*applied* to Guix in the next six months?

Patch flow is a pipeline, to change it we could:

a. Increase the number of committers - more people to do the
work
b. Increase the efficiency of existing committers
c. Open the gates by decreasing the quality expected from patches

We essentially decided to focus our discussion on (b). We looked at
things that 'hinder' and 'help' patch review:


Hinders
========

- All our patch reviewers are volunteers doing it in their spare time.

- For a volunteer reviewing someone else's work is not very rewarding, 
most would prefer to use that precious time to scratch their own itch.

- Can feel like an Sisyphean task: no matter how many patches someone 
reviews there are more, exacerbated by the number of Guix packages.

- Sense of responsibility: the minute that a reviewer looks at the patch 
they are now stuck with it

- Repetitive and boring: often patches have minor issues, but it's the 
same sorts of issues time and time again.

- Risk of negative social interaction: having to tell someone that their 
patch is incorrect, or that their contribution cannot be used is 
difficult and draining. Some people felt it was better to say nothing, 
rather than to respond to a patch.


Helps
======

This led us to the focus on the fact that **reviewing and applying
patches can be different people**

We looked for ideas to create more reviewers, make reviewing easier and
more fun:


- Share in the work
--------------------

1. encourage new reviewers to step forward - making it more known that 
reviewing patches helps to get them applied. Anyone can review patches.

2. create directed 'how-to' documentation for reviewing and connect it 
to QA so that 'new reviewers' know what to do

3. create documentation about 'when' and 'how' it's appropriate to send 
a 'v2' version of a patch so that the QA system builds and accepts it. 
Sometimes, patches rot because non-committers don't want to be seen as 
'stealing' someone's work with a v2 patch - but making the small changes 
and resubmitting to QA is what is required.

4. Pay someone else to do it. Noted but out of scope.
5. Remove old packages overhead. Old untouched packages create mental 
overhead, and make the task of maintaining the repository in a good 
state more difficult. We could remove old 'untouched' packages and ones 
that no-longer compile. We have methods to hide and notify.


- Make it more fun
-------------------

1. do online sessions around reviews, some sprints or pairing - both 
social and a way to spread skills
2. find ways to recognise and appreciate reviewers - 'reviewer of the month'
3. make it a game - we could have a 'Guix London' vs 'Guix Paris' leader 
board for reviews. Make it a group goal 'can we beat januarys reviews 
number'
4. create some graphs / leaderboard so we know how many patches are 
being reviewed and we can recognise the contribution


- Automate it away
-------------------

1. Chris is continuing to try and automate away the boring work - 
general agreement in the group that QA has made a lot of difference.

2. general discussion about create a 'guix review' command (Nix has one) 
which would download a branch with the appropriate patch and build it 
locally. This is for instances where some adjustment is needed or to 
check a build. While this can be done today, it's a number of steps and 
quite involved.


Agreed Actions
==============

* [Chris]: continuing his work to improve QA automation. Implication was 
we'll need some reports / graphs - but these were not discussed in detail.

* [Futurile]: organise a **patch review online sessions**. To run every 
13 days (so it rotates through the week) - for 3 months to see if it has 
any traction. Co-ordinate with maintainers so that patches that are 
reviewed can be committed


Actions looking for someone - you?
====================================

* Carry forward the 'guix review' command idea

* Write an RFC and discuss the idea of removing older 'bit-rot' packages

* write 'how-to' documentation for reviews and when it's socially
acceptable to do a v2 patch. A checklist-like approach.


If you were in the discussion and I've misrepresented your point, or 
forgotten an important aspect please please reply and correct me.

Also, if you would like to help on any of the tasks please email back to 
the group so we can all co-ordinate.

Finally, thank-you to everyone who came along and put their shared brain 
power to the task - look forward to doing some patch reviews together 
online in the coming weeks!

Thanks,

Steve/Futurile

Re: Guix Days: Patch flow discussion

[Clément Lassieur]

Hello,

On Mon, Feb 05 2024, Steve George wrote:

> Hi,
>
> Our goal for the discussion:
>
> 	How do we double the number of patches that are *reviewed* and
> 	*applied* to Guix in the next six months?
>
> Patch flow is a pipeline, to change it we could:
>
> a. Increase the number of committers - more people to do the
> work

I don't think reviewers have to be committers.  If a patch is marked as
‘reviewed’ by a non-commiter, another person can just apply the patch
and push.  Although the commiter will take some responsibility in doing
so, and make sure the review was done correctly, it still makes their
work way easier.

My point is that not being committer shouldn't discourage people to
review patches.

Clément

Re: Guix Days: Patch flow discussion

[Felix Lechner via Development of GNU Guix and the GNU System distribution.]

Hi Clément,

On Mon, Feb 05 2024, Clément Lassieur wrote:

> I don't think reviewers have to be committers.

How much more evidence does the project need to see in order to realize
that the plan is not working?

I'll spare the list a lengthy analysis of the social dynamics but the
delegation of competency to the bottom is an unsuccessful practice in
many places.

Maintainers must close bugs in service to the public. There is no other
way.

Kind regards
Felix

Re: Guix Days: Patch flow discussion

[Clément Lassieur]

On Mon, Feb 05 2024, Felix Lechner via "Development of GNU Guix and the GNU System distribution." wrote:

> Hi Clément,
>
> On Mon, Feb 05 2024, Clément Lassieur wrote:
>
>> I don't think reviewers have to be committers.
>
> How much more evidence does the project need to see in order to realize
> that the plan is not working?

The best review I've had in years was done by a person who is not a
committer.  I see no evidence here.  And I'm unsure which plan you are
talking about (the plan?).

> I'll spare the list a lengthy analysis of the social dynamics but the
> delegation of competency to the bottom is an unsuccessful practice in
> many places.

What do you mean with "bottom"?

> Maintainers must close bugs in service to the public. There is no other
> way.

Reviewing != Closing

Re: Guix Days: Patch flow discussion

[Felix Lechner via]

On Mon, Feb 05 2024, Clément Lassieur wrote:

> On Mon, Feb 05 2024, Felix Lechner via "Development of GNU Guix and the GNU System distribution." wrote:
>
> I see no evidence here.  And I'm unsure which plan you are talking
> about (the plan?).

Two people can look at the same thing and reach different conclusions. I
see no evidence that large numbers of non-committers are eager to review
patches.

> What do you mean with "bottom"?

I'm sorry to put words into your mouth. I meant to quote an executive at
a bank who explained that strategy to me. The word "bottom" was his and
should have been a quote.

The executive referred to people without the authority to act on behalf
of the group.

I believe Guix would be better off to delegate responsibility (rather
than competency) by handing out commit access more generously but
imposing limits as to the type of changes a person may make.

The honor system will work fine.

> Reviewing != Closing

Maybe they should be the same. Two people looking at a patch (submitter
and committer) are more efficient than three people, i.e. a submitter, a
reviewer, and a committer.

It's one of several bottlenecks at Guix. Another is that committers
should commit what they think is right rather than ask for revised
patches.

Please give authorship to the submitter.

Kind regards
Felix

Re: Guix Days: Patch flow discussion

[Clément Lassieur]

On Mon, Feb 05 2024, Felix Lechner via wrote:

> On Mon, Feb 05 2024, Clément Lassieur wrote:
>
>> On Mon, Feb 05 2024, Felix Lechner via "Development of GNU Guix and the GNU System distribution." wrote:
>>
>> I see no evidence here.  And I'm unsure which plan you are talking
>> about (the plan?).
>
> Two people can look at the same thing and reach different conclusions. I
> see no evidence that large numbers of non-committers are eager to review
> patches.

I believe very few people (commiter or not) are eager to review patches.

>> What do you mean with "bottom"?
>
> I'm sorry to put words into your mouth. I meant to quote an executive at
> a bank who explained that strategy to me. The word "bottom" was his and
> should have been a quote.
>
> The executive referred to people without the authority to act on behalf
> of the group.
>
> I believe Guix would be better off to delegate responsibility (rather
> than competency) by handing out commit access more generously but
> imposing limits as to the type of changes a person may make.
>
> The honor system will work fine.
>
>> Reviewing != Closing
>
> Maybe they should be the same. Two people looking at a patch (submitter
> and committer) are more efficient than three people, i.e. a submitter, a
> reviewer, and a committer.

Committers are trusted, and obviously it takes time to be trusted, so
not everyone can be at once.  Reviewing requires less trust because the
committer can do a quick check that everything is in order before
submitting the patch.

It's less efficient if you compare flows for two patches, but for 1000
patches, it makes it easier to find reviewers, so in the end it's more
efficient.

> It's one of several bottlenecks at Guix. Another is that committers
> should commit what they think is right rather than ask for revised
> patches.

I sometimes update the commit a little bit before pushing, and then
explain what I changed in my email.  But if the changes are big, I often
believe it makes more sense to ask for a v2.

> Please give authorship to the submitter.

I think it's the case?

Cheers,
Clément

Re: Guix Days: Patch flow discussion

[Steve George]

On 05/02/2024 15:57, Clément Lassieur wrote:
> Hello,
> 
> On Mon, Feb 05 2024, Steve George wrote:
> 
>> Hi,
>>
>> Our goal for the discussion:
>>
>> 	How do we double the number of patches that are *reviewed* and
>> 	*applied* to Guix in the next six months?
>>
>> Patch flow is a pipeline, to change it we could:
>>
>> a. Increase the number of committers - more people to do the
>> work
> 
> I don't think reviewers have to be committers.  If a patch is marked as
> ‘reviewed’ by a non-commiter, another person can just apply the patch
> and push.  Although the commiter will take some responsibility in doing
> so, and make sure the review was done correctly, it still makes their
> work way easier.
> 
> My point is that not being committer shouldn't discourage people to
> review patches.
(...)

Agreed - I put it a bit lower in the notes - clearly managed to bury it :-)

The discussion focused on increasing the number of Reviewers. A reviewer 
can be anyone - they don't need to be a committer.

The purpose of the proposed 'Patch Review' sessions is for contributors 
to come along and review patches together.

Thanks,

Steve

Re: Guix Days: Patch flow discussion

[Edouard Klein]

I, for one, would be willing to review patches, hoping that in turn my
patches would be reviewed instead of staying in limbo forever, which is
a drag on me submitting more patches.

Is there a procedure to follow, or do I just start replying "LGTM" to
patch email threads ?

Cheers,

Edouard.
Steve George <steve@futurile.net> writes:

> Hi,
>
> Our goal for the discussion:
>
> 	How do we double the number of patches that are *reviewed* and
> 	*applied* to Guix in the next six months?
>
> Patch flow is a pipeline, to change it we could:
>
> a. Increase the number of committers - more people to do the
> work
> b. Increase the efficiency of existing committers
> c. Open the gates by decreasing the quality expected from patches
>
> We essentially decided to focus our discussion on (b). We looked at
> things that 'hinder' and 'help' patch review:
>
>
> Hinders
> ========
>
> - All our patch reviewers are volunteers doing it in their spare time.
>
> - For a volunteer reviewing someone else's work is not very rewarding, most
>  would prefer to use that precious time to scratch their own itch.
>
> - Can feel like an Sisyphean task: no matter how many patches someone reviews
>  there are more, exacerbated by the number of Guix packages.
>
> - Sense of responsibility: the minute that a reviewer looks at the patch they
>  are now stuck with it
>
> - Repetitive and boring: often patches have minor issues, but it's the same
>  sorts of issues time and time again.
>
> - Risk of negative social interaction: having to tell someone that their patch
>   is incorrect, or that their contribution cannot be used is difficult and
>   draining. Some people felt it was better to say nothing, rather than to
>  respond to a patch.
>
>
> Helps
> ======
>
> This led us to the focus on the fact that **reviewing and applying
> patches can be different people**
>
> We looked for ideas to create more reviewers, make reviewing easier and
> more fun:
>
>
> - Share in the work
> --------------------
>
> 1. encourage new reviewers to step forward - making it more known that reviewing
> patches helps to get them applied. Anyone can review patches.
>
> 2. create directed 'how-to' documentation for reviewing and connect it to QA so
> that 'new reviewers' know what to do
>
> 3. create documentation about 'when' and 'how' it's appropriate to send a 'v2'
> version of a patch so that the QA system builds and accepts it. Sometimes,
> patches rot because non-committers don't want to be seen as 'stealing' someone's
> work with a v2 patch - but making the small changes and resubmitting to QA is
> what is required.
>
> 4. Pay someone else to do it. Noted but out of scope.
> 5. Remove old packages overhead. Old untouched packages create mental overhead,
> and make the task of maintaining the repository in a good state more difficult.
> We could remove old 'untouched' packages and ones that no-longer compile. We
> have methods to hide and notify.
>
>
> - Make it more fun
> -------------------
>
> 1. do online sessions around reviews, some sprints or pairing - both social and
> a way to spread skills
> 2. find ways to recognise and appreciate reviewers - 'reviewer of the month'
> 3. make it a game - we could have a 'Guix London' vs 'Guix Paris' leader board
> for reviews. Make it a group goal 'can we beat januarys reviews number'
> 4. create some graphs / leaderboard so we know how many patches are being
> reviewed and we can recognise the contribution
>
>
> - Automate it away
> -------------------
>
> 1. Chris is continuing to try and automate away the boring work - general
> agreement in the group that QA has made a lot of difference.
>
> 2. general discussion about create a 'guix review' command (Nix has one) which
> would download a branch with the appropriate patch and build it locally. This is
> for instances where some adjustment is needed or to check a build. While this
> can be done today, it's a number of steps and quite involved.
>
>
> Agreed Actions
> ==============
>
> * [Chris]: continuing his work to improve QA automation. Implication was we'll
>  need some reports / graphs - but these were not discussed in detail.
>
> * [Futurile]: organise a **patch review online sessions**. To run every 13 days
>   (so it rotates through the week) - for 3 months to see if it has any traction.
>   Co-ordinate with maintainers so that patches that are reviewed can be
>  committed
>
>
> Actions looking for someone - you?
> ====================================
>
> * Carry forward the 'guix review' command idea
>
> * Write an RFC and discuss the idea of removing older 'bit-rot' packages
>
> * write 'how-to' documentation for reviews and when it's socially
> acceptable to do a v2 patch. A checklist-like approach.
>
>
> If you were in the discussion and I've misrepresented your point, or forgotten
> an important aspect please please reply and correct me.
>
> Also, if you would like to help on any of the tasks please email back to the
> group so we can all co-ordinate.
>
> Finally, thank-you to everyone who came along and put their shared brain power
> to the task - look forward to doing some patch reviews together online in the
> coming weeks!
>
> Thanks,
>
> Steve/Futurile

Re: Guix Days: Patch flow discussion

[Steve George]

Hi Edouard,

On  6 Feb, Edouard Klein wrote:
> I, for one, would be willing to review patches, hoping that in turn my
> patches would be reviewed instead of staying in limbo forever, which is
> a drag on me submitting more patches.
> 
> Is there a procedure to follow, or do I just start replying "LGTM" to
> patch email threads ?

Starting to review patches would be brilliant.

I agreed to organise some 'patch review' online sessions in the next couple of 
weeks. 

Organising a basic process is a good topic for that online session. For 
example, elsewhere in the thread someone mentions some tags we could use 
consistently so maintainers can find patches that have been reviewed easily. It 
would be great to agree those - try them for a bit - and document them in a 
'howto' so that everyone uses the same process.

Steve

Re: Guix Days: Patch flow discussion

[Skyler Ferris]

On 2/6/24 05:39, Steve George wrote:
> I agreed to organise some 'patch review' online sessions in the next couple of
> weeks.
>
> Organising a basic process is a good topic for that online session. For
> example, elsewhere in the thread someone mentions some tags we could use
> consistently so maintainers can find patches that have been reviewed easily. It
> would be great to agree those - try them for a bit - and document them in a
> 'howto' so that everyone uses the same process.
Have these been announced somewhere yet (eg, with url & exact time)? If 
not will being subscribed to the help-guix list and/or checking the Guix 
blog be sufficient to receive notification? As someone who has sent 
patches in the past and intends to continue sending more in the future, 
I'd like to do my part to keep the project in a good state. However, I 
am new to interacting with large FLOSS projects so I'm nervous about 
causing more problems than I solve if I just start doing things.

Re: Guix Days: Patch flow discussion

[Edouard Klein]

Skyler Ferris <skyvine@protonmail.com> writes:

> On 2/6/24 05:39, Steve George wrote:
>> I agreed to organise some 'patch review' online sessions in the next couple of
>> weeks.
>>
>> Organising a basic process is a good topic for that online session. For
>> example, elsewhere in the thread someone mentions some tags we could use
>> consistently so maintainers can find patches that have been reviewed easily. It
>> would be great to agree those - try them for a bit - and document them in a
>> 'howto' so that everyone uses the same process.
> Have these been announced somewhere yet (eg, with url & exact time)? If
> not will being subscribed to the help-guix list and/or checking the Guix
> blog be sufficient to receive notification? As someone who has sent
> patches in the past and intends to continue sending more in the future,
> I'd like to do my part to keep the project in a good state. However, I
> am new to interacting with large FLOSS projects so I'm nervous about
> causing more problems than I solve if I just start doing things.

Same here.

Re: Guix Days: Patch flow discussion

[Andreas Enge]

Hello,

Am Fri, Feb 09, 2024 at 05:35:44PM +0100 schrieb Edouard Klein:
> Am Thu, Feb 08, 2024 at 07:56:48PM +0000 schrieb Skyler Ferris:
> > I'd like to do my part to keep the project in a good state. However, I
> > am new to interacting with large FLOSS projects so I'm nervous about
> > causing more problems than I solve if I just start doing things.
> Same here.

no need to be nervous! I would suggest to just start somewhere and learn
by doing. For patches, anyway the committer takes the final responsibility;
so if you can just add, for instance, "I use this package and it works for
me" this will be a useful point.

And do not be afraid to make mistakes; I have been taking part in Guix for
a long time now, and of course make mistakes from time to time. That
happens, and they can be corrected if they occur. What I find important
is to be upfront about them, and to ask for help if needed.

Andreas

Re: Guix Days: Patch flow discussion

[Steve George]

On  9 Feb, Edouard Klein wrote:
> 
> Skyler Ferris <skyvine@protonmail.com> writes:
> 
> > On 2/6/24 05:39, Steve George wrote:
> >> I agreed to organise some 'patch review' online sessions in the next couple of
> >> weeks.
> >>
> >> Organising a basic process is a good topic for that online session. For
> >> example, elsewhere in the thread someone mentions some tags we could use
> >> consistently so maintainers can find patches that have been reviewed easily. It
> >> would be great to agree those - try them for a bit - and document them in a
> >> 'howto' so that everyone uses the same process.
> > Have these been announced somewhere yet (eg, with url & exact time)? If
> > not will being subscribed to the help-guix list and/or checking the Guix
> > blog be sufficient to receive notification? As someone who has sent
> > patches in the past and intends to continue sending more in the future,
> > I'd like to do my part to keep the project in a good state. However, I
> > am new to interacting with large FLOSS projects so I'm nervous about
> > causing more problems than I solve if I just start doing things.
> 
> Same here.
>

Hi Skyler & Edouard - you haven't missed an announcement :-) I'll get it organised next week and will mail out here - plus I'll email you both directly as you've both shown interest.

Thanks,

Steve / Futurile

Re: Guix Days: Patch flow discussion

[Simon Tournier]

Hi Steve,

  ( On a side note, the triage of old bugs is a similar problem.  They
    are easy to find [2], read, check and send an email to
    12345@debbugs.gnu.org does not appear to me an issue with any tool.
    
    For what it is worth and without any willing of being harsh, I am
    able to count the people doing this boring task.

    What is hard to solve is the incentives for doing the boring, but
    necessary, collective tasks.

    Bah the usual problem of lengthy discussions with roommates in any
    shared apartment: who clean the bathroom? :-) )


On lun., 05 févr. 2024 at 09:39, Steve George <steve@futurile.net> wrote:

> Our goal for the discussion:
>
> 	How do we double the number of patches that are *reviewed* and
> 	*applied* to Guix in the next six months?

Thanks for these notes and leading the session.  On my side, it was a
fruitful discussion.

Well, let me try to quickly summarize my conclusion of the session:

 1. We have a social/organisational problem.

 2. We have some tooling annoyances.



The easy first: #2 about tools.  The email workflow is often cited as
part of the issue.  That’s a false-problem, IMHO.

Projects that use PR/MR workflow have the same problem.  For instance,
Julia [1] has 896 open PR.  On my browser, it means 36 pages so if I go
to – 25 PRs per page – the still open submitted PRs:

   + the 6th page:  around Sept.2023 and Oct. 2023
   + the 12th page: around Apr. 2023 and Mar. 2023
   + the 18th page: around Jul. 2022 and Mar. 2022
   + the 24th page: around Jun. 2021 and May  2021
   + the 30th page: around Mar. 2020 and Oct. 2019
   + the 36th page: around Mar. 2017 and May. 2014

Obviously, an example is not a proof or an evidence.  It is just a
first clue. :-)

I will not speak about the channel ’nonguix’ but it gives another
clue.

That said, for sure, the tools need more love.  Thanks all the people
for all hard work over the years in this area – no name, you know, I
fear to forget someone. ;-)

So, yeah we need to smooth the technical burden for reviewing in order
to focus on the review itself.

To be clear, the email workflow might add burden on submitter side but I
am doubtful it is really part of the bottleneck for reviewing and
pushing submissions.


Although the tools might add some unnecessary friction, the net of the
issue is IMHO #1: reviewing is just boring and time-consuming.

Who feel accountable?  And for what?  That’s the question, IMHO.

If the number of submission is doubled, how do we increase the number of
people that feel enough accountable for doing the boring work?

  ( Maybe accountable is not the correct word.  Obligation neither.
    Well the kind of feeling that is okish if you skip the task but you
    know it will be better if you do it. )


Well, the difficult part is not pressing some buttons for merging and
pushing – whatever the tools or workflow.  The difficult part is to
scrutinize the submission.

I think the bottleneck is not the number of people able to push.
Instead, I think the bottleneck is the number of people confident with
the change for then pushing it.

The question is thus: how to build this confidence?


Look, when a committer has some free-time, most of the time, what is the
process: take first the “easy“ submissions for committing them – from
trivial updates to simple updates.  If free-time remains, then engage
with more “complex” submissions… ah no more free-time. :-)

Why starting by the “easy” submission?  Because it is less boring and
time-consuming; somehow it is easier to feel confident with that sort of
change for pushing it.

As a rule of thumb, about the time it takes – on average –, the order of
magnitude for reviewing is similar as the one for submitting.  Well,
from my experience and although I never did stats. :-)


All in all, I see two paths to move forward:

i) Non-committers can help.  On two fronts:

   + Answer to submitter with the changes for being compliant with Guix
     standards.
   
   + Follow-up on patches already commented but without an updated
     revision: upgrade the re-roll count by sending this revision.

     It eases for merging if I do not have to make many tiny edits
     myself.

ii) Create more teams or at least more people should commit to be part
    of a team and help in reviewing what they know.

    For instance, since Sept. (167 days ago) I have been CC in 108
    patches submissions.  Most of them are from ’core’ team that I would
    qualify as “complex”. :-)

    Many patches assigned to ’core’ team are sent by committers.  The
    issue is not being a committer or not.  Instead, being more eyes
    commenting would increase the confidence.  Thus it would reduce the
    workload.

    That’s the same for any team, IMHO.
    
    And I do not speak about patches that are not assigned to any team.


Somehow, we need to think how people would feel “accountable” for doing
the collective tasks with low, no direct or personal reward.

As with many non-technical topics, it is not easy.  Because it is a
collective journey not clearly identified – and not a kind of
reproducible bug to fix, even the tougher.

Last, the manual says:

    « As a rule of thumb, a contributor should have accumulated
      fifty (50) reviewed commits to be considered as a committer
      and have sustained their activity in the project for at least
      6 months. »

So if people are willing to take more responsibility to help the
project…


Well, while writing that I could have give a look to patches… ;-)

Cheers,
simon


1: https://github.com/JuliaLang/julia
2: https://issues.guix.gnu.org/forgotten

Re: Guix Days: Patch flow discussion

[Giovanni Biscuolo]

[-- Attachment #1: Type: text/plain, Size: 3922 bytes --]

Hello Simon,

first and foremost: I'd like to say a big thank you to all the people
working in the Guix community...

...and apologise if I still cannot do more to help.

Simon Tournier <zimon.toutoune@gmail.com> writes:

[...]

> Well, let me try to quickly summarize my conclusion of the session:
>
>  1. We have a social/organisational problem.
>
>  2. We have some tooling annoyances.
>
>
> The easy first: #2 about tools.  The email workflow is often cited as
> part of the issue.  That’s a false-problem, IMHO.

yes, we (as a community) already had several discussions around the
false-problem named "email worfkow is too hard", I also dared to send a
*very* lenghty analysis comparing the _so_called_ "pull request model" [1] 

Unfortunately I'm pretty sure that _this_ false issue will be cited
again and again and again when discussing about "how to better help Guix
maintainers"

...unless the (info "(guix) Submitting Patches") one day will finally
(briefly) explain why the project is using an email based workflow and
not a "so called PR workflow" (to understand why PR workflow is "so
called" please read [1]) 

But all this discussion on the "email workflow" issue is more useless
when considering the commit authetication mechanism _embedded_ in Guix
since 2020;  I recently studied this blog post:

https://guix.gnu.org/en/blog/2020/securing-updates/

and it states:

--8<---------------cut here---------------start------------->8---

To implement that, we came up with the following mechanism and rule:

1 The repository contains a .guix-authorizations file that lists the
 OpenPGP key fingerprints of authorized committers.

2 A commit is considered authentic if and only if it is signed by one of
 the keys listed in the .  guix-authorizations file of each of its
 parents. This is the authorization invariant.

[...]

The authorization invariant satisfies our needs for Guix. It has one
downside: it prevents pull-request-style workflows. Indeed, merging the
branch of a contributor not listed in .  guix-authorizations would break
the authorization invariant. It’s a good tradeoff for Guix because our
workflow relies on [patches carved into stone tablets] (patch tracker),
but it’s not suitable for every project out there.

--8<---------------cut here---------------end--------------->8---

[patches carved into stone tablets] is a link to:

https://lwn.net/Articles/702177/
«Why kernel development still uses email»
By Jonathan Corbet, October 1, 2016 

an article with another ton of reasons why "all patch management tools
sucks, email just sucks less.

Anyway, since Guix is using the "authorization invariant" since 2020,
the "email workflow" is embedded in Guix :-D

Am I missing something?

> Projects that use PR/MR workflow have the same problem.  For instance,
> Julia [1] has 896 open PR. 

[...]

> I will not speak about the channel ’nonguix’ but it gives another
> clue.

I will not speak about kubernetes, cited in the above cited LWN article,
I will not speak about Gerrit, also cited there...

[...]

> To be clear, the email workflow might add burden on submitter side but I
> am doubtful it is really part of the bottleneck for reviewing and
> pushing submissions.

Email workflow makes the reviewing workflow _extremely_ easy, provided a
good MUA and a _little_ bit of self-discipline following the /easy/
guidance in (info "(guix) Reviewing the Work of Others")

> Although the tools might add some unnecessary friction, the net of the
> issue is IMHO #1: reviewing is just boring and time-consuming.

This is the one and only reason.

[...]

I don't have anything to add, for now.


Happy hacking! Gio'


[1] id:87y1ha9jj6.fsf@xelera.eu aka
    https://yhetil.org/guix/87y1ha9jj6.fsf@xelera.eu/

-- 
Giovanni Biscuolo

Xelera IT Infrastructures

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 849 bytes --]

Re: Guix Days: Patch flow discussion

[Matt]

 ---- On Wed, 28 Feb 2024 18:51:16 +0100  Giovanni Biscuolo  wrote --- 
 
 > ...and apologise if I still cannot do more to help.

We do what we can when we can.  And you have done things to help.  Thank you for sharing your thoughts and perspective!  If there's nothing you can do at the moment, that's okay.  I have no doubt you'll step up again when you see an opportunity.  Who knows, someone may even delegate... ;)

Re: Guix Days: Patch flow discussion

[Daniel Littlewood]

Hi! I'm a complete newbie to Guix, which has the dual effects that I'm much
more excited about it than I otherwise might be, but the disadvantage that I
know nothing and am more likely to cause problems than not at the moment.
Nevertheless I thought I might be able to contribute something to the
discussion.

Something that is not obvious to me when people refer to reviewing patches, is
whether this is purely a matter of adding new packages to the main guix
channel, or of reviewing changes to the system in general, or both. As a
novice, I can imagine becoming comfortable as a package reviewer much more
quickly than as a reviewer of core patches to the system.

It's also not obvious to me whether you mean exactly "reviewing a backlog of
existing patches" or additionally "increasing the amount of patches submitted
and applied". I feel like both are probably good things but I can't tell what
you're focussing on exactly. If lots of gems were imported from other repos
like RubyGems and PyPi, which as I understand it is currently a
partly-automatic partly-manual process, would that be considered a win? What
about increasing version coverage among those packages that are covered?

One point brought up here is about tooling. I wonder whether there is any scope
for fully automatic review. In other words, for some classes of package we
might be able to say that if it builds in CI, and maybe the built package has a
command that's considered enough to say "it's working", then that's all the
review that's needed. I guess I'm mostly thinking here of cases where the
package is imported, partially or entirely automatically, from another repo (as
with `guix import`). So one could argue that as long as it looks like "the
rubygems import is working" then the guix package thus imported can be assumed
to be working too. I guess when I say this I am still mostly thinking of
importing entire histories of a package (lots of versions simultaneously)
without multiplying out the work involved to a huge degree.

One of the points brought up earlier is whether it would be better to have
interactive "review parties" or comprehensive documentation about what
constitutes a good review. Obviously these are not mutually exclusive. But for
my part, I think documentation gives you more "bang for your buck", for a few
reasons:

  1. Documentation is online all the time, whereas interactive sessions are
subject to people's work schedules, time zones, personal commitments, etc.

  2. Documentation is more likely to be complete, and to become more likely to
be complete over time, whereas in meetings (or when doing your first solo
review following the meeting) it is easy to forget some steps or criteria to
check.

  3. Reading docs can be done by anyone, with no commitment to follow up. I
think some people are just scared off socially by the idea of having to join a
meeting in order to learn how to do reviews well. Obviously this is a personal
thing and varies - some people find a welcoming atmosphere in a virtual meeting
much better than a faceless documentation screen. For my part I think I would
prefer to read until I know the basics, and then might prefer to do it
alongside other people. A "patch party" would not convert me from
non-contributor to contributor, but it might convert me from a
seldom-contributor to an active one. Again, there is obviously variation here
among people.

In case it is not obvious, I have only recently joined guix-devel, but would be
keen to help out if I can. If there are things I can read to get "up to speed"
that would be really helpful!

All the best,
Dan