Group: Guix/PatchReviewSessions2024
Line 25: | Line 25: | ||
<div class="mw-collapsible-content"> | <div class="mw-collapsible-content"> | ||
This will be a patch review session. | This will be a patch review session. | ||
+ | </div></div> | ||
+ | |||
+ | <div class="toccolours mw-collapsible mw-collapsed" style="width:600px; overflow:auto;"> | ||
+ | <div style="font-weight:bold;line-height:1.6;">Thursday, 29th August 2024 (talk by Jgart)- [https://www.meetup.com/guix-london/events/xxx/ Register on Meetup]</div> | ||
+ | <div class="mw-collapsible-content"> | ||
+ | At this meetup, Jgart will conduct a code review session that will serve a dual-purpose of demoing a killer new feature of the Mumi command-line interface that will 10X your patch review productivity, while also orientating new Guixers to the contribution process by diving into a curated code review together. | ||
+ | </div></div> | ||
+ | |||
+ | <div class="toccolours mw-collapsible mw-collapsed" style="width:600px; overflow:auto;"> | ||
+ | <div style="font-weight:bold;line-height:1.6;">Tuesday, 17th September 2024 (talk by Ekaitz Zarraga)- [https://www.meetup.com/guix-london/events/xxx/ Register on Meetup]</div> | ||
+ | <div class="mw-collapsible-content"> | ||
+ | Ekaitz will be talking about the work that's been done to bootstrap RISV-V. He'll introduce RISC-V itself, what boostrapping is all about, where the work is and where the next challenge is. | ||
</div></div> | </div></div> | ||
Revision as of 15:19, 10 July 2024
Contents
-
1 Patch Review Sessions 2024
- 1.1 Calendar
- 1.2 July 11th Session Agenda (talk by Vagrant Cascadian)
- 1.3 Guix systems
- 1.4 Patch review overview
- 1.5 Patch review states and reports
-
1.6 Patch review process - CLI tools
- 1.6.1 1. Get into a constructive state of mind
- 1.6.2 2. Choose something to review
- 1.6.3 3. Preflight checks
- 1.6.4 4. Set yourself as the reviewer
- 1.6.5 5. Create a new worktree
- 1.6.6 6. Find the patch to apply
- 1.6.7 6.2 Import as a patch
- 1.6.8 7.0 Build the patch for Guix
- 1.6.9 8.0 Test install the package
- 1.6.10 9. Review the contribution
- 1.6.11 10. Add a Reviewed-by Trailer
- 1.6.12 11. Prepare and send the reviewed patches
- 1.6.13 12. Change the issue state
- 1.7 Patch review process - Guix Emacs package
- 1.8 Pairing with Upterm
- 1.9 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.
- 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:
Vagrant Cascadian is a Guix and Debian developer who works on Reproducible Builds. Vagrant will be talking about Reproducible builds and the tools we have in Guix (such as Guix challenge) to tackle this important area. We'll then pick some patches to review and work through them as a group.
This will be a patch review session.
At this meetup, Jgart will conduct a code review session that will serve a dual-purpose of demoing a killer new feature of the Mumi command-line interface that will 10X your patch review productivity, while also orientating new Guixers to the contribution process by diving into a curated code review together.
Ekaitz will be talking about the work that's been done to bootstrap RISV-V. He'll introduce RISC-V itself, what boostrapping is all about, where the work is and where the next challenge is.
July 11th Session Agenda (talk by Vagrant Cascadian)
This session will run at:
17:00 UTC, 18:00 BST (London), 19:00 CEST (Paris), 13:00 EDT (New York)
- Talk by Vagrant Cascadian on Reproducible builds.
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 --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):
./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-reviewed 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.