CodeReviews

Revision 106 as of 2010-12-08 12:57:05

Clear message

Workflow

Sponsoring

Members of the ~ubuntu-sponsors team

  • will triage bugs (in the bug lists mentioned in SponsorshipProcess) weekly,

  • pick bugs they're interested (based on their area of expertise and preference),
  • subscribe to bugs where a review might take longer to themselves to avoid duplication of work.

If the patch was deemed not to be good enough yet, the bug will be re-assigned to the patch author and set to In Progress. The preferred way of closing bugs is ClosingBugsFromChangelog.

Patch Pilots

Inspired by the Bazaar Patch Pilot programme we want to try to have dedicated patch pilots doing their best to help contributors have a good experience getting fixes into Ubuntu.

Patch pilots are available in #ubuntu-devel so you can talk to them and discuss issues there. The current patch pilot is listed in the topic.

The general difference between sponsors and patch pilots is:

  • sponsors will often just pick items from the queue based on their preference (be it urgency or area of expertise, etc.)
  • patch pilots make themselves available on IRC (indicated in the topic in #ubuntu-devel). Just type in @pilot in in the channel and the topic will be updated for you. @pilot out will indicate that you're done for your shift.

  • patch pilots follow these instructions to get patches ready, not necessarily upload them, but do their best to get the item reviewed and it to a state where it can be included

Expectations:

  • The goal is to welcome the contributor.
  • Make sure that your first response thanks them profusely.
  • Making contributors feel welcome and included makes working together more enjoyable and makes collaboration in Ubuntu a lot better. It lightens the load on everyone.
  • Follow these instructions as well as you can

  • Update the topic in #ubuntu-devel to indicate that you're on duty. (@pilot in and @pilot out)

  • Help to get changes to Debian and upstream.
  • If you can't find anything suitable in the QA Sponsoring Report, focus on reviewing the many other patches for Ubuntu

Good tips:

  • When you're piloting, put some concentrated effort into helping people have a good and satisfying experience contributing to Ubuntu. Just how you do that is up to you.
  • Optional: Send a brief mail before and/or after your stint, to say what you're going to do or what you did, and how it worked out. If you have feedback on the review system or the process, speak up.

  • You're not obliged to deal with all the open patches. We appreciate what you do do.
  • You can prioritize whichever you think best achieves the goal of helping people enjoy getting things done in Ubuntu. That might be the newest ones, neglected patches, easy patches, or those from new contributors. The QA Sponsoring Report sorts by age.

  • You may find the sponsor-patch (from ubuntu-dev-tools package) program helpful.

  • If you are unfamiliar with the package, make sure you review everything you can (checklist below), it's not necessarily your job to upload it. If, after you did your review, you can get the contributor in touch with somebody who knows the codebase better, you already helped out a lot.
  • Read more from the Bazaar team.

  • If you can't find anything suitable in the QA Sponsoring Report, focus on reviewing the many other patches for Ubuntu

Schedule

2010 December

Week

Day

Who

Who

Who

49

6

mdeslaur

kees

7

didrocks

ken-vandine

8

doko

kirkland

9

ev

zulcss

10

jr

50

13

sconklin

apw

14

smoser

clint-fewbar

15

seb128

chasedouglas

16

stefan-bader-canonical

17

timg-tpi

51

20

mcasadevall

mvo

21

dholbach

ogra

22

mterry

23

raof

24

Canonical folks will be on holidays

52

27

Canonical folks will be on holidays

28

Canonical folks will be on holidays

29

Canonical folks will be on holidays

30

Canonical folks will be on holidays

31

Canonical folks will be on holidays

2011 January

1

3

4

barry

robert-ancell

5

themuso

brian-murray

6

bryce

chrisccoulson

7

cjwatson

2

10

pitti

11

davewalker

jdstrand

TBD.

(Members of Canonical's Ubuntu Platform team who can upload to the archive will do at least 4 hours of patch piloting a month.)

Keeping the Sponsoring Queue manageable

To avoid making the bug lists too huge, the following measures might be useful.

Bugs fixing small details

  1. Ask the contributor to forward the patch upstream.

  2. Open an empty upstream bug task.
  3. Mark the Ubuntu task as 'Fix Committed'.
  4. Unsubscribe ubuntu-sponsors.

Not suitable for the current release period

  1. Let the contributor know that the patch is not suitable for the current release period.
  2. Unsubscribe ubuntu-sponsors.

  3. Subscribe yourself to the bug report (this ensures it shows up in the following url)
  4. Milestone the bug to 'later'.
  5. Visit https://bugs.launchpad.net/people/+me/+bugs/?field.milestone%3Alist=196 once the new release opens and upload the fix.

Prior to sponsoring

For patch authors who can't upload yet we have the SponsorshipProcess. Unfortunately, there's also a big list of bugs with patches that are not following the process. Likely, because they are not aware of it.

The ~ubuntu-reviews team will

  • review bugs with patches attached
  • review and test the patches
  • upload the patches if they're good (and if the reviewer can upload) or
  • guide the patches towards the SponsorshipProcess

  • if patches are unsuitable add the patch-needswork tag to the bug

As a member of this team you are not expected to be an uploader, but to know the process and to be able to test the patch for suitability.

The common workflow is:

  1. make sure the attached patch is a genuine patch, if it isn't:
    1. let the author know in a bug comment
    2. remove the patch flag
  2. if there's an upstream bug linked and in the upstream bug a conversation about the fix is going on, add the tag patch-needswork

  3. check if the fix is in a code branch, make sure the branch is linked and a proposal for merging has been made
  4. if you can't upload
    1. make use of the SponsorshipProcess and subscribe the right team

Review

As a reviewer you should make sure the following rules are observed:

  • adhere to StableReleaseUpdates, FreezeExceptionProcess, SyncRequestProcess, Merge policy,

  • patches should apply, the resulting package should build and work correctly, built packages should install cleanly,
  • don't hesitate to let the patch go through several review iterations, if you're unsure or unhappy about it.
  • sync requests don't needs sponsoring or uploads; if you're sufficiently happy with the sync request of a non team member, simply
    • mark it as confirmed,
    • add your ACK in a comment,

    • unsubscribe the sponsors team,
    • subscribe ubuntu-archive to the bug.

  • keep the name of the patch author in debian/changelog and use -k<keyid> to sign the package with your key.

  • make sure to ask the patch author to submit the patch to Debian and/or Upstream.

Packages maintained in Launchpad's Code Hosting

Special attention is required if packages are maintained on Launchpad's Code Hosting. You might run into a message like this, when getting the source package:

$ apt-get source ubuntu-artwork
Reading package lists... Done
Building dependency tree
Reading state information... Done
NOTICE: 'ubuntu-artwork' packaging is maintained in the 'Bzr' version control system at:
https://code.launchpad.net/~ubuntu-art-pkg/ubuntu-artwork/ubuntu
Please use:
bzr get https://code.launchpad.net/~ubuntu-art-pkg/ubuntu-artwork/ubuntu
to retrieve the latest (possible unreleased) updates to the package.
[...]
$

In these cases, please either:

  • ask the contributor to register a merge proposal

  • (if you can commit to the branch in question) check out the branch and commit the change

Being picky

It makes sense to be picky in the following cases:

It makes NO sense to be picky in the following cases:

  • small typo in the changelog
  • small mistake somewhere

In the cases of small mistakes, please

  • try to fix them yourself,
  • let the contributor know what you fixed, so they can learn from their mistakes (attach the resulting diff).

This will keep the process lightweight and fast, but still train new contributors.

Credits

  • Give credit where it's due.
  • It's not possible to credit everybody in the Changed-By when you merge multiple debdiffs, but still keep everybody's name in the changelog.

To upload, do a source only build of the package as normal, but make sure that your name is not in the Maintainer: or Changed-By: headers of the changes file. The easiest way to do this is to use the -k option to dpkg-buildpackage or debsign to sign it with your key (but leave it otherwise unchanged). Do not use the -m or -e flags to dpkg-buildpackage!

References

Review Tips

Source

  • Review debdiff of version in the archive with the proposed package.
  • Check the release target in debian/changelog.

  • If it's a new upstream, check if the .orig.tar.gz is the same as the upstream one. (md5sum(1)). The reasons for differing tarballs must be in debian/changelog.

  • Use interdiff(1) (patchutils package) against the .diff.gz and debdiff(1) (devscripts package) against your test-built binary packages to examine what you've changed (and ensure it tallies with what you expected to change).

    • Use filterdiff to exclude generated or annoying parts
  • Check if debian/copyright is correct, accurate and complete.

  • Check for unuseful comments (e.g. autogenerated # dh_X in debian/rules) in scripts

Build

  • Check if the clean target works properly (build twice).
  • NEW packages should be tried to build in pbuilder (to verify sufficient Build-Depends).
  • Run dpkg -c on the built package or debc in the source tree to see if everything installed to the right place.

  • Run lintian on the <package>_<version>_<arch>.changes file.

Forwarding Patches Upstream

Guideline Criteria for New Package Inclusion

A package NEW to Ubuntu should be actively maintained upstream and receive security- and bugfixes regularly. If this can't be fulfilled, the package maintainer is 100% responsible for the above. Since Ubuntu package maintenance is by team, this is a team responsibility. If individuals have a deep interest in a particular package that they want to provide dedicated maintenance to, they should get the package in Debian and maintain it there (issues can still be fixed in Ubuntu as needed, but doing the bulk of the maintenance work in Debian suites both projects better). In every development cycle there are far more new packages for review than can be properly reviewed and included. Redirecting people to Debian to get new packages into Debian and Ubuntu is a great way to reduce this overload and give back to Ubuntu's primary upstream.

Maintainers are encouraged to check if work is already being done by Debian maintainers at (http://www.debian.org/devel/wnpp/requested and http://www.debian.org/devel/wnpp/being_packaged). Following up on existing bug reports with a link to your source package. (Also check out the Debian Mentoring FAQ to find out how to get your package included in Debian.)

  • Packaging review
    1. MUST:
      1. Package must meet Ubuntu versioning & Maintainer requirements

      2. Package must match current Ubuntu (and Debian) packaging policies
      3. Package must build, install, run, remove, and purge cleanly
    2. SHOULD:
      1. Package should be lintian clean
      2. Contents of debian/ should be sane
      3. Changelog should close a "needs-packaging" bug
      4. Package should follow http://www.debian.org/doc/manuals/developers-reference/best-pkging-practices.html

  • Maintenance review
    1. MUST:
      1. Package must contain a watch file or get-orig-source rule
        • If upstream is no more, the packager should consider adopting the upstream package somewhere
        • Packages who implement get-orig-source for packages with watch files get extra points
      2. Packaged version must not have any known security or critical bugs
    2. SHOULD:
      1. Packaging scripts should be readable and readily comprehensible
      2. Upstream should be responsive, and maintain a bug tracker
      3. Packaged version should be latest upstream
      4. Package should not be native without an approved spec
  • Suitability review
    1. MUST:
      1. Package must meet copyright / licensing requirements
      2. Non-native packages must have verifiable cryptographic path to upstream source
      3. Package must be advocated by at least two members of ubuntu-dev (the packager may count as one)
    2. SHOULD:
      1. Package should work on a standard Ubuntu/Kubuntu/Xubuntu/etc. system
      2. Package should provide hints to system services (app-install-data, menus, etc.) to ease installation and use
      3. Package should provide Ubuntu-specific documentation for variances in behaviour from upstream
      4. Package should provide a Homepage: header in debian/control

MUST, as used above, indicates that a package not meeting that test is not appropriate for inclusion in the archive.

SHOULD, as used above, indicates that the reviewer should explicitly agree to the variance from the condition prior to advocating the package for inclusion in the archive.


Go back to UbuntuDevelopment.
CategoryProcess