More thoughts on Patchwork and Guix patch review/quality assurance

More thoughts on Patchwork and Guix patch review/quality assurance

[Christopher Baines]

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

Hey,

It's over 2 years now since I started to build tooling around reviewing
Guix patches, which is a little shocking! This email [1] from back in
March details where some of the time has gone.

1: https://lists.gnu.org/archive/html/guix-devel/2020-03/msg00476.html

Since then, I've managed to get the patch testing setup working again,
which was a matter of working out how to delete data from the Guix Data
Service, so that it doesn't keep filling up the disk.

Also, it now uses the Guix Build Coordinator for building the
packages. This is great because the Guix Data Service quickly finds out
about the results, and the builds can be prioritised by the number of
packages being built for a patch series.

If you're interested in using this to review patches, the place to start
is Patchwork, so visit:

  https://patchwork.cbaines.net/

If you want an account, please email me. Unfortunately I had to disable
registration due to spam.

When you click through to a patch, the thing to look at is the
checks. These should link you to the relevant bug, Git branch and Guix
Data Service comparison. For the Guix Data Service comparison, the
things to note are the lint warnings and then clicking through to the
"Compare package derivations" page.

Changing direction a bit, it would be good to discuss more generally how
to make getting changes in to Guix/patch review better (faster, more
rigorous, more enjoyable, ...). Here are some thoughts I have, but I'd
love other ideas as well.


### Use Patchwork, rather than Debbugs to track patches

So this has been a thing for 4 years now (see [2] for some graphs). I
think I was there in the bar in Brussels when it was discussed.

2: https://debbugs.gnu.org/rrd/guix-patches.html

The main disadvantage I see of using Debbugs is that you have to get a
bug number before you can Git send-email, in the case where you're
sending multiple patches.

The main advantage I get from Debbugs is that searching via the bug
number is really useful.

I'm unsure, I think Debbugs was introduced to help keep track of
patches, and avoid them getting forgotten in guix-devel. Patchwork will
do this too, but maybe Debbugs is providing more value than the cost of
multi-patch series being slightly more difficult to submit?


### Scheduled and regular collaboration on IRC to review patches

When I can make some time, I'll try this out, but if anyone has some
time they can set aside, please go ahead!


### Help users with the submitting part of submitting patches

Using git send-email works well with Patchwork at least, and it's fine
with Debbugs if you've got a single patch, but a little more time
consuming if you're got more than one patch.

Attaching a single patch file to an email works OK I think, however
attaching multiple patch files to the same email confuses git am and
Patchwork I believe.

I think other approaches like copying the patch in to the body of an
email, or just copying the output of git diff in to an email are very
brittle and can make it more time consuming to try and recover or
recreate the commit(s).

Having the emails for patches is useful for reviewing, but maybe there's
a way users could push a branch somewhere and then have some service do
the git send-email thing on their behalf?


### Notify interested people about patches

This is something I've been thinking about more generally, supporting
email subscriptions for things like a new version of a package being
available, or the package failing to build on
master/staging/core-updates.

But specifically for patches, maybe there could be a way of subscribing
so that you're emailed when a patch series upgrades a package you're
subscribed to, or breaks a package you're subscribed to.

This could help get more people involved in reviewing patches, by making
it easier for the interested people to find out about patches they're
interested in.


### Helping people who aren't committers review patches

I don't know how much of this happens, but it might be something to
better support?

Better supporting it could mean documenting how to get things that have
passed review merged in, like emailing a list of committers when a patch
series is ready to merge.


### Making automated testing more rigorous

So, providing the patches can be automatically processed and apply
successfully, I'm currently building packages for x86_64-linux and
i686-linux.

There are more things that could be built if I amend the relevant
script:

 - System tests (maybe just x86_64-linux is relevant?

 - Cross-compiled packages (the Guix Data Service currently only
   generates these for x86_64-linux)

 - Building packages multiple times, this could help to find
   reproducibility issues (not just with the output, but also with
   succeeding or failing)

There are also some things that would require more work/hardware:

 - Building packages for architectures other than
   x86_64-linux/i668-linux.

   - I have some ideas on getting the Guix Build Coordinator agent
     running in a childhurd, some general substitute availability would
     also be good for getting this to work

   - ARM hardware would be good to add to the mix, once there's real
     hardware available, QEMU could be used as well. I wouldn't want to
     just use QEMU as then it makes it hard to work out if something is
     failing just on QEMU.

 - Building packages on a range of hardware with a range of
   resources/configurations. This is kind of a better version of just
   building a package multiple times. It would help spot odd failures
   earlier, and give valuable data about the reproducibility of the
   build outputs.

   - Single core vs many cores

   - Not much RAM vs plenty of RAM

   - Different filesystems (btrfs, ext4, ...)

   - Different Linux-libre versions

   - Different system times (2025, 2101, ...) 

   - Maybe building on other OS's (Debian, ...)

   These things require hardware availability and/or being able to
   control resources for a single build through the guix-daemon. It also
   requires a way in the Guix Build Coordinator to specify that you want
   builds to happen in this way, so build a single derivation many
   times, in each of these environments.

This is general quality assurance stuff, which would be valuable to do
on a continuous basis, and while doing this kind of testing is maybe a
bit trickier at patch review time, if you can manage it, it'll stop
problems being introduced.

Something to note here is that as the Guix Build Coordinator is packaged
for Guix, running the agent is pretty easy. So if you have some hardware
to volunteer to build packages changed by patches, and do general
quality assurance stuff, then get in touch! It doesn't even need to be
on all the time, or that reliable.


### Other ideas?

Thanks,

Chris

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

Re: More thoughts on Patchwork and Guix patch review/quality assurance

[Ludovic Courtès]

Hi Chris!

Christopher Baines <mail@cbaines.net> skribis:

> If you're interested in using this to review patches, the place to start
> is Patchwork, so visit:
>
>   https://patchwork.cbaines.net/
>
> If you want an account, please email me. Unfortunately I had to disable
> registration due to spam.
>
> When you click through to a patch, the thing to look at is the
> checks. These should link you to the relevant bug, Git branch and Guix
> Data Service comparison. For the Guix Data Service comparison, the
> things to note are the lint warnings and then clicking through to the
> "Compare package derivations" page.

Very nice!  This is useful info when reviewing, and it’s much harder to
get this at a glance locally.

> ### Use Patchwork, rather than Debbugs to track patches
>
> So this has been a thing for 4 years now (see [2] for some graphs). I
> think I was there in the bar in Brussels when it was discussed.
>
> 2: https://debbugs.gnu.org/rrd/guix-patches.html

I remember that night in Brussels, it’s been a while!  :-)

> The main disadvantage I see of using Debbugs is that you have to get a
> bug number before you can Git send-email, in the case where you're
> sending multiple patches.
>
> The main advantage I get from Debbugs is that searching via the bug
> number is really useful.
>
> I'm unsure, I think Debbugs was introduced to help keep track of
> patches, and avoid them getting forgotten in guix-devel. Patchwork will
> do this too, but maybe Debbugs is providing more value than the cost of
> multi-patch series being slightly more difficult to submit?

To me, the main advantages of Debbugs are:

  1. the Emacs interface (I use it a lot as a reviewer, no need to open
     a browser);

  2. the clean web interface at <https://issues.guix.gnu.org>, which I
     find less intimidating, less cluttered, and hopefully more familiar
     than that of Patchwork; it also has a useful search interface now;

  3. Having numbers and short URLs to reference issues.

Now, Patchwork “checks” and how you use them are great; simplifying
multi-patch submissions is great too.

If Mumi had hooks that could be used to run and display those checks,
I’d be super happy.  But I don’t know whether it’s reasonable, nor
whether that’s something others would also like.  :-)

All this could be complemented by CLIs to “the official Guix review
services” (Patchwork, Mumi, Data Service, etc.).  You could run:

  guix review 1234

and it’d fetch the patch(es), apply them, and/or display a status
summary based on data published by Patchwork & co.  It’s probably not
that much work if those services have HTTP APIs, and it could facilitate
adoption.

> ### Scheduled and regular collaboration on IRC to review patches
>
> When I can make some time, I'll try this out, but if anyone has some
> time they can set aside, please go ahead!

Yes!  A weekly meeting where committers and submitters are around and
focusing on getting patches merged would be great!

> ### Help users with the submitting part of submitting patches
>
> Using git send-email works well with Patchwork at least, and it's fine
> with Debbugs if you've got a single patch, but a little more time
> consuming if you're got more than one patch.
>
> Attaching a single patch file to an email works OK I think, however
> attaching multiple patch files to the same email confuses git am and
> Patchwork I believe.
>
> I think other approaches like copying the patch in to the body of an
> email, or just copying the output of git diff in to an email are very
> brittle and can make it more time consuming to try and recover or
> recreate the commit(s).
>
> Having the emails for patches is useful for reviewing, but maybe there's
> a way users could push a branch somewhere and then have some service do
> the git send-email thing on their behalf?

Yes, that would be nice.  To me that’s less important than getting the
checks you implemented actually used, though.

> ### Notify interested people about patches
>
> This is something I've been thinking about more generally, supporting
> email subscriptions for things like a new version of a package being
> available, or the package failing to build on
> master/staging/core-updates.
>
> But specifically for patches, maybe there could be a way of subscribing
> so that you're emailed when a patch series upgrades a package you're
> subscribed to, or breaks a package you're subscribed to.
>
> This could help get more people involved in reviewing patches, by making
> it easier for the interested people to find out about patches they're
> interested in.

+1!

> ### Helping people who aren't committers review patches
>
> I don't know how much of this happens, but it might be something to
> better support?
>
> Better supporting it could mean documenting how to get things that have
> passed review merged in, like emailing a list of committers when a patch
> series is ready to merge.

Agreed.  Non-committers do review occasionally and I find it very useful.

> ### Making automated testing more rigorous
>
> So, providing the patches can be automatically processed and apply
> successfully, I'm currently building packages for x86_64-linux and
> i686-linux.
>
> There are more things that could be built if I amend the relevant
> script:
>
>  - System tests (maybe just x86_64-linux is relevant?
>
>  - Cross-compiled packages (the Guix Data Service currently only
>    generates these for x86_64-linux)

ci.guix.gnu.org is doing that and (gnu ci) as well as
etc/system-tests.scm and etc/release-manifest.scm formalize it.

> There are also some things that would require more work/hardware:
>
>  - Building packages for architectures other than
>    x86_64-linux/i668-linux.
>
>    - I have some ideas on getting the Guix Build Coordinator agent
>      running in a childhurd, some general substitute availability would
>      also be good for getting this to work

Note that GNU/Hurd currently uses out-of-chroot builds; IMO that part
needs to be addressed (see <https://issues.guix.gnu.org/43857>).

>  - Building packages on a range of hardware with a range of
>    resources/configurations. This is kind of a better version of just
>    building a package multiple times. It would help spot odd failures
>    earlier, and give valuable data about the reproducibility of the
>    build outputs.
>
>    - Single core vs many cores
>
>    - Not much RAM vs plenty of RAM
>
>    - Different filesystems (btrfs, ext4, ...)
>
>    - Different Linux-libre versions
>
>    - Different system times (2025, 2101, ...) 
>
>    - Maybe building on other OS's (Debian, ...)
>
>    These things require hardware availability and/or being able to
>    control resources for a single build through the guix-daemon. It also
>    requires a way in the Guix Build Coordinator to specify that you want
>    builds to happen in this way, so build a single derivation many
>    times, in each of these environments.

Some of these tests could be done by offloading to “childguix” VMs or by
creating derviations that build derivations in VM.

Thanks!

Ludo’.