Contributing Guide

This page is for those who would like to contribute a new feature or bugfix to Variorum. The guide will show a few examples of contributing workflows and discuss the granularity of pull requests (PRs). It will also discuss the tests your PR must pass in order to be accepted into Variorum.

The changes proposed in a PR should correspond to one feature, bugfix, etc. PRs can contain changes relevant to different ideas, however reviewing these PRs becomes tedious and error prone. If possible, try to follow the one-PR-per-bugfix/feature rule.

Branches

Variorum is in active development. The dev branch has the latest contributions to Variorum. All pull requests should start from the dev branch and target dev.

There is a branch for each release, each originating from dev, and follows the naming convention releases/v<major>.<minor>.x.

Continuous Integration

Variorum uses Github Actions for Continuous Integration testing. For each pull request, a series of tests will be run to make sure the code change does not accidentally introduce any bugs into Variorum. Your PR must pass all of these tests before being accepted. While you can certainly wait for the results of these tests after submitting a PR, we recommend that you run them locally to speed up the review process.

We currently test against several gcc versions and different build options on Linux and perform 2 types of tests:

Unit Tests

Unit tests ensure that core Variorum features like building for supported platforms are working as expected.

The status of the unit tests can be checked on the Github Actions tab.

These tests may take several minutes to complete depending on how quickly the runners are launched.

Style Tests

Variorum uses two formatters for style checking. Flake8 to test for PEP 8 conformance. PEP 8 is a series of style guides for Python that provide suggestions for everything from variable naming to indentation. Flake8 is a code linter; it provides warnings of syntax errors, possible bugs, stylistic errors, etc.

To check for compliance, you will want to run the following two commands at the root of Variorum:

$ flake8
$ black --check --diff --exclude "/(src/thirdparty_builtin/googletest|build|src/docs)/" .

If your code is compliant, flake8 will output nothing:

$ flake8

and black will show:

$ black --check --diff --exclude "/(src/thirdparty_builtin/googletest|build|src/docs)/" .

All done! ✨ 🍰 ✨
4 files would be left unchanged.

However, if your code is not compliant with PEP 8, flake8 and black will complain:

$ flake8
./src/utilities/verify_msr_kernel.py:35:1: E302 expected 2 blank lines, found 1
./src/utilities/verify_opal.py:12:1: F401 'numpy' imported but unused

$ black --check --diff --exclude "/(src/thirdparty_builtin/googletest|build|src/docs)/" .
--- src/utilities/verify_opal.py     2022-07-07 05:09:42.145667 +0000
+++ src/utilities/verify_opal.py     2022-07-07 05:09:46.232596 +0000
@@ -10,10 +10,11 @@
 import os
 import sys
 import pandas

 import getopt
+

 def check_opal_files_presence(verbose):
     if verbose:
         print("-- Check if OPAL files exist")

would reformat src/utilities/verify_opal.py
Oh no! 💥 💔 💥
1 file would be reformatted, 3 files would be left unchanged.

As you address these errors with the addition or removal of lines, the line numbers will change, so you will want to re-run flake8 and black again to update them.

Alternatively, fixing the errors in reverse order will eliminate the need for multiple runs of flake8 and black just to re-compute line numbers.

Additionally, Variorum uses Artistic Style for formatting C/C++ files.

Note

We have a helper script in Variorum for calling astyle locally and checking for style compliance of your C/C++ files. To call this script to format C/C++ files, use scripts/check-code-format.sh.

Contributing Workflow

(Thanks to Spack for providing a great overview of the different contributing workflows described in this section.)

Variorum is under active development, so new features and bugfixes are constantly being merged into the dev branch. The recommended way to contribute a pull request is to fork the Variorum repository in your own space (if you already have a fork, make sure is it up-to-date), and then create a new branch off of dev.

$ git checkout dev
$ git fetch upstream && git merge --ff-only upstream/dev
$ git branch <descriptive_branch_name>
$ git checkout <descriptive_branch_name>

Here, we assume that the upstream remote points at https://github.com/llnl/variorum.git.

We prefer that commits pertaining to different pieces of Variorum (new hardware port, specific hardware feature, docs, etc.) prefix the component name in the commit message (for example <component>: descriptive message.

Now, you can make your changes while keeping the dev branch unmodified. Edit a few files and commit them by running:

$ git add <files_to_be_part_of_the_commit>
$ git commit --message <descriptive_message_of_this_particular_commit>

Next, push it to your remote fork (that is, origin points at https://github.com/<your_user_name>/variorum.git) and create a PR:

$ git push origin <descriptive_branch_name>

GitHub provides a tutorial on how to file a pull request. When you send the request, make dev the destination branch.

If you have multiple PRs that build on top of one another, one option is to keep a branch that includes all of your other feature branches:

$ git checkout dev
$ git branch <your_branch_with_all_features>
$ git checkout <your_branch_with_all_features>
$ git rebase <descriptive_branch_name>

This can be done with each new PR you submit. Just make sure to keep this local branch up-to-date with upstream dev too.

Rebasing

Other developers are making contributions to Variorum, possibly to the same files that your PR has modified. If their PR is merged before yours, it can create a merge conflict. This means that your PR can no longer be automatically merged without a chance of breaking your changes. In this case, you will be asked to rebase your branch on top of the latest upstream dev.

First, make sure your dev branch is up-to-date:

$ git checkout dev
$ git fetch upstream
$ git merge --ff-only upstream/dev

Now, we need to switch to the branch you submitted for your PR and rebase it on top of dev:

$ git checkout <descriptive_branch_name>
$ git rebase dev

Git will likely ask you to resolve conflicts. Edit the file that it says can’t be merged automatically and resolve the conflict. Then, run:

$ git add <file_with_a_conflict>
$ git rebase --continue

You may have to repeat this process multiple times until all conflicts are resolved. Once this is done, simply force push your rebased branch to your remote fork:

$ git push --force origin <descriptive_branch_name>