| Contributing |
| ============ |
| Some tips and guidelines for developers hacking on BuildStream |
| |
| |
| .. _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. |
| |
| |
| .. _contributing_fixing_bugs: |
| |
| 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. |
| |
| 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 |
| <https://mail.gnome.org/mailman/listinfo/buildstream-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 :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. |
| |
| 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. |
| |
| |
| .. _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. |
| |
| |
| Branch names |
| ~~~~~~~~~~~~ |
| Branch names for merge 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. |
| |
| |
| 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 |
| |
| 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>`_. |
| |
| 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 |
| ``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. |
| |
| 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. |
| |
| |
| |
| 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. 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. |
| |
| |
| 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. |
| |
| 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.) |
| |
| 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. |
| |
| Further information |
| ------------------- |
| |
| .. 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 |
| hacking/ui.rst |