Difference between revisions of "Guidelines/Contribution process"

From Apertis
Jump to: navigation, search
m (s/Secure Automotive Cloud/Apertis/)
m (s/arc/git-phab/)
Line 1: Line 1:
 
= Contribution process =
 
= Contribution process =
  
The process for contributing code to Apertis is the same as for many other open source projects: check out the code, make changes locally, then submit them for review and merging. Apertis uses git and Bugzilla.
+
The process for contributing code to Apertis is the same as for many other open source projects: check out the code, make changes locally, then submit them for review and merging. Apertis uses git and Phabricator.
  
 
This page assumes good working knowledge of git; see [[Guidelines/Version control|Version control]] for specific guidelines.
 
This page assumes good working knowledge of git; see [[Guidelines/Version control|Version control]] for specific guidelines.
Line 24: Line 24:
 
# Commit the changes (see [[Guidelines/Version control#Guidelines for making commits]]), test them, and [[#Sign-offs|check you are allowed to submit them under the project’s licence]].
 
# Commit the changes (see [[Guidelines/Version control#Guidelines for making commits]]), test them, and [[#Sign-offs|check you are allowed to submit them under the project’s licence]].
 
#* <code>git commit -i -s</code>
 
#* <code>git commit -i -s</code>
# If you have git privileges, push the patches to a <tt>wip/[branch name]</tt> branch on git.apertis.org and submit that branch for review on [https://bugs.apertis.org/enter_bug.cgi Bugzilla] or [https://phabricator.apertis.org/ Phabricator]
+
# If you have git privileges, push the patches to a <tt>wip/[branch name]</tt> branch on git.apertis.org
#* Use arc: <code>arc diff origin/master..</code>
+
# Create a task for the patch on Phabricator (<tt>git-phab</tt> can do this automatically; or, if passed <tt>--task Txxx</tt> it will attach the patch to an existing task). Ensure the task is descriptive and up to date.
#* Use [[#git-bz|git-bz]]: <code>git bz file origin/master..</code>
+
#* You will need to [https://phabricator.apertis.org register for an account on Phabricator]
#* …or <code>git format-patch origin/master..</code>
+
# Patches are reviewed by the project maintainers and the differential revisions are updated to the accepted state.
# [https://bugs.apertis.org/enter_bug.cgi File a bug report on Apertis Bugzilla] (if an appropriate one does not already exist) and attach the patches.
+
# If any modifications are required, they are discussed on the differential revisions in Phabricator and updated patches need to be attached as appropriate.
#* You will need to [https://bugs.apertis.org/createaccount.cgi register for an account on Bugzilla]
+
# Patches are merged to upstream git (see [[Guidelines/Version control#Merging procedure|Merging procedure]]) and the differential revisions are closed. If the entire task is complete, it can also be closed (and a follow-up task opened for updating the relevant Debian package, if appropriate).
#* Use [[#git-bz|git-bz]]: <code>git bz file origin/master..</code>
 
#* …or <code>git format-patch origin/master..</code>
 
# Patches are reviewed by the project maintainers and the bug is updated to the IN_PROGRESS state (similarly for Phabricator revisions).
 
# If any modifications are required, they are discussed on the bug and updated patches need to be attached as appropriate.
 
# Patches are merged to upstream git (see [[Guidelines/Version control#Merging procedure|Merging procedure]]) and the bug is updated to the RESOLVED state.
 
  
=== git-bz ===
+
=== git-phab ===
  
[http://blog.fishsoup.net/2008/11/16/git-bz-bugzilla-subcommand-for-git/ git-bz] ([http://git.fishsoup.net/cgit/git-bz/ git repository]) is a git extension which simplifies filing bugs in Bugzilla and attaching patches to them from the git command line.
+
[https://phabricator.freedesktop.org/project/profile/60/ git-phab] is a git extension which simplifies creating differential revisions in Phabricator from the command line. It is an essential tool.
  
After installing git-bz, execute the following commands for each Apertis repository you intend to submit patches from:
+
It requires <tt>arc</tt> to function, and some of the Apertis modules use custom lint checks which means they require a patched version of <tt>arc</tt>, available [https://git.collabora.com/cgit/phabricator/arcanist.git/ here].
<pre>git config --global bz-tracker.bugs.apertis.org.https true
 
git config bz.default-tracker bugs.apertis.org
 
git config bz.default-product Application
 
git config bz.default-component [repository name]</pre>
 
  
Given a git repository with a work-in-progress branch to be submitted to Bugzilla, the following git-bz command will file a bug report and attach all the patches:
+
All Apertis repositories have been set up for <tt>git-phab</tt> usage already (with a <tt>.arcconfig</tt> file), and should need no further configuration.
<pre>git bz file origin/master..</pre>
 
  
 
== Privileged processes ==
 
== Privileged processes ==

Revision as of 17:13, 2 March 2016

Contribution process

The process for contributing code to Apertis is the same as for many other open source projects: check out the code, make changes locally, then submit them for review and merging. Apertis uses git and Phabricator.

This page assumes good working knowledge of git; see Version control for specific guidelines.

Sign-offs

Like the git project and the Linux kernel, Apertis requires all contributions to be signed off by someone who takes responsibility for the open source licensing of the code being contributed. The aim of this is to create an auditable chain of trust for the licensing of all code in the project.

Each commit which is pushed to git master must have a Signed-off-by line, created by passing the --signoff/-s option to git commit. The line must give the real name of the person taking responsibility for that commit, and indicates that they have agreed to the Developer Certificate of Origin. There may be multiple Signed-off-by lines for a commit, for example, by the developer who wrote the commit and by the maintainer who reviewed and pushed it:

Signed-off-by: Random J Developer <random@developer.example.org>
Signed-off-by: Lucky K Maintainer <lucky@maintainer.example.org>

Apertis closely follows the Linux kernel process for sign-offs, which is described in section 11 of the kernel guide to submitting patches.

Contribution process

This is the basic contribution process which is used by all developers, including core and third party contributors.

  1. Check out git repository.
  2. Make code changes locally.
  3. Commit the changes (see Guidelines/Version control#Guidelines for making commits), test them, and check you are allowed to submit them under the project’s licence.
    • git commit -i -s
  4. If you have git privileges, push the patches to a wip/[branch name] branch on git.apertis.org
  5. Create a task for the patch on Phabricator (git-phab can do this automatically; or, if passed --task Txxx it will attach the patch to an existing task). Ensure the task is descriptive and up to date.
  6. Patches are reviewed by the project maintainers and the differential revisions are updated to the accepted state.
  7. If any modifications are required, they are discussed on the differential revisions in Phabricator and updated patches need to be attached as appropriate.
  8. Patches are merged to upstream git (see Merging procedure) and the differential revisions are closed. If the entire task is complete, it can also be closed (and a follow-up task opened for updating the relevant Debian package, if appropriate).

git-phab

git-phab is a git extension which simplifies creating differential revisions in Phabricator from the command line. It is an essential tool.

It requires arc to function, and some of the Apertis modules use custom lint checks which means they require a patched version of arc, available here.

All Apertis repositories have been set up for git-phab usage already (with a .arcconfig file), and should need no further configuration.

Privileged processes

Pushing commits to git.apertis.org requires commit rights which are only granted to trusted contributors. All commits must have a Signed-off-by line assigning responsibility for their open source licensing.

Packaging up and releasing new versions of Apertis modules as Debian packages requires packaging rights on build.collabora.co.uk (OBS), which are issued separately from commit rights.

Submitting automated test runs on lava.collabora.co.uk requires CI rights, which are granted similarly to packaging rights. However, CI results may be viewed read-only by anyone.

Getting commit rights

Commit rights (to allow direct pushes to git, and potentially access to the package building system, build.collabora.co.uk) may be granted to trusted third party contributors if they regularly contribute to Apertis, with high quality contributions.

API review process

Before a public API may be marked as stable and suitable for use by third-party apps, it must be reviewed. The process for this is:

  1. Some or all of the APIs of a component are selected for review. For example, it might be decided to review the C APIs for a component, but not the D-Bus APIs which it uses internally to communicate with a background service.
  2. The component authors produce:
    • A description of the goals of the component and the APIs under review. These should be high-level descriptions, covering the use cases for that component and its APIs, goals of the implementation, and specific non-goals. Where possible, this should refer back to a design document. These descriptions should not be simply a copy of the API documentation describing how the goals are implemented.
    • A list of required features and APIs which must pass review before the API may be marked as stable.
    • A list of other components which depend on the APIs under review, and a description of how those other components should interact with the APIs under review. This ensures the final APIs are still usable as intended from other components, and is another way of describing the use cases of the API.
    • An explicit description of where privilege boundaries are required in the system, and whether any of the APIs cross those boundaries (for example, if they implement functionality which should not be callable without elevated privileges).
    • A description of how the API may need to be extended. For example, should it be extensible by the component maintainers, by third-party apps, or not at all?
  3. The reviewers examine the APIs and provided documentation, and make recommendations for the component as a whole, its interactions with other components, and specific recommendations for each API under review (for example, ‘keep this API unchanged’, ‘remove this API’, ‘keep it with the following changes’, etc.). See the API design guidelines for some principles that the reviewers might apply.
  4. The feedback is discussed, potentially iterated, and changes are made to the public API accordingly.
  5. The changes are re-reviewed, and the process iterates until the reviewers are happy.
  6. The APIs are marked as public and stable, and their API documentation is updated.

The main goal of the review process is to ensure that the stable APIs, which have to be supported for a long time in the future, are a good match for all current and anticipated use cases, and follow good system architecture practices. A large part of this effort is in ensuring that all use cases for the APIs are known and documented. A non-goal of the review process is to look at each API individually and mechanically run through some ‘API review checklist’ — this can easily miss mismatches between the API and the component’s use cases; architectural issues which are important to catch.

Components’ APIs should not be marked as stable until they have been reviewed.