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>