CONTRIBUTING.rst 11.1 KB
Newer Older
1 2
Contributing
============
Tristan Van Berkom's avatar
Tristan Van Berkom committed
3 4 5
Some tips and guidelines for developers hacking on BuildStream


6 7 8 9 10 11 12 13 14
.. _contributing_filing_issues:

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 <https://gitlab.com/BuildStream/buildstream/issues>`_
to see if the issue is already filed, and `open an issue <https://gitlab.com/BuildStream/buildstream/issues/new>`_
if no issue already exists.

15

16
.. _contributing_fixing_bugs:
17

18 19 20 21 22
Fixing bugs
-----------
Before fixing a bug, it is preferred that an :ref:`issue be filed <contributing_filing_issues>`
first in order to better document the defect, however this need not be followed to the
letter for minor fixes.
23

24 25 26 27 28 29 30 31
Patches which fix bugs should always come with a regression test.


.. _contributing_adding_features:

Adding new features
-------------------
Feature additions should be proposed on the `mailing list
32
<https://lists.apache.org/[email protected]>`_
33 34 35 36 37 38 39 40 41
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 :ref:`file an issue <contributing_filing_issues>` to track the
work. Please use the *bst_task* template for issues which represent
feature additions.

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

43 44 45
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
46 47 48
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.
49 50


51 52 53 54 55 56 57 58 59 60 61 62 63 64 65 66
.. _contributing_submitting_patches:

Submitting patches
------------------


Ask for developer access
~~~~~~~~~~~~~~~~~~~~~~~~
If you want to submit a patch, do ask for developer permissions, either
by asking us directly on our public IRC channel (irc://irc.gnome.org/#buildstream)
or by visiting our `project page on GitLab <https://gitlab.com/BuildStream/buildstream>`_
and using the GitLab UI to ask for permission.

This will make your contribution experience smoother, as you will not
need to setup any complicated CI settings, and rebasing your branch
against the upstream master branch will be more painless.
67

68

69 70
Branch names
~~~~~~~~~~~~
71
Branch names for merge requests should be prefixed with the submitter's
72 73 74 75 76 77 78 79 80 81 82 83 84 85 86 87 88
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.


Merge 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:username/fix-that-bug

89 90 91
GitLab will respond to this with a message and a link to allow you to create
a new merge request. You can also `create a merge request for an existing branch
<https://gitlab.com/BuildStream/buildstream/merge_requests/new>`_.
92

93 94 95
You may open merge 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 merge request title must be prefixed with the
96 97 98 99 100 101 102
``WIP:`` identifier. GitLab `treats this specially
<https://docs.gitlab.com/ee/user/project/merge_requests/work_in_progress_merge_requests.html>`_,
which helps reviewers.

Consider marking a merge 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.
103

104 105 106 107 108 109 110 111 112
As a general rule of thumb, after a month of no activity from the submitter of 
a non-WIP MR, we'll put it back into WIP with a polite note. Then after another 
month with no activity we'll close the MR off entirely with another note. 
In this way we are trying to ensure all of the MRs in our backlog are relevant
and up to date. We have some `boilerplate text
<https://gitlab.com/BuildStream/buildstream/blob/master/.gitlab/merge_request_templates/stale_MR_message.md>`_,
to help us when writing these notes.


113

114 115
Organized commits
~~~~~~~~~~~~~~~~~
116
Submitted branches must not contain a history of the work done in the
117 118 119 120 121 122 123 124 125 126
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.
127

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

132 133
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
Richard Dale's avatar
Richard Dale committed
134
with the same commit, so that every commit passes its own tests.
135

136 137 138 139 140 141
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. GitLab has `good support
<https://docs.gitlab.com/ee/user/project/merge_requests/versions.html>`_ for
diffing between pushes, so 'fixup!' commits are not necessary for reviewers.

142

James Ennis's avatar
James Ennis committed
143
Commit messages
144
~~~~~~~~~~~~~~~
145 146
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.
147 148 149 150

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

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

154 155 156 157 158 159 160 161
**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.

162
  Fixes #123
163

164 165 166 167 168 169 170 171 172 173
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.

174 175 176 177 178 179 180 181 182
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**::
Tristan Van Berkom's avatar
Tristan Van Berkom committed
183

184
  artifact cache: Fixed automatic expiry in the local cache
Tristan Van Berkom's avatar
Tristan Van Berkom committed
185

186 187 188 189 190 191 192 193 194 195
    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.

196
  This is a part of #123
197 198


Laurence Urhegyi's avatar
Laurence Urhegyi committed
199 200 201 202 203 204 205 206 207 208 209 210 211 212 213 214 215 216 217 218 219 220 221 222 223 224 225 226 227 228 229 230 231 232 233 234 235 236 237 238 239 240 241 242 243 244 245 246
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.

Whenever someone is granted (or revoked) commit access, an Owner or Maintainer
should run the the script located at `contrib/update_committers.py` with their
personal access token, updating the COMMITTERS.rst list and opening an MR
with their changes.

.. _COMMITTERS: https://gitlab.com/BuildStream/buildstream/blob/master/COMMITTERS.rst


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.)

247 248 249 250 251 252 253 254 255 256 257
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.
Laurence Urhegyi's avatar
Laurence Urhegyi committed
258

259
Further information
260
-------------------
261

262 263 264 265 266 267 268 269 270 271 272 273
.. toctree::
   :maxdepth: 1

   hacking/coding_guidelines.rst
   hacking/using_the_testsuite.rst
   hacking/writing_documentation.rst
   hacking/writing_plugins.rst
   hacking/measuring_performance.rst
   hacking/making_releases.rst
   hacking/grpc_protocols.rst
   hacking/managing_data_files.rst
   hacking/updating_python_deps.rst
James Ennis's avatar
James Ennis committed
274
   hacking/ui.rst