Trace Compass/Contributor Guidelines
This page explains how to contribute code to the Trace Compass project.
When to submit patches
As with any open-source project, external contributions are very appreciated! If you have a simple and straightforward fix to an obvious bug, feel free to push it directly to the project's Gerrit (see below).
If you wish to work on a larger problem or feature, it would be a good idea to contact us first, either on the mailing list or in the IRC channel. It could avoid duplicate work in case somebody is already working on the same thing. For substantial new features, it is always good to discuss their design and integration first on bugzilla.
What to submit
- Your patch should not introduce any error or warning.
- Follow the project's coding style. It is defined in Eclipse project settings, which are committed in the tree, so simply hitting Ctrl+Shift+F in Eclipse should auto-format the current file.
- Basically, 4 spaces for indentation, opening brackets on the same line.
- We've turned off the auto-wrapping and unwrapping of lines, because it was doing more harm than good. Please wrap lines to reasonable lengths (100-120 is a good soft limit). Do NOT wrap after '.' (method invocations), it makes the code less readable. After '(' or ',' is usually good.
- Split your larger contributions in smaller, independant commits that could go in independently of each other. It is not unsual to have patch #1 go in quickly, but then have patch #2 go through many rounds of review. Smaller commits make the process faster for everyone (remember, the time taken to review a patch is n^2, where n is the number of lines in the patch!)
- Use short and meaningful commit titles. The commit message should be clear and explain thoroughly the nature of the commit. Good advice about commit titles/messages can be found at this page. Also make sure your commits have Change-Id (see below) and the email address of the committer must be associated with the Eclipse account with a signed Eclipse Contributor Agreement (see below).
- Not a hard requirement, but we normally prefix the commit titles with the name of the component to which the patch applies, in lowercase. This normally corresponds to the plugin name, so "lttng: xxxx" or "tmf: xxxx".
- If the patch is significant enough to have a mention in the new and noteworthy, annotate the patch with [Fixed], [Added], [Changed], [Deprecated], or [Security] with a short description of what the change does.
Where to submit
The Trace Compass maintainers use the Gerrit installation hosted at Eclipse to submit patches and review external contributions. This means anybody can push a branch to a special Git URL, which will create an entry on the Web UI where the review process can be followed.
Instructions can be found at the Gerrit page on the Eclipse wiki. The most important parts are:
- Make sure you have an Eclipse account, and that you have signed the Eclipse Eclipse Contributor Agreement.
- Upload your SSH key via the [Gerrit Web UI].
- Set up the git-commit hook, and make sure your commits have a Change-Id line in them.
If any of these are missing, Gerrit will reject your patches when you try to push them, with a message explaining what should be done to rectify it.
To push patches to the Trace Compass Gerrit, use the following remote configuration:
[remote "eclipse-review"] url = ssh://<username>@git.eclipse.org:29418/tracecompass/org.eclipse.tracecompass.git push = HEAD:refs/for/master
<username> with your Eclipse user name.
The review process
We will try to give feedback to new patches in a reasonable amount of time, usually in the following days. Smaller patches makes the review go faster.
We try to have two (2) committers review every patch that goes in. For a patch from a committer, this means a second committer should review it. For a patch from a non-committer, one committer should do a thorough review, and at least a second one should give a quick look.
For a patch to be approved, it need to be marked as Code Review and Verified. Code Review means the patch "looks" good, it follows the coding style, uses relevant algorithms and data structures, etc. Verified in general means that the patch technically works, it compiles, the tests pass, and it does what it is supposed to do.
The meanings of each score are listed below.
+2: The reviewer considers that the patch follows the coding style, and implements the expected functionality correctly. At least one +2 is required for the patch to go in. Only committers can give this score.
+1: The reviewer considers that this patch is good, but that someone else should also take a look at parts or the totality of the patch. Should be accompanied with a comment indicating what is missing to go to +2 if the review is from a committer. Anybody can give this score.
0: Comments can accompany a "0" score, meaning that the reviewer considers them optional, and they would not block the merge of the patch.
-1: The overall direction of the patch is good, however there are some things that should be fixed first. Should always be accompanied with general and/or inline comments indicating areas of the code that need improvement. All comments that are given are expected to be either A) fixed in a subsequent patchset or B) replied to with explanation/justification. Anybody can give this score.
-2: Can mean different things:
- From the point of view of the reviewer, the overall direction of the patch is not the right one and it warrants further discussion. Meaning discussions on Bugzilla, the mailing list, or in person. Gerrit is not the best place for design discussions.
- The patch should not go into the tree for some reason, like if it is just a test patch, or for IP-related reasons.
- Because Gerrit will prioritize +2 over -1, a -2 can be used temporarily instead of a -1, to overshadow a +2 and make sure the comments are seen by the author of the patch.
- -2 is the only score that will persist through subsequent patchsets (all other scores are reset when a new patchset is pushed). For this reason, it can be used to "pin" a -1 until a particular condition is fulfilled, like another dependant patch being merged, or if it is awaiting on a CQ.
A patch cannot be merged through Gerrit if there is at least one -2 on it. In all cases, it should be accompanied with a comment explaining what the reason is. Only committers can give this score.
To "verify" a patch typically means to pull it locally and test running the program with it. Reviewers are strongly encouraged to actually download and try the patches, except very trivial ones.
+1: The patch works as expected: for features this means the feature is implemented and works correctly. For bug fixes it means the bug can be reproduced without the patch, and disappears once the patch is applied.
0: Comments that are focused on the code only typically go with a 0 Verified score.
-1: The patch fails to fix the bug, or there is a bug in the usage of the new feature. Can also be used to indicate test failures.
Only committers can give "Verified" scores. A non-committer is of course welcome to test patches, and if they find problems they can report them using a -1 Code Review score.
The Gerrit-Hudon plugin should trigger a Hudson build for all patches pushed to Gerrit. If there is a compilation or test failure, Hudons will apply a -1 Verified score. If the build is successful, it will give a +1 Code Review score. It gives +1 CR and not +1 Verified, to still encourage real humans to pull and test the patches, and not just assume a lack of test failure means the patch works as expected.
Pushing new patchsets
To push a new version of a Gerrit patch, simply amend (using
git commit --amend or by using the amend button) the commit and push again to the same branch. This is where the Change-Id line comes into play: if Gerrit sees that same Change-Id targeted at the same branch, it will add a new version to the existing entry instead of creating a new entry.