During our time using review board, I've noticed a few things that make it work better or worse, and I thought I'd share them here in case others were interested:
- Closing requests when they are done (committed, rejected, withdrawn, etc) is really important to keeping the queue useful. If I can't distinguish between reviews that have been languishing unattended for a month or a review that has been committed but not closed it isn't helpful.
- Screenshots for things that change the UI are hugely valuable. I've been tempted to start a "if it has a visual consequence and there is not screenshot, it won't be approved" policy to try and get more people doing this, but I also don't want to raise the bar too high to contribution. Still, screenshots make it so much faster and easier to communicate about a give patch.
- Give the patch a useful name before uploading it, otherwise I end up with a directory full of patches named "bug(N).diff" and other similarly unhelpful information. Names like "improve_foo_visual.diff" are much nicer. It makes it faster to apply them and lets me clean out my patches dir quicker if I can identify which is which.
- Start the diff at the lowest sensible directory. Diffing a change against a component 3 directories deep against the top level module isn't the most helpful: I tend to apply not at the top level but where the work is being done, e.g. at the application's or plugin's directory if in a combined module. For example, if patching the plasma-destop app, start the diff at kdebase/workspace/plasma/desktop/shell as opposed to kdebase or kdebase/workspace. This makes it easy to guess where the diff will start from and leave me at the "right" place in the tree once the diff is applied.
- Keep conversations on the review request. Usually, at least in KDE projects, all comments get CC'd to a mailing list. Replying to mailing list, however, results in the conversation being split up, partly on the review request and partly on the mailing list. Keeping it all on the review request keeps the comments together and closest to the actual data in question.
Do you have your own set of best practices for review board? Share them in the comments! I plan on adding such a list to Techbase for future reference. As review board is becoming a more and more common part of our workflow, using it efficiently is important.
17 comments:
How do you guys cope with the monolithic nature of requests on reviewboard? I hated to use it back then for anything bigger than a simple "fix buy xyz" or "improve feature asdf".
As soon as you start to develop something new alltogether, you often (git svn, or plain git) use lots of commits - maybe even a whole new branch. You cannot use reviewboard in this case since, it only accepts on patch. If you want to add a second one, that depends on some other change, reviewboard will error out (or did this change?).
I'm really interested, as we won't have gitorious' merge requests anymore once we move kdevelop to git.kde.org and I really came to like merge requests (even thought they have their own quirks and issues).
thanks
@milianw: "How do you guys cope with the monolithic nature of requests on reviewboard?"
the answer is to use branches for large developments and do the review "in-scm" and use review board for smaller changes. there are enough smaller changes, particularly ones that still have significant impact and thus beg for review, to make review board highly useful.
The thing is that with in-scm development there's no nice way to comment on changes except by resorting to reading commit-mails and replying to them. Also thats not really possible for getting outside contributions for a KDE project and kdevelop got a couple of such bigger changes recently via MR's. I think we really need to find a suitable replacement in our own infrastructure.
Yea big reviews are annoying on review board.
The solution is have a Git branch, and then just upload the diff of the branch over mainline.
In my opinion everything in reviewboard should be a git branch somewhere. Even trivial patches sometimes break no longer apply againist mainline occasionally. Its easier to keep a git branch up-to-date since it records what version it is a diff against.
So currently its not possible to force all reviews to be in a git branch through technological means. But maybe, like screenshots, it could just be highly encouraged.
Maybe if there was a common "reviewboard" repo for each project and then a way to indicate the branch name in RB.
@apaku: "The thing is that with in-scm development there's no nice way to comment on changes except by resorting to reading commit-mails and replying to them."
branches simplify that quite a bit. inline commenting on a line-by-line basis is still nice, though, if somewhat unwieldy with huge change sets.
"Also thats not really possible for getting outside contributions for a KDE project"
git with branches helps there a lot. they aren't a silver bullet, but they are, with merge requests, a happy medium.
"I think we really need to find a suitable replacement in our own infrastructure."
review board does just fine for things that are hundreds of lines long. for things bigger than that, a gitorious style merge request on steroids would be nice, though.
all imho.
@Ian: "Maybe if there was a common "reviewboard" repo for each project and then a way to indicate the branch name in RB."
fwiu, that's the plan with the new git infrastructure.
Have you looked at Gerrit? It has really tight Git integration, and we have been using it for Avogadro development and more recently starting experimenting with it at Kitware. You add it as an extra git remote, and push to it. Things show up in the interface, when they are merged they are removed.
@Millianw: That's why patch-centric tools such as reviewboard don't work for me. I don't believe that patches are the relevant "unit" here. Personnally I prefer to use a issue-centric approach, so when I develop a feature consisting of 10 patches, I create one issue, attach 10 patches to them, and have all the discussion centralized on this one issue. This way, adjusting approach (replacing patches) is easy and doesn't disrupt the discussion on this issue. IMO the right tool for this issue-centric approach is (newer versions of) Bugzilla, see e.g. this Mozilla issue with lots of patches:
https://bugzilla.mozilla.org/show_bug.cgi?id=564991
Unfortunately, KDE has a apparently an antique bugzilla version and/or terrible bugzilla configuration; but a good recent bugzilla setup is all you need IMO.
Then there's a SCM problem too, you need a SCM with good patch management, but Git is not too bad here with things such as "git stash" though it's not as good imo as hg queues.
@Benoit Jacob: the vast majority of changes requiring review are single change sets. they may be made up of many changes to many files, but they usually make up one change set.
every once in a while we get a multi-repository changeset, and then we get a separate review request for each one. this is, in my mind, not as efficient as it could be (perfect efficiency would be all of those in one review request). it's not horrible, though, as at least all comments for a given changeset are kept together.
and that's with a project (Plasma) that has core shared components in kdelibs and kdebase-runtome, key components kdebase-workspace and an additional collection of components in kdeplasma-addons. we're essentially spread out over 3 or 4 (depending on how you count kdebase) modules, and the problem only rears its head occasionally, and the result is mild inefficiency.
compared to bugzilla, it's a panacea.
in the example you provide there is no threading of discussion, no inline commenting on the patch contents (THE most useful thing ever), only the most rudimentary of version control of the patches as they are updated ("this patch invalidates this older one"), no annotation and commenting of the screen shots .. just one big spew.
and looking at the patches in question: those would all appear as one review request in reviewboard with each change in each file reviewable within it.
if we look at just the number of clicks it takes to offer useful feedback on that bugzilla entry and then measure how easy it is to take that feedback and match it to the patches, bugzilla loses by miles.
i would never want to go back to that horribly antiquated system.
reviewboard certainly has room for improvement, but it's not even comparable to what bugzilla offers, or doesn't as the case may be.
note that reviewboard supports linking review requests to bug reports as well (we use that fairly regularly). this is one area that reviewboard could improve, though: being able to automatically close the review request with the corresponding commit(s)/merge(s) (which would also allow tracking which commit/merge is associated with the successful request), and having an accepted review request be able to close the corresponding bug requests with suitable comments (like we do with BUG: comments in our commit log messages) would be terrific. would require some bugzilla hackery, though.
"@Benoit Jacob: the vast majority of changes requiring review are single change sets. they may be made up of many changes to many files, but they usually make up one change set."
Maybe that is the first problem? It's generally considered a good thing to make changesets as small as possible, though of course there's a limit (ideally each changeset should keep the unit tests passing, etc). I guess that reflects the fact that KDE is still using SVN, and could change a lot once you've switched to Git.
Back on topic, I guess the fundamental question is: what's your project's central development discussion channel. KDE is mailing-list-based, and in that context, reviewboard makes sense. On the other hands, in issue-tracker-based projects, all the cute features that reviewboard brings have a hard time paying for their own cost in terms of disrupting the issue-centric workflow.
@Benoit Jacob: "It's generally considered a good thing to make changesets as small as possible"
agreed. as small as possible, though no smaller. the majority of changes are small. it's a minority of changes that are large enough to benefit from review of whole branches.
so reviewboard helps (a lot) with the majority of changes while keeping the overhead very low.
for new and casual contributors, nearly all changes fall into the "small" category, so as barrier lowering device, reviewboard is also quite valuable.
"I guess that reflects the fact that KDE is still using SVN, and could change a lot once you've switched to Git."
i doubt it. even if/when we put each change into a different branch (either locally or in a shared repository), the ratio of small changes appropriate for tools like review board and large ones is unlikely to change. the small changes may end up having multiple commits during their development (which is a good thing) as well as be more easily compartmentalized during development (also good) and testing multiple overlapping changes will be easier (also good), but at the end of the day the "here's my changes as they stand right now, what is your feedback?" process doesn't change. what is likely to change is that instead of review requests of uncommited work, we'll be looking at review reqests of unmerged work.
"On the other hands, in issue-tracker-based projects, all the cute features that reviewboard brings have a hard time paying for their own cost in terms of disrupting the issue-centric workflow."
first, the features in reviewboard aren't "cute". they are fundamentally better and improving quality, spreading knowledge through feedback and speed up the time to merge.
reviewboard can also be used as discussion centric or issue centric.
i think the challenge you are running into is that reviewboard is a not an issue tracker, but issue trackers don't provide the features of reviewboard. reviewboard allows you to reference issues via the Bug field, but that's about it. allowing discussion (and other links) to replicate from reviewboard to an issue in a tracker like bugzilla as well as have a "closed in one, closed in the other" bridge would likely fix a lot of that. iow, having tighter integration between bugzilla and reviewboard would be great.
projects that are issue centric and do everything via an issue tracker might do well to contribute changes to reviewboard to make that happen.
honestly, it would be a huge turn-off for me if a project was bugzilla centric to the extent that it lacked any other useful collaboration tools. :)
and now that i've said that, it occurs to me that instead of "discussion vs issue" being the determining factor, collaboration and support to casual (inc new) developers vs driving a project by bookkeeping is probably more key of a decision factor in which way a project will lean.
What solution do you have to improve the EBN score of the KDE code base? Recently the number of issues rather went up. Do you favour targeted janitorial teams to solve specific issues across the code base or should a commit or KDE release inclusion depend on solving/improving the EBN issues? What role do you see for static analysis and build bots. Although KDE nurtures the fiction that it is all about code, not builds, a "build early build often" policy and the Mozilla "tinderbox" experience and automated testing looks like a great approach leading to higher code and runtime quality. It is so much more convenient and time-saving to keep clean things clean...
@Kurt - let me comment on this since I am working at Mozilla.
I wouldn't actually suggest copying exactly the Mozilla tinderbox system in KDE, that wouldn't really work, but yes there are many things to copy from it that would help a lot.
For any KDE dev reading this, the relevant Mozilla facts here are:
1. mozilla-central code is nearly 100% covered by unit tests. These can be compiled c++ tests (for low level library features) or HTML pages (comparing rendering pixel-for-pixel, or running JS tests, or measuring load/rendering times...) or browser-interface tests.
2. everytime anyone pushes changesets to the tree, everything is built and all tests are run on all platforms. Any regression means that you have to back out your changeset.
3. you can see the results here:
http://tbpl.mozilla.org/
4. there is also a "try server" to which you can push changes to see if they cause any problem, see:
http://tbpl.mozilla.org/?tree=MozillaTry
5. there is a strong emphasis on bugs blocking releases (which means either they get fixed, or the changes get dropped, or the release is delayed)
Advantages: Mozilla's quality assurance standards are much, much higher than KDE's.
Inconvenients: it's painful to push anything to the tree as you have to be on hold until your changes have passed all builds/tests; it requires a huge amount of computing power; it doesn't scale all that well as any change on any part of mozilla code triggers a whole rebuild; there is a lot of noise with false positives / intermittent failures.
I would definitely suggest adopting these aspects in KDE:
A. a much greater emphasis on unit testing (and, beyond that, testability in the first place should be a major aspect of software design from an early stage on). Yes I know that KDE has some unit tests, especially in kdelibs, but every app should be unit tested.
B. Have at least some build/test servers.
C. Have more serious discussion of blocking bugs, see point 5 above. Develop the notion that no matter how cool a feature is, it can't land until its serious regressions are fixed.
Just my 2 cents :)
@ Kurt: let me comment on this since I work at Mozilla.
There are parts of Mozilla's methodology with tests that would benefit any project, including KDE; there are other parts about which I'm not so sure :)
Yes a tinderbox-like system would help a lot (see http://tbpl.mozilla.org). But in order for it to be useful, the software needs to have enough unit tests, and KDE's first problem here is that it doesn't have nearly enough unit tests (at least most KDE apps don't).
What I would NOT copy from Mozilla is how everytime you push to the repo, everything is built on all platforms and all tests are run, and if anything fails you must immediately back out. At least some modularity would help here --- while it's true that a push to kdelibs could break any app hence it's useful to rebuild everything, a push to, say, kdegraphics should not trigger a full rebuild of every other module.
A little bit offtopic comment may be.
Mr. Seigo, did you see this?
http://www.markshuttleworth.com/archives/551
Can you make some comments? What does it mean for KDE? Do you have any projects with Wayland?
Thank you in advance:)
Hi,
Just to throw an idea I had into the ring ... would it be useful to be able to attach a Git Bundle to a RB request?
This would potentially offer the same as an external contributor pushing a clone to somewhere public like github/gitorious and pointing people to it - except that it's easier (as they don't even need an account at a git hosting service).
It's also less bandwidth intensive to upload a bundle containing just a topic branch than to push a clone of an entire (potentially large) repo somewhere.
Cheers,
David.
Post a Comment