Review Thoughts

As at March, 2011 our processes are fine for the cost but can lead to patches being unreviewed and untested for up to four weeks. This page captures what we currently have and some ideas for improving for the same or less cost.

Michael is concerned that we're spending too much time on correctness and integration. Spending more time would be a hard to justify.

For a given patch, the following happens:

  • Developer:
    • Creates a feature branch
    • Fixes the problem or adds the feature
    • Sends a merge request
  • After the merge request, the change is:
    • Reviewed
    • Merged into an integration branch
    • Built
    • Tested using the GCC testsuite and others
    • Merged into trunk

The interesting parts out of that are:

  • Review
  • Build
  • Test
  • Merge

Currently Andrew does all of these steps in one burst near the end of the monthly cycle. The review is of first-pass quality. It costs about the same to do five merge requests as one, so doing things every week would increase the cost four times.

Note that gcc-linaro is a mature product. Running the testsuite properly requires access to three architectures and days of computing time.

The costly parts are:

  • Build - the build is automated, but does require watching and reporting
  • Test - same as for build
  • Merge:
    • There are always conflicts, at least in the ChangeLog

    • bzr itself takes time to pull and merge

The potentially costly parts are:

  • Review:
    • Code has faults that would be found through review
    • More eyes are better
    • Early is better
    • Outside the group, such as Matthias using fixes for a rebuild, is better
  • Combinations:
    • New faults due to combined effects are detected late
    • Patches exist in isolation until merged

Ideas

Ramana, Andrew, and Michael talked this over on 2011-03-07. The possible approaches are:

  1. Feature branch / Review / Build+Test+Merge / Release (the current method)
  2. Feature branch / Auto Build+Test of the branch / Review / Build+Test+Merge / Release
  3. Feature branch / Auto Merge into '-next' branch / Auto Build+Test / Review / Build+Test+Merge / Release
  4. Feature branch / Developer merges to trunk / Auto Build+Test of trunk / Review / Release

For want of better names, Michael calls these 'Current', 'Auto Current', 'Next', and 'On Trunk'.

'Auto Current' monitors for new merge requests and fires off builds of the feature branch. The builder would put a comment on the request with the build results and give it a +1 if the build passes. This brings the Build+Test stage earlier and gives you more confidence in the patch.

'Next' monitors merge requests and automatically merges these into a 'gcc-linaro-next' branch. Conflicts in common, unimportant files like the ChangeLog would be discarded and as such wouldn't replace the manual merge. Conflicts in other files would need costly manual intervention. This approach brings the Build+Test stage earlier and tests for combinations of patches.

'On Trunk' pushes the merge cost onto the developer. The developer can either merge immediately if confident, or otherwise wait for review before merging. The bot will monitor the branch and Build+Test on every checkin. This approach brings the Build+Test and Merge stages earlier and tests for combinations of patches. The traditional problem with on trunk is having an unstable trunk and people being blocked while trunk is fixed. This is probably not a concern with modern VCS such as bzr or git as you can easily branch from a 'known good' location and then merge into trunk.

Discussion

'Next' is too much work for too little benefit. Both 'Auto Current' and 'On Trunk' are fine. 'On Trunk' has more advantages for the same cost, but only if bzr handles the branch earlier/merge trunk step well.

None of this addresses review. Patches that are done by FSF trunk will get review via the wider GCC community. For Linaro specific patches, we could:

  • Assign a group, say of three, who should review
  • Do the same, but rotate people so that each week one person (two?) is responsible for reviewing patches in a timely manor.

Follow up

Ramana, Andrew, and Michael talked this over on 2011-03-14. The discussion also went on on linaro-dev@:

The two candidates are 'Auto Current' and 'On Trunk'. The main difference between them is who does the merge: with auto current it's the merge master, with on trunk it's the developer.

We discussed having a review roster where every week someone would be responsible for reviewing any new merge requests in a timely manor. This seems fine and makes responsibilities explicit.

Decision

  • Pick 'On Trunk'.
  • Start a review roster
  • Automatically build trunk on any change
  • Automatically build any branch with a new merge request

For the merge request handling:

  • Builds all supported architectures
  • Need to ensure enough capacity that any merge request is built within 24 hours + the time to build (currently ~48 hours)
  • For the merge request:
    • Add a vote as each architecture builds and tests
    • Add a link to the logs
    • Stage 2: send the delta in the testsuite
  • Consider adding cross compiler tests for the architectures we support, and those that we have experience in (powerpc + mips)

For stage 1, assume that bzr is cheap enough. Therefore:

  • Obvious fixes may be committed directly
  • All other changes should go through a feature branch

For stage 2, consider an upstream model where patches for small features can be sent to linaro-toolchain@. This will need some way of building.

MichaelHope/Sandbox/ReviewThoughts (last modified 2011-03-15 00:30:30)