unofficial mirror of guix-devel@gnu.org 
 help / color / mirror / code / Atom feed
* Guix Days: Patch flow discussion
@ 2024-02-05  9:39 Steve George
  2024-02-05 14:07 ` Leo Famulari
                   ` (4 more replies)
  0 siblings, 5 replies; 94+ messages in thread
From: Steve George @ 2024-02-05  9:39 UTC (permalink / raw)
  To: guix-devel; +Cc: help-guix

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





^ permalink raw reply	[flat|nested] 94+ messages in thread
* Re: Guix Days: Patch flow discussion
@ 2024-02-05 17:21 Suhail
  2024-02-05 17:25 ` Vivien Kraus
  2024-02-06 11:27 ` Josselin Poiret
  0 siblings, 2 replies; 94+ messages in thread
From: Suhail @ 2024-02-05 17:21 UTC (permalink / raw)
  To: Tomas Volf; +Cc: Leo Famulari, Steve George, guix-devel

Tomas Volf <~@wolfsden.cz> writes:

> In ideal world, there would always be *some* reaction from the
> project, even if that reaction would be "we do not want this change".

Agreed.  A reduction in the time between a patch (or patch revision)
submission and a review/reaction would be a positive change in my
opinion.

> Even if it would be just an auto-close (for the "contributor can't
> work effectively..." case).

Are there current instances of "contributor can't work effectively"?  If
not, I propose that decisions concerning those situations be deferred
till we have actual instances to guide us.

> Or, to put it in a different way: The problem is not that too few patches get
> merged.  The problem is that too few patches get reviewed.

Agreed.

I believe the below steps would help with the current situation, and
perhaps both should be pursued (among possibly others).

1. It would help to eliminate the need for review in certain kinds of
   patches.  Some version upgrades for a package $foo where there exists
   no package $bar that depends on $foo may fall in this category.  More
   generally, some "known to be safe with reasonable confidence" changes
   may be safe to be auto-committed without needing manual review.
   Recognizing such changes could be challenging, but we don't have to
   correctly label all such changes in order for this to be helpful.

2. It would help if it were easier for the community to be able to identify the next
   steps, given a patch submission.  It might help to (more?) clearly
   differentiate between the below states:
   - patch *needs review* (automatically set)
   - patch *needs revision* (set explicitly by the reviewer, say, by
     using a specific keyword in their email)
   - patch *needs followup review* (automatically set if there's a
     revised patch)
   - patch has a *satisfied reviewer* (set explicitly by the reviewer,
     say, by using a specific keyword in their email)

-- 
Suhail



^ permalink raw reply	[flat|nested] 94+ messages in thread
[parent not found: <34679.2393991322$1707153777@news.gmane.org>]
* Re: Guix Days: Patch flow discussion
@ 2024-02-05 18:44 Suhail
  2024-02-05 19:59 ` Hartmut Goebel
  0 siblings, 1 reply; 94+ messages in thread
From: Suhail @ 2024-02-05 18:44 UTC (permalink / raw)
  To: Hartmut Goebel; +Cc: guix-devel

Hartmut Goebel <h.goebel@crazy-compilers.com> writes:

> This list is missing one point - which has been discussed several
> times already without any result:
>
> The current mail-based workflow is too complicated for new and
> occasional committers.

Could you please share a reference where the key difficulties you
encountered wrt the "current mail-based workflow" are summarized.  Is
the difficulty regd. checking out the code at the right commit and
installing the patches, or something else?

-- 
Suhail



^ permalink raw reply	[flat|nested] 94+ messages in thread
* Re: Guix Days: Patch flow discussion
@ 2024-02-05 18:51 Suhail
  2024-02-06 16:51 ` Steve George
  0 siblings, 1 reply; 94+ messages in thread
From: Suhail @ 2024-02-05 18:51 UTC (permalink / raw)
  To: Felix Lechner
  Cc: Clément Lassieur, Steve George, Guix-devel mailing list,
	Help-Guix mailing list

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



^ permalink raw reply	[flat|nested] 94+ messages in thread
* Re: Guix Days: Patch flow discussion
@ 2024-02-05 19:33 Andy Tai
  2024-02-05 20:50 ` Clément Lassieur
  0 siblings, 1 reply; 94+ messages in thread
From: Andy Tai @ 2024-02-05 19:33 UTC (permalink / raw)
  To: guix-devel

Hi, this is a sugestion on guix patches:

Other GNU/Linux distributions often have fixed maintainers (or
packagers) for specific packages.
While that model may not apply directly to the Guix project as anyone
can send in patches for anything in the git repo, maybe one thing the
Guix project can do is to recognize if someone has sent in patches for
specific projects many times in the past, then that means the person
has knowledge, interest, familiarity and expertise about this package
and the reviewers can spend minimal time of his/her future patches for
the same package(s).

Thus this creates kind of pseudo package maintainer in Guix, reducing
workload of reviewers and speeding up package update.

Guix can have a text file in the Guix repo recording say someone who
is "responsible" or is a "pseudo packager" for specific packages,
which the committers can refer to to allow quick processing of
patches.  This does not mean the person responsible for certain
package has git commit access but just to allow a quick path for patch
processing by the commiters of certain patches from the familiar
person.

Of course others can still send in patches for these packages already
with pseudo packagers.


^ permalink raw reply	[flat|nested] 94+ messages in thread
[parent not found: <65c12e7e.0c0a0220.d7729.823cSMTPIN_ADDED_BROKEN@mx.google.com>]
* Re: Guix Days: Patch flow discussion
@ 2024-02-05 21:45 Suhail
  0 siblings, 0 replies; 94+ messages in thread
From: Suhail @ 2024-02-05 21:45 UTC (permalink / raw)
  To: Hartmut Goebel; +Cc: Suhail, guix-devel

Hartmut, thank you for elaborating.

Hartmut Goebel <h.goebel@crazy-compilers.com> writes:

>  * when has this issue/patch been worked on last - is somebody
>    currently working on it
>  * what issue/patches I started to review?
> ...
>  * Even when using the debbugs interface in emacs
> ...
>      o It does not tell what issues/patches I've been working on
>        already - and waiting for a reply

I believe the state tracking could be improved, and have given some
concrete suggestions in another thread.  However, I believe these are
orthogonal to whether or not the workflow is "mail-based".

For clarity, within Emacs' Debbugs interface I can use a combination of
tags and marks to highlight the issues of interest.

>  * commenting on code requires to download the patch - strip out parts
>    which are okay, comment, then mail the commented code to the correct
>    issue number

I believe the ease or not of this would depend on the email client of
choice.

>  * Even when using the debbugs interface in emacs
> ...
>      o It is an insurmountable obstacle for those not using emacs.

This is probably an important issue if true.  To clarify, in "those not
using [E]macs" are you considering developers using other email clients
such as Alpine, Mutt etc?

>      o It does not tell which issues are stale

What does "stale" mean to you?  Do you mean something other than
Debbugs' notion of staleness?

> And as long as vocal (and active :-) members of the community insist
> on being able to work via e-mail — while also not adopting modern
> e-mail-capable forges — this situation will not change.

In an attempt to focus the discussion on the specific features, it seems
if, in addition to improved state tracking, the below were available it
might help (to varying degrees depending on the user):

1. Some tooling that helps in "checking out" a specific revision of a
   patch series.  I don't know if this already exists, given my relative
   inexperience with Debbugs.

2. An ability to leave comments inline on
   <https://issues.guix.gnu.org/>.

While "forges" provide both of the above in some form, they may not be
the only way to accomplish the above.

-- 
Suhail



^ permalink raw reply	[flat|nested] 94+ messages in thread
* Re: Guix Days: Patch flow discussion
@ 2024-02-06 19:42 Suhail
  0 siblings, 0 replies; 94+ messages in thread
From: Suhail @ 2024-02-06 19:42 UTC (permalink / raw)
  To: Steve George; +Cc: Edouard Klein, guix-devel, help-guix

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



^ permalink raw reply	[flat|nested] 94+ messages in thread
* Re: Guix Days: Patch flow discussion
@ 2024-02-06 19:47 Suhail
  0 siblings, 0 replies; 94+ messages in thread
From: Suhail @ 2024-02-06 19:47 UTC (permalink / raw)
  To: Steve George
  Cc: Suhail, Felix Lechner, Clément Lassieur,
	Guix-devel mailing list, Help-Guix mailing list

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



^ permalink raw reply	[flat|nested] 94+ messages in thread
* Re: Guix Days: Patch flow discussion
@ 2024-02-06 20:00 Suhail
  2024-02-07 13:41 ` Josselin Poiret
  0 siblings, 1 reply; 94+ messages in thread
From: Suhail @ 2024-02-06 20:00 UTC (permalink / raw)
  To: Josselin Poiret; +Cc: Hartmut Goebel, Suhail, guix-devel

Josselin Poiret <dev@jpoiret.xyz> writes:

> One thing I would like to get rid of though is debbugs.  It causes a
> lot of pain for everyone, eg. when sending patchsets, it completely
> breaks modern email because it insists on rewriting DMARC-protected
> headers, thus needing to also rewrite "From:" to avoid DMARC errors.

Thank you for sharing (what seems to be) a technical limitation of
Debbugs.  Could you please explain what the consequences of the above
are?  Specifically, how does the rewriting of above headers affect the
contributors' workflow?

> b4/lei is a nice example (we already have yhetil.org as a back-end,
> but maybe a more blessed one would be better) of a tool that lets you
> completely automate applying a patchset to a branch.
>
> patchwork is a nice tool to gather up and track patchsets, with status
> indicators like "under review", "accepted", etc.  Chris already
> deploys one as part of QA, more integration with it would be nice.

It seems (based on above) that "patchwork" can co-exist with debbugs.
Is that also the case with b4/lei?  Specifically, are the
users/reviewers able to benefit from using the above tools at present?
Or are there some reasons (over and above their lack of familiarity with
the above tools) that would prevent them from doing so?

-- 
Suhail



^ permalink raw reply	[flat|nested] 94+ messages in thread
* Re: Guix Days: Patch flow discussion
@ 2024-02-06 21:01 Suhail
  0 siblings, 0 replies; 94+ messages in thread
From: Suhail @ 2024-02-06 21:01 UTC (permalink / raw)
  To: Josselin Poiret
  Cc: Suhail, Tomas Volf, Leo Famulari, Steve George, guix-devel

Josselin Poiret <dev@jpoiret.xyz> writes:

> Your mailing system is sending out emails that contain an invalid
> Message-ID header (missing right part of the msgid as specified in
> [1]),
> ...
> [1] https://datatracker.ietf.org/doc/html/rfc2822#section-3.6.4

My apologies (to everyone)!  Thank you for bringing this to my
attention.

> which causes erratic behavior from other participants (the gmail smtp
> servers for example just rewrite the message id to indicate it is
> malformed, breaking the reply chain).

Ouch.

> Could you please fix this?

I believe this is now fixed.  Please let me know in case the issue
persists, or you notice others (though, perhaps, off list).

-- 
Suhail



^ permalink raw reply	[flat|nested] 94+ messages in thread
[parent not found: <35786.5440238797$1707248619@news.gmane.org>]
* Re: Guix Days: Patch flow discussion
@ 2024-02-18  2:31 Suhail
  0 siblings, 0 replies; 94+ messages in thread
From: Suhail @ 2024-02-18  2:31 UTC (permalink / raw)
  To: Clément Lassieur
  Cc: Suhail, Steve George, Edouard Klein, guix-devel, help-guix

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



^ permalink raw reply	[flat|nested] 94+ messages in thread

end of thread, other threads:[~2024-03-01 22:05 UTC | newest]

Thread overview: 94+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-02-05  9:39 Guix Days: Patch flow discussion Steve George
2024-02-05 14:07 ` Leo Famulari
2024-02-05 15:00   ` Tomas Volf
2024-02-05 22:08     ` Wilko Meyer
2024-02-06 11:49       ` Tomas Volf
2024-02-06 12:09         ` Clément Lassieur
2024-02-06 12:53           ` Tomas Volf
2024-02-28 16:05             ` simple service extensions/composizions (Re: Guix Days: Patch flow discussion) Giovanni Biscuolo
2024-02-05 21:57   ` Guix Days: Patch flow discussion Steve George
2024-02-05 15:57 ` Clément Lassieur
2024-02-05 17:10   ` Felix Lechner via Development of GNU Guix and the GNU System distribution.
2024-02-05 17:28     ` Clément Lassieur
2024-02-05 18:27       ` Felix Lechner via
2024-02-05 18:50         ` Clément Lassieur
2024-02-05 22:10   ` Steve George
2024-02-05 18:07 ` Hartmut Goebel
2024-02-05 22:29   ` Steve George
2024-02-05 22:31   ` Steve George
2024-02-05 22:31   ` Steve George
2024-02-05 22:31   ` Steve George
2024-02-05 22:31   ` Steve George
2024-02-05 22:31   ` Steve George
2024-02-05 22:31   ` Steve George
2024-02-05 22:32   ` Steve George
2024-02-05 22:32   ` Steve George
2024-02-05 22:33   ` Steve George
2024-02-05 22:50   ` Steve George
2024-02-08 11:59     ` patch workflow without emacs (Was: Re: Guix Days: Patch flow discussion) Efraim Flashner
2024-02-06 13:27 ` Guix Days: Patch flow discussion Edouard Klein
2024-02-06 13:39   ` Steve George
2024-02-08 19:56     ` Skyler Ferris
2024-02-09 16:35       ` Edouard Klein
2024-02-09 16:46         ` Andreas Enge
2024-02-11 10:03         ` Steve George
2024-02-14 21:40 ` Simon Tournier
2024-02-28 17:51   ` Giovanni Biscuolo
2024-02-28 19:21     ` Matt
2024-02-29 15:41       ` Daniel Littlewood
2024-02-29 18:09         ` Andreas Enge
  -- strict thread matches above, loose matches on Subject: below --
2024-02-05 17:21 Suhail
2024-02-05 17:25 ` Vivien Kraus
2024-02-06 11:27 ` Josselin Poiret
     [not found] <34679.2393991322$1707153777@news.gmane.org>
2024-02-05 17:36 ` Clément Lassieur
2024-02-05 18:44 Suhail
2024-02-05 19:59 ` Hartmut Goebel
2024-02-06 11:14   ` Josselin Poiret
2024-02-05 18:51 Suhail
2024-02-06 16:51 ` Steve George
2024-02-05 19:33 Andy Tai
2024-02-05 20:50 ` Clément Lassieur
2024-02-06  7:58   ` Andy Tai
     [not found] <65c12e7e.0c0a0220.d7729.823cSMTPIN_ADDED_BROKEN@mx.google.com>
2024-02-05 19:52 ` Felix Lechner via
2024-02-05 21:45 Suhail
2024-02-06 19:42 Suhail
2024-02-06 19:47 Suhail
2024-02-06 20:00 Suhail
2024-02-07 13:41 ` Josselin Poiret
2024-02-07 13:46   ` Josselin Poiret
2024-02-11 17:24     ` Vagrant Cascadian
2024-02-22  5:42       ` Felix Lechner via Development of GNU Guix and the GNU System distribution.
2024-02-11 20:04     ` Felix Lechner via Development of GNU Guix and the GNU System distribution.
2024-02-11 20:21       ` Clément Lassieur
2024-02-11 20:39         ` Felix Lechner via Development of GNU Guix and the GNU System distribution.
2024-02-11 22:08           ` Clément Lassieur
2024-02-12 10:35       ` Josselin Poiret
2024-02-12 11:19         ` Clément Lassieur
2024-02-12 15:57         ` Felix Lechner via Development of GNU Guix and the GNU System distribution.
2024-02-13  9:31           ` Josselin Poiret
2024-02-13 14:30             ` Felix Lechner via Development of GNU Guix and the GNU System distribution.
2024-02-14  9:21               ` Josselin Poiret
2024-02-07 14:30   ` Clément Lassieur
2024-02-07 20:18   ` Ricardo Wurmus
2024-02-08  2:39     ` Kyle Meyer
2024-02-11 16:38       ` Maxim Cournoyer
2024-02-14 15:48         ` Simon Tournier
2024-02-15 11:07           ` Josselin Poiret
2024-02-15 12:45             ` Simon Tournier
2024-02-15 11:45           ` Clément Lassieur
2024-02-15 11:51             ` Clément Lassieur
2024-02-15 15:32               ` Felix Lechner via Development of GNU Guix and the GNU System distribution.
2024-02-15 17:19                 ` Simon Tournier
2024-02-16 14:27               ` Maxim Cournoyer
2024-02-15 13:06             ` Simon Tournier
2024-02-15 17:24               ` Clément Lassieur
2024-02-15 18:40                 ` Simon Tournier
2024-02-15 22:08                   ` Clément Lassieur
2024-03-01 22:04                   ` Attila Lendvai
2024-02-16 14:25           ` Maxim Cournoyer
2024-02-06 21:01 Suhail
     [not found] <35786.5440238797$1707248619@news.gmane.org>
2024-02-16 10:56 ` Clément Lassieur
2024-02-16 11:03   ` Andreas Enge
2024-02-16 11:28     ` Clément Lassieur
2024-02-16 12:06       ` Christopher Baines
2024-02-18  2:31 Suhail

Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/guix.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).