CONTRIBUTING.md 14.5 KB
Newer Older
1 2 3
Contributing to Nektar++
========================

4 5 6 7 8 9 10 11 12 13 14 15 16
## Contents
This is a reasonably complete guide to help if you're interested in contributing
to Nektar++, either in reporting bugs or, hopefully, trying to fix them! It's
split up into a number of sections:

- [Issues and bug reports](#issues-and-bug-reports)
- [How to contribute](#how-to-contribute)
- [Submission checklist](#submission-checklist)
- [Git cheatsheet](#git-cheatsheet)
- [Testing and Buildbot](#testing-and-buildbot)
- [Documentation](#documentation)
- [Formatting guidelines](#formatting-guidelines)

17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46
## Issues and bug reports
Think you've found a bug or issue with Nektar++? We're very keen to hear about
it!
- In the first instance, you should raise an issue on the
  **[issue tracker](https://gitlab.nektar.info/nektar/nektar/issues)** -- be
  sure to do a quick search and see if anyone has reported the same thing first.
- Alternatively you can
  **[join the mailing list](https://mailman.ic.ac.uk/mailman/listinfo/nektar-users)**
  for more advice.

It's *really helpful* if you can include a small session file that reproduces
the error, and can give a good description of the problem you're having.

## How to contribute
If you've got a patch or feature, please consider contributing it back to the
project. It's a pretty simple process:

1. Fork the Nektar++ repository in `nektar/nektar` into your username's space.
2. Create a branch with the naming convention:
   - `feature/myawesomebranch`: a new feature that wasn't in Nektar++ already.
   - `fix/mygreatfix`: fixes an issue that isn't tracked in the issue tracker.
   - `ticket/123-myfantasticpatch`: fixes an issue that is tracked in the issue
     tracker (please include the issue number somewhere!)
   - `tidy/mybrillianttidying`: cosmetic fixes to bring existing files up to the
     Nektar++ code guidelines.
3. Make sure you've gone through the checklist below.
4. Submit a merge request to merge into `master`. If you just want to see the
   diff and are not quite ready to merge, use the `[WIP]` tag in the title to
   prevent your code from being accidentally merged.
5. Put a comment in the MR saying that it's ready to be merged.
Dave Moxey's avatar
Dave Moxey committed
47 48 49
6. If your branch is a minor fix that could appear in the next patch release,
   then add the `Proposed patch` label to the merge request.
7. Respond to any comments in the code review.
50 51 52 53 54

## Submission checklist
- Did you add regression tests (for fixes) or unit tests and/or normal tests for
  new features?
- Have you run your branch through buildbot and do all the tests pass?
55 56 57 58 59 60 61 62 63
- Have you fixed any new compiler warnings your code has introduced into the
  compilation step for all of the Linux buildbots?
  - **unused parameters**: if these are genuinely needed (e.g. virtual functions
    in a derived class, please use `boost::ignore_unused()` to mark as such.
  - **switch case may fall-through**: for switch statements which
    *intentionally* exploit fall-through between cases, mark the end of such
    cases with the comment `/* Falls through. */` to suppress the warning.
  - Avoid `ASSERTL0(false, msg)`; instead use `NEKERROR(ErrorUtil:efatal, msg)`.
  - Ensure variables are initialised with sensible default values.
64
- Is there documentation in the user guide and/or developer guide?
65
- Have you added a CHANGELOG entry, including the MR number?
66
- Are there any massive files you might have added in the commit history? We try
67 68 69 70 71 72 73 74
  to keep test files as small as possible. If so you'll need to rebase or
  filter-branch to remove those from the commit history.
- Is the code formatted correctly?
  - **Note:** unfortunately, Nektar++ has pretty inconsistent code formatting at
    the moment. To help in reviewing your submission, new files should be
    formatted according to the guidelines (or use `clang-format` as described
    below) -- otherwise, try to keep formatting consistent with the file you're
    working on.
75

Dave Moxey's avatar
Dave Moxey committed
76 77 78 79 80 81 82 83 84 85 86
## Git cheatsheet
Although Gitlab gives a nice interface to view the diff between a branch and
master, for large merges, it can be slow. The following `git` aliases can
provide a quicker alternative. You can use these by inserting them into the
`.gitconfig` file in your home directory, or inside the `nektar++/.git/config`
file.

```
[alias]
branch-name = "!git rev-parse --abbrev-ref HEAD"
diff-nows = diff --color -w
87 88
log-branch = log --pretty='%C(green)%h %C(red)%an %C(reset)(%C(blue)%ad%C(reset))%n%s' master..
diff-branch = diff -U5 --minimal --color -w master...
Dave Moxey's avatar
Dave Moxey committed
89 90 91 92 93 94 95 96 97
```

This gives you four commands:

- `git branch-name` displays the current branch name
- `git diff-nows` shows a diff of your current commit in colour, without
  whitespace changes.
- `git log-branch` shows a minimised log of all the commits on the current
  branch that are not in `master`.
98 99 100 101
- `git diff-branch` shows a diff of the current branch against `master`, without
  showing changes from `master` that aren't present in the branch (i.e. `git
  diff master...branch`), without whitespace changes. (This should be roughly
  equivalent to Gitlab's diff).
Dave Moxey's avatar
Dave Moxey committed
102

103 104 105
If you prefer a graphical interface to see the files that have changed in your
commit, you can additionally use the `git gui` command to bring up a simple
interface. `git difftool` can also be used in combination with a GUI diff
106
viewer, to graphically view the output of `git diff`.
107

108 109 110 111
## Testing and Buildbot
Your new features or fixes should include tests that cover the code you've
added. There are numerous examples within the various `Tests` directory lying
within the source trees, and there is an example of writing `.tst` files for our
112 113 114 115
`Tester` executable in the `tests/Examples` directory. Once you've written your
tests, add them to the `CMakeLists.txt` file for the relevant solver, or to the
appropriate demos directory for library features in whatever directory you are
working in.
116 117 118 119 120 121

You should also test your branch on the
[Nektar++ buildbot](http://buildbot.nektar.info/), which will compile and test
the code against a number of Linux, Mac and Windows operating systems, both 32-
and 64-bit. If your tests don't pass, we can't merge the code into master.

122 123 124 125 126 127 128 129 130 131 132 133 134 135 136 137 138
When you submit a merge request testing on buildbot will happen automatically,
unless you have marked the merge request as a work-in-progress (WIP: prefix).
Each time you push commits to a non-WIP merge request branch, it will also
trigger a build.

For WIP merge request branches, you can force a build on individual builders
using the buildbot web interface:

1. Go to the buildbot site and log in using the drop-down menu (top right).
2. Go to the console display and select the builder from the column headings on
   which you would like to force test.
3. Select 'Force' from the top-right of the page.
4. Enter the details as required. If you're working on a fork, then the *Fork*
   box should be changed to `username/nektar`.
5. Hit the *Start build* button.
6. Return to the *console* display to see progress. You can click on the
   flashing indicator for your active build to see progress and output.
Dave Moxey's avatar
Dave Moxey committed
139

140 141
## Documentation
Nektar++ has a fairly comprehensive user guide and a developer guide that is
142 143 144 145 146 147 148 149
presently very incomplete. The following are rough guidelines for what you
should provide:

- If you are writing user-exposed features, you should add some documentation to
  the user guide on how to use them.
- Any functions/classes should include Doxygen documentation.
- Generally, code should be well-commented using regular C++ comments to explain
  its function to help in reviewing it.
150

151 152 153 154
Nektar++ also has a growing number of tutorials to help introduce users and
developers to the use of the library and the range of application solvers. These
are stored in a separate repository, but are available from the main repository
through a git submodule. To populate the docs/tutorial directory run `git
155 156 157
submodule init` followed by `git submodule update --remote`. The latter command
will ensure you have the latest master branch of the tutorials within your
source tree.
158

159 160 161 162 163 164 165
## Code review and merging
All merge requests will be reviewed by one of the senior developers. We try to
stick to the following process:
- Senior developer will be assigned, MR will be assigned a milestone to target a
  release.
  - If the branch is deemed to be minor and passes the checklist above, senior
    developer will handle the request by themselves.
166 167 168
  - Otherwise, senior developer will ask one or more other developers to review
    the code.
- Submission checklist will be checked by the reviewers.
169 170
- Where appropriate, reviewers will comment on regions of code that need further
  development and/or improvement.
171 172 173
- In addition to any coding comments/suggestions, reviewers are asked to check
  the branch passes the regression tests and appropriate documentation has been
  added.
174 175 176
- Once feedback received from the branch author (if necessary) and reviewers are
  happy, the branch will be merged.

Dave Moxey's avatar
Dave Moxey committed
177 178 179 180 181 182 183 184
## Release branches
Nektar++ releases are versioned in the standard form `x.y.z` where `x` is a
major release, `y` a minor release and `z` a patch release:

- major releases are extremely infrequent (on the order of every 2-3 years) and
  denote major changes in functionality and the API;
- minor releases occur around twice per year and contain new features with minor
  API changes;
185 186
- patch releases are targeted on roughly a monthly basis and are intended to
  fix minor issues in the code.
Dave Moxey's avatar
Dave Moxey committed
187 188 189 190 191 192 193 194 195 196 197 198 199 200 201 202 203 204 205 206 207 208 209 210 211 212 213 214 215 216 217 218 219 220 221 222 223 224 225 226 227 228 229 230 231

The repository contains a number of _release branches_ named `release/x.y` for
each minor release, which are intended to contain **fixes and very minor
changes** from `master` and which form the next patch release. This allows us to
use `master` for the next minor release, whilst still having key fixes in patch
releases.

### Cherry-picking process

Any branches that are marked with the `Proposed patch` label should follow the
following additional steps to cherry pick commits into the `release/x.y` branch.

1. If the branch is on a remote other than `nektar/nektar`, make sure that's
   added to your local repository.
2. On a local terminal, run `git fetch --all` to pull the latest changes. It's
   important for the commands below that you do this _before_ you merge the
   branch into `master`.
3. Merge the branch into master as usual using GitLab.
4. Switch to the appropriate branch with `git checkout release/x.y` and update
   with `git pull`.
5. Now check the list of commits to cherry-pick.

   ```bash
   git log --oneline --no-merges --reverse origin/master..REMOTE/fix/BRANCHNAME
   ```

   where `REMOTE` is the remote on which the branch lives and `BRANCHNAME` is
   the fix branch. If the list is empty, you probably did a `git fetch` after
   you merged the branch into `master`; in this case use `origin/master^`.
6. If you're happy with the list (compare to the MR list on the GitLab MR if
   necessary), cherry-pick the commits with the command:

   ```bash
   git cherry-pick -x $(git rev-list --no-merges --reverse origin/master..REMOTE/fix/BRANCHNAME)
   ```

7. It's likely you'll encounter some conflicts, particularly with the
   `CHANGELOG`. To fix these:
   - `git status` to see what's broken
   - Fix appropriately
   - `git commit -a` to commit your fix
   - `git cherry-pick --continue`
8. If everything becomes horribly broken, `git cherry-pick --abort`.
9. Once you're happy, `git push` to send your changes back to GitLab.

232 233 234 235 236 237 238 239 240 241 242 243 244 245 246 247 248
Steps 5 and 6 can be simplified by creating a script
```bash
#!/bin/bash
src=$1

logopts="--oneline --no-merges --reverse"
commits=`git log $logopts master..$1 | cut -f 1 -d " " | xargs`

echo "Will cherry-pick the following commits: $commits"
echo "Press ENTER to continue..."
read

cherryopts="-x --allow-empty --allow-empty-message"
git cherry-pick $cherryopts $commits
```
which accepts the name of the source branch as the sole argument.

249 250 251 252 253 254 255 256 257 258 259 260 261 262 263
## Formatting guidelines
Nektar++ uses C++, a language notorious for being easy to make obtuse and
difficult to follow code. To hopefully alleviate this problem, there are a
number of fairly simple formatting guidelines you should follow. We are
reasonably relaxed about code formatting, but if you can follow the guidelines
below this would be fantastic.

### Basic rules
- All code should be wrapped to 80 characters.
- Indentation should be 4 spaces with **no tabs**. Namespaces should not be
  indented to give more room in the 80 character width.
- Please comment your code with Doxygen and inline comments wherever possible --
  but don't use trailing inline comments to save the 80 character limit!
- All code blocks (even one-line blocks) should use braces, and braces should be
  on new lines; for instance
264
  
265
  ```c++
266 267
  if (someCondition)
  {
268 269 270
      myAwesomeFunction();
  }
  ```
271

272
- **Don't use preprocessor directives and macros unless there is no viable
Dave Moxey's avatar
Dave Moxey committed
273 274
  alternative.**
- However, please make sure you do have a header guard inside your `.h` files,
Dave Moxey's avatar
Dave Moxey committed
275
  which you should be sure to include in any headers you contribute.
276 277 278 279 280 281 282
- Use one `.cpp` and `.h` file per C++ class, and try to keep `inline` header
  code to a minimum (unless performance is a major factor).
- Put spaces around binary operators and constants.
- Put spaces after `if`, `while`, etc., but not after function names (see the
  example above).

### Variables and naming
Dave Moxey's avatar
Dave Moxey committed
283 284 285 286
- Please use sensible names and use camelCase as a broad naming convention.
  - Variables should start with a lowercase letter, e.g. `myAwesomeVariable`.
  - Function, `class`, `struct` and `typedef` names should begin with capital
    letters, e.g. `MyAwesomeFunction`.
287 288 289 290 291 292 293 294 295 296
- Inside classes, member variables should be prefixed with `m_`,
  e.g. `m_myAwesomeVariable`.
  - Global constants used throughout the library should be prefixed with `k`
    (e.g. `kGeometricTolerance`), and enumerations should be prefixed with `e`
    (e.g. `eGeometry`).
- Use all uppercase letters with underscores between words for pre-processor
  definitions and macros.

### Using `clang-format`
Code formatting is reasonably boring, so Nektar++ comes with a `.clang-format`
297 298 299 300
file to allow for automatic code formatting. As noted above, you can use this
for new files, or cosmetic `tidy/*` branches, but try to stick to existing
formatting elsewhere.

301
Installing it is straightforward on most package managers. Nektar++ relies on
302
options that are used in version 3.7 or later.
303 304 305 306 307 308 309 310 311 312 313 314

There are a number of instructions on how to use `clang-format` inside a number
of text editors on the
[CLang website](http://clang.llvm.org/docs/ClangFormat.html). However at a
minimum, you should consider downloading the
[``git-clang-format``](https://llvm.org/svn/llvm-project/cfe/trunk/tools/clang-format/git-clang-format)
script into one of your `$PATH` locations. You can then run the command

    git clang-format

before you do a `git commit`, and `clang-format` will automatically format your
diff according to the guidelines.