== Dev Week --  Operation Cleansweep - reviewing patches -- nigelbabu &  DavidFutcher -- Wed, Jul 14th, 2010 ==
{{{
(12:00:46 PM) nigelb: ok! Now, we're kicking
(12:00:54 PM) nigelb: Hello everyone, I'm Nigel and with me is David.  We're going to talk about patch review.
(12:01:03 PM) ***bobbo waves
(12:01:08 PM) nigelb: I'd like you all to take a look at https://wiki.ubuntu.com/OperationCleansweep
(12:01:32 PM) nigelb: This cycle, this is a very critical operation that we'd like to see carried out.  As you can read
(12:01:40 PM) nigelb: This cycle, this is a very critical operation that we'd like to see carried out.  As you can read
(12:01:43 PM) nigelb: grr
(12:01:51 PM) nigelb: in the wiki page, operation cleansweep is about reviewing all the bugs in Launchpad against the Ubuntu project which have a patch attached.
(12:01:59 PM) nigelb: (sorry folks, paste fail ;)
(12:02:17 PM) nigelb: We have a lot of awesome contributors who write code to fix bugs, but we lose out on this awesomeness because we're slow to review the patches.
(12:02:26 PM) nigelb: Its not something thats new to an open source project, but its something that we in Ubuntu would like to pay attention to and we NEED your help!
(12:02:54 PM) bobbo: So how can you help out with patch reviews?
(12:03:05 PM) bobbo: We have a script which automatically finds bugs with patches that need reviewing
(12:03:20 PM) bobbo:  Of course, it can't decide whether the patch is good or not, we need a human for this. This is where you guys come in
(12:03:37 PM) bobbo: The script subscribes us to all Ubuntu bugs with a patch attached minus the packages that we exclude (https://wiki.ubuntu.com/ReviewersTeam/ExcludingPackages)
(12:04:02 PM) bobbo: The first step to reviewing patches is to look at the current work queue.
(12:04:40 PM) bobbo: We've got a big Launchpad query which filters out all the reviewed patches and just shows the ones we need to look at
(12:05:01 PM) bobbo: You can find the bug list at http://tinyurl.com/2u7kf3b
(12:05:33 PM) bobbo: AS you can see was have currently around 1500 bugs in the unreviewed queue
(12:05:46 PM) bobbo: Those are bugs that have patches that need to be reviewed and we haven't gotten around to do
(12:06:08 PM) bobbo: Just for perspective, we started out with just under 2000 bugs in that list
(12:06:28 PM) bobbo: Yeah, we're getting there, but unfortunately new patches are added every day!  We get almost 100 new patches a month, maybe more.
(12:06:40 PM) bobbo: also nigelb's laptop just broke, so that's slowing us down too :D
(12:06:50 PM) nigelb: heh, its getting fixed :)
(12:06:55 PM) nigelb: We'll quickly summarize the process and we will show you some specific examples to get a better grasp of the idea.
(12:07:11 PM) nigelb: Can all of us open the review guide?  Its at https://wiki.ubuntu.com/ReviewersTeam/ReviewGuide
(12:07:37 PM) nigelb: So now you have the list of bugs.  Pick a bug to work on.
(12:07:47 PM) nigelb: Once you pick a bug, you have to reproduce the bug again.  Sometimes the bug is fixed and someone forgot to close it from changelog or it was fixed upstream.
(12:08:09 PM) nigelb: No point of the patch if the bug is already solved.  Close the bug as "Fix Released"
(12:08:22 PM) nigelb: If the bug is reproducible, the next step is to check if the patch applies.
(12:08:44 PM) nigelb: If the patch is old, the source may have changed so much that the patch does not apply any longer.
(12:09:12 PM) nigelb: If the patch fails to apply or fix the bug, you should add the patch-needswork tag and ideally leave a comment explaining what is wrong with the patch
(12:09:22 PM) nigelb: and ask the original author if he can re-write the patch.
(12:10:30 PM) bobbo: f you can make sense of the code enough to correct, that would be even more great
(12:10:37 PM) bobbo: fail, If*
(12:10:52 PM) bobbo: This bug though will be off our list since we can't work with the patch unless its reworked.
(12:11:10 PM) bobbo: If the patch applies and the bug still exists, we need to check if it fixes the bug.
(12:11:27 PM) bobbo: Again, no point in a patch if the bug is present after applying the patch and run the program again.
(12:12:06 PM) bobbo: If the patch works, we have a few options
(12:12:36 PM) bobbo: Ideally we should forward it upstream for their take on it because we don't want to conflict with them if they have a better way of fixing it or if we introduce some dangerous regressions
(12:12:51 PM) bobbo: Also, since upstream is the original author of the code, they have a good idea of what makes sense.
(12:13:28 PM) nigelb: Encourage the patch author to forward the bug upstream or forward the bug upstream yourself, once the patch is forwarded upstream, add the patch-fowarded-upstream tag.
(12:13:42 PM) nigelb: This helps us keep track of the good that came out of it (and jcastro gets the credit :p)
(12:14:07 PM) nigelb: Depending on what upstream feels about it, you may need to modify the tag to patch-accepted-upstream or patch-rejected-upstream.
(12:14:21 PM) nigelb: Sometimes though, the bug maybe a bit more critical than to wait for upstream to fix it and we're sure the patch is going to help us.
(12:14:38 PM) nigelb: In that case, we forward the bug to Debian and add a 'patch-forwarded-debian' tag.
(12:15:24 PM) bobbo: Again, depending on feedback from Debian, we mark the patch as and patch-accepted-upstream and patch-rejected-upstream.
(12:15:44 PM) bobbo:  In very rare cases though, the patch is just wrong and we have to reject it.
(12:16:02 PM) bobbo: In that case, we tag the bug with 'patch-rejected'
(12:16:38 PM) bobbo: In other cases the bug is so severe that the need to deal with it as quickly as possible
(12:17:11 PM) bobbo: or the patch is so perfect, well tested and unlikely to cause regressions that we won't lose sleep if we apply it without upstream looking at it
(12:17:37 PM) bobbo: In these cases, the patch is applied in the Ubuntu package, and then forwarded to upstream or debian
(12:17:59 PM) bobbo: One extra complication is that sometimes patches onfly affect the packaging of an application
(12:18:23 PM) nigelb: ok, so you've all asked some questions, we'll be answering those now
(12:18:31 PM) ClassBot: devildante67 asked: Does Operation Cleansweep include bzr branches or only patches?
(12:18:59 PM) nigelb: It includes only patches.  the bzr branches would follow udd and are sponsored in directly.
(12:19:08 PM) nigelb: udd = ubuntu distributed development
(12:19:29 PM) ClassBot: joaopinto asked: when we have the technical ability to do so, is it ok to rework the patch ourselves or should we wait for the path feedback instead ?
(12:19:52 PM) nigelb: like bobbo said earlier, if you can rework it, by all means, go ahead :)
(12:20:10 PM) ClassBot: AudioFranky asked: How do I know/find the exact Ubuntu source package version against which the original patch submitted created his patch?
(12:20:45 PM) nigelb: If a bug was reported via apport, should be there in the bug report.  Otherwise, its going to be a bit of guesswork based on version of ubuntu, date of filing, etc
(12:21:00 PM) ClassBot: joaopinto asked: are those tags/workflow written somewhere ?
(12:21:09 PM) bobbo: You can often check using the date the bug was reported and matching it up with the changelog :)
(12:21:22 PM) nigelb: thanks bobbo :)
(12:21:41 PM) nigelb: Yes, Review process is documented
(12:21:44 PM) bobbo: https://wiki.ubuntu.com/ReviewersTeam/ReviewGuide#Workflow
(12:21:47 PM) nigelb: https://wiki.ubuntu.com/ReviewersTeam/ReviewGuide is your friend :)
(12:21:59 PM) ClassBot: mythos asked: the ability of the reviewer is never checked, so everybody can review patches?
(12:22:38 PM) nigelb: Yes, everyone can review patches.  Noone blocks you on anything.  There is a team, but not being a member does not block you from contributing.
(12:23:02 PM) ClassBot: dupondje asked: when a patch is created for a important bug in for example lucid, and its fixed in maverick, do we still need to review it to get it into lucid then ?
(12:23:25 PM) nigelb: It is a case-by-case situation, but this calls for an sru and the sru process
(12:23:50 PM) nigelb: often, I've taken up sru for hardy or earlier releases when the patch was there, but it was fixed in a later ubuntu release and everyone forgot about sru
(12:23:56 PM) bobbo: sru = Stable Release Update
(12:24:43 PM) nigelb: ok, so ClassBot tells me the good news that question queue is empty
(12:24:52 PM) nigelb: lets move onto some specfic examples
(12:25:07 PM) nigelb: Bug #544242
(12:25:08 PM) ubot2: Launchpad bug 544242 in empathy (Ubuntu) (and 1 other project) "[PATCH] Empathy should allow users to toggle auto-away mode on/off (affects: 1) (heat: 38)" [Wishlist,Fix released] https://launchpad.net/bugs/544242
(12:25:22 PM) nigelb:  This bug was opened with a patch provided by the  reporter.  It was subscribed by the subscription script with the patch  tag.
(12:25:51 PM) nigelb: The patch was forwarded upstream, and recieved the patch-forwarded-upstream  tag
(12:26:07 PM) nigelb: After upstream accepted this patch, it recieved the patch-accepted-upstream  tag and is ready to be fixed in Ubuntu.
(12:26:34 PM) bobbo: Bug #544242
(12:26:35 PM) ubot2: Launchpad bug 544242 in empathy (Ubuntu) (and 1 other project) "[PATCH] Empathy should allow users to toggle auto-away mode on/off (affects: 1) (heat: 38)" [Wishlist,Fix released] https://launchpad.net/bugs/544242
(12:26:59 PM) bobbo: fail
(12:27:07 PM) bobbo: Bug #462193
(12:27:08 PM) ubot2: Launchpad bug 462193 in djvulibre (Debian) (and 2 other projects) "djvulibre-bin produces garbage in the root (/man1/*) (affects: 16) (dups: 2) (heat: 56)" [Unknown,Unknown] https://launchpad.net/bugs/462193
(12:27:32 PM) bobbo: This patch only had to go to Debian as the changes only affected the application's packaging
(12:27:57 PM) bobbo: it makes non sense in most cases to send patches against the packaging to the upstream as they normally don't touch it at all
(12:28:12 PM) bobbo: so it was forwarded to Debian and received patch-forwarded-debian
(12:28:20 PM) bobbo: it was then accepted and receive patch-accepted-debian
(12:28:45 PM) bobbo: and now we're waiting for the fix to be merged or synced into the Ubuntu archives
(12:29:14 PM) bobbo: One last example
(12:29:19 PM) ClassBot: joaopinto asked: Once a bug gets patch-accepted-upstream, how do we ensure that the patch is applied to the current development package ?
(12:29:36 PM) nigelb: bobbo: finish the example and we'll answer this :)
(12:29:41 PM) bobbo: okay :)
(12:29:56 PM) bobbo: Bug #33288
(12:30:01 PM) ubot2: Launchpad bug 33288 in poppler (Ubuntu Lucid) (and 2 other projects) "Evince doesn't handle columns properly (affects: 28) (dups: 9) (heat: 191)" [Medium,Fix released] https://launchpad.net/bugs/33288
(12:30:33 PM) nigelb: ah, that was a famous one :)
(12:30:40 PM) bobbo: The patch was forwarded upstream, where it was rejected
(12:31:12 PM) bobbo: it's now been fixed, however with a different patch
(12:31:31 PM) bobbo: </examples>
(12:31:40 PM) nigelb: heh, thanks bobbo :)
(12:31:48 PM) nigelb: ok, so about the question
(12:32:11 PM) nigelb: in our hurry to write this session up, we missed a step, which actually answers the question
(12:32:47 PM) nigelb: when you decide to change the tga or comment on the bug, subscribe yourself to the bug.  That way you can keep track of the bug.
(12:33:21 PM) nigelb: When the patch is accepted upstream, you can either get it into ubuntu directly if its very severe or wait for it to flow downstream
(12:33:53 PM) nigelb: But essentially, our mission would be complete, i.e. patches don't rot in launchpad and die
(12:34:17 PM) nigelb: Also, contributors who write awesome patches don't get discouraged from writing more code
(12:35:01 PM) ClassBot: devildante67 asked: In the last example, the patch has been accepted, but the upstream bug is still set to Confirmed, even with the tag patch-accepted-upstram. How's that possible?
(12:35:09 PM) bobbo: I've just checked this up
(12:35:19 PM) bobbo: either the upstream bug report simply hasn't been updated
(12:36:11 PM) nigelb: the upstream bug hasn't been updated - there is a comment "pushed to git master"
(12:36:16 PM) bobbo: or it may be because we have cherry picked our patch from their git repository and they don'[t move from COnfirmed until it's actually been pushed out in a final release
(12:36:57 PM) nigelb: joaopinto: To answer your question, you have to take the initiative.
(12:37:26 PM) nigelb: If its a main application, talk to desktop team or other subteams if its in their packageset
(12:37:34 PM) nigelb: they'll glady welcome the help
(12:37:55 PM) nigelb: If its universe, try mailing the DM who's responsible for the package.
(12:37:56 PM) bobbo: as long as there is an active package maintainer in Debian, all upstream accepted patches should filter down at some point
(12:38:28 PM) nigelb: Now, if there is no active maintainer and the patch is good, you have to do some effort on the debian side
(12:39:44 PM) nigelb: If a package is orphaned in debian, you can do a QA upload.
(12:39:59 PM) nigelb: You'll need a DD who can sponsor you though.
(12:40:24 PM) nigelb: A quick mail to  debian-mentors mailing list should help you in that case
(12:40:34 PM) bobbo: If you're not a packager, you can tell a MOTU and they'll hopefully keep track of the package for you
(12:40:58 PM) nigelb: I'd like to point your attention to a package called galrey
(12:41:01 PM) nigelb: http://packages.debian.org/sid/galrey
(12:41:01 PM) bobbo: dropping a mail to the MOTU mailing list or shouting in #ubuntu-motu will at least put it onto a packager's radar
(12:41:38 PM) nigelb: this package had a patch in ubuntu, its an orphaned package in debian
(12:42:08 PM) nigelb: I spoke to a MOTU friend and she recommended uploading to debian directly, since we don't want to carry a diff.
(12:42:41 PM) nigelb: A quick hop into oftc, #debian-mentors and in some time it was awaiting sponsorship and the DD who guided me uploaded into the debian archive
(12:42:52 PM) nigelb: I put in a sync request and we got into lucid in a few days
(12:44:00 PM) nigelb: joaopinto: I'll answer in here for that one
(12:44:20 PM) nigelb: joaopinto asked "that's a bit odd, because a bug reporter expects a bug to be fixed for the current release, isn't that the goal of testing development releases :) ?"
(12:44:58 PM) nigelb: Expectations are high, but that leads to trouble like diverging from upstream and diverging from debian.
(12:45:23 PM) nigelb: If you've every done a 3-way merge, you'll know that we *really* don't want that
(12:46:03 PM) nigelb: When packages are synced that's a better use of our time.  Even if we don't work on Ubuntu directly at times.
(12:46:38 PM) nigelb: Its a bit of a pain, but the reward is much better.  More distro's carry the patch and more people have a happy time.
(12:46:55 PM) nigelb: joaopinto: does that answer your question to some extent?
(12:47:47 PM) nigelb: Anymore questions folks?
(12:48:18 PM) nigelb: You see, bobbo and I both lost notes for this session and wrote it in 20 minutes before session started, so its a bit short :)
(12:48:19 PM) bobbo: dupondje asked: if a bugreport has status 'incomplete' it has been rejected and doesn't need check anymore? Also 'Fix Commited' means is has been accepted? So no check needed neither ?
(12:48:48 PM) nigelb: incomplete = we need more information abaout the bug from the reporter
(12:48:49 PM) bobbo: Incomplete means that not enough information has been given in the report and we have asked for more
(12:49:01 PM) bobbo: Fix Committed is a bit of an odd one in Ubuntu
(12:49:13 PM) bobbo: it can mean many different things and it's often not used correctly
(12:49:33 PM) nigelb: and desktop team uses it for a specific purpose which confuses us often.
(12:50:19 PM) nigelb: Generally, fixed commited means the fix in repositry, but not yet rolled into a release.
(12:50:31 PM) bobbo: basically for patch revirew purposes, it's best to ignore Fix Committed and focus on the patch-* tags on the bug instead
(12:50:58 PM) ClassBot: dupondje asked: if a bugreport has status 'incomplete' it has been rejected and doesn't need check anymore? Also 'Fix Commited' means is has been accepted? So no check needed neither ?
(12:51:09 PM) nigelb: we did that one :)
(12:51:11 PM) bobbo: fail, I can't work classbot
(12:51:23 PM) nigelb: heh
(12:51:37 PM) bobbo: empty queue, does anyone have any questions for the final 9 minutes?
(12:52:22 PM) bobbo: none? Okay then
(12:52:37 PM) bobbo: We'd lvoe for you guys to help out reviewing patches and reach the goals of Operation Cleansweep
(12:52:49 PM) nigelb: Join us in #ubuntu-reviews
(12:52:58 PM) nigelb: talk about it at your loco events
(12:53:03 PM) bobbo: if you want more information or would liek some help getting started please feel free to drop into #ubuntu-reviews
(12:53:16 PM) nigelb: Help us by reviewing 1 patch a day
(12:53:31 PM) nigelb: Also, we have this beautiful progressbar that daker made for us
(12:53:46 PM) nigelb: if you can showcase it on your website or blog that would be great
(12:54:03 PM) ClassBot: devildante67 asked: Is Operation Cleansweep a Maverick only operation, or do you plan on extending it for future releases?
(12:54:30 PM) bobbo: The original plan was to get all 2000 patches reviewed by this cycle's releaase date
(12:54:30 PM) nigelb: The focus is on getting unreviewed patches to 0 by maverick release
(12:54:55 PM) nigelb: we'll have more patches and then our focus would be keeping it low all the time, reviewing them instantly
(12:55:23 PM) bobbo: This i project was started to get rid of the huge pile of patches sitting in LP
(12:55:43 PM) bobbo: so that in future, patches will be reviewed much quicker
(12:55:56 PM) nigelb: jono just spontaneously came up with the idea
(12:56:08 PM) nigelb: (and spontaneously assigned it to me :p)
(12:56:33 PM) dholbach: nigelb: you asked for it :)
(12:56:43 PM) nigelb: dholbach: heh
(12:56:50 PM) ClassBot: ean5533 asked: So who's fault is it that it got this bad? nigelb or bobbo?
(12:56:54 PM) nigelb: Well, the situation got bad because we never had a system to deal with it
(12:57:16 PM) nigelb: About 6 months back, I was innocently hanging out in #ubuntu-motu and said "I'm bored"
(12:57:28 PM) bobbo: (big mistake)
(12:57:54 PM) nigelb: yes, in retrospect :p
(12:57:54 PM) ClassBot: penguin42 asked: What about patches in debian bug tracking system that fix bugs shared with us?
(12:58:14 PM) nigelb: Emmet a.k.a. persia said, "go review patches" and I started writing the workflow which we all perfected.
(12:58:14 PM) bobbo: They are essentially patch-forwarded-debian
(12:58:34 PM) bobbo: Okay we'd better clear off before the next session
(12:58:50 PM) bobbo: thanks for listening and please get involved with Operation Cleansweep!
(12:59:23 PM) nigelb: Yes, Thank you folks.  I'm not going anywhere though.  I'll talk about the "how to forward patches" with pedro :)
(12:59:28 PM) pedro_: We're having a bug day for Operation Cleansweep next week so stay tune ;-)
}}}