Group: Guix/PatchReviewSessions2024
Contents
-
1 Patch Review Sessions 2024
- 1.1 Calendar
- 1.2 Session Agenda
- 1.3 Guix systems
- 1.4 Patch review overview
-
1.5 Patch review process - CLI tools
- 1.5.1 1. Get into a constructive state of mind
- 1.5.2 2. Choose something to review
- 1.5.3 3. Preflight checks
- 1.5.4 4. Set yourself as the reviewer
- 1.5.5 5. Create a new worktree
- 1.5.6 6. Find the patch to apply
- 1.5.7 6.2 Import as a patch
- 1.5.8 7.0 Build the patch for Guix
- 1.5.9 8.0 Test install the package
- 1.5.10 9. Review the contribution
- 1.5.11 10. Add a Reviewed-by Trailer
- 1.6 Patch review process - Guix Emacs package
- 1.7 Patch review resources
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. Patch reviewing is a great way to learn about Guix packages and the Guix system generally.
These are great sessions if you'd like to:
- 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:
Our first patch review session. TBC will give an introduction to their process for reviewing patches. We'll then pick some patches to review and work through them as a group.
TBC will give an introduction to their process for reviewing patches. We'll then pick some patches to review and work through them as a group.
TBC will give an introduction to their process for reviewing patches. We'll then pick some patches to review and work through them as a group.
With an introduction by TBC. We'll look at xxxx areas.
TBC will give an introduction to their process for reviewing patches. We'll then pick some patches to review and work through them as a group.
TBC will give an introduction to their process for reviewing patches. We'll then pick some patches to review and work through them as a group.
TBC will give an introduction to their process for reviewing patches. We'll then pick some patches to review and work through them as a group.
Session Agenda
Each session is run at 18:00 UTC, 18:00 BST (London), 19:00 CET (Paris), 13:00 EST (New York) and we'll generally try to:
- Introduction to someone's patch review process (30 mins)
- Patch review together (1 hour)
This will be an informal, friendly and interactive environment where anyone can ask questions, show how they do something or make suggestions. It's on-line which restricts things a little, but the goal is to be interactive and learn together.
If there are particular topics or questions you'd like to explore please add them below.
Next session questions / topics
- [a patch you want to review / a question you have? Add it here with your name]
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 process - CLI tools
1. Get into a constructive state of mind
2. Choose something to review
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 !
Set a user-tag: ::
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.
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
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 --localstatedir=/var --sysconfdir=/etc [env]$ make --jobs 8 [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 --apply-cover-trailers <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 --apply-cover-trailers <msg-id>
# if you downloaded it already use --use-local-mbox b4 shazam --use-local-mbox <cover.mbx> --apply-cover-trailers <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)::w
./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
Patch review process - Guix Emacs package
[link to the guix emacs package]
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.
