LAVA development¶

Obtaining the repository¶

There is a single LAVA codebase, including the code available as lava-server and lava-dispatcher binary packages.

git clone https://git.linaro.org/git/lava/lava.git
cd lava

There is also lavacli which is gaining more support for operations involving the Lava Dispatcher Design:

git clone https://git.linaro.org/git/lava/lavacli.git
cd lavacli

Setting up git-review¶

If you have not done so already, git review needs to be setup for each clone of each source:

git review -s

Create a topic branch¶

We recommend never working off the master branch (unless you are a git expert and really know what you are doing). You should create a topic branch for each logically distinct change you work on.

Before you start, make sure your master branch is up to date:

git checkout master
git pull

Now create your topic branch off master:

git checkout -b my-change master

Run the unit tests¶

Extra dependencies are required to run the tests. On Debian based distributions, you can install lava-dev.

To run the tests, use the ci-run script:

$ ./ci-run

..seealso:: Testing the design and

Preparing for LAVA development

Static code analysis¶

It is essential to run pep8 --ignore E501 routinely on your local changes as ./ci-run will fail on any PEP8 errors. All automated tests occur using Debian Stretch.

It is important to run tools like pylint3, particularly when adding new files, to check for missing or unused imports. Other analysis tools should also be used, for example from within your IDE.

Functional testing¶

Unit tests cannot replicate all tests required on LAVA code, some tests will need to be run with real devices under test. On Debian based distributions, see Developer package build. See Writing a LAVA test shell definition for information on writing LAVA test jobs to test particular device functionality.

Make your changes¶

• Follow PEP8 style for Python code.
• Make one commit (and hence one review) per logical change.
• Use one topic branch for each logical change.
• Include unit tests in the commit of the change being tested.
• Write good commit messages. There are a number of useful guides:
  • A note about git commit messages
  • 5 useful tips for a better commit message
  • Avoid putting documentation into the commit message. Keep the commit message to a reasonable length (about 10 to 12 lines at most).
  • Usage examples need to go into the documentation, not the commit message. Everything which is intended to help users to add this support to their own test jobs must be in the documentation.
  • Avoid duplicating or summarising the documentation in the commit message, reviewers will be reading the documentation as well.
  • Use comments in the code in preference to detailed commit messages.
  • Avoid putting updates into the commit message for each patch set. The review comments are the correct place for details of what changed at which patch set.

Add some unit tests¶

Some changes will always need additional unit tests and reviews will not be merged without this support. The purpose is to ensure that future changes in the codebase have some assurance that existing support has not been affected. The intent is that as much as possible of the test job and device configuration is covered by at least one unit test. Some examples include:

# Changes to an existing jinja2 device-type template which change the

output YAML of the device configuration need a unit test to show that the change is being included.

# Adding a new deployment or boot method needs unit tests (including

sample test jobs) which check that all validate() functions work correctly and particular tests checking for the specific details of the new method.

# Adding a change to an existing deployment or boot method which

changes the construction of the pipeline based on test job or device configuration. Unit tests will be required to show that the change is being made.

Reviewers may ask for unit test support for any change, so talk to us during development. You can also use an RFC prefix in your git commit to indicate that the change is not ready for merging but is ready for comments.

$ python3 -m unittest -vcf lava_scheduler_app.tests.test_fastboot_templates

$ python3 -m unittest -vcf lava_scheduler_app.tests.test_qemu_templates

$ python3 -m unittest -vcf lava_scheduler_app.tests.test_uboot_templates

$ make -C doc/v2 clean
$ make -C doc/v2 html

Re-run the unit tests¶

Make sure that your changes do not cause any failures in the unit tests:

$ ./ci-run

Wherever possible, always add new unit tests for new code.

Testing local changes¶

For any sufficiently large change, building and installing a new package on a local instance is recommended. Ensure that the test instance is already running the most recent production release.

If the test instance has a separate worker, ensure that the master and the worker always have precisely the same code applied. For some changes, it may be necessary to have a test instance which is a clone of a production instance, complete with devices. Never make live changes to a production instance. (This is why integrating new device types into LAVA requires multiple devices.)

Once your change is working successfully:

1. Ensure that your local branch is clean - check for left over debug code.
2. Ensure that your local branch has been rebased against current master
3. Build and install a package from the master branch. If you have added any new files in your local change, make sure these have been removed. Reproduce the original bug or problem.
4. Build and install a package from your local branch and repeat the tests.

PYTHONDIR=/usr/lib/python3/dist-packages/
sudo cp <path_to_file> $PYTHONDIR/<path_to_file>

sudo service lava-server-gunicorn restart

sudo service lava-master restart

make -C doc/v2 clean
make -C doc/v2 html

Send your commits for review¶

From each topic branch, just run:

git review

If you have multiple commits in that topic branch, git review will warn you. It’s OK to send multiple commits from the same branch, but note that:

1. commits are review and approved individually and
2. later commits will depend on earlier commits, so if a later commit is approved and the one before it is not, the later commit will not be merged until the earlier one is approved.
3. you are responsible for rebasing your branch(es) against updates on master and this can become much more difficult when there are multiple commits on one local branch. It can become a lot of work to make the correct changes in the correct commit on a single branch.
4. Fixes from comments or unit test failures in one review are not acceptable as separate reviews, so don’t be tempted to make another commit at the top of the branch.
5. It is common for reviews to go through repeated cycles of comments and updates. This is not a reflection on the usefulness of the change or on any particular contributors, it is a natural evolution of the code. Comments may reflect changes being made in other parallel reviews or reviews merged whilst this change was being reviewed. Contributors may be added to other reviews where the team consider this to be useful for feedback or where the documentation is being updated in areas which relate to your change. The number of comments per review is no indication of the quality of that review and does not affect when the review would be merged.
6. It is common for changes to develop merge conflicts during the review process as other reviews are merged. Unfortunately, gerrit does not email reviewers when a review gains a merge conflict. The team will usually ping the review if it looks like the reviewer has not noticed a merge conflict when the review is considered ready to be merged.
7. If a review has been given -1 by lava-bot, a reviewer or the author, the team will generally ignore that review unless it relates to parallel work on a bug fix or other feature.

Therefore the recommendations are:

1. Always use a separate local branch per commit
2. Think carefully about whether to base one local branch on another local branch. This is recommended when one change logically extends an earlier change and makes it a lot easier than having multiple commits on a single branch.
3. Keep all your branches up to date with master regularly. It is much better to resolve merge conflicts one change at a time instead of having multiple merge commits all in the one rebase operation.
4. Check gerrit intermittently and ensure that you address all comments on the review. LAVA software releases tend to be within the first week of the month. Towards the end of each month, pay particular attention to comments made in gerrit and check if your review has gained a merge conflict. Resolving these problems will make it easier to get your change into the next LAVA release.

Adding reviewers¶

Reviews submitted for lava and lavacli will automatically have the LAVA software team added as reviewers when the review is first submitted.

Other reviewers can also be added to individual reviews. The Owner of the review is always added. Reviewers will get email for all changes relating to that review. All reviewers need to Register with Linaro as a Community contributor, email will go to the @linaro.org account of that reviewer.

If you know that there are still problems to fix in the review, please use the Gerrit interface to reply to the review and give the review a score of -1 and summarize your concerns in the comment. This indicates to the software team that this review should not be considered for merging into master at this time. You may still get comments.

Optionally, you can put [RFC] or similar at the start of your git commit message and then amend the message when the review is ready to merge.

Submitting a new version of a change¶

When reviewers make comments on your change, you should amend the original commit to address the comments, and not submit a new change addressing the comments while leaving the original one untouched.

Gerrit handles this by adding a ChangeId to your commit message. Keep this Id unchanged when amending commit messages.

Locally, you can make a separate commit addressing the reviewer comments, it’s not a problem. But before you resubmit your branch for review, you have to rebase your changes against master to end up with a single, enhanced commit. For example:

$ git branch
  master
* my-feature
$ git show-branch master my-feature
! [master] Last commit on master
 ! [my-feature] address reviewer comments
--
 + [my-feature] address reviewer comments
 + [my-feature^] New feature or bug fix
-- [master] Last commit on master
$ git rebase -i master

git rebase -i will open your $EDITOR and present you with something like this:

pick xxxxxxx New feature or bug fix
pick yyyyyyy address reviewer comments

You want the last commit to be combined with the first and keep the first commit message, so you change pick to fixup ending up with something like this:

pick xxxxxxx New feature or bug fix
fixup yyyyyyy address reviewer comments

If you also want to edit the commit message of the first commit to mention something else, change pick to reword and you will have the chance to do that. Just remember to keep the Change-Id unchanged.

NOTE: if you want to abort the rebase, just delete everything, save the file as empty and exit the $EDITOR.

Now save the file and exit your $EDITOR.

In the end, your original commit will be updated with the changes:

$ git show-branch master my-feature
! [master] Last commit on master
 ! [my-feature] New feature or bug fix
--
 + [my-feature] New feature or bug fix
-- [master] Last commit on master

Note that the “New feature or bug fix” commit is now not the same as before since it was modified, so it will have a new hash (zzzzzzz instead of the original xxxxxxx). But as long as the commit message still contains the same Change-Id, gerrit will know it is a new version of a previously submitted change.

Handling your local branches¶

After placing a few reviews, there will be a number of local branches. To keep the list of local branches under control, the local branches can be easily deleted after the merge. Note: git will warn if the branch has not already been merged when used with the lower case -d option. This is a useful check that you are deleting a merged branch and not an unmerged one, so work with git to help your workflow.

$ git checkout bugfix
$ git rebase master
$ git checkout master
$ git branch -d bugfix

If the final command fails, check the status of the review of the branch. If you are completely sure the branch should still be deleted or if the review of this branch was abandoned, use the -D option instead of -d and repeat the command.

Reviewing changes in clean branches¶

If you haven’t got a clone handy on the instance to be used for the review, prepare a clone as usual.

Gerrit provides a number of ways to apply the changes to be reviewed, so set up a test branch as usual - always ensuring that the master branch of the clone is up to date before creating the review branch.

$ git checkout master
$ git pull
$ git checkout -b review-111

To pull in the changes in the review already marked for commit in your local branch, use the pull link in the patch set of the review you want to run.

Alternatively, to pull in the changes as plain patches, use the patch` link and pipe that to patch -p1. In this full example, the second patch set of review 159 is applied to the review-159 branch as a patch set.

$ git checkout master
$ git pull
$ git checkout -b review-159
$ git fetch https://review.linaro.org/lava/lava-server refs/changes/59/159/2 && git format-patch -1 --stdout FETCH_HEAD | patch -p1
$ git status

Handle the local branch as normal. If the reviewed change needs modification and a new patch set is added, revert the local change and apply the new patch set.