CodeReviews
Size: 9708
Comment:
|
← Revision 188 as of 2023-08-21 12:01:15 ⇥
Size: 14868
Comment: Updated the patch pilot section to point to its new home
|
Deletions are marked like this. | Additions are marked like this. |
Line 1: | Line 1: |
||<tablestyle="float:right; font-size: 0.9em; width:40%; background:#F1F1ED; margin: 0 0 1em 1em;" style="padding:0.5em;"><<TableOfContents>>|| = Team Purpose = A team of UbuntuDevelopers has agreed on doing periodical reviews of code. To accelerate SponsorshipProcess members of the team will check * the `main`/`restricted` sponsoring queue: http://bugs.launchpad.net/ubuntu/+bugs?field.subscriber=ubuntu-main-sponsors * the `universe`/`multiverse` sponsoring queue: http://bugs.launchpad.net/ubuntu/+bugs?field.subscriber=ubuntu-universe-sponsors 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. |
||<tablestyle="float:right; font-size: 0.9em; width:40%; background:#F1F1ED; margin: 0 0 1em 1em;" style="padding:0.5em;"><<TableOfContents>><<BR>>'''[[http://reports.qa.ubuntu.com/reports/sponsoring-stats/|Sponsoring Statistics]]'''|| |
Line 18: | Line 4: |
Members of the team * will triage bugs (in the bug lists mentioned above) weekly, |
== Sponsoring == === Sponsors Team === Members of the ~ubuntu-sponsors team * will triage bugs (in the bug lists mentioned in SponsorshipProcess) weekly, |
Line 22: | Line 11: |
* 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. |
* subscribe themselves to bugs where a review might take longer to avoid duplication of work. To get the queue of merge proposals / patches under control, if the patch was deemed not to be good enough yet, * the merge proposal will be marked as `Work In Progress`. * 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 === In addition to the Sponsor team is the [[https://ubuntu.com/community/contribute/ubuntu-development/ubuntu-patch-pilots|Ubuntu Patch Pilots]] team. Each Patch Pilot has a dedicated time each month to be available on IRC in `#ubuntu-devel` so you can talk to them and discuss issues there; the current pilot on call is listed in the topic in `#ubuntu-devel`. Information on how to chat with a Patch Pilot or how to become one can be found in the [[https://ubuntu.com/community/contribute/ubuntu-development/ubuntu-patch-pilots|Ubuntu Patch Pilots program documentation]] |
Line 30: | Line 28: |
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.)'' | To avoid making the bug lists too huge, the following measures might be useful. |
Line 37: | Line 35: |
1. Unsubscribe either `ubuntu-universe-sponsors` or `ubuntu-main-sponsors`. | 1. Unsubscribe `ubuntu-sponsors`, or mark the merge proposal status as "Work in Progress". (Be sure to tell the contributor to reverse the process) |
Line 43: | Line 41: |
1. Unsubscribe either `ubuntu-universe-sponsors` or `ubuntu-main-sponsors`. | 1. Unsubscribe `ubuntu-sponsors`, or mark the merge proposal status as "Work in Progress". (Be sure to tell the contributor to reverse the process) 1. Subscribe yourself to the bug report (this ensures it shows up in the following url) |
Line 46: | Line 45: |
=== Changes needed, but no response from submitter === If you come across sponsoring items which need fixing from the submitter, and we haven't heard anything back in a while, you might want to unsubscribe `ubuntu-sponsors` or mark the merge proposal as 'Work in Progress' and inform the submitter about it. == Prior to sponsoring == For patch authors who can't upload yet we have the SponsorshipProcess. Unfortunately, there's also a [[https://bugs.launchpad.net/ubuntu/+bugs?field.has_patch=on|big list of bugs with patches]] that are not following the process. Likely, because they are not aware of it. The ReviewersTeam 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 1. remove the patch flag 1. 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` 1. check if the fix is in a code branch, make sure the branch is linked and a proposal for merging has been made 1. if you can't upload 1. make use of the SponsorshipProcess and subscribe `ubuntu-sponsors` |
|
Line 53: | Line 79: |
* 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. |
* sync requests are to be uploaded using the `syncpackage` tool. |
Line 60: | Line 82: |
== Working with Merge Proposals == [[http://packaging.ubuntu.com/html/udd-intro.html|The Ubuntu Distributed Development documentation]] has good information if you want to * [[http://packaging.ubuntu.com/html/udd-working.html|work on a package]] * [[http://packaging.ubuntu.com/html/udd-sponsorship.html|seek sponsorship]] * [[http://packaging.ubuntu.com/html/udd-uploading.html|upload a package]] == 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. [...] $ }}} Please note that this case differs from packages that merely make use of DistributedDevelopment. The history of almost all source packages is mirrored in `lp:ubuntu/<package>` branches. In this specific case in addition to the mirrored branch of upload history, the upstream source is maintained in Launchpad too. In these cases, please either: * ask the contributor to register a [[DistributedDevelopment/Documentation/SeekingSponsorship|merge proposal]] * (if you can commit to the branch in question) check out the branch and commit the change == Kernel patches == If you encounter a patch for the linux kernel, you should generally follow the [[Kernel/Dev/KernelBugFixing|kernel process for bugs]]. Generally the patch should be [[Kernel/Dev/KernelPatches#Upstream|forwarded upstream first]] and [[Kernel/Dev/KernelPatches#UbuntuInclusion|can be included]] once it has landed there. There is [[Kernel/Dev/KernelBugFixing#TestBuild|good documentation on test-building]] as well. If you need any help, join `#ubuntu-kernel` talk to the people there. |
|
Line 99: | Line 154: |
* [[https://bugs.launchpad.net/ubuntu/+bugs?field.searchtext=&orderby=-importance&field.status%3Alist=NEW&field.status%3Alist=INCOMPLETE_WITH_RESPONSE&field.status%3Alist=INCOMPLETE_WITHOUT_RESPONSE&field.status%3Alist=CONFIRMED&field.status%3Alist=TRIAGED&field.status%3Alist=INPROGRESS&assignee_option=any&field.assignee=&field.bug_reporter=&field.bug_supervisor=&field.bug_commenter=&field.subscriber=&field.component-empty-marker=1&field.tag=&field.tags_combinator=ANY&field.status_upstream-empty-marker=1&field.has_cve.used=&field.omit_dupes.used=&field.omit_dupes=on&field.affects_me.used=&field.has_no_package.used=&field.has_patch.used=&field.has_patch=on&field.has_branches.used=&field.has_no_branches.used=&field.has_no_branches=on&search=Search|Bugs with patches attached]] | |
Line 113: | Line 169: |
* Always make sure it is clean, who the copyright holder is, and the year of copyright | * Always make sure it is clear, who the copyright holder is, and the year of copyright |
Line 123: | Line 179: |
== Forwarding Patches Upstream == <<Include(PackagingGuide/Intro/PatchesForwarding, , from="StartEnglish", to="EndEnglish")>> |
|
Line 126: | Line 185: |
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. | 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. |
Line 131: | Line 190: |
1. Package must meet Ubuntu versioning & Maintainer requirements 1. Package must match current Ubuntu (and Debian) packaging policies 1. Package should be linda & lintian clean 1. Contents of debian/ should be sane 1. Package must build, install, run, remove, and purge cleanly 1. Changelog should close a "needs-packaging" bug 1. Package should follow http://www.debian.org/doc/manuals/developers-reference/ch-best-pkging-practices.en.html |
a. MUST: 1. Package must meet Ubuntu versioning & Maintainer requirements 1. Package must match current Ubuntu (and Debian) packaging policies 1. Package must build, install, run, remove, and purge cleanly a. SHOULD: 1. Package should be lintian clean 1. Contents of debian/ should be sane 1. Changelog should close a "needs-packaging" bug 1. Package should follow http://www.debian.org/doc/manuals/developers-reference/best-pkging-practices.html |
Line 140: | Line 201: |
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 1. Packaging scripts should be readable and readily comprehensible 1. Upstream should be responsive, and maintain a bug tracker 1. Packaged version should be latest upstream 1. Packaged version must not have any known security or critical bugs 1. Package should not be native without an approved spec |
a. 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 1. Packaged version must not have any known security or critical bugs a. SHOULD: 1. Packaging scripts should be readable and readily comprehensible 1. Upstream should be responsive, and maintain a bug tracker 1. Packaged version should be latest upstream 1. Package should not be native without an approved spec |
Line 150: | Line 213: |
1. Package should work on a standard Ubuntu/Kubuntu/Xubuntu/etc. system 1. Package must meet copyright / licensing requirements 1. Package should provide hints to system services (app-install-data, menus, etc.) to ease installation and use 1. Package should provide Ubuntu-specific documentation for variances in behaviour from upstream 1. Package should provide a Homepage: header in debian/control 1. Non-native packages must have verifiable cryptographic path to upstream source 1. 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. |
a. MUST: 1. Package must meet copyright / licensing requirements 1. Non-native packages must have verifiable cryptographic path to upstream source 1. Package must be advocated by at least two members of ubuntu-dev (the packager may count as one) a. SHOULD: 1. Package should work on a standard Ubuntu/Kubuntu/Xubuntu/etc. system 1. Package should provide hints to system services (app-install-data, menus, etc.) to ease installation and use 1. Package should provide Ubuntu-specific documentation for variances in behaviour from upstream 1. 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. |
Contents |
Workflow
Sponsoring
Sponsors Team
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 themselves to bugs where a review might take longer to avoid duplication of work.
To get the queue of merge proposals / patches under control, if the patch was deemed not to be good enough yet,
the merge proposal will be marked as Work In Progress.
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
In addition to the Sponsor team is the Ubuntu Patch Pilots team. Each Patch Pilot has a dedicated time each month to be available on IRC in #ubuntu-devel so you can talk to them and discuss issues there; the current pilot on call is listed in the topic in #ubuntu-devel.
Information on how to chat with a Patch Pilot or how to become one can be found in the Ubuntu Patch Pilots program documentation
Keeping the Sponsoring Queue manageable
To avoid making the bug lists too huge, the following measures might be useful.
Bugs fixing small details
Ask the contributor to forward the patch upstream.
- Open an empty upstream bug task.
- Mark the Ubuntu task as 'Fix Committed'.
Unsubscribe ubuntu-sponsors, or mark the merge proposal status as "Work in Progress". (Be sure to tell the contributor to reverse the process)
Not suitable for the current release period
- Let the contributor know that the patch is not suitable for the current release period.
Unsubscribe ubuntu-sponsors, or mark the merge proposal status as "Work in Progress". (Be sure to tell the contributor to reverse the process)
- Subscribe yourself to the bug report (this ensures it shows up in the following url)
- Milestone the bug to 'later'.
Visit https://bugs.launchpad.net/people/+me/+bugs/?field.milestone%3Alist=196 once the new release opens and upload the fix.
Changes needed, but no response from submitter
If you come across sponsoring items which need fixing from the submitter, and we haven't heard anything back in a while, you might want to unsubscribe ubuntu-sponsors or mark the merge proposal as 'Work in Progress' and inform the submitter about it.
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 ReviewersTeam 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:
- make sure the attached patch is a genuine patch, if it isn't:
- let the author know in a bug comment
- remove the patch flag
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
- check if the fix is in a code branch, make sure the branch is linked and a proposal for merging has been made
- if you can't upload
make use of the SponsorshipProcess and subscribe ubuntu-sponsors
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 are to be uploaded using the syncpackage tool.
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.
Working with Merge Proposals
The Ubuntu Distributed Development documentation has good information if you want to
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. [...] $
Please note that this case differs from packages that merely make use of DistributedDevelopment. The history of almost all source packages is mirrored in lp:ubuntu/<package> branches. In this specific case in addition to the mirrored branch of upload history, the upstream source is maintained in Launchpad too.
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
Kernel patches
If you encounter a patch for the linux kernel, you should generally follow the kernel process for bugs. Generally the patch should be forwarded upstream first and can be included once it has landed there. There is good documentation on test-building as well. If you need any help, join #ubuntu-kernel talk to the people there.
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
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!
References
MOTU/Sponsorship/SponsorsQueue - Emmet Hikory wrote up an excellent guide to getting the universe queue under control.
- Process documentation
http://people.debian.org/~mpalmer/sponsorship_checklist.html
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.
See http://lists.debian.org/debian-devel-announce/2003/12/msg00007.html and http://lists.debian.org/debian-legal/2003/12/msg00194.html
- If a package uses several licences, list which file is under which licence
- Always make sure it is clear, who the copyright holder is, and the year of copyright
- 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
- MUST:
Package must meet Ubuntu versioning & Maintainer requirements
- Package must match current Ubuntu (and Debian) packaging policies
- Package must build, install, run, remove, and purge cleanly
- SHOULD:
- Package should be lintian clean
- Contents of debian/ should be sane
- Changelog should close a "needs-packaging" bug
Package should follow http://www.debian.org/doc/manuals/developers-reference/best-pkging-practices.html
- MUST:
- Maintenance review
- MUST:
- 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
- Packaged version must not have any known security or critical bugs
- Package must contain a watch file or get-orig-source rule
- SHOULD:
- Packaging scripts should be readable and readily comprehensible
- Upstream should be responsive, and maintain a bug tracker
- Packaged version should be latest upstream
- Package should not be native without an approved spec
- MUST:
- Suitability review
- MUST:
- Package must meet copyright / licensing requirements
- Non-native packages must have verifiable cryptographic path to upstream source
- Package must be advocated by at least two members of ubuntu-dev (the packager may count as one)
- SHOULD:
- Package should work on a standard Ubuntu/Kubuntu/Xubuntu/etc. system
- Package should provide hints to system services (app-install-data, menus, etc.) to ease installation and use
- Package should provide Ubuntu-specific documentation for variances in behaviour from upstream
- Package should provide a Homepage: header in debian/control
- MUST:
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)