Interested in developing new features, fixing bugs, or contributing code you have laying around? Read on to find out how.
We follow a development process designed to reduce errors, encourage collaboration, and make high quality code. The process may seem rigid and tedious, but every step is worth the effort (especially if you like applications that work).
We will use the Gazebo repository as an example, but the step apply equally well to other repositories maintained by OSRF.
Are you sure?
Run through this mental checklist before getting started.
Has your idea already been done, or maybe someone is already working on it?
Check answers.gazebosim.org and the issue tracker.
Get feedback from the Gazebo core team. Post a topic on community.gazebosim.org, post a question on answers.gazebosim.org, or use the issue tracker to get feedback from Gazebo developers.
This will create your own personal copy of Gazebo. All of your development should take place in your fork.
Work out of a branch: git checkout -b my_new_branch_name
Always work out of a new branch, never off of master. This is a good habit to get in, and will make your life easier.
Choose a base branch.
If your changes will break API or ABI, then base your new branch off of master. If your changes don't break API/ABI and you would like them to be released to an existing gazebo release with major version
N
, then use branchgazeboN
as the base.
Write your code.
This is the fun part.
Write tests.
A pull request will only be accepted if it has tests. See the
Test coverage
section below for more information.
Compiler warnings.
Code must have zero compile warnings. This currently only applies to Linux.
Style
A tool is provided in Gazebo (and other repositories) to check for correct style. Your code must have no errors after running the following command from the root of the source tree:
sh tools/code_check.sh
The tool does not catch all style errors. See the Style section below for more information.
Tests pass
There must be no failing tests. You can check by running
make test
in your build directory.
Documentation.
Document all your code. Every class, function, member variable must have doxygen comments. All code in source files must have documentation that describes the functionality. This will help reviewers, and future developers.
Review your code.
Before submitting your code through a pull request, take some time to review everything line-by-line. The review process will go much faster if you make sure everything is perfect before other people look at your code. There is a bit of the human-condition involved here. Folks are less likely to spend time reviewing your code if it's bad.
Small pull requests
A large pull request is hard to review, and will take a long time. It is worth your time to split a large pull request into multiple smaller pull requests. For reference, here are a few examples:
Submit a pull request from your GitHub fork when you are ready.
Review
At least two other people have to approve your pull request before it can be merged. Please be responsive to any questions and comments.
We have a dashboard that lists all open pull requests. Only pull requests above the green line can be merged, and they must also have at least two approvals. We do this to encourage the review of pull requests. If you submit a pull request, then try to review other open pull requests. This will reduce the time it takes for your code to get accepted, and also gets more eyes on more code.
Done, phew.
Once you have met all the requirements, your code will be merged. Thanks for improving Gazebo!
This section is targeted mostly for people who have commit access to the main repositories.
In addition to the general development process, please follow these steps before submitting a pull request. Each step is pass/fail, where the test or check must pass before continuing to the next step.
In general, we follow Google's style guide. However, we add in some extras.
This pointer
All class attributes and member functions must be accessed using the
this->
pointer. Here is an example.
Underscore function parameters
All function parameters must start with an underscore. Here is an example.
Do not cuddle braces
All braces must be on their own line. Here is an example.
Multi-line code blocks
If a block of code spans multiple lines and is part of a flow control statement, such as an
if
, then it must be wrapped in braces. Here is an example
++ operator
This occurs mostly in
for
loops. Prefix the++
operator, which is slightly more efficient than postfix in some cases.
PIMPL/Opaque pointer
If you are writing a new class, it must use a private data pointer. Here is an example, and you can read more here.
const functions
Any class function that does not change a member variable should be marked as
const
. Here is an example.
const parameters
All parameters that are not modified by a function should be marked as
const
. This applies to parameters that are passed by reference, pointer, and value. Here is an example.
Pointer and reference variables
Place the
*
and&
next to the varaible name, not next to the type. For example:int &variable
is good, butint& variable
is not. Here is an example.
Camel case
In general, everything should use camel case. Exceptions include SDF element names, and protobuf variable names. Here is an example.
Class function names
Class functions must start with a capital letter, and capitalize every word.
void MyFunction();
: Allowed
void myFunction();
: Not Allowed
void my_function();
: Not Allowed
Variable names
Variables must start with a lower case letter, and capitalize every word thereafter.
int myVariable;
: Allowed
int myvariable;
: Not Allowed
int my_variable;
: Not Allowed
No inline comments
//
style comments may not be placed on the same line as code.
speed *= 0.44704; // miles per hour to meters per second
: Not Allowed
Accessors must not start with Get
Member functions granting read access to protected or private data must look like a noun.
public: ::ServerConfig ServerConfig() const;
: Allowed
public: ServerConfig GetServerConfig() const;
: Not AllowedCorner Cases
- A class name may conflict with an accessor function. For example,
Model(int)
would conflict with aModel
class. In these cases, try to follow theNoun
-By
pattern. For example:
ModelByName(const std::string &_name)
: Allowed > *ModelById(const int _id)
: Allowed
- A template function that returns a data type may use a stand-alone
Get
. For example:
- public: template
T Get();` : Allowed
Mutators must start with Set
Member functions granting write access to protected data must begin with
Set
.
public: void SetServerConfig(ServerConfig &_config);
: Allowed
public: void ServerConfig(::ServerConfig &_config);
: Not Allowed
Check to make sure someone else is not currently working on the same feature, before embarking on a project to add something to Gazebo. Simply send a quick email to the Gazebo mailing list expressing your interest and idea. Someone will get back to you shortly about your idea.
All code should have a corresponding unit test. Gazebo uses GTest for unit testing. All regression test should be placed in <gazebo_sources>/test/regresssion/
.
Before creating a new regressions test file, check the current test files. If one closely matches the topic of your new code, simply add a new test function to the file. Otherwise, create a new test file, and write your test.
The goal is to achieve 100% line and branch coverage. However, this is not always possible due to complexity issues, analysis tools misreporting coverage, and time constraints. Try to write as complete of a test suite as possible, and use the coverage analysis tools as guide. If you have trouble writing a test please ask for help in your pull request.
Gazebo has a build target called make coverage
that will produce a code coverage report. You'll need lcov and gcov installed.
In your build
folder, compile Gazebo and the tests with -DCMAKE_BUILD_TYPE=Coverage
cmake -DCMAKE_BUILD_TYPE=Coverage ..\
make
make tests
Run a single test, or all the tests
make test
Make the coverage report
make coverage
View the coverage report
firefox coverage/index.html
An assertion is a check, which always produce a boolean result, that developers place in the code when want to be sure that check is always true. They are aimed to detect programming errors and should check for impossible situations in the code. If the assertion check failed, the assertion will stop the program immediately.
Object * p = some_crazy_function()
GZ_ASSERT(p != NULL, "Object from some_crazy_function should never point to NULL")
p->run()
In Gazebo, the GZ_ASSERT macro is designed to handle all our runtime assertions
GZ_ASSERT(<condition to check>,<fail msg>)
Some of the benefits of using the assertions:
While assertions are intended for impossible situations resulting from programming errors, exceptions handle all kinds of expected errors and unusual but logically possible code situations.
Let's review an example: suppose we are writing a math library and created a really fast method to calculate square roots, but it only works for positive numbers. Something declared as:
double sqrt_for_positives(double number)
What could be an assertion and what would be an exception for our revolutionary function?
Exception: If the incoming number is negative (our function only accepts positive numbers), then we will throw an exception. It was an error by the user, but we should consider it a possible scenario since we are offering a public interface.
Assertion: Our square root should never return a negative number. This is not a logical error; it is a completely unexpected error which should be impossible if the function is written correctly.
In order to provide meaningful backtraces when using a debugger, such as GDB, Gazebo should be compiled with debugging support enabled. When using the ubuntu packages, specially the ''-dbg'' package, this support is limited but could be enough in most situations. There are three level of traces which can be obtained:
'''Maximum level of debugging support'''
: This can only be obtained by compiling Gazebo from source and setting the CMAKE_BUILD_TYPE
to DEBUG
. This will set up debugging symbols with no optimizations. It may be required by developers for problems that are especially difficult to track down.
'''Medium level of debugging support'''
: This can be obtained installing the ''gazebo-dbg'' package (since 1.4 version) or compiling Gazebo from source using the RELWITHDEBINFO
CMAKE_BUILD_TYPE
mode (which is the default if no mode is provided). This will set up ''-O2'' optimization level but provide debugging symbols. This should be the default when firing up gdb to explore errors and submit traces.
'''Minimum level of debugging support'''
: This one is present in package versions previous to 1.4 (no ''-dbg'' package present) or compiling Gazebo from source using the RELEASE
CMAKE_BUILD_TYPE
option. This will set up the maximum level of optimizations and does not provide any debugging symbol information. These traces are particularly difficult to follow.
Code pushed into the Gazebo repository should pass a few simple tests. It is also helpful if patches submitted through GitHub pass these tests. Passing these tests is defined as generating no error or warning messages for each of the following tests.
In your Gazebo build directory run make test
:
make test
All the tests should pass. If they do not, you can run and debug the tests individually. For example, to run the transport test from your build directory:
./test/regression/transport
Static code checking analyzes your code for bugs, such as potential memory leaks, and style. The Gazebo static code checker uses cppcheck, and a modified cpplint. You'll need to install cppcheck on your system. Ubuntu users can install via:
sudo apt-get install cppcheck
To check your code, run the following script from the root of the Gazebo sources:
sh tools/code_check.sh
It takes a few minutes to run. Fix all errors and warnings until the output looks like:
Total errors found: 0
CMAKE_BUILD_TYPE=Check
compiles with no warningsThis test compiles Gazebo with numerous warning flags enabled. The source code for Gazebo should compile cleanly. This does not include code in the deps
directory. As a rule of thumb, start looking for compilation warnings after the proto messages are built which appear as a series of blue text:
Linking CXX executable gazebomsgs_out
Running C++ protocol buffer compiler on axis.proto
Running C++ protocol buffer compiler on boxgeom.proto
Running C++ protocol buffer compiler on camerasensor.proto
Running C++ protocol buffer compiler on collision.proto
Running C++ protocol buffer compiler on color.proto
Running C++ protocol buffer compiler on contact.proto
Running C++ protocol buffer compiler on contacts.proto