wiki:DeveloperDocumentation

Version 11 (modified by flichtenheld, 2 years ago) (diff)

Mention Patchwork at least once

Introduction

Most of the content here is the result of the the weekly IRC discussions.

Developer communication channels

There are two primary developer channels:

  • openvpn-devel mailinglist: primary communication channel, subscription required
  • #openvpn-devel (at) irc.freenode.net: used for generic development discussions and the weekly IRC meetings (requires Freenode registration)

Development processes

Daily development

The basic development process we follow is outlined in the diagram. So, the daily development process works like this:

  • All patches must be sent to "openvpn-devel" mailing list for review. The subject should preferably be prefixed with [PATCH]
  • All patches need to be reviewed and accepted (ACK). The ACK process is logically split into two (see below).
  • All accepted patches go to the OpenVPN "testing" tree (Git)

The author must send patches to the devel mailing list. This is so to open up for a public discussion of the changes, and to allow the ACK process to work. All patches going into the tree will contain explicit Acked-by: references, to document who gave the approval.

NOTE:

  • You can see a list of replies to a patch including the ACKs in Patchwork.
  • Patches sent directly to a development tree maintainer will be rejected
  • GitHub? pull requests can be useful with large patchsets, or if you want to gauge interest in your patch. The final patch needs to go to "openvpn-devel" mailing list, however.

Adding placeholder bug reports

In the IRC meeting on 22nd Apr 2010 it was agreed that placeholder bug reports should be filed even for bugs which are fixed before anyone reports them as bugs. This allows users to check if the bugs they encounter have already been fixed.

Making a release

Prior to making a release the following happens:

  • Features to include will be selected from the "testing" tree
  • Features will be stabilized until they don't change much
  • A staging "stable candidate" branch will be created
  • Beta release will be published based on this staging branch (to get people to _really_ test the code)
    • This should have most features for the release included, and as such be mostly testing and bug fixing. However, new features could still change as much as needed and old features could be changed if absolutely needed.
  • RC release will published based on this staging branch
    • This should focus only on stabilisation and bugfixes. At this point we know for sure which features should be included. Only in worst cases should a feature taken out, e.g. if it really is horrendous to stability.
  • Once staging branch is deemed stable enough, an official release will be made

In the IRC meeting on 22nd July 2010 it was decided to aim for 6-12 month release cycle to keep the need for large-scale testing minimal.

Feature deprecation

Feature removal process (aka FRP) is another workflow which also goes through the general workflow to get a feature removed.. This process serves two purposes:

  • Maintain backwards compatibility and minimize the impact of feature removal (for users)
  • Keeping the codebase clean and understandable (for developers)

The Feature removal process is described in detail on this page.

Community patches and the acceptance process of these patches

All patches needs to receive an ACK (Acknowledged) before being accepted in to the Git repository. To encourage participation from non-developers the ACK is split into two parts:

  • "This patch does the right thing ACK". Also known as feature ACK. One does not have to be a developer to give this ACK.
  • "This patch does the thing (it does) right". Also known as code quality ACK. People who understand the code can give this.

Note that both ACKs can be given by a single person.

A patch has to get both ACKs to be included into the development tree. The ACK process is used to ensure that only useful and high-quality code gets into OpenVPN. Hopefully that will make it easier for those who don't feel they are "good enough" developers to try to send patches anyway. This whole review process is to give and receive feedback, where we as a community can help making OpenVPN better - together. For example, the more experienced developers can share their advices to those who are less experienced. Those being less experienced can share their understanding of the code as well, which might shed some light to issues or solutions which are better. Non-developers can also participate by sharing their expertise on OpenVPN in general by giving feature ACKs (NACKs) when applicable. This way everyone can learn something and hopefully even get wiser.

To explain the Signed-off-by and Acked-by process a bit further. In git you have to "data fields" which are populated automatically when commits are done, author and commiter. The commiter will be the one who applies patches from the mailing list or the one doing commits to a tree which are fetched via the remote access features in git.

Then you have the author field which is either present in the patch file itself. This field might be missing if git format-patch has not been used to create the patch file sent to the mailing list. In these cases, it is expected that the author of the patch is the same as the sender - unless the commit message in the patch indicates something else.

If the author information is present, the sender field will be treated differently as wel, in those cases where someone sends a patch on behalf of somebody else.

So to summarize, we track these fields almost automatically:

  • author - information about the person who wrote the patch
  • sender - information about who sent / forwarded the patch for inclusion
  • committer - information about who applied the patch to the git tree

Then a patch author sends his patch to somewhere, he should make sure it contains a 'Signed-off-by: {Full name} <email@…>' line in the commit text. This is to officially announce that declare that this patch is ready for further processing. When doing git commits, you can add this indication automatically by doing 'git commit -s' (or --signed-off).

The one who receives the patch and finds it good to go even further (if want you do some kind of "internal" review before sending it further to the public) will add his/hers "Signed-off-by" reference as well when sending further again. This way we can track who have been looking at the patch. And the more people who have eye-balled the patch and adds their "Signed-off" line to the patch, the better! By doing so, the reviewer indicates that the patch is good.

Before it goes into the tree, a final "Acked-by:" note is added, indicating who accepted it for inclusion into the tree,

Example of patch submission and the review process

Lets say John Doe writes a patch, sends it to Jane Doe and she sends it Bob Boss for inclusion. The process would be something like this:

John Doe:

\tAuthor: John Doe <john.doe@example.com>

        Short summary of the patch

\tA more detailed description of this patch fixing x.y.z.  This can
        even span over several lines to explain the patch.

\tSigned-off-by: John Doe <john.doe@example.com>

\t<the patch itself>

Jane Doe will send it further as:

\tAuthor: John Doe <john.doe@example.com>

        Short summary of the patch

\tA more detailed description of this patch fixing x.y.z.  This can
        even span over several lines to explain the patch.

\tSigned-off-by: John Doe <john.doe@example.com>
\tSigned-off-by: Jane Doe <jane@doe.net>

\t<the patch itself>

When Bob Boss ACK's the patch, the final commit to be found in the source code repository will be:

\tAuthor: John Doe <john.doe@example.com>

        Short summary of the patch

\tA more detailed description of this patch fixing x.y.z.  This can
        even span over several lines to explain the patch.

\tSigned-off-by: John Doe <john.doe@example.com>
\tSigned-off-by: Jane Doe <jane@doe.net>
\tAcked-by: Bob Boss <bob@bigbucks.com>
\tSigned-off-by: David Sommerseth <dazo@users.sourceforge.net>


\t<the patch itself>

The Acked-by: can be the one who does the final commit to the source code repository, but it can also be someone who don't do the final commit. The last Signed-off-by should normally be the person the one who does the final commit. This is usually the person who "touched" the patch by adding the Acked-by line(s).

However, you should not see any commits which is written by (author) and Acked-by by the same person. Then the process have failed to give a qualified review.

You may ask why we have this "bureaucracy" (which is a fair question!). It is simply to keep all who send in patches, those who reviews them and finally accepts them into the source code repository more accountable for their work. This process will document each accepted patch and give an indication that it has been through a certain set of reviews. This is to ensure that we don't accept bad code easily into the source code repository. This is just to keep this project as transparent and open as possible.

For now, the only patches which which "breaks" this process in the git tree are the patches coming in from James' SVN BETA21 branch. But we trust that James tracks the patches he pulls in according to the standards he needs to feel comfortable with the OpenVPN code.

Handling security issues

The way OpenVPN project handles security issues was discussed and agreed upon in the IRC meeting on 15th July 2010. The goal is to disclose security issues in 3 weeks - or less, if a fix is ready. If a fix is not ready in 3 weeks the issue should be disclosed nevertheless and provide workarounds (if any) to users and then fix the issue a.s.a.p. Also, all security issues - whether they're theoretical or being exploited - should be fixed. Also agreed that our users should be informed about vulnerabilities in external software OpenVPN depends on (e.g. OpenSSL). This will be done after developers of the external software have already disclosed the vulnerability.

Patch quality

Formatting

Existing codebase uses an Allman based style. Please visit the Code Style wikipage for more details.

Conventions

All patches, regardless of their type need to be created following a few rules:

  • Must be sent as unified diff (diff -u) or preferably generated by git format-patch
  • Patches must have a short but descriptive one-line summary of the change, followed by a more descriptive text explaining in plain English why and how the patch is written. This description should not be too technical, as the patch itself will reveal technical details. More on writing good commit messages here
  • Patches should contain a 'Signed-off-by:' line at the end (git commit --signoff). If missing, it will be added with the sender of the patch as the signed-off person.
  • Patches must apply cleanly, without merge conflicts. Please state which code base the patch is written against. If based on the git tree, please state which branch it is to be applied to.
  • Everyone who has contributed to this patch should be mentioned out of courtesy and respect to all contributors and helpers on the way. If suitable valid e-mail address should be used, preferably in addition with full name.

Code contributions

All code patches need to meet certain generic quality criteria before being accepted:

  • All code should be useful and beneficial for several OpenVPN users. This way we avoid spoiling the code base with features which is only requested for very special conditions.
  • New features need to make use of #ifdef's so that they can be disabled at compile-time. This is to enable better support for embedded systems and to track which code belongs to which feature.
  • Patch needs to respect our Code Style to keep the code base understandable and maintainable.

Also note that the documentation (e.g. man pages) need to be updated, if

  • New functionality has been introduced
  • Old functionality has changed
  • Functionality has been removed

Shell scripts

Patches to shell scripts such as those in easy-rsa should be POSIX-compliant for portability reasons. If the existing scripts don't fulfill this requirement, please provide a patch :). He're are a few links to relevant resources:

Code repositories

Contents of this section are now here.

Practical issues

Using Git

If you're unfamiliar with git in general, take a look at these links:

Bisecting commits to detect introduction of a bug

In most cases, it's easiest to use git-bisect to find the commit which introduced a problem. However, if the buildsystem is not in perfect working order all the way through from last known good commit to known bad, you may need to do manual bisecting such as was done here. In practice, you can reset to an earlier commit with

$ git reset --hard <commit-id>

Next build and test. If it still fails, move further back in history and retry, until you find a version that works. Optimally, you should bisecting at the middle of commits between known good and known bad, then repeat the procedure until you pinpoint the bad commit.

If you have hunch which commit might have introduced the bug, you can try reverting it to see what happens:

git revert <commit-id>

In case no later commits conflict with the commit, this will work. If there are conflicts, fix them manually or abort the revert and write a reverting commit manually.

Applying patches from emails

It's often necessary to test individual patches sent to a mailing list. Patches that are real attachments are trivial to download and merge. However, sometimes the patches are stored inline, in the message body, which makes thing slightly more difficult. If you're running Mozilla Thunderbird, you export emails containing inline emails as mbox file using ImportExportTools? add-on. These can then be applied to a git repository using git am <mbox-filename>.

Sending GitHub pull requests to the mailing list

Getting a "git am"-compatible patch out of a GitHub pull requests is simple:

$ wget https://github.com/OpenVPN/openvpn/pull/<pr-number>.patch

You can then apply the patch:

$ git am <pr-number>.patch

Then you should amend the commit message to add information and to fix errors (if any):

  • What pull request the patch was created from
  • Who ACKed the patch
  • Who relayed the patch (in case if you're not the author)
  • Fix formatting issues

An example below:

$ git commit -s --amend
Update contrib/pull-resolv-conf/client.up for no DOMAIN

When no DOMAIN is received from push/pull, do not add either domain or
search to the resolv.conf. Fix typo in comment resolv.con[f]. Only add
new line when using domain or search.

URL: https://github.com/OpenVPN/openvpn/pull/34
Acked-by: Steffan Karger <steffan@karger.me>
Signed-off-by: Samuli Seppänen <samuli@openvpn.net>

# Please enter the commit message for your changes. Lines starting
# with '#' will be ignored, and an empty message aborts the commit.
#
# Author:    Jeffrey Cutter <jeff_m_cutter@yahoo.com>
# Date:      Sat Sep 12 20:03:18 2015 -0400
...

After adjusting the commit message you can send the patch using git-send-email.

Sending patches with git-send-email

A single commit can be sent with:

$ git send-email --to=openvpn-devel@lists.sourceforge.net HEAD^

Adjust the revision range as necessary if there are more than one patch.

Please bear in mind that the sender of the email is set from the commit author and the email address has to be subscribed to the list.

Converting SVN revisions to Git patches

There's a Python script available that converts SVN revisions into patches git am can understand:

To use this script, copy it into the SVN repository root as svnrev2git.py. Then create an authors file using this format:

svnusername, firstname lastname, email

For example:

johndoe, John Doe, john.doe@domain.com

To use the script, call it like this:

$ python svnrev2git.py authors <revision>
$ python svnrev2git.py authors <revision_start>-<revision_end>

For example:

$ python svnrev2git.py authors 8126
$ python svnrev2git.py authors 8126-8225
$ for REV in 8206 8212 8219 8225; do python svnrev2git.py authors $REV;done

Note that the script is slow in processing long revision ranges: it's usually a better idea to pick the required revisions by hand.

Poor-man's Symdiff

Andj came up with a clever script in an IRC meeting to generate diffs that make reviewing refactoring patches easier:

#!/bin/bash
git diff $1 $2 >/tmp/difftmp123.txt
cat /tmp/difftmp123.txt |grep "^-" |sed s/^-// >/tmp/removed123.txt
cat /tmp/difftmp123.txt |grep "^+" |sed s/^+// >/tmp/added123.txt
diff /tmp/removed123.txt /tmp/added123.txt -u

This is similar to Symdiff.

Attachments (1)

Download all attachments as: .zip