Skip to content

Latest commit

 

History

History
549 lines (423 loc) · 20.5 KB

CONTRIBUTING.md

File metadata and controls

549 lines (423 loc) · 20.5 KB
Raw

Contributing to open62541

Contributions to open62541 include code, documentation, answering user questions, running the project's infrastructure, and advocating for all types of open62541 users.

The open62541 project welcomes all contributions from anyone willing to work in good faith with other contributors and the community. No contribution is too small and all contributions are valued.

This guide explains the process for contributing to the open62541 project's core repository and describes what to expect at each step. Thank you for considering these point.

Your friendly open62541 community!

Code of Conduct

The open62541 project has a Code of Conduct that all contributors are expected to follow. This code describes the minimum behavior expectations for all contributors.

Pull Requests

Everybody can propose a pull request (PR). But only the core-maintainers of the project can merge PR.

The following are the minimal requirements that every PR needs to meet.

  • Pass Continuous Integration (CI): Every PR has to pass our CI. This includes compilation with a range of compilers and for a range of target architectures, passing the unit tests and no detected issues with static code analysis tools.

  • Code-Style: Please consider the Code-Style recommendations when formatting your code.

  • Signed CLA: Every contributor must sign the Contributor License Agreement (CLA) before we can merge his first PR. The signing can be done online. The link automatically appears on the page of the first PR. In addition, the CLA text can be accessed here.

  • Separation of Concerns: Small changes are much easier to review. Typically, small PR are merged much faster. For larger contributions, it might make sense to break them up into a series of PR. For example, a PR with a new feature should not contain other commits with only stylistic improvements to another portion of the code.

  • Feature Commits: The same holds true for the individual PR as well. Every commit inside the PR should do one thing only. If many changes have been applied at the same time, git add --patch can be used to partially stage and commit changes that belong together.

  • Commit Messages: Good commit messages help in understanding changes. See the next section.

  • Linear Commit History: Our goal is to maintain a linear commit history where possible. Use the git rebase functionality before pushing a PR. Use git rebase --interactive to squash bugfix commits.

These labels can be used for the PR title to indicate its status.

  • [WIP]: The PR is work in progress and at this point simply informative.
  • [Review]: The PR is ready from the developers perspective. He requests a review from a core-maintainer.
  • [Discussion]: The PR is a contribution to ongoing technical discussions. The PR may be incomplete and is not intended to be merged before the discussion has concluded.

The core-maintainers are busy people. If they take especially long to react, feel free to trigger them by additional comments in the PR thread. Again, small PR are much faster to review.

It is the job of the developer that posts the PR to rebase the PR on the target branch when the two diverge.

Changes to the Public API

The public API is the collection of header files in the /include folder.

Changes to the public API are under especially high scrutiny. Public API changes are best discussed with the core-maintainers early on. Simply to avoid duplicate work when changes to the proposed API become necessary.

You can create a special issue or PR just for the sake of discussing a proposed API change. The actual implementation can follow later on.

Breaking changes are frowned upon. If you cannot avoid API changes, try to create a new one and keep the old API alive (maybe with an addition UA_DEPRECATED macro).

Breaking changes have to be mentioned in the CHANGELOG file.

Commit and PR Hygiene

We have very precise rules over how our git commit messages can be formatted. This leads to more readable messages that are easy to follow when looking through the project history. But also, we use the git commit messages to generate the change log.

This convention is identical to the Conventional Commits specification or the one used by Angular.

Each commit message consists of a Header, a Body and a Footer. The header has a special format that includes a Type, a Scope and a Subject:

<type>(<scope>): <subject>
<BLANK LINE>
<body>
<BLANK LINE>
<footer>

The Header is mandatory and the Scope of the header is optional.

Any line of the commit message cannot be longer 100 characters! This allows the message to be easier to read on GitHub as well as in various git tools.

The footer should contain a closing reference to an issue if any.

Samples: (even more samples)

docs(server): add function documentation

fix(core): fix parsing of endpoint url

Parsing of endpoint urls now also supports https

If the commit reverts a previous commit, it should begin with revert: , followed by the header of the reverted commit. In the body it should say: This reverts commit <hash>., where the hash is the SHA of the commit being reverted.

The commit Type Must be one of the following:

  • build: Changes that affect the build system or external dependencies
  • ci: Changes to our CI configuration files and scripts (example scopes: travis, appveyor, fuzz)
  • docs: Documentation only changes
  • feat: A new feature
  • fix: A bug fix
  • perf: A code change that improves performance
  • refactor: A code change that neither fixes a bug nor adds a feature
  • style: Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc)
  • test: Adding missing tests or correcting existing tests

The commit Scope is optional, but recommended to be used. It should be the name of the component which is affected (as perceived by the person reading the changelog generated from commit messages). The following is the list of supported scopes:

  • arch: Changes to specific architecture code in root/arch
  • el: Changes to the eventloop and associated event sources (also networking)
  • client: Changes only affecting client code
  • core: Core functionality used by the client and server
  • ex: Example code changes
  • mt: Changes specifically for multithreading
  • nc: Nodeset compiler
  • pack: Packaging setting changes
  • plugin: Change to any (optional) plugin
  • pubsub: Changes to the pubsub code
  • sec: Changes to security, encryption, etc.
  • server: Changes only affecting server code

The Subject contains a succinct description of the change:

  • Use the imperative, present tense: "change" not "changed" nor "changes"
  • Don't capitalize the first letter
  • No dot (.) at the end

For the Body, Just as in the Subject, use the imperative, present tense: "change" not "changed" nor "changes". The body should include the motivation for the change and contrast this with previous behavior.

The Footer should contain any information about Breaking Changes and is also the place to reference GitHub issues that this commit Closes.

Code Style

General

  1. Avoid the use of Magic Numbers. For example use UA_STATUSCODE_BADINTERNALERROR instead of 0x80020000
  2. Avoid using global variables. If you for some reason really need to use global variables, prefix them with g_ so that it is immediately clear in the code, that we are dealing with a global variable.
  3. Almost all of the code is indented by four (4) spaces and regardless of which is better, spaces or tabs, you should indent with four spaces in order to be consistent.
  4. If you find yourself copy-pasting code, consider refactoring it into a function and calling that. Try keeping functions short (about 20-30 lines, that way they will in most cases fit onto the screen). In case of simple switch statements for example this can be exceeded, but there is not much complexity that needs to be understood. Most of the time, if functions are longer than 30 lines, they can be refactored into smaller functions that make their intent clear.
  5. Use of comments

Use C-style comments as follows:

/* Lorem ipsum dolor sit amet */

/* Lorem ipsum dolor sit amet, consetetur sadipscing elitr, sed diam nonumy
 * eirmod tempor invidunt ut labore et dolore magna aliquyam erat, sed diam
 * voluptua. At vero eos et accusam et justo duo dolores et ea rebum. Stet clita
 * kasd gubergren, no sea takimata sanctus est Lorem ipsum dolor sit amet. Lorem
 * ipsum dolor sit amet, consetetur sadipscing elitr, sed diam nonumy eirmod
 * tempor invidunt ut labore et dolore magna aliquyam erat, sed diam voluptua.
 * At vero eos et accusam et justo duo dolores et ea rebum. Stet clita kasd
 * gubergren, no sea takimata sanctus est Lorem ipsum dolor sit amet. */

Note that public header files (in the /include directory) use comments beginning with a double-star to generate documentation. Just follow along the formatting that is already in place.

/** This is documentation */

Every (good) editor can reflow comments for nice formatting.

Do not use C++ comments (double-slash //).

See the following general rules for comments: [1]

Comments are good, but there is also a danger of over-commenting. NEVER try to explain HOW your code works in a comment: it's much better to write the code so that the working is obvious, and it's a waste of time to explain badly written code.

Generally, you want your comments to tell WHAT your code does, not HOW. Also, try to avoid putting comments inside a function body: if the function is so complex that you need to separately comment parts of it, you should probably go back to chapter 6 for a while. You can make small comments to note or warn about something particularly clever (or ugly), but try to avoid excess. Instead, put the comments at the head of the function, telling people what it does, and possibly WHY it does it.

Methods

  1. Public methods are prefixed with UA_. If a public method operates on a server it begins with UA_Server_ and takes a pointer to UA_Server as its first arguments (the same for the client).
  2. Methods and instances are camelCased starting with lower case Examples: UA_Server_new, UA_Int32_calculateSize
  3. Use const on variables that will not be changed. For example in functions, make all parameters const, that are not used as out arguments. When declaring local variables in a function make them const, if you know that they will not be modified. When using pointers, remember to declare both the pointer and the actual type as const, if they are never modified: const int *const myInt
  4. In- variables should be always in front of the out- variables in argument lists

Types

  1. Public types are prefixed with UA_
  2. Types are CamelCased starting with upper case. Example: UA_ResponseHeader
  3. Don't use var = {0} to zero-initialize variables. This results in warnings for some exotic environments (e.g. some versions of the Android NDK). Use memset(ptr, 0, size) insted.

Constants, enums, defines

  1. Constants and defines are in full capital letters, eg.: UA_CONST
  2. Enums are preferred when defining several related constants, for example the different states of a secure channel: 
    typedef enum {
    UA_SECURECHANNELSTATE_FRESH,
    UA_SECURECHANNELSTATE_OPEN,
    UA_SECURECHANNELSTATE_CLOSED
} UA_SecureChannelState;

Variable Naming

  1. Variables should be descriptive and use camelCase starting with a lower case letter.
  2. Some commonly known abbreviations or variable name conventions can be used. This includes for example loop variables like i and temporary variables like tmp. Use common sense, and think about if you would still understand your own code if you looked at it two weeks in the future.
  3. Do not use Hungarian notation. The code base so far does not use it and neither should you, to be consistent. For some reasons, why Hungarian notation might be bad, see here

Formatting

  1. Braces: Opening braces start on the same line as a function, if statement, etc. and the closing brace is placed on a separate line. If the next keyword after a closing braces logically belongs to the previous keyword (i.e. if and else), the keyword is placed next to the closing brace on the same line with one space in between. Example: 
    if(a) {
    doSomething();
} else if(b) {
    doSomething();
} else doSomething();
  2. Switch statement: Do not indent the case keyword and place the content of each case in new lines that are indented. If using a code block to limit the scope of variables, follow the above convention. Example: 
    switch(a) {
case 0:
    doA();
case 1: {
    doB();
    doC();
}
}
  3. Align multiline expressions. Example: 
    int a = 1 +
        2 +
        3;

int b = ((1) + 2) -
        (4 * 5 / 6 % 7);
  4. Keep the number of blank lines to a minimum, but leave some if it makes the code more readable.
  5. Surround binary operators with spaces except for member access operators. Do not use spaces around unary operators.
  6. Do not place a space between a keyword and following parentheses.
  7. Do not place spaces within parentheses.
  8. Place a space before, but not after a pointer symbol (*). Example: const int *const **const myPtr;

If you are using CLion you can configure your IDE by importing this scheme.

Tests

Tests are contained in the /tests dir and start with check_ prefix followed by a package name (corresponding .c file).

example: check_encoder.c, check_securityLayer.c

Checking returns, error logging and cleanup

Error handling and corresponding reactions like cleanup, logging or updating internal error states is important for a stable program flow.

For checking return values within functions there are specific macros that are used. Handling check values and errors belongs to three different categories:

  1. Handling UA_StatusCode check values
  2. Handling memory allocation errors
  3. Handling general boolean check values

A very frequent use case is the checking of a UA_StatusCode return value and performing a specific action upon error. The workflow and usage is the same for all of the above categories. The basic check macro follows the structure UA_CHECK_STATUS(STATUSCODE, EVAL_ON_ERROR), performing a check on the STATUSCODE and execute EVAL_ON_ERROR if an erroneous state is detected. The below example thus checks rv and executes return rv on error.

UA_StatusCode rv = someAction();
UA_CHECK_STATUS(rv, return rv);

Similar to the above there exist logging variants for each check macro that logs to a specific logging level. The macro names are appended with a suffix _FATAL, _ERROR, _WARN or _INFO.

In this code snippet, the status code gets checked with a specified error action, while logging a specified error level message:

UA_Logger *logger = &server->config.logger;

UA_StatusCode rv = someAction();
/* logging "error" level with the "_ERROR" suffix */
UA_CHECK_STATUS_ERROR(rv, return rv, logger, UA_LOGCATEGORY_SERVER, "my message");

In some situations it is nice to use the goto command in C. This is the case e.g. for jumping to cleanup routines that are used to perform a series of commands.

For the development of open62541 we employ a specific rule for which cleanup routines should be used: If the same cleanup routine is used from at least two places in the same function then this routine should be called via a goto statement.

In this example e.g. only the second error check needs to free up some memory upon encountering an error, thus a jump to a cleanup routine is not indicated.

void noCleanupRoutine(void *data) {
   
    data = malloc(sizeof(int));
    UA_CHECK_MEM(data, return UA_STATUSCODE_BADOUTOFMEMORY);
     
    UA_StatusCode rv = do_something();
    UA_CHECK_STATUS(rv, free(data); return rv);
    
    return UA_STATUSCODE_GOOD;
}

The below example on the other hand, shows two functions that use the same routine for cleanup. A goto routine should thus be used.

void yesCleanupRoutine(void *data) {
    
    data = malloc(sizeof(int));
    UA_CHECK_MEM(data, return UA_STATUSCODE_BADOUTOFMEMORY);
  
    /* jumps to cleanup routine upon encountering a bad statuscode */
    UA_StatusCode rv = do_something();
    UA_CHECK_STATUS(rv, goto cleanup);
 
    /* jumps to the same cleanup routine */
    rv = do_something_else();
    UA_CHECK_STATUS(rv, goto cleanup);
   
    return UA_STATUSCODE_GOOD;
    
cleanup:
    free(data);
    return rv;
}

In this example only the status codes are checked and the corresponding action is evaluated on error. There is no message logging included.

static UA_StatusCode
foo(int *errorCounter) {
    
    UA_StatusCode rv = do_something();
    /* if rv != UA_STATUSCODE_GOOD then "return rv" gets evaluated */
    UA_CHECK_STATUS(rv, return rv);
  
    rv = do_another_thing();
    
    /* 
    EVAL_ON_ERROR can take multiple statements
    (e.g. first modifying some value then return the error code) 
    */
    UA_CHECK_STATUS(rv, rv = UA_STATUSCODE_BAD; errorCounter++; return rv);

    return UA_STATUSCODE_GOOD;
}

In this example status codes are checked while logging message with different levels and a specified LOGCATEGORY are generated with a given logger.

static UA_StatusCode
foo(UA_Server *server) {
    
    /* assign the logger for later simple use */
    UA_Logger *logger = &server->config.logger;
    
    UA_StatusCode rv = do_something_error();

    /* 
    if rv != UA_STATUSCODE_GOOD then 
    an error logging message is generated and 
    "return rv" gets evaluated
    */
    UA_CHECK_STATUS_ERROR(rv, return rv,
                   logger, UA_LOGCATEGORY_SERVER,
                   "My error logging message with special info: %d", 42);

    rv = do_something_warn();
    UA_CHECK_STATUS_WARN(rv, return rv,
                   logger, UA_LOGCATEGORY_SERVER,
                   "My warning logging message with special info: %d", 42);

    rv = do_something_info();
    UA_CHECK_STATUS_INFO(rv, return rv,
                   logger, UA_LOGCATEGORY_SERVER,
                   "My info logging message with special info: %d", 42);

    rv = do_something_fatal();
    UA_CHECK_STATUS_FATAL(rv, return rv,
                   logger, UA_LOGCATEGORY_SERVER,
                   "My fatal logging message with special info: %d", 42);


    return UA_STATUSCODE_GOOD;
}

All the other check macros function analogously to the above UA_CHECK_STATUS examples, with a short example to explain the basic usage:

static UA_StatusCode
foo() {
 
    /* assign the logger for later simple use */
    UA_Logger *logger = &server->config.logger;
    
    UA_Boolean mustBeTrue = do_something();
    /* if mustBeTrue != true then "return UA_STATUSCODE_BAD" gets evaluated */
    UA_CHECK(mustBeTrue, return UA_STATUSCODE_BAD);
   
    UA_StatusCode rv = UA_STATUSCODE_GOOD;
    /*
    EVAL_ON_ERROR can take multiple statements
    (e.g. first assigning some value then return the error code) 
    */
    UA_CHECK(mustBeTrue, rv = UA_STATUSCODE_BAD; return rv);

    UA_Boolean mustBeTrue = do_something_else();
    /* 
    if mustBeTrue != true then 
    an error logging message is generated and 
    "return UA_STATUSCODE_BAD" gets evaluated 
    */
    UA_CHECK_ERROR(mustBeTrue, return UA_STATUSCODE_BAD,
                   logger, UA_LOGCATEGORY_SERVER,
                   "My logging message with special info: %d", 42);

 
    void *data = malloc(1, 1);
    UA_CHECK_MEM(data, return UA_STATUSCODE_BADOUTOFMEMORY);
   
    void *data2 = malloc(1, 1);
    UA_CHECK_MEM_ERROR(data2, return UA_STATUSCODE_BADOUTOFMEMORY,
                   logger, UA_LOGCATEGORY_SERVER,
                   "My logging message with special info: %d", 42);
 
    return UA_STATUSCODE_GOOD;
}

Still unsure?

If any questions arise concerning code style, feel free to start an issue.