Guidelines/HowToWorkWithPatches

From Apertis
Jump to: navigation, search

This document aims to explain how to submit patches to Apertis projects.

Contents

Preparing Tools

In Apertis, Phabricator is used to handle git patches. Before creating patches, all contributors should be familiar with these tools. These tools are available through the Apertis SDK. We do not recommend using arc from Arcanist to send patches because arc has a different commit message format compared to the Apertis convention. If you are working on SDK, you can skip the next sections.

git-phab

On Apertis SDK image, git-phab is ready. If you want to install manually, you can clone from the official repository.

$ git clone git://anongit.freedesktop.org/git-phab

To install, you need to follow the steps in README.md.

clang-format-linter

If you are ready to use git and git-phab, you also need to install clang-format-linter to verify general code format.

A custom local install of the clang-format-linker can be easily done:

On an Apertis image or on Debian stretch:

apt-get install arcanist-clang-format-linter

Or when packages for clang-format-linter aren’t available for your distribution:

<change to the path where you want to put the linter, e.g. ~/source>
git clone https://github.com/pwithnall/morefas-phabricator.git clang-format-linter
arc set-config load "[ \"$(pwd)/clang-format-linter\" ]"

Check the installation:

arc linters  # should list ‘clang-format’ as ‘configured’

We are using a custom clone of the original clang-format-linter which adds support for splitting the diff into chunks and adds some more help.

Make sure clang-format is recent

Again, this should be done on each developer’s machine.

Install clang-format 3.7 or later, and make sure that running "clang-format" results in running that version. For instance, in Debian unstable as of January 2016 it's called clang-format-3.7, so you might add ~/bin to your PATH and make a symbolic link:

ln -s /usr/bin/clang-format-3.7 ~/bin/clang-format

Update .arcconfig

Update the .arcconfig file for the project (in git) to add the linter configuration and load the clang-format-linter arcanist extension:

"lint.engine": "ArcanistConfigurationDrivenLintEngine"

Add a .clang-format file

clang-format accepts its format options from a hidden file, defined in YAML format. Create .clang-format and commit it to git:

# See https://wiki.apertis.org/Guidelines/Coding_conventions#Code_formatting
BasedOnStyle: GNU
AlwaysBreakAfterDefinitionReturnType: All
BreakBeforeBinaryOperators: None
BinPackParameters: false
SpaceAfterCStyleCast: true

These are the current recommended formatting options from the coding conventions, but may change in future as clang-format evolves or corner cases in the configuration are found and fixed.

Add a .arclint file

To enable the linters to be used with arc lint (and hence with arc diff and git-phab attach), add a .arclint file to the project and commit it to git. This example file contains XML, Python and C linters. Note that you should adjust the include path for the clang-format-default linter to include the source directories in your project, while excluding any generated C or H files.

{
  "linters": {
    "clang-format-default": {
      "type": "clang-format",
      "version": ">=3.7.0",
      "include": "(^(src|tests|canterbury)/.*\\.(c|h)$)"
    },
    "pep8-default": {
      "type": "pep8",
      "flags": ["--ignore=E402"],
      "include": "(\\.py$)"
    },
    "xml-default": {
      "type": "xml",
      "include": "(\\.xml$)"
    }
  }
}

We ignore PEP8 error E402 because the recommended copyright headers, editor modelines and encoding lines push the first import statement below the recommended line number according to PEP8.

Patch workflow

Before writing any code, you should discuss your planned work with the package maintainers or read up on any previous discussions.

Create a Maniphest task on Phabricator

There are a number of helper templates available for you to base your new task:

If none of the pre-defined templates fit to your task, you can also create one without a template or adapt one of the templates. The templates are intended to help you write a good task which other developers will be able to understand. You will need to use the task number later to correctly associate your patches with the task and for the automated integration & testing to work correctly.

In case of a Review Task, the purpose will be described in #When patches are ready section.

Preparing Patches

Guidelines for making commits

Commits should be as small as possible, but no smaller. Each commit should address a single issue, containing only changes related to that issue. The message for each commit should describe the issue, explain what causes it, and explain how it has been fixed if it is not obvious. If the commit is associated with a bug report or Phabricator Differential revision, the full URI for the bug report or revision should be put on a line by itself at the bottom of the commit message. Similarly, the ID for the git commit (e.g. from git log --oneline) should be copied into the bug report once the commit has been pushed, so it is easy to find one from the other (this is done automatically for Phabricator Differential revisions).

The changes in each commit should be easy to read. For example, they should not unnecessarily change whitespace or indentation. Large, mechanical changes, such as renaming a file or function, should be put in separate commits from modifications to code inside that file or function, so that the latter changes do not get buried and lost in the former.

The following principles give the reasoning for all the advice above:

  • Each commit should take the repository from one working state to another, otherwise bisection is impossible.
  • Each commit should be individually revertable. If it later turns out that the commit was a bad idea, git revert [commit ID] should take the repository from a working state to another working state.
  • The reasoning for each commit, and its relationship to external resources like specifications and bug reports, should be clear, to the extent that commits written by one developer a year in the past should still be understandable by a second developer without having to trace through the changes and work out what they do. There is good guidance online for how to write good commit messages.
  • Each commit should be written once, and designed to be read many times, by many reviewers and future programmers.

However, we have more process policies outside of codes.

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.

 $ git commit -s

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.

  • real name: At least two words which starts with a capital letter
  • email: you should provide a valid email between two brakets which start with < and end with >

Otherwise, jenkins will mark your patch as invalid. 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.

Commit message

See the style guide for commit messages and how to make them suitable to automatically generate debian/changelog.

Automatically Closing Task

The commit message is very important to support automatically closing tasks.

You should never close a task manually. To automatically close the task when your changes appear in the packages, add a meta-tag, in the git commit message description, pointing to the Apertis task:

Apertis: https://phabricator.apertis.org/T12345

When debian/changelog file is generated from the commit messages by gbp dch, that meta-tag is transformed into (Apertis: T12345) appended to the changelog message. When the package is built and uploaded, the task will be automatically closed.

By default, only the first line of the commit message is used for the changelog entry.

The example message will be converted into debian/changelog by the following entry:

* Update website link to demo images.
  Thanks to John Smith for pointing out the issue (Apertis: T12345)

Submitting Patches

The usual process is already explained in Contribution Process, and if your patches are only for specific version, you need to follow the steps of Patches for specific version.

In any case, the submitted patches should always be associated with a Maniphest task. If you don't have a proper task, follow the step above, create Maniphest task on Phabricator.

When patches are ready

When you submit a patchset, Phabricator and Jenkins will run some general health checks on it, including:

  1. Linting is checked by Phabricator
  2. Check commit subject and body length by Jenkins
  3. Check if Signed-off-by is presented and well-formed by Jenkins
  4. Jenkins checks if project will build after patchset is applied

Your patchset will always need to pass all the checks before your patchset will be reviewed. If the checks are green on Phabricator, you should change the status of the task to Proposed so it is clear that the patchset is ready for review.

You will now need to create a subtask using the review task template, and tag it with "reviewers" In addition, the status of the created subtask should be Confirmed.

Patch reviews

You will always have at least once review round for every patch, and often it will take a few review rounds before the patch is ready. Sometimes, reviewers can ask you to update or split a proposed commit. In that case, you will often have to rebase you patchset on top of master because there will often be work done on the project by other people at the same time. When you rebase on top of master, you should always go through the pre-commit checklist again.

If all of the proposed patches are accepted by reviewers, the remaining steps are landing and releasing. Usually, the maintainers of the repository will take responsibility for those works, and the maintainer can be a different developer from the reviewer. However, maintainer is supposed to be subscribed to all activity for their project so you can expect to land the patches in time.

Work done

This is a task for maintainers. If reviewing process is done and all of the patches are landed, the Maniphest task should be automatically marked as Submitted by Jenkins. Then the Maniphest task will be closed by #Automatically Closing Task process when released.

Personal tools
Namespaces

Variants
Actions
Navigation
Tools