Version 3 (modified by 7 years ago) (diff) | ,
---|
Table of Contents
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
Development process
The basic development process we follow is outlined in this diagram:
In a nutshell:
- Patch is sent to "openvpn-devel" mailing list for public review
- Patch will be reviewed and either accepted (ACK) or rejected (NACK). The ACK consists of two parts:
- Feature-ACK: does the patch make sense to have?
- Code-ACK: does the patch meet our code quality standards?
- Accepted patches go to the Git "master" branch
GitHub pull requests can be used to preliminary review, or to gather feedback about the patch.
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. It was 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 the following formatting conventions:
- Indentation is 2 spaces (no tabs)
- Indentation follows GNU Coding Style
- Emacs
- c-style gnu
- c-basic-offset 2
- indent-with-tabs nil
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 coding conventions? 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:
- Bashism - Greg's Wiki
- Checkbashisms: a tool to check for "Bashisms". Also included in Debian/Ubuntu? "devscripts" package.
- DASH: a minimal shell with "POSIX-compliant features only", probably useful for testing
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:
- git crash course
- Git homepage
- http://progit.org/book/
- http://www.kernel.org/pub/software/scm/git/docs/gittutorial.html
- git for SVN users
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 with git-send-email
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
And 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:
$ git-send-email --to=openvpn-devel@lists.sourceforge.net HEAD^1...HEAD
Adjust as necessary if you there is more than one patch.
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.