Difference between revisions of "Development Team/Code Review"

From Sugar Labs
Jump to navigation Jump to search
 
(63 intermediate revisions by 16 users not shown)
Line 1: Line 1:
−
<noinclude>{{ GoogleTrans-en | es =show | bg =show | zh-CN =show | zh-TW =show | hr =show | cs =show | da =show | nl =show | fi =show | fr =show | de =show | el =show | hi =show | it =show | ja =show | ko =show | no =show | pl =show | pt =show | ro =show | ru =show | sv =show }}</noinclude>
+
<noinclude>{{TOCright}}</noinclude>
−
= Review Process =
 
−
We are using a mix of email and trac for the review process. In the future we might change to a specialized tool like [http://www.review-board.org/ review board] which we are currently investigating.
 
  
−
== Process guidelines ==
+
= Introduction =
−
Break your projects into small pieces that can be submitted for review independently. Submit for review whenever you think you need a review - you can tell that best. The reviewer will use review+ or r+ whenever the patch is accepted and ready to go into git master.
+
Because Sugar is an open source project, anyone can contribute to it and thus improve the learning experience of children all around the world.
  
−
The workflow should be something like:
+
Once you have fixed a bug or implemented a new feature, you ''must'' send it to the current maintainers of Sugar by following the code review process.
−
* code a small part
 
−
* submit to review
 
−
* push to master
 
−
* repeat
 
  
−
There will be cases when this will not be possible but we want to try to follow this guidelines.
+
This process allows the maintainers to feel confident about taking the responsibility of delivering your code to children. This means applying it to the current code base, fixing any bugs that are later discovered in your code or as consequences of it, modifying your code to gain new features, assisting packagers, deployers and translators, etc.
  
−
We could consider that 1500 lines starts to be too much for a diff, although factors like the add/delete ratio or the invasiveness of the changes should be taken in account.
+
It is strongly recommended that you read about the review process and [[Development Team/Code guidelines|Code guidelines]] before starting to code.
  
−
== Patch guidelines ==
+
The process is composed by the following phases, explained below: preliminary discussion, proposal, discussion and acceptance.
−
Please try to use [http://www.logilab.org/857 pylint] to verify your patch for things like exceeding 80 columns etc, unused imports and unused variables. Pylint is by far a tool you can 100% rely on but it helps to follow some guidelines and to avoid the most stupid errors like typos. Here is a pylintrc you can use: [http://dev.laptop.org/git?p=sugar-jhbuild;a=blob_plain;f=scripts/data/pylintrc pylintrc]
 
  
−
In the sugar packages use 'make distcheck' to make sure all files are included and the POTFILES.in is up to date.
+
= Phases =
  
−
=== Create a patch ===
+
== Preliminary discussion ==
−
Normally you can get a patch from git with:
 
−
git diff
 
  
−
If you already committed part or all of the new code you need to specify the id of the last commit before yours:
+
Except when the patch will be trivial, it's a good idea to discuss your idea and possible approaches on [[Mailing Lists#Developer Lists|sugar-devel]]. This avoids duplicated work and will give you an idea of what is considered a good idea by others (maintainers, [[Design Team]], etc.) before you commit substantial work in the patch itself.
−
git diff 34a4876f93894309f77b00281b5cb1bb72b3a1e4
 
  
−
Or if the commit you want the diff of was the most-recent commit, you can do:
+
This is especially important if you're going to make large changes. You will probably be asked to split up your changes in reasonably small, but logically independent parts that can be reviewed somewhat independently.
−
git diff HEAD^!
 
  
−
Another alternative that will give you a patch in a file called 0001-My-cool-patch.patch:
 
−
git-add new_file1 new_file2
 
−
git-commit -a -m 'My cool patch'
 
−
git-format-patch HEAD^
 
−
git-reset --hard HEAD^
 
  
−
== Patch submission ==  
+
== Proposal ==
  
−
* If your patch addresses an issue which is not registered in [http://dev.laptop.org/query trac], please open a new ticket for it.
+
* If your patch addresses a bug which is not registered on the [https://bugs.sugarlabs.org/ bug tracker] yet, please open a new ticket for it.
−
* Attach the patch to the ticket
+
* Check your code follows the [[Development Team/Code guidelines|code guidelines]]. [[Development Team/Code guidelines#Tools|Automated style checkers]] can be quite helpful.
−
* Add the keyword 'r?' to call the attention of reviewers.
+
* Reflect on and explain what your patch does by coming up with a patch description (see [[Activity Team/Git Introduction#Commit message guidelines|commit message guidelines]]).
 +
* Send your patch to [[Mailing Lists#Developer Lists|sugar-devel]] (see [[#Patch submission|below]] for details).
 
* Add a testcase in a ticket comment. You need to mark it with |TestCase|. This adds better readability and our script that pulls together the test cases for each release is able to find it as well. For example:
 
* Add a testcase in a ticket comment. You need to mark it with |TestCase|. This adds better readability and our script that pulls together the test cases for each release is able to find it as well. For example:
 
  |TestCase|
 
  |TestCase|
Line 47: Line 32:
 
  Click on Browse, Read, Pippy icons in the homepage and make sure all of them starts correctly.
 
  Click on Browse, Read, Pippy icons in the homepage and make sure all of them starts correctly.
  
−
If your patch is a new feature and reasonably big, you may prefer to submit it for review to the Sugar [http://lists.laptop.org/listinfo/sugar mailing list]. If you do please add the review keyword to the ticket anyway and reference the mailing list thread.
+
== Patch submission ==
  
−
In order to make it easy for the reviewer please:
+
We follow the [http://www.kernel.org/doc/Documentation/SubmittingPatches Linux patch submission process] for the most part. Some points specific to Sugar Labs are:
  
−
* note which module is effected e.g. sugar, sugar-toolkit...
+
* the affected module (e.g. sugar-toolkit) should be mentioned in the subject prefix
−
* note possible dependencies e.g. the patch is for sugar but depend on the current HEAD of sugar-toolkit which went in 5 seconds ago
+
* all patches get sent to [[Mailing Lists#Developer Lists|sugar-devel]]
 +
* you need to clearly note all new dependencies in the patch description (see also [[#Dependencies|below]])
  
−
== Reviewer guidelines ==
+
If you have revised your patch, addressing review comments, please:
  
−
* For complex patches you might want to start with an overall design of the patch.
+
* add a version number to the subject prefix (e.g. <code>git send-email --subject-prefix="PATCH v2 sugar"</code>)
−
* Make sure the submitter provided a testcase before approving the patch
+
* mention your changes in a changelog below the marker line (the one with just <code>---</code> on it). E.g.:
−
* Change the r? keyword to r- to indicate that the patch needs work.
+
v1->v2: break up _recurse_dir into smaller functions
−
* Change the r? keyword to r+ if the patch is ok to be pushed.
 
  
−
== Push the patch ==
+
You will first need to set up <code>git send-email</code> so it can send emails through your mail provider. The manual page contains an [http://www.kernel.org/pub/software/scm/git/docs/git-send-email.html#_use_gmail_as_the_smtp_server example for Gmail users]. You can use the [[Service/smtp|sugarlabs.org SMTP server]] if you have a [[Service/shell|shell account]]. Feel free to ask for help with setting up <code>git send-email</code> in [[IRC#irc.freenode.net_channels|#sugar]] or on [[Mailing Lists#Developer Lists|sugar-devel]].
−
If you have write access to the sugar repository you can just do it yourself.  
 
  
−
If you don't have write access to the repository commit the patch to your local repository.
+
Recommended git user configuration:
−
git commit
 
  
−
Generate a patch set using git. The following command will create one file per commit in the current directory.
+
* <code>git config --global diff.renames copies</code>
−
git-format-patch origin
+
* <code>git config --global sendemail.confirm always</code>
 +
* <code>git config --global 'url.gitorious@git.sugarlabs.org:.pushInsteadOf' git://git.sugarlabs.org/</code>
 +
* <code>git config --global alias.send-to-ml-single '!git send-email --stat'</code>
 +
* <code>git config --global alias.send-to-ml-series '!git send-email --cover-letter --summary --annotate --stat'</code>
  
−
Make sure that the ticket number is referenced in the commit description!
+
If you agree to publish your changes under the same license as the software you are changing, please add <code>-s</code> to the two aliases above (send-to-ml-single and send-to-ml-series). If you don't agree, don't post the patch! The reason this is mentioned as a separate step instead of adding it to the examples above is so you need to make a conscious decision and can't sue us for copyright infringement.
  
−
== Remind the maintainers about patches ==
+
Recommended per-repository git configuration, e.g. for sugar-datastore:
−
If you don't get any response from the maintainers about the patches you submitted in a few days, feel free to remind about them using the Sugar mailing list or IRC channel.
 
  
 +
* <code>cd ~/sugar-jhbuild/source/sugar-datastore</code>
 +
* <code>git config format.subjectprefix "PATCH sugar-datastore"</code>
 +
* <code>git config sendemail.to "sugar-devel &lt;sugar-devel@lists.sugarlabs.org&gt;"</code>
  
−
[[Category:DevelopmentTeam]]
+
Git configuration options that might come in handy:
 +
 
 +
* <code>git config --global merge.conflictstyle diff3</code>
 +
* <code>git config format.headers "Mail-Followup-To: &lt;sugar-devel@lists.sugarlabs.org&gt;"</code>
 +
* <code>git config sendemail.envelopesender sascha-ml-sugar-devel@dev.null</code>
 +
 
 +
Examples (assuming the above configuration, including the aliases):
 +
 
 +
* <code>git send-to-ml-single HEAD^..HEAD</code>
 +
* after doing the code changes requested during review:
 +
** <code>git commit -a --amend</code>
 +
** <code>git send-to-ml-single --annotate --subject-prefix="PATCH v2 sugar"</code>
 +
*** add the changelog during this step
 +
*** adjust the subject prefix to use the name of the module you're modifying instead of <code>sugar</code>
 +
 
 +
== Discussion ==
 +
 
 +
Maintainers and other developers will review your patch as their time permits. Please allow for a few days, but feel free to "ping" (by either asking on [[IRC#irc.freenode.net_channels|#sugar]] or replying to your patch) if you haven't received any reply within a week.
 +
 
 +
Everyone (including you!) is invited to review patches. Not only does this free up time of the maintainers (they can wait until someone else did a first round of review), but also ensures we make the best use of our collective knowledge (no, the maintainers are neither perfect nor omniscient ;) ).
 +
 
 +
If a reviewer thinks your patch is good (and revelant) enough to be included in the upstream version of the software, (s)he will grant his or her <code>Reviewed-By:</code> tag (see the [http://www.kernel.org/doc/Documentation/SubmittingPatches Linux patch submission process] for details).
 +
 
 +
Similarly, everyone is encouraged to try out your patch and give their <code>Tested-By:</code> tag if it worked well for them.
 +
 
 +
After you addressed review comments, you can submit a new version of your patch (see [[#Patch submission|above]] for how to note what you changed). Please make it a reply to your original patch by using the <code>[http://www.kernel.org/pub/software/scm/git/docs/git-format-patch.html#_options --in-reply-to]</code> option to <code>git send-email</code>. E.g.: <code>git send-to-ml-single --in-reply-to=&lt;1290857483.723474@twin.sascha.silbe.org&gt;</code>
 +
 
 +
== Acceptance ==
 +
 
 +
After your patch has been acknowledged by a maintainer, (s)he will push your code to the repository, including any tags (<code>Reviewed-By:</code>, <code>Tested-By:</code>, etc.) your patch got.  From now on your code is part of the Sugar code base and the community (which includes you ;) ) will maintain it for you.
 +
 
 +
If you are a developer with commit access, a maintainer may ask you to commit it with the tags above.  On the other hand, there may be temporal restrictions that the maintainer may want to mention.
 +
 
 +
= Dependencies =
 +
 
 +
Dependencies need to be fulfilled on all platforms running Sugar (i.e., they need to be available at all and need to be installed which means occupying disk space), so think twice before introducing new ones. Please discuss them on [[Sugar Labs/Communication channels#Developer Lists|sugar-devel]] in advance and clearly state them in the ticket. It's a always a good idea to check whether sufficiently recent versions of your proposed dependencies are already packaged for all (stable releases of the) major distributions, on '''all''' of their (semi-)supported architectures (e.g., ARM and PowerPC). If not, start requesting them (at the distribution bug trackers) ASAP and also mention it on sugar-devel and the ticket.
 +
 
 +
= Internal process (maintainers only) =
 +
 
 +
You can get a ready-to-push version of the patch (including <code>Reviewed-By:</code> etc.) by downloading it in mbox format from [https://patchwork.sugarlabs.org/project/sugar/list/ Patchwork].
 +
 
 +
After pushing a patch, please change its state in [https://patchwork.sugarlabs.org/project/sugar/list/ Patchwork] to Accepted + Archived and mark all previous versions (if any are remaining) as Superseded + Archived. If the patch fixes a ticket in [https://bugs.sugarlabs.org/ our bug tracker] please close it, too.
 +
 
 +
[[Category:Development Team]]

Latest revision as of 21:51, 2 November 2011

Introduction

Because Sugar is an open source project, anyone can contribute to it and thus improve the learning experience of children all around the world.

Once you have fixed a bug or implemented a new feature, you must send it to the current maintainers of Sugar by following the code review process.

This process allows the maintainers to feel confident about taking the responsibility of delivering your code to children. This means applying it to the current code base, fixing any bugs that are later discovered in your code or as consequences of it, modifying your code to gain new features, assisting packagers, deployers and translators, etc.

It is strongly recommended that you read about the review process and Code guidelines before starting to code.

The process is composed by the following phases, explained below: preliminary discussion, proposal, discussion and acceptance.

Phases

Preliminary discussion

Except when the patch will be trivial, it's a good idea to discuss your idea and possible approaches on sugar-devel. This avoids duplicated work and will give you an idea of what is considered a good idea by others (maintainers, Design Team, etc.) before you commit substantial work in the patch itself.

This is especially important if you're going to make large changes. You will probably be asked to split up your changes in reasonably small, but logically independent parts that can be reviewed somewhat independently.


Proposal

  • If your patch addresses a bug which is not registered on the bug tracker yet, please open a new ticket for it.
  • Check your code follows the code guidelines. Automated style checkers can be quite helpful.
  • Reflect on and explain what your patch does by coming up with a patch description (see commit message guidelines).
  • Send your patch to sugar-devel (see below for details).
  • Add a testcase in a ticket comment. You need to mark it with |TestCase|. This adds better readability and our script that pulls together the test cases for each release is able to find it as well. For example:
|TestCase|

Click on Browse, Read, Pippy icons in the homepage and make sure all of them starts correctly.

Patch submission

We follow the Linux patch submission process for the most part. Some points specific to Sugar Labs are:

  • the affected module (e.g. sugar-toolkit) should be mentioned in the subject prefix
  • all patches get sent to sugar-devel
  • you need to clearly note all new dependencies in the patch description (see also below)

If you have revised your patch, addressing review comments, please:

  • add a version number to the subject prefix (e.g. git send-email --subject-prefix="PATCH v2 sugar")
  • mention your changes in a changelog below the marker line (the one with just --- on it). E.g.:
v1->v2: break up _recurse_dir into smaller functions

You will first need to set up git send-email so it can send emails through your mail provider. The manual page contains an example for Gmail users. You can use the sugarlabs.org SMTP server if you have a shell account. Feel free to ask for help with setting up git send-email in #sugar or on sugar-devel.

Recommended git user configuration:

  • git config --global diff.renames copies
  • git config --global sendemail.confirm always
  • git config --global 'url.gitorious@git.sugarlabs.org:.pushInsteadOf' git://git.sugarlabs.org/
  • git config --global alias.send-to-ml-single '!git send-email --stat'
  • git config --global alias.send-to-ml-series '!git send-email --cover-letter --summary --annotate --stat'

If you agree to publish your changes under the same license as the software you are changing, please add -s to the two aliases above (send-to-ml-single and send-to-ml-series). If you don't agree, don't post the patch! The reason this is mentioned as a separate step instead of adding it to the examples above is so you need to make a conscious decision and can't sue us for copyright infringement.

Recommended per-repository git configuration, e.g. for sugar-datastore:

  • cd ~/sugar-jhbuild/source/sugar-datastore
  • git config format.subjectprefix "PATCH sugar-datastore"
  • git config sendemail.to "sugar-devel <sugar-devel@lists.sugarlabs.org>"

Git configuration options that might come in handy:

  • git config --global merge.conflictstyle diff3
  • git config format.headers "Mail-Followup-To: <sugar-devel@lists.sugarlabs.org>"
  • git config sendemail.envelopesender sascha-ml-sugar-devel@dev.null

Examples (assuming the above configuration, including the aliases):

  • git send-to-ml-single HEAD^..HEAD
  • after doing the code changes requested during review:
    • git commit -a --amend
    • git send-to-ml-single --annotate --subject-prefix="PATCH v2 sugar"
      • add the changelog during this step
      • adjust the subject prefix to use the name of the module you're modifying instead of sugar

Discussion

Maintainers and other developers will review your patch as their time permits. Please allow for a few days, but feel free to "ping" (by either asking on #sugar or replying to your patch) if you haven't received any reply within a week.

Everyone (including you!) is invited to review patches. Not only does this free up time of the maintainers (they can wait until someone else did a first round of review), but also ensures we make the best use of our collective knowledge (no, the maintainers are neither perfect nor omniscient ;) ).

If a reviewer thinks your patch is good (and revelant) enough to be included in the upstream version of the software, (s)he will grant his or her Reviewed-By: tag (see the Linux patch submission process for details).

Similarly, everyone is encouraged to try out your patch and give their Tested-By: tag if it worked well for them.

After you addressed review comments, you can submit a new version of your patch (see above for how to note what you changed). Please make it a reply to your original patch by using the --in-reply-to option to git send-email. E.g.: git send-to-ml-single --in-reply-to=<1290857483.723474@twin.sascha.silbe.org>

Acceptance

After your patch has been acknowledged by a maintainer, (s)he will push your code to the repository, including any tags (Reviewed-By:, Tested-By:, etc.) your patch got. From now on your code is part of the Sugar code base and the community (which includes you ;) ) will maintain it for you.

If you are a developer with commit access, a maintainer may ask you to commit it with the tags above. On the other hand, there may be temporal restrictions that the maintainer may want to mention.

Dependencies

Dependencies need to be fulfilled on all platforms running Sugar (i.e., they need to be available at all and need to be installed which means occupying disk space), so think twice before introducing new ones. Please discuss them on sugar-devel in advance and clearly state them in the ticket. It's a always a good idea to check whether sufficiently recent versions of your proposed dependencies are already packaged for all (stable releases of the) major distributions, on all of their (semi-)supported architectures (e.g., ARM and PowerPC). If not, start requesting them (at the distribution bug trackers) ASAP and also mention it on sugar-devel and the ticket.

Internal process (maintainers only)

You can get a ready-to-push version of the patch (including Reviewed-By: etc.) by downloading it in mbox format from Patchwork.

After pushing a patch, please change its state in Patchwork to Accepted + Archived and mark all previous versions (if any are remaining) as Superseded + Archived. If the patch fixes a ticket in our bug tracker please close it, too.

Retrieved from "https://wiki.sugarlabs.org/index.php?title=Development_Team/Code_Review&oldid=70730"