Contributing

Some tips and guidelines for developers hacking on BuildStream

Filing issues

If you are experiencing an issue with BuildStream, or would like to submit a patch to fix an issue, then you should first search the list of open issues to see if the issue is already filed, and open an issue if no issue already exists.

Fixing bugs

Before fixing a bug, it is preferred that an issue be filed first in order to better document the defect, however this need not be followed to the letter for minor fixes.

Patches which fix bugs should always come with a regression test.

Adding new features

Feature additions should be proposed on the mailing list before being considered for inclusion. To save time and avoid any frustration, we strongly recommend proposing your new feature in advance of commencing work.

Once consensus has been reached on the mailing list, then the proposing party should file an issue to track the work.

New features must be well documented and tested in our test suite.

It is expected that the individual submitting the work take ownership of their feature within BuildStream for a reasonable timeframe of at least one release cycle after their work has landed on the master branch. This is to say that the submitter is expected to address and fix any side effects, bugs or regressions which may have fell through the cracks in the review process, giving us a reasonable timeframe for identifying these.

Submitting patches

Submitting patches works in the regular GitHub workflow of submitting pull requests.

Branch names

If you are an apache member with access to the main repository, and are submitting a pull request for a branch within the main repository, then please be careful to use an identifiable branch name.

Branch names for pull requests should be prefixed with the submitter’s name or nickname, followed by a forward slash, and then a descriptive name. e.g.:

username/fix-that-bug

This allows us to more easily identify which branch does what and belongs to whom, especially so that we can effectively cleanup stale branches in the upstream repository over time.

Pull requests

Once you have created a local branch, you can push it to the upstream BuildStream repository using the command line:

git push origin username/fix-that-bug

GitHub will respond to this with a message and a link to allow you to create a new merge request. You can also create a pull request using the GitHub UI.

You may open pull requests for the branches you create before you are ready to have them reviewed and considered for inclusion if you like. Until your merge request is ready for review, the pull request title must be prefixed with the WIP: identifier.

Consider marking a pull request as WIP again if you are taking a while to address a review point. This signals that the next action is on you, and it won’t appear in a reviewer’s search for non-WIP merge requests to review.

Organized commits

Submitted branches must not contain a history of the work done in the feature branch. For example, if you had to change your approach, or have a later commit which fixes something in a previous commit on your branch, we do not want to include the history of how you came up with your patch in the upstream master branch.

Please use git’s interactive rebase feature in order to compose a clean patch series suitable for submission upstream.

Every commit in series should pass the test suite, this is very important for tracking down regressions and performing git bisections in the future.

We prefer that documentation changes be submitted in separate commits from the code changes which they document, and newly added test cases are also preferred in separate commits.

If a commit in your branch modifies behavior such that a test must also be changed to match the new behavior, then the tests should be updated with the same commit, so that every commit passes its own tests.

These principles apply whenever a branch is non-WIP. So for example, don’t push ‘fixup!’ commits when addressing review comments, instead amend the commits directly before pushing.

Commit messages

Commit messages must be formatted with a brief summary line, followed by an empty line and then a free form detailed description of the change.

The summary line must start with what changed, followed by a colon and a very brief description of the change.

If the commit fixes an issue, or is related to an issue; then the issue number must be referenced in the commit message.

Example:

element.py: Added the frobnicator so that foos are properly frobbed.

The new frobnicator frobnicates foos all the way throughout
the element. Elements that are not properly frobnicated raise
an error to inform the user of invalid frobnication rules.

Fixes #123

Note that the ‘why’ of a change is as important as the ‘what’.

When reviewing this, folks can suggest better alternatives when they know the ‘why’. Perhaps there are other ways to avoid an error when things are not frobnicated.

When folks modify this code, there may be uncertainty around whether the foos should always be frobnicated. The comments, the commit message, and issue #123 should shed some light on that.

In the case that you have a commit which necessarily modifies multiple components, then the summary line should still mention generally what changed (if possible), followed by a colon and a brief summary.

In this case the free form detailed description of the change should contain a bullet list describing what was changed in each component separately.

Example:

artifact cache: Fixed automatic expiry in the local cache

  o _artifactcache/artifactcache.py: Updated the API contract
    of ArtifactCache.remove() so that something detailed is
    explained here.

  o _artifactcache/cascache.py: Adhere to the new API contract
    dictated by the abstract ArtifactCache class.

  o tests/artifactcache/expiry.py: Modified test expectations to
    match the new behavior.

This is a part of #123

Committer access

Committers in the BuildStream project are those folks to whom the right to directly commit changes to our version controlled resources has been granted.

While every contribution is valued regardless of its source, not every person who contributes code to the project will earn commit access. The COMMITTERS file lists all committers.

How commit access is granted

After someone has successfully contributed a few non-trivial patches, some full committer, usually whoever has reviewed and applied the most patches from that contributor, proposes them for commit access. This proposal is sent only to the other full committers - the ensuing discussion is private, so that everyone can feel comfortable speaking their minds. Assuming there are no objections, the contributor is granted commit access. The decision is made by consensus; there are no formal rules governing the procedure, though generally if someone strongly objects the access is not offered, or is offered on a provisional basis.

This of course relies on contributors being responsive and showing willingness to address any problems that may arise after landing patches. However, the primary criterion for commit access is good judgement.

You do not have to be a technical wizard or demonstrate deep knowledge of the entire codebase to become a committer. You just need to know what you don’t know. Non-code contributions are just as valuable in the path to commit access. If your patches adhere to the guidelines in this file, adhere to all the usual unquantifiable rules of coding (code should be readable, robust, maintainable, etc.), and respect the Hippocratic Principle of “first, do no harm”, then you will probably get commit access pretty quickly. The size, complexity, and quantity of your patches do not matter as much as the degree of care you show in avoiding bugs and minimizing unnecessary impact on the rest of the code. Many full committers are people who have not made major code contributions, but rather lots of small, clean fixes, each of which was an unambiguous improvement to the code. (Of course, this does not mean the project needs a bunch of very trivial patches whose only purpose is to gain commit access; knowing what’s worth a patch post and what’s not is part of showing good judgement.)

Windows CI

The infrastructure for running the CI against Windows is different from the usual runners, due to a combination of licensing technicalities and differing containerisation support.

The scripts used to generate a CI runner can be found at https://gitlab.com/BuildStream/windows-startup-script. The wsl branch can be used to generate a runner for WSL, and the win32 branch can be used to generate a native-windows runner.