Latest revision as of 08:06, 5 November 2024

Patch Review Sessions 2024

We're running online patch review sessions so that everyone can become a patch reviewer. The goal of reviewing patches is to help Guix project accept contributions while maintaining our quality standards.

Learn how to do patch reviews together
Do some patch reviews in a friendly hacking session

For each session please join through the Meetup link next to each event to get updates. If you don't want to join through Meetup the session is run on Jitsi at https://meet.jit.si/london-guix-meetup

Calendar

These are the sessions we're planning on running - click Expand to check the topic, and register using the Meetup link next to each one:

Wednesday, October 9th 2024 - Patch review & hacking session - Register on Meetup

It's been a while so we're going to have a patch review and hacking session. There will also be plenty of time for general Guix chat.

Tuesday, 29th October 2024 - Register on Meetup

Patch review session.

Friday, 22nd November 2024 - Register on Meetup

Patch review session.

Thursday, 12th December 2024 - Register on Meetup

Our last Guix Social session of 2024.

9th October Session Agenda

This session will run at:

18:00 UTC, 18:00 GMT (London), 19:00 CET (Paris), 14:00 EDT (New York)

Guix.social updates (upcoming talks etc)
Patch review & hacking
Q&A on the talk / general Guix chat

Guix systems

Guix QA Automatically builds the latest patches that are sent to the devel mailing list. Take the link to patches to see what's recently been built. Click into the issue number and it will cross-link you to other places like Issues and the Patchwork instance.

Guix Issues provides a nice front-end to the data that's stored in the Debbugs instance. It has search and an easy way to get the Message-ID.

Debbugs guix-patches is a shared GNU project debbugs. It's old and difficult to work with - can be better if you use the Debian bts package.

Get patches from guix-patches public inbox, or through Guix Patchwork, or through the bug number on Issues/Debbugs.

Patch review overview

The purpose of a patch review is to make it easier for committer to accept the contribution. This means checking that it follows the generally accepted standards of the project and working with the contributor to help them if possible. Not everyone is a seasoned Guix developer, so think of reviews as having a couple of levels:

1. Submission review

Generally checking that the submission is in good shape:
- Patch commit message and format: correctness, no spelling or punctuation errors
- Package license: check that new package licenses are correct
- Patch application - does it apply cleanly?
- Run guix lint as this gives lots of information

2. Functionality review

Check that the patch does what it's supposed to do.
- Does it build in a repeatable way?
- Does it install correctly?
- Does the package work?
- Does guix lint pass?

3. Code review

Review the code for problems
- Is the code formatted in a way that's similar to other code around it
- Does the change make sense?
- Does it use guix functions correctly?

General patch review comments

Remember someone has put a lot of effort into their contribution - approach it positively
Guix practises one change per patch
If there is a problem be specific about how to reproduce it
When leaving comments for the next reviewer / committer provide some reasonable detail on what you looked at and any questions you had
It's fine not to know something - leave a comment for the committer so they can respond
Participate on the bug - so that your review is part of the conversation around the contribution

Patch review states and reports

Debbugs has some clever capabilities for handling issue reporting and tagging them. Due to the shared configuration we're a bit limited in what we can do, but usertags give us some clear ways set-up reports

The states I've used:

patch-review-hackers-list: patches that will be used in a patch-review session
- This tag is for patches that can be reviewed. Generally that's older patches that aren't currently being built by the QA system.

under-review: patches that someone is actively reviewing.
- It's best if someone reviewing also sets themselves as the 'owner'.

escalated-review-request: patches that are complex or serious concerns where a second pair of eyes is requested.
- This is for patches where the reviewer is saying they don't think it can move forward and they'd like some help.

waiting-on-contributor: patches where some revision is needed by the contributor.
- It's generally fine to revise patches a little bit to help out the contributor, but if it's taking a lot of effort then it may be best to send it back and ask them to revise.

reviewed-looks-good: patches that can proceed to automated building and for a Committer to review
- A committer will still review the patch and they may find other things. There's no 'auto-commit' , but by sending it in this state you should be signalling that you're happy. The cover-letter notes let you ask questions or point out areas.

Patch review process - CLI tools

Tools that are useful are:

stgit (also called StackedGit): presents git commits as a "stack" of patches and it works well with the patch format. Makes it easy to move patches up and down in the stack, and to edit each one as a patch. Underneath it's still git.
bts (Guix package is devscripts): the easiest way to quickly get a Message-ID from debbugs. It can also pull down mbox and shows reports. There's other functionality that doesn't appear to work with GNU's debbugs install.
b4 - the easiest way to massage a mbox and convert into commits. It can check if there are later versions and gets the author and other reviewers right. b4 can also prepare and send patches I think.
Mutt/Neomutt - for reading/sending patches - I actually read the guix-patches list using Neomutt and NNTP, but I'm just strange like that.

As an alternative to playing the mailing list git-pw (not in guix) talks directly to patchworks. If you'd rather deal directly with git then patman lets you keep notes about patches and reviewers in a git commit note.

   git config user.name "Your Name" 
   git config user.email "your@email" 
   git config b4.attestation-policy off 
   git config b4.midmask https://yhetil.org/guix-patches/%s
   # search mask is used when you look for updated trailers
   git config b4.searchmask 'https://yhetil.org/guix-patches/?x=m&t=1&q=%s'

1. Get into a constructive state of mind

Patch review is fun and a great way to learn about various parts of the system. However, it's quite difficult for people to send patches and sometimes it's boring seeing the same "mistakes" multiple times. Try to be nice and polite to contributors!

2. Choose something to review

Use QA and Debbugs to find patches to review. There's plenty of unloved patches in the system:

3. Preflight checks

[ ] check whether Guix QA processed it (for QA go to qa.guix.gnu.org/issues/NNNN)
[ ] existing package - check dependencies and reverse-dependencies
[ ] existing package - check ci/git for existing update: https://git.savannah.gnu.org/cgit/guix.git/log/
[ ] check Guix Issues for existing update:
[ ] look in whereiseveryone toys service in case someone else did a a package

4. Set yourself as the reviewer

Check that no-one else has set themselves as the owner by looking at the bug:

   export DEBEMAIL=your-email@domain.com
   bts --bts-server https://debbugs.gnu.org/ --mbox show NNNN

Make yourself the owner:

   export DEBEMAIL=your-email@domain.com
   bts --bts-server https://debbugs.gnu.org --mutt owner NNNN !

If you are doing it by email:

   to: control@debbugs.gnu.org
   subject: setting owner on NNNN
   owner NNNN !
   quit

The subject doesn't really matter (debbugs will ignore it); the owner can be anyone, the exclamation mark is a shorthand for your own email address

Set a user-tag of under-review:

   bts --bts-server https://debbugs.gnu.org --mutt user guix@debbugs.gnu.org , usertag NNNN + under-review

A bit annoyingly you will have to edit the 'user' part of the email to be 'user guix' without the rest of the email. The comma between the user and the usertag is important as it tells debbugs to format it as two messages.

If you don't want to use Mutt or bts then you can also send a plain text email:

   to: control@debbugs.gnu.org
   subject: <doesn't matter>
   user guix
   usertag NNN + under-review
   quit

The plus sign between the usertag and the usertag itself (under-review in this example) is important.

To check that your bug is now tagged correctly see this Debbugs under-review report. The alternative is:

   export BROWSER=w3m
   bts --bts-server=https://debbugs.gnu.org bugs usertag:under-review users=guix status=open

5. Create a new worktree

Use a worktree or a branch to use with this patch and review. This is just one way to do it:

 # update the local repository
 cd workspace/guix-packages/guix
 git checkout master
 git pull

 # create the worktree: <location it will be> -b <branch> <origin to track> --track
 git worktree add $HOME/user/guix-packages/worktrees/issue-NNNN -b issue-NNNN origin/master --track

5.1 Initialise StackedGit (optional)

Initialise StackedGit on the worktree - I find this easiest because it shows a 'stack of patches'.

 cd $HOME/user/workspace/guix-packages/worktrees/issue-NNNN
 stg init
 stg branch --describe "Patch review for NNNN"

5.2 Build guix

Start a separate build split to build guix:

 $ guix shell --container --nesting --development guix 
 [env]$ make clean && make clean-go
 
 [env]$ ./bootstrap
 [env]$ ./configure
 [env]$ make
 [env] exit

6. Find the patch to apply

Find the patch and download it to the local system so we can apply it.

6.1 Grab it from the bug system (bts/b4)

Open up the bug with bts and read it in Mutt: what's really nice is that we can see the whole thread and use the bug number to find everything. We could save the patch using Mutt, but it's actually really easy to save the whole thread using 'b4'. ::

   export DEBEMAIL=your-email@domain.com
   bts --bts-server https://debbugs.gnu.org/ --mbox show NNNN

In Mutt find the cover letter and hit 'h' to show headers and copy the 'Message-ID'. The easiest way to download the patch with the correct settings is to use b4

   b4 am --check-newer-revisions <Message-ID>
   b4 am -c -t <blah>

What's really nice here is that it downloads the cover-letter separately from the patches in an mbox which makes it easy to go back and re-read the cover (mutt -f xx) if you need a reminder.

NOTE the whole thing could be done in one command bts --bts-server http://debbugs.gnu. org --mbox show 67257 | b4 shazam

6.1 (Option 2) Grab from git

If it's been successfully processed it can be grabbed from:

   https://git.guix-patches.cbaines.net/guix-patches/refs/heads

If it was not processed then we have to get it from the system. If it wasn't then you have to grab the email from the public-inbox mailing list `b4 shazam` will be the easiest way.

6.2 Import as a patch

We have the patch (or the msg-id) so we can now apply it to the local tree:

   b4 shazam <msg-id>

   # if you downloaded it already use --use-local-mbox
   b4 shazam --use-local-mbox <cover.mbx> <msg-id>

If you're using StackedGit you then need to 'suck' the commits that were just done into the the 'Stack'

   # tell it the number to pull in 
   stg uncommit --number=N

   # check it sees the whole stack correctly
   stg series --reverse --indices

7.0 Build the patch for Guix

Start a separate build environment and build the patch that's been applied.

 guix shell --development guix --container --network --nesting coreutils
 ./pre-inst-env guix build <package@version>

8.0 Test install the package

Open a separate terminal or Tmux split to create a test environment:

 guix shell --development guix --container --nesting coreutils nss-certs

 ./pre-inst-env guix package --install <package>

 <try the package out>

8.1 Test the packages build determinism

This tests that the build is deterministic. Switch back into the **build split** (or terminal):

   ./pre-inst-env guix build --no-grafts --no-substitutes --check --source <package>

   ./pre-inst-env guix build --no-grafts --no-substitutes --check <package>

9. Review the contribution

Now it's time to check the patch for commit format, spelling and so forth:

 stg edit

Check the code itself:

 vim <gnu/packages/whatever.scm>
 stg refresh <gnu/packages/whatever.scm>

You can run the styler, but a lot of the code isn't similarly formatted so use carefully:

 ./pre-inst-env guix style --dry-run <package>
 ./pre-ins-env guix style --styling=arguments <package>

Check that guix lint doesn't have a problem:

 ./pre-inst-env guix lint <package>

10. Add a Reviewed-by Trailer

Add a 'Reviewed-by' patch trailer that goes above the `Change-Id` trailer that's automatically provided:

 stg edit

 Reviewed-by: Jane Smith <jsmith@smith.com>

At this point go back to the start to review the next patch in the series. If you're using Stacked Git this should be:

 stg series --reverse --indices
 stg goto N

11. Prepare and send the reviewed patches

At this point there are some patches with local edits, and at a minimum a changed 'Reviewed-by' Trailer. We need to generate a new cover-letter, reroll the patches and send them off.

First get the Message-ID of the original 'cover-letter' that was sent, so our patches are sent as 'replies' to that one:

 export DEBEMAIL=user@userplace.com
 bts --bts-server https://debbugs.gnu.org/ --mbox show NNNN

This brings up the emails in your email client (in Mutt hit h for to see the headers).

We're now ready to export our updated patches:

 $ cd <worktree location>
 $ guix shell --development guix git:send-email
 [env]$ keychain --agents ssh,gpg

 # remove any old patch exports previously done
 $ rm ../stg-out/*

 $ stg email format --output-directory ../stg-out --numbered  --base=auto  \
     --to xxx@debbugs.gnu.org  --in-reply-to <cover-letter-Message-ID> --thread=shallow \
     --cover-letter \
     --reroll-count=2 \
     --all

The ``--thread=shallow`` option means that all emails will be a shallow thread against the message in ``--in-reply-to`` - so just one indent. It's a reroll as we've added the trailers, and particularly if there are any other changes. Emailing to the bug seems to automatically reflect it to guix-patches

Now edit the cover letter and provide your comments from the review:

 vim ../stg-out/v2-000-cover-letter.patch

Describe any changes that were made:

   Review: 
     * submission: checked commit format, synopsis and description
     * submission: license, patch applies cleanly
     * functionality: test installed, guix lint is clean
     * code: checked style to packages around it
     * added: Reviewed-by commit trailer
     * re-roll to trigger QA build

     * Question for committer: XXX

When ready send the email to the bug:

 git send-email --to=xxx@debbugs.gnu.org --cc=guix-patches@gnu.org \
      --no-thread --dry-run --no-chain-reply-to ../stg-out/*

It automatically adds the original person and the Reviewed-by people.

12. Change the issue state

Due to the re-roll the QA system will pick it up, you should also change the state so that others know what to do:

It's currently got the usertag 'under-review' - this can be removed if you've finished.

Three possible options:

1. If there were questions to the developer and it can't move forward -> 'waiting-on-contributor'

2. If there are serious questions that you want other eyes on -> 'escalated-review-request'

3. If it's ready to go forward 'reviewed-looks-good'

Using bts you would do:

   # remove your under-review tag
   export DEBEMAIL=your-email@domain.com
   bts --bts-server https://debbugs.gnu.org --mutt user guix@debbugs.gnu.org , usertag NNNN - under-review

   # add either waiting-on-contributor, escalated-review-request 
   # or reviewed-looks-good
   bts --bts-server https://debbugs.gnu.org --mutt user guix@debbugs.gnu.org , usertag NNNN + reviewed-looks-good

Otherwise, send email:

   to: control <at> debbugs.gnu.org
   subject: reviewed usertag bug#NNNN
   user guix
   usertag NNNN - under-review
   usertag NNNN + reviewed-looks-good
   quit

Check that the bug has had the under-review usertag removed.

Confirm that it's got the waiting-on-contributor, escalated-review-request or reviewed-looks-good usertags.

Congratulations you've done the review - there may be some further conversation and eventually it should be applied - time to look at the next one!

Patch review process - Guix Emacs package

[link to the guix emacs package]

Pairing with Upterm

Upterm is a pair programming capability. We can use it to watch someone doing a demo (read-only) or to work together on a patch (read-write).

If you are the 'leader' this is what you do:

 ssh user@host

 # upterm and tmux have already been configured to start a session
 q

 # to show the details for other users to SSH into do
 upterm session current

All other users connect by:

 ssh <details show from upterm session current>

To leave a session that is read-write do NOT use 'quit' - or you will quite it for everyone. To leave shut the SSH session:

~ .

What do do if you get an odd error message - create a default SSH key:

ssh-keygen -t ed25519 -q -a 50 -f ~/.ssh/id_ed25519 -N ""

Patch review resources

A new Quality Assurance tool for Guix
An overview of the Guix QA system and the work that's been done on it.

Guix Manual: Submitting Patches
How to submit patches to Guix.

How to review patches for PostgresSQL

Drupal's Issue etiquette

Navigation menu

Group: Guix/PatchReviewSessions2024