CodeReviews

Differences between revisions 24 and 26 (spanning 2 versions)
Revision 24 as of 2008-08-06 17:01:20
Size: 8813
Editor: localhost
Comment:
Revision 26 as of 2008-10-23 10:33:42
Size: 9708
Editor: i59F70A30
Comment:
Deletions are marked like this. Additions are marked like this.
Line 2: Line 2:
Line 10: Line 9:
 * `needs-packaging` bugs that are marked as `Fix Committed`: https://bugs.launchpad.net/ubuntu/+bugs?field.status%3Alist=Fix+Committed&field.tag=needs-packaging
Line 13: Line 11:
|| '''main''' / '''restricted''' || https://launchpad.net/~ubuntu-main-sponsors || https://lists.ubuntu.com/mailman/listinfo/ubuntu-main-sponsors ||
|| '''universe''' / '''multiverse''' || https://launchpad.net/~ubuntu-universe-sponsors || https://lists.ubuntu.com/mailman/listinfo/ubuntu-universe-sponsors ||
|| '''Section''' || '''LP Team''' || '''Mailing List''' || '''Who''' ||
|| '''main''' / '''restricted''' || https://launchpad.net/~ubuntu-main-sponsors || https://lists.ubuntu.com/mailman/listinfo/ubuntu-main-sponsors || anybody in `~ubuntu-core-dev` who likes to help out ||
|| '''universe''' / '''multiverse''' || https://launchpad.net/~ubuntu-universe-sponsors || https://lists.ubuntu.com/mailman/listinfo/ubuntu-universe-sponsors || anybody in `~ubuntu-dev` who likes to help out ||

Still efforts of interested new contributors in reviewing patches are appreciated.
Line 19: Line 20:
 * have either to be in `~ubuntu-core-dev` (for `main`/`restricted`) or `~ubuntu-dev` (for `universe`/`multiverse`) to be able to upload reviewed patches. Still efforts of interested new contributors in reviewing patches are appreciated,
Line 22: Line 22:
 * assign bugs where a review might take longer to themselves to avoid duplication of work.  * subscribe to bugs where a review might take longer to themselves to avoid duplication of work.
Line 26: Line 26:
The administrators of the team will
 * review the list of bugs weekly,
 * assign long standing bugs (>= 2 weeks) to team members who might be up to the job.

Possible conflicts and problems will be discussed in the last minutes of the weekly Distro Team meeting.
Line 35: Line 30:
To avoid the bug lists to huge, the following measures might be useful. To avoid the bug lists to huge, the following measures might be useful. ''(Note: to unsubscribe the team, there's a [[http://people.ubuntu.com/~dholbach/unsub|small script]] available.)''
Line 53: Line 48:

Line 56: Line 49:
Line 69: Line 61:
http://people.ubuntu.com/~dholbach/sponsoring/ has an overview of the bugs in those lists with assignees. == Being picky ==

It makes sense to be picky in the following cases:
 * the reason for the patch is not documented very well
 * the origin of the patch is not documented very well
 * [[Bugs/Upstream|Upstream]] should comment about the patch first (especially [[Debian/Bugs]])
 * [[UbuntuDevelopment/PatchTaggingGuidelines]] is not adhered to

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`!
Line 74: Line 89:
 * https://help.launchpad.net/PPAQuickStart  * https://help.launchpad.net/PPA

Team Purpose

A team of UbuntuDevelopers has agreed on doing periodical reviews of code. To accelerate SponsorshipProcess members of the team will check

If you are a member of the UbuntuDevelopers, it's highly appreciated if you join the team and help out:

Section

LP Team

Mailing List

Who

main / restricted

https://launchpad.net/~ubuntu-main-sponsors

https://lists.ubuntu.com/mailman/listinfo/ubuntu-main-sponsors

anybody in ~ubuntu-core-dev who likes to help out

universe / multiverse

https://launchpad.net/~ubuntu-universe-sponsors

https://lists.ubuntu.com/mailman/listinfo/ubuntu-universe-sponsors

anybody in ~ubuntu-dev who likes to help out

Still efforts of interested new contributors in reviewing patches are appreciated.

Workflow

Members of the team

  • will triage bugs (in the bug lists mentioned above) 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.

Keeping the Sponsoring Queue manageable

To avoid the bug lists to huge, the following measures might be useful. (Note: to unsubscribe the team, there's a small script available.)

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 either ubuntu-universe-sponsors or ubuntu-main-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 either ubuntu-universe-sponsors or ubuntu-main-sponsors.

  3. Milestone the bug to 'later'.
  4. Visit https://bugs.launchpad.net/people/+me/+bugs/?field.milestone%3Alist=196 once the new release opens and upload the fix.

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.

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.

Guideline Criteria for New Package Inclusion

A package NEW to Ubuntu should be actively maintained upstream and receive security- and bugfixes regulary. If this can't be fulfilled, the package maintainer is 100% responsible for the above.

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. Package must meet Ubuntu versioning & Maintainer requirements

    2. Package must match current Ubuntu (and Debian) packaging policies
    3. Package should be linda & lintian clean

    4. Contents of debian/ should be sane
    5. Package must build, install, run, remove, and purge cleanly
    6. Changelog should close a "needs-packaging" bug
    7. Package should follow http://www.debian.org/doc/manuals/developers-reference/ch-best-pkging-practices.en.html

  • Maintenance review
    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. Packaging scripts should be readable and readily comprehensible
    3. Upstream should be responsive, and maintain a bug tracker
    4. Packaged version should be latest upstream
    5. Packaged version must not have any known security or critical bugs
    6. Package should not be native without an approved spec
  • Suitability review
    1. Package should work on a standard Ubuntu/Kubuntu/Xubuntu/etc. system
    2. Package must meet copyright / licensing requirements
    3. Package should provide hints to system services (app-install-data, menus, etc.) to ease installation and use
    4. Package should provide Ubuntu-specific documentation for variances in behaviour from upstream
    5. Package should provide a Homepage: header in debian/control
    6. Non-native packages must have verifiable cryptographic path to upstream source
    7. Package must be advocated by at least two members of ubuntu-dev (the packager may count as one)

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

UbuntuDevelopment/CodeReviews (last edited 2023-08-21 12:01:15 by kewisch)