This the multi-page printable view of this section. Click here to print.

Return to the regular view of this page.

Contribution Guidelines

How to contribute to the projects

This section provides information about how users can contribute to the project, either by reporting issues, suggesting new features or developing fixes.

1 - Using gitlab

Steps to setup and use a freedesktop.org gitlab account

The Mobile Broadband group in the gitlab instance maintained by the freedesktop.org community contains the following projects:

The following sections provide a quick overview of what the basic functionality is and how to set it up by users or first-time developers. This Community Edition gitlab instance is no different to others, so users that have used gitlab in the past should see no problem in using this site. For detailed help on all the features supported, the gitlab instance also publishes its own help.

1.1 - Basic setup

Basic gitlab user usage

Cloning the upstream repository

If the user only wants to build and install the latest development versions (without suggesting any changes to them), the upstream source repositories can be checked out using git and the HTTPS paths for each of them. These paths are given in the dialog displayed after clicking the Clone button.

Clone dialog

The git clone commands for each of these repositories would be:

  $ git clone https://gitlab.freedesktop.org/mobile-broadband/libqrtr-glib
  $ git clone https://gitlab.freedesktop.org/mobile-broadband/libmbim.git
  $ git clone https://gitlab.freedesktop.org/mobile-broadband/libqmi.git
  $ git clone https://gitlab.freedesktop.org/mobile-broadband/ModemManager.git

Creating a new user account

In order to access the basic functionality of the gitlab instance, like reporting new issues or commenting on already existing ones, a new user account should be registered in the gitlab instance.

Reporting new issues

The user can browse existing issues or report new ones in the project specific Issues page:

Once the issue is reported, the maintainers may request to add additional explanations, or provide debug logs with detailed protocol information.

1.2 - Developer setup

Advanced gitlab usage for developers

Adding SSH keys to the user account

If the user is prepared to suggest changes to the upstream repositories, the first step is to setup at least one SSH key. You can create a public/private key pair locally in your PC as follows:

  $ ssh-keygen -t rsa

The previous command will generate a public key (~/.ssh/id_rsa.pub) and a private key (~/.ssh/id_rsa). The user should then navigate in the gitlab site to the “SSH keys” section in the “User Settings” of the new user profile in order to register the SSH key. The user should copy and paste the public key that was generated (i.e. ~/.ssh/id_rsa.pub).

Fork and checkout the project

Once the user is logged in the new account, a new Fork button will be shown at the top-right corner in each of the upstream project pages.

Project page signed in

The user should only fork the project that needs to modify. There is really no need to create user forks if the user isn’t going to suggest any modification to the upstream repositories. E.g. if the user is planning to fix a bug in ModemManager, there would be no need to fork libmbim, libqmi or libqrtr-glib.

Once the project is forked, gitlab will create a whole new project page under the user account. E.g. if all four projects are forked by username:

These forked projects will have their own Clone buttons, and when they are clicked, the user-specific clone paths will be shown.

User project clone dialog

The user should use the paths listed under Clone with SSH, and the clone operations will succeed if the SSH keys were configured properly in an earlier step.

  $ git clone git@gitlab.freedesktop.org:username/libqrtr-glib.git
  $ git clone git@gitlab.freedesktop.org:username/libmbim.git
  $ git clone git@gitlab.freedesktop.org:username/libqmi.git
  $ git clone git@gitlab.freedesktop.org:username/ModemManager.git

Setup upstream remote

As soon as the new user-specific git repositories are cloned, they will have the origin remote configured to point to them. These git repositories will not be automatically kept in sync with the original upstream repositories, it is up to the user to do that.

In order to make sure the user can sync with the upstream repositories periodically, a new additional remote can be setup as follows:

  $ cd libqrtr-glib
  $ git remote add upstream https://gitlab.freedesktop.org/mobile-broadband/libqrtr-glib

  $ cd libmbim
  $ git remote add upstream https://gitlab.freedesktop.org/mobile-broadband/libmbim.git

  $ cd libqmi
  $ git remote add upstream https://gitlab.freedesktop.org/mobile-broadband/libqmi.git

  $ cd ModemManager
  $ git remote add upstream https://gitlab.freedesktop.org/mobile-broadband/ModemManager.git

Once the upstream remotes are setup, the user can now sync the master branch between both. E.g.:

  $ cd libqrtr-glib
  $ git checkout master                // change to the master branch
  $ git fetch upstream                 // fetch updates from upstream remote
  $ git merge upstream/master          // merge changes from upstream master branch
  $ git push origin master             // update master branch in origin remote

  $ cd libmbim
  $ git checkout master
  $ git fetch upstream
  $ git merge upstream/master
  $ git push origin master

  $ cd libqmi
  $ git checkout master
  $ git fetch upstream
  $ git merge upstream/master
  $ git push origin master

  $ cd ModemManager
  $ git checkout master
  $ git fetch upstream
  $ git merge upstream/master
  $ git push origin master

During development, it’s suggested to keep separate per-feature or per-fix branches, and keep the master branch always in sync with the upstream repository.

2 - Coding style

How the source code should look like

Quoting the C coding style guidelines from the GNOME project:

The Single Most Important Rule when writing code is this: check the surrounding code and try to imitate it. As a maintainer it is dismaying to receive a patch that is obviously in a different coding style to the surrounding code. This is disrespectful, like someone tromping into a spotlessly-clean house with muddy shoes. So, whatever this document recommends, if there is already written code and you are patching it, keep its current style consistent even if it is not your favorite style.

So, the best way to learn and get used to the coding style used in this projects is really to read the already existing code, as that will help solve most of the doubts one may have when writing new code. If reading the code is not enough, the following coding style guidelines should help as well.

Basic formatting

Line length

There is no strict requirement on the length of the source code lines, especially since there are very long type and function names in e.g. libqmi or libmbim. It is up to the developer how to split lines if appropriate.

Comments

Code comments are always written between /* and */, and multiline comments will have a * prefix in all lines except for the first one.

{
    /* a very long multiline comment to explain something
     * with a lot of detail; see how all the lines after the
     * first one have the asterisk prefix.
     */

    /* single line comments also in the same way, never with // */
}

Indentation and alignment

The sources use 4 spaces to indent, there should be no tabs or longer indentations. In the same way, the alignment is exclusively done with spaces, never with tabs.

{
    /* indented 4 spaces */

    if (something) {
        /* indented 4 more spaces */
        do_something (variable1,
                      variable2,
                      variable3,
                      variable4);
        /* aligned  --^-- here */
    }
}

Spacing

Both method calls and conditionals (e.g. if, for, while…) should always have a whitespace before the ( character.

{
    /* v-------- whitespace before ( */
    if (something) {
                 /* v-------- whitespace before ( */
        do_something (variable1,
                      variable2,
                      variable3,
                      variable4);
    }
}

There should be no whitespace after ( or before ).

Braces

Each function has its opening brace at the next line on the same indentation level as its header.

The blocks inside a function, however, have their opening braces at the same line as their respective control statements; closing braces remain in a line of their own, unless followed by a keyword else or while.

If the contents of the statement are one single command, the braces may be optionally skipped.

static void
a_nice_function (void)
{                                  /* <--------- open brace of function in next line */
    if (something) {               /* <--------- open brace of statement in same line */
        ...
        do_something ();
    } else if (something_else) {   /* <--------- closing brace followed by else-if */
       ...
       do_something_else ();
    } else                         /* <--------- braces skipped for one-command contents */
       do_something_completely_different ();
}

Variables

Variables are always named in lowercase, and separating words with underscores.

The name of the variable should clearly define the purpose of the variable. Using generic names are only allowed for certain very specific cases, e.g. a short name like i is allowed for loop iterators and such.

Variables are defined always at the beginning of the code block where they’re scoped. If a variable is scoped inside an inner scope, it’s also better to define it inside the inner block itself.

If possible, the names of the variables given in the same block should also be vertically aligned. Depending on the length of the type names in the same variable block, more than one vertically aligned groups may be given.

After the definition of all variables, there should be a whiteline before the first method call.

{
    /* all variables defined together at the beginning of the block */
    GError *inner_error = NULL;
    gint    i;
                                                     /* <---- whiteline here! */
    for (i = 0; i < SOME_MAX; i++) {
        /* inside an inner block, variables that apply to the block;
         * in this case with some variable types that are very long
         * so they're aligned vertically in different groups */
        SomeVeryLongTypeNameWeFindHere        input_message;
        AnotherDifferentVeryLongTypeNameWeGot output_message;
        const gchar *name;
        gint         weight;
                                                     /* <---- whiteline here! */
        ...
        name = do_something (i);
        if (name)
            break;
    }
}

Methods

Methods are always named in lowercase, and separating words with underscores.

The name of the method should clearly define the purpose of the method.

If possible, the names of the method arguments given in the method declaration and definitions should also be vertically aligned. Depending on the length of the type names, more than one vertically aligned groups may be given.

When a method is defined, it should provide the return type of the method in a separate line, before the name of the method. When a method is declared, the return type should be given in the same line as the name of the method.

Definition of module private methods

Methods that are going to be used exclusively in the same source file where they are defined (private methods) should be made static, and not declared in any header. The name of this kind of methods doesn’t need to have any explicit prefix.

/* return type and method name in separate lines */
static gboolean
do_something (gint          max,
              gint          min,
              const gchar  *text,
              GError      **error)
{
    /* definition here */
}

Declaration and definition of module public methods

Methods that are meant to be used out of the source file where they are defined (public methods) should have a clear declaration in a header file and the definition inside the source file with the same file name. All the public methods in the same module should have the same string prefix, clearly stating the module they come from.

E.g. in the header file some-module.h:

/* return type and method name in the same line */
gboolean some_module_do_something (const gchar  *text,
                                   GError      **error);

And in source file some-module.c:

/* return type and method name in separate lines */
gboolean
some_module_do_something (const gchar  *text,
                          GError      **error)
{
    /* definition here */
}

Symbols

Symbols are always named in uppercase, and separating words with underscores.

The name of the symbol should clearly define the purpose of the symbol.

Symbols are usually defined at the beginning of the header or source file, with an explanation of what they are for given in a comment.

In the same way as methods, if the symbols are defined in the header file of a module, the name of the symbol should have the common prefix used in the module.

/* Maximum time to wait for the operation to complete, in seconds */
#define SOME_MODULE_MAX_TIMEOUT_SECS 10

3 - git commit style

How the git commits should look like

Having a proper style in the git commits suggested for inclusion in the projects is also a very important thing. This section provides some hints of what is expected from the git commits suggested by contributors.

As with the source code coding style, the best way to see what kind of git commit format is expected is to look at other commits from other developers in the same project.

Authorship

The author of the git commit should preferably a real person name (Firstname Lastname), as the author of the email is implicitly the one who holds the copyright of the changes done in the commit, unless explicitly stated e.g. with additional copyright lines in the source code.

Basic message formatting

The message of a git commit must be composed of:

  • An initial first line with a short description of the change, prefixed by a “module: " string. The module could be some specific object in the code, or some protocol, or some other similar thing. If unsure, the best thing to get a feeling of what you should use as module would be to run git log <FILE> (being the main file being modified in the commit) and read what other developers have used as module when modifying that same file.
  • Then an empty whiteline.
  • Then the body of the commit, including an optional longer description. This would be optional only for trivial changes for which the first description line is enough. For any change that is a bit more complex, the longer description can be considered mandatory. The body of the message may be composed by multiple paragraphs, and may also include additional information like real program logs, crash backtraces reported by gdb, or even valgrind memcheck log snippets (e.g. when fixing memory leaks).

For text content in the commit message (either first line or body) it is suggested to keep the maximum line length at 70 characteres maximum. This is not a strict rule for the first line though, sometimes just a few more characters are needed, and that would be acceptable.

For log snippets or backtraces included in the commit message, it is suggested to indent them a few characters. Also, there is no line length limitation in this case, but it is suggested to pretty-format them so that the information included is all valuable. E.g. when inserting program logs in the commit message, the developer may first clean them up to remove unrelated log lines, or even remove information that isn’t necessary (e.g. maybe the timestamps are irrelevant for the purpose of the included information, so they can be removed).

Commits may have a Signed-off-by: line (e.g. as per git commit -s) but this is not strictly necessary.

When fixing issues reported in the project gitlab, a Fixes <Bug URL> line is suggested. When this commit is merged to the project git master, gitlab will automatically close the referred issue.

Examples

This is a properly formatted commit message:

base-modem: plug leaks when organizing ports

The GLists maintained in the logic need to be explicitly freed (just
the lists, not the list contents) if we exit early on error or if we
end up deciding that the specific ports are available but unsupported
by the plugin (e.g. if a plugin that doesn't support net ports finds
net ports in the modem).

  ==225333== 24 bytes in 1 blocks are definitely lost in loss record 2,024 of 5,525
  ==225333==    at 0x483E77F: malloc (vg_replace_malloc.c:307)
  ==225333==    by 0x506C539: g_malloc (in /usr/lib/libglib-2.0.so.0.6600.7)
  ==225333==    by 0x508DC8F: g_slice_alloc (in /usr/lib/libglib-2.0.so.0.6600.7)
  ==225333==    by 0x50636B4: g_list_append (in /usr/lib/libglib-2.0.so.0.6600.7)
  ==225333==    by 0x17E671: mm_base_modem_organize_ports (mm-base-modem.c:1298)
  ==225333==    by 0x1E4409: mm_plugin_create_modem (mm-plugin.c:1094)
  ==225333==    by 0x162C81: mm_device_create_modem (mm-device.c:481)
  ==225333==    by 0x15DE60: device_support_check_ready (mm-base-manager.c:218)
  ==225333==    by 0x4EA8173: ??? (in /usr/lib/libgio-2.0.so.0.6600.7)
  ==225333==    by 0x4EAC6E8: ??? (in /usr/lib/libgio-2.0.so.0.6600.7)
  ==225333==    by 0x16730F: device_context_run_ready (mm-plugin-manager.c:1533)
  ==225333==    by 0x4EA8173: ??? (in /usr/lib/libgio-2.0.so.0.6600.7)

These are NOT a properly formatted commit messages:

Update src/mm-broadband-modem.c, update src/mm-broadband-modem.h, update src/mm-modem-helpers.c, update src/mm-modem-helpers.h, update src/mm-base-modem.h .......

(lacks description of what the commit is doing)

Added dual sim support to Quectel plugin

(no module: prefix; e.g. should have been quectel: added dual sim support)

mm-broadband-modem: Lorem ipsum dolor sit amet, consectetur adipiscing elit. Ut turpis turpis, euismod at gravida at, tincidunt at metus. Maecenas placerat laoreet arcu non luctus. Duis sit amet augue ullamcorper, tincidunt magna ac, porttitor metus.

(first line more than 70 chars)

Contents of the commit

The maintainers of the project will review the commits one by one and therefore, each commit should be self-contained:

  • A single commit must only provide one single logical change. It is acceptable to include in the commit additional minor coding style modifications affecting the code being fixed, but that’s it.
  • If the changes being suggested in a merge request are a lot, it is therefore expected to have a series of commits implementing the changes. Merge requests that contain one single commit with 5000 line changes across the project will be rejected right away.
  • Commits fixing only coding style are also accepted. This is probably an exception to most free/open source software projects, so don’t assume this to be the default elsewhere.

The changes done in a single commit must NEVER break the build, because otherwise git bisect-ing would not be possible.

4 - Merge requests

Steps to create new merge requests

Create a new local branch for the fix or feature

Once a local checkout of the project is available, the user can now create a new local branch for the new feature being developed or for the fix that is going to be suggested. The new branch should be based always on top of upstream/master.

  $ cd ModemManager
  $ git fetch upstream
  $ git checkout -b my-branch upstream/master

At this point, the user can start modifying files and using git to keep track of the changes, as usual.

Prepare and cleanup the branch

Once the user has developed a series of commits for a given fix or new feature, it is time to suggest the changes for review.

The first thing to do is to make sure the new branch is rebased on top of the latest upstream/master. E.g. if the branch where the fix was developed was created 2 months ago, it is very likely that new changes have gone to the master branch in the upstream repository; the user should make sure all those changes don’t conflict with the new ones being suggested.

  $ cd ModemManager
  $ git fetch upstream
  $ git checkout my-branch
  $ git rebase upstream/master

If the rebase cannot finish automatically, the user will need to perform an interactive rebase in order to fix any conflict that may have arised.

  $ git rebase -i upstream/master

Checklist before pushing the branch for review

Key requirements

  • Make sure the gitlab pipelines pass for your branch.
  • Make sure git commit style is according to the expected format.
  • Make sure coding style is according to the expected format.

The branch must allow git bisecting

In order to allow git bisecting the branch, no single commit should break the build at any point. This can be checked by running a build after each commit in the following way:

  $ git rebase -i upstream/master --exec "make -j8"

No new gtk-doc warnings

The developer should make sure the local builds are done with --enable-gtk-doc, and if there are new warnings introduced by the branch, these must be fixed before suggesting it for review.

E.g. adding a new MBIM service in libmbim could end up triggering the following warnings in the gtk-doc step:

  DOC   Scanning header files
  DOC   Introspecting gobjects
  DOC   Building XML
./libmbim-glib-unused.txt:1: warning: 21 unused declarations. They should be added to libmbim-glib-sections.txt in the appropriate place.
  DOC   Building HTML
  DOC   Fixing cross-references
html/libmbim-glib-QDU.html:287: warning: no link for: "MbimQduFileType" -> (<span class="type">MbimQduFileType</span>).

Learning how to fix these type of issues is easy, just investigate how other similar methods or types are documented, and do the same.

Push the branch and create a merge request

In order to create a new merge request against the upstream repository, the user should push the branch to the user-specific repository (origin remote).

  $ cd ModemManager
  $ git checkout my-branch
  $ git push origin my-branch
  Enumerating objects: 57, done.
  Counting objects: 100% (57/57), done.
  Delta compression using up to 4 threads
  Compressing objects: 100% (26/26), done.
  Writing objects: 100% (47/47), 8.76 KiB | 2.92 MiB/s, done.
  Total 47 (delta 35), reused 32 (delta 21), pack-reused 0
  remote:
  remote: To create a merge request for my-branch, visit:
  remote:   https://gitlab.freedesktop.org/username/ModemManager/-/merge_requests/new?merge_request%5Bsource_branch%5D=my-branch
  remote:
  To gitlab.freedesktop.org:username/ModemManager.git
   * [new branch]        my-branch -> my-branch

Once the branch is pushed, the git server will provide a link to create a merge request for the newly pushed branch, as seen above.

The user should open the given link, and gitlab will provide a short form to fill before the merge request is finished. Very important things to consider when filling in the form:

  • If the merge request contains a series of commits, the user should write a description that explains the purpose of the whole series.
  • If the merge request contains one single commit, the description of the merge request can be the commit message.
  • The Allow commits from members who can merge to the target branch option in the Contribution section must be enabled. With this option, the project maintainers are allowed to push or rework the changes of the merge request in its original location (the user branch).

Once the Submit merge request button is clicked, the process to create the merge request would be finished, and it would be now time for maintainers to review the changes suggested.

The review process

The review of the merge request is done commit by commit, and therefore each commit should be self-contained, in the sense that it should explain clearly the change being done by itself, even if it is part of a series of commits. If this is not done, e.g. if a huge 1000 line commit changing 100 different files at the same time is suggested, it is very likely that the merge request will be rejected. Not because the change is not good, it may be perfectly good, but because it’s not review-able.

Reworking the branch after review

If the maintainer asks for changes to be done, work on them. Don’t add additional commits on top of the original ones for the changes suggested by the maintainer; instead, fixup the changes in the correct commit. It is very suggested that the user knows how to use git commit --fixup and git commit --squash, as these are very powerful tools to have the commits right. A simple usage example of --fixup can be seen in the following snippet, where a quick fix is added on a given already existing commit in 2 steps: first a new fixup commit is created, then an interactive rebase reorders the commits automatically to apply the fixup commit in the correct place:

  $ git log --oneline
  91b399cd shared-qmi: acquisition order preference TLV always same items
  ef55409b shared-qmi: process all feature checks in SSP response together
  1a3c3b9e shared-qmi: if getting TP/SSP fails, assume unsupported
  c1dc9339 shared-qmi: add missing feature check flag initialization to UNKNOWN

  $ git status
  On branch my-branch
  Changes not staged for commit:
    (use "git add <file>..." to update what will be committed)
    (use "git restore <file>..." to discard changes in working directory)
  	modified:   src/mm-base-modem.c

  no changes added to commit (use "git add" and/or "git commit -a")

  $ git commit -a --fixup 1a3c3b9e
  my-branch 9788d020] fixup! shared-qmi: if getting TP/SSP fails, assume unsupported
  1 file changed, 3 insertions(+), 1 deletion(-)

  $ git log --oneline
  9788d020 fixup! shared-qmi: if getting TP/SSP fails, assume unsupported
  91b399cd shared-qmi: acquisition order preference TLV always same items
  ef55409b shared-qmi: process all feature checks in SSP response together
  1a3c3b9e shared-qmi: if getting TP/SSP fails, assume unsupported
  c1dc9339 shared-qmi: add missing feature check flag initialization to UNKNOWN

  $ git rebase -i --autosquash upstream/master
     // inside editor, the fixup commit is reordered and setup to fixup
     pick c1dc9339 shared-qmi: add missing feature check flag initialization to UNKNOWN
     pick 1a3c3b9e shared-qmi: if getting TP/SSP fails, assume unsupported
     fixup 9788d020 fixup! shared-qmi: if getting TP/SSP fails, assume unsupported
     pick ef55409b shared-qmi: process all feature checks in SSP response together
     pick 91b399cd shared-qmi: acquisition order preference TLV always same items

  Successfully rebased and updated refs/heads/aleksander/my-branch.

  $ git log --oneline
  82ca83ad shared-qmi: acquisition order preference TLV always same items
  0a24c403 shared-qmi: process all feature checks in SSP response together
  0cdbba14 shared-qmi: if getting TP/SSP fails, assume unsupported
  c1dc9339 shared-qmi: add missing feature check flag initialization to UNKNOWN

Once the updated branch is considered ready, the user should re-push the branch to the same place, i.e. the origin remote and the same branch name as was originally used when creating the merge request. If the branch has been re-created (e.g. after applying fixup/squash commits), the user will need to force push it. There is no need to create a full new merge request after updating the branch.

  $ git push --force origin my-branch
  Enumerating objects: 9, done.
  Counting objects: 100% (9/9), done.
  Delta compression using up to 4 threads
  Compressing objects: 100% (5/5), done.
  Writing objects: 100% (5/5), 1.14 KiB | 1.14 MiB/s, done.
  Total 5 (delta 4), reused 0 (delta 0), pack-reused 0
  remote:
  remote: View merge request for my-branch:
  remote:   https://gitlab.freedesktop.org/mobile-broadband/ModemManager/-/merge_requests/477
  remote:
  To gitlab.freedesktop.org:username/ModemManager.git
   + 08c3de0b...9e75a1ff my-branch -> origin/my-branch (forced update)

As seen above, gitlab will provide the link to the already existing merge request that is being updated.

Updating the local branch after a forced push by others

Sometimes the maintainers themselves will do some reworks in the user branch, and ask the user for review of those changes. In that case, the forced push from the previous section may have been done by the maintainers (if the Allows commits from members who can merge to the target branch option has been enabled in the merge request), and the user will need to update the local branch with the latest commits from the remote branch before doing any additional change or in order to locally review the new updates.

The same will be required if the branch is re-created by gitlab itself (e.g. after running Rebase in the merge request page in gitlab).

Synchronizing the remote with the local branch can easily be done with a fetch operation first (see how the message reports a forced update), and then a hard reset to point the local branch to the new remote one:

  $ git fetch origin
  remote: Counting objects: 11, done.
  remote: Compressing objects: 100% (6/6), done.
  remote: Total 6 (delta 4), reused 0 (delta 0)
  Unpacking objects: 100% (6/6), 746 bytes | 746.00 KiB/s, done.
  From gitlab.freedesktop.org:username/ModemManager.git
   + b0b78e8...693b464 my-branch -> origin/my-branch  (forced update)

  $ git checkout my-branch

  $ git reset --hard origin/my-branch

After the last commit, the local branch will be in sync with the latest contents from the user remote.

Accepting the merge request and merging to master

Once the suggested changes are accepted by the maintainers, one of them will merge the merge request on top of git master. If the changes are bugfixes that apply to older stable releases, the maintainer may also decide to cherry-pick them to the older stable branches. The user should not need to do that at this point.