summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorTristan Van Berkom <tristan.vanberkom@codethink.co.uk>2018-10-06 17:15:41 +0900
committerTristan Van Berkom <tristan.vanberkom@codethink.co.uk>2018-10-06 23:40:46 +0900
commit53d9f97758073d72931376a34b1e4dcf4d70aa32 (patch)
treea164f3c5fddfb4e275268adc7d0174e5e0c9afa3
parentd3921ab8bd515fb3dc890c831b7fb33d34d6de05 (diff)
downloadbuildstream-53d9f97758073d72931376a34b1e4dcf4d70aa32.tar.gz
CONTRIBUTING.rst: Updating CONTRIBUTING guidelines
This is almost a complete rewrite of the CONTRIBUTING guide. * The patch submission guidelines have become less ambiguous and more strict * Some general restructuring and reordering of the file took place * The codeing guidelines have changed completely. o There is much less room for ambiguity here now o More emphasis on consistency in the codebase o Added some more abstract points which should be considered when writing and reviewing patches * The policy on public/private symbols has been greatly clarified * Added new section about adding new core plugins and what needs to be done as a consequence of that
-rw-r--r--CONTRIBUTING.rst1162
1 files changed, 994 insertions, 168 deletions
diff --git a/CONTRIBUTING.rst b/CONTRIBUTING.rst
index 6632d7055..d6b26dc31 100644
--- a/CONTRIBUTING.rst
+++ b/CONTRIBUTING.rst
@@ -3,72 +3,136 @@ Contributing
Some tips and guidelines for developers hacking on BuildStream
-Feature additions
------------------
-Major feature additions should be proposed on the
-`mailing list <https://mail.gnome.org/mailman/listinfo/buildstream-list>`_
-before being considered for inclusion, we strongly recommend proposing
-in advance of commencing work.
+.. _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.
+
+For policies on how to submit and issue and how to use our project labels,
+we recommend that you read the `policies guide
+<https://gitlab.com/BuildStream/nosoftware/alignment/blob/master/BuildStream_policies.md>`_
+
-If you are experiencing an issue with BuildStream or would like to submit a small patch/feature, then
-you can `oepn an issue <https://gitlab.com/BuildStream/buildstream/issues/new?issue%5Bassignee_id%5D=&issue%5Bmilestone_id%5D=>`_
+.. _contributing_fixing_bugs:
-For policies on how to submit and issue and how to use our project labels, we recommend that you read the `policies guide <https://gitlab.com/BuildStream/nosoftware/alignment/blob/master/BuildStream_policies.md>`_
+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.
-New features must be well documented and tested either in our main
-test suite if possible, or otherwise in the integration tests.
+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 and
-bugs which may have fell through the cracks in the review process, giving us
-a reasonable timeframe for identifying these.
+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.
-Patch submissions
------------------
-If you want to submit a patch, do ask for developer permissions on our
-IRC channel first (GitLab's button also works, but you may need to
-shout about it - we often overlook this) - for CI reasons, it's much
-easier if patches are in branches of the main repository.
+.. _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.
-Branches must be submitted as merge requests in gitlab. If the branch
-fixes an issue or is related to any issues, these issues must be mentioned
-in the merge request or preferably the commit messages themselves.
+Branch names
+~~~~~~~~~~~~
Branch names for merge requests should be prefixed with the submitter's
-name or nickname, e.g. ``username/implement-flying-ponies``.
+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 may open merge requests for the branches you create before you
-are ready to have them reviewed upstream, as long as your merge request
-is not yet ready for review then it must be prefixed with the ``WIP:``
-identifier.
+are ready to have them reviewed and considered for inclusion. As long as
+your merge request is not yet ready for review then the merge request
+title must be prefixed with the ``WIP:`` identifier.
+
+Organized commits
+~~~~~~~~~~~~~~~~~
Submitted branches must not contain a history of the work done in the
-feature branch. Please use git's interactive rebase feature in order to
-compose a clean patch series suitable for submission.
+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 new test cases are also preferred
-in separate commits.
+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. Ideally every commit in the history of master passes
-its test cases, this makes bisections more easy to perform, but is not
-always practical with more complex branches.
+with the same commit, so that every commit in series passes it's own tests.
Commit messages
~~~~~~~~~~~~~~~
-Commit messages must be formatted with a brief summary line, optionally
-followed by an empty line and then a free form detailed description of
-the change.
+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 should always be referenced in the commit message.
+
**Example**::
element.py: Added the frobnicator so that foos are properly frobbed.
@@ -77,14 +141,48 @@ a very brief description of the change.
the element. Elements that are not properly frobnicated raise
an error to inform the user of invalid frobnication rules.
+ Fixes #123
-Coding style
-------------
-Coding style details for BuildStream
+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
-Style guide
-~~~~~~~~~~~
+ 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.
+
+ Fixes #123
+
+
+Coding guidelines
+-----------------
+This section discusses coding style and other guidelines for hacking
+on BuildStream. This is important to read through for writing any non-trivial
+patches and especially outlines what people should watch out for when
+reviewing patches.
+
+Much of the rationale behind what is layed out in this section considers
+good traceability of lines of code with *git blame*, overall sensible
+modular structure, consistency in how we write code, and long term maintenance
+in mind.
+
+
+Approximate PEP-8 Style
+~~~~~~~~~~~~~~~~~~~~~~~
Python coding style for BuildStream is pep8, which is documented here: https://www.python.org/dev/peps/pep-0008/
We have a couple of minor exceptions to this standard, we dont want to compromise
@@ -93,15 +191,377 @@ code readability by being overly restrictive on line length for instance.
The pep8 linter will run automatically when running the test suite.
+.. _contributing_documenting_symbols:
+
+Documenting symbols
+~~~~~~~~~~~~~~~~~~~
+In BuildStream, we maintain what we call a *"Public API Surface"* that
+is guaranteed to be stable and unchanging across stable releases. The
+symbols which fall into this special class are documented using Python's
+standard *docstrings*, while all other internals of BuildStream are documented
+with comments above the related symbol.
+
+When documenting the public API surface which is rendered in the reference
+manual, we always mention the major version in which the API was introduced,
+as shown in the examples below. If a public API exists without the *Since*
+annotation, this is taken to mean that it was available since the first stable
+release 1.0.
+
+Here are some examples to get the hang of the format of API documenting
+comments and docstrings.
+
+**Public API Surface method**::
+
+ def frobnicate(self, source, *, frobilicious=False):
+ """Frobnicates this element with the specified source
+
+ Args:
+ source (Source): The Source to frobnicate with
+ frobilicious (bool): Optionally specify that frobnication should be
+ performed fribiliciously
+
+ Returns:
+ (Element): The frobnicated version of this Element.
+
+ *Since: 1.2*
+ """
+ ...
+
+**Internal method**::
+
+ # frobnicate():
+ #
+ # Frobnicates this element with the specified source
+ #
+ # Args:
+ # source (Source): The Source to frobnicate with
+ # frobilicious (bool): Optionally specify that frobnication should be
+ # performed fribiliciously
+ #
+ # Returns:
+ # (Element): The frobnicated version of this Element.
+ #
+ def frobnicate(self, source, *, frobilicious=False):
+ ...
+
+**Public API Surface instance variable**::
+
+ def __init__(self, context, element):
+
+ self.name = self._compute_name(context, element)
+ """The name of this foo
+
+ *Since: 1.2*
+ """
+
+**Internal instance variable**::
+
+ def __init__(self, context, element):
+
+ self.name = self._compute_name(context, element) # The name of this foo
+
+**Internal instance variable (long)**::
+
+ def __init__(self, context, element):
+
+ # This instance variable required a longer explanation, so
+ # it is on a line above the instance variable declaration.
+ self.name = self._compute_name(context, element)
+
+
+**Public API Surface class**::
+
+ class Foo(Bar):
+ """The main Foo object in the data model
+
+ Explanation about Foo. Note that we always document
+ the constructor arguments here, and not beside the __init__
+ method.
+
+ Args:
+ context (Context): The invocation Context
+ count (int): The number to count
+
+ *Since: 1.2*
+ """
+ ...
+
+**Internal class**::
+
+ # Foo()
+ #
+ # The main Foo object in the data model
+ #
+ # Args:
+ # context (Context): The invocation Context
+ # count (int): The number to count
+ #
+ # Returns:
+ # (Foo): A newly created Foo object
+ #
+ class Foo(Bar):
+ ...
+
+
+.. _contributing_class_order:
+
+Class structure and ordering
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+When creating or modifying an object class in BuildStream, it is
+important to keep in mind the order in which symbols should appear
+and keep this consistent.
+
+Here is an example to illustrate the expected ordering of symbols
+on a python class in BuildStream::
+
+ class Foo(Bar):
+
+ # Public class-wide variables come first, if any.
+
+ # Private class-wide variables, if any
+
+ # Now we have the dunder/magic methods, always starting
+ # with the __init__() method.
+
+ def __init__(self, name):
+
+ super().__init__()
+
+ # NOTE: In the instance initializer we declare any instance variables,
+ # always declare the public instance variables (if any) before
+ # the private ones.
+ #
+ # It is preferred to avoid any public instance variables, and
+ # always expose an accessor method for it instead.
+
+ #
+ # Public instance variables
+ #
+ self.name = name # The name of this foo
+
+ #
+ # Private instance variables
+ #
+ self._count = 0 # The count of this foo
+
+ ################################################
+ # Abstract Methods #
+ ################################################
+
+ # NOTE: Abstract methods in BuildStream are allowed to have
+ # default methods.
+ #
+ # Subclasses must NEVER override any method which was
+ # not advertized as an abstract method by the parent class.
+
+ # frob()
+ #
+ # Implementors should implement this to frob this foo
+ # count times if possible.
+ #
+ # Args:
+ # count (int): The number of times to frob this foo
+ #
+ # Returns:
+ # (int): The number of times this foo was frobbed.
+ #
+ # Raises:
+ # (FooError): Implementors are expected to raise this error
+ #
+ def frob(self, count):
+
+ #
+ # An abstract method in BuildStream is allowed to have
+ # a default implementation.
+ #
+ self._count = self._do_frobbing(count)
+
+ return self._count
+
+ ################################################
+ # Implementation of abstract methods #
+ ################################################
+
+ # NOTE: Implementations of abstract methods defined by
+ # the parent class should NEVER document the API
+ # here redundantly.
+
+ def frobbish(self):
+ #
+ # Implementation of the "frobbish" abstract method
+ # defined by the parent Bar class.
+ #
+ return True
+
+ ################################################
+ # Public Methods #
+ ################################################
+
+ # NOTE: Public methods here are the ones which are expected
+ # to be called from outside of this class.
+ #
+ # These, along with any abstract methods, usually
+ # constitute the API surface of this class.
+
+ # frobnicate()
+ #
+ # Perform the frobnication process on this Foo
+ #
+ # Raises:
+ # (FrobError): In the case that a frobnication error was
+ # encountered
+ #
+ def frobnicate(self):
+ frobnicator.frobnicate(self)
+
+ # set_count()
+ #
+ # Sets the count of this foo
+ #
+ # Args:
+ # count (int): The new count to set
+ #
+ def set_count(self, count):
+
+ self._count = count
+
+ # get_count()
+ #
+ # Accessor for the count value of this foo.
+ #
+ # Returns:
+ # (int): The count of this foo
+ #
+ def set_count(self, count):
+
+ return self._count
+
+ ################################################
+ # Private Methods #
+ ################################################
+
+ # NOTE: Private methods are the ones which are internal
+ # implementation details of this class.
+ #
+ # We can be absolutely sure that nobody is ever
+ # going to call these functions from outside of
+ # this class definition.
+ #
+ # Even though these are private implementation
+ # details, they still MUST have API documenting
+ # comments on them.
+
+ # _do_frobbing()
+ #
+ # Does the actual frobbing
+ #
+ # Args:
+ # count (int): The number of times to frob this foo
+ #
+ # Returns:
+ # (int): The number of times this foo was frobbed.
+ #
+ def self._do_frobbing(self, count):
+ return count
+
+
+.. _contributing_public_and_private:
+
+Public and private symbols
+~~~~~~~~~~~~~~~~~~~~~~~~~~
+BuildStream mostly follows the PEP-8 for defining public and private symbols
+for any given class, with some deviations. Please read the `section on inheritance
+<https://www.python.org/dev/peps/pep-0008/#designing-for-inheritance>`_ for
+reference on how the PEP-8 defines public and non-public.
+
+* A public symbol is any symbol which you expect to be used by clients
+ of your class or module within BuildStream.
+
+ Public symbols are written without any leading underscores.
+
+* A private symbol is any symbol which is entirely internal to your class
+ or module within BuildStream. These symbols cannot ever be accessed by
+ external clients or modules.
+
+ A private symbol must be denoted by a leading underscore.
+
+* When a class can have subclasses (for example, the ``Sandbox`` or ``Platform``
+ classes which have various implementations, or the ``Element`` and ``Source``
+ classes which plugins derive from), then private sumbols should be denoted
+ by two leading underscores.
+
+ The double leading underscore naming convention invokes Python's name
+ mangling algorithm which helps prevent namespace collisions in the case
+ that subclasses might have a private symbol with the same name.
+
+In BuildStream, we have what we call a *"Public API Surface"*, as previously
+mentioned in :ref:`contributing_documenting_symbols`. In the :ref:`next section
+<contributing_public_api_surface>` we will discuss the *"Public API Surface"* and
+outline the exceptions to the rules discussed here.
+
+
+.. _contributing_public_api_surface:
+
+Public API Surface
+~~~~~~~~~~~~~~~~~~
+BuildStream exposes what we call a *"Public API Surface"* which is stable
+and unchanging. This is for the sake of stability of the interfaces which
+plugins use, so it can also be referred to as the *"Plugin facing API"*.
+
+Any symbols which are a part of the *"Public API Surface*" are never allowed
+to change once they have landed in a stable release version of BuildStream. As
+such, we aim to keep the *"Public API Surface"* as small as possible at all
+times, and never expose any internal details to plugins inadvertently.
+
+One problem which arises from this is that we end up having symbols
+which are public according to the :ref:`rules discussed in the previous section
+<contributing_public_and_private>`, but must be hidden away from the
+*"Public API Surface"*. For example, BuildStream internal classes need
+to invoke methods on the ``Element`` and ``Source`` classes, wheras these
+methods need to be hidden from the *"Public API Surface"*.
+
+This is where BuildStream deviates from the PEP-8 standard for public
+and private symbol naming.
+
+In order to disambiguate between:
+
+* Symbols which are publicly accessible details of the ``Element`` class, can
+ be accessed by BuildStream internals, but must remain hidden from the
+ *"Public API Surface"*
+
+* Symbols which are private to the ``Element`` class, and cannot be accessed
+ from outside of the ``Element`` class at all.
+
+We denote the former category of symbols with only a single underscore, and the latter
+category of symbols with a double underscore. We often refer to this distinction
+as *"API Private"* (the former category) and *"Local Private"* (the latter category).
+
+Classes which are a part of the *"Public API Surface"* and require this disambiguation
+were not discussed in :ref:`the class ordering section <contributing_class_order>`, for
+these classes, the *"API Private"* symbols always come **before** the *"Local Private"*
+symbols in the class declaration.
+
+Modules which are not a part of the *"Public API Surface"* have their python files
+prefixed with a single underscore, and are not imported in BuildStream's the master
+``__init__.py`` which is used by plugins.
+
+.. note::
+
+ The ``utils.py`` module is public and exposes a handful of utility functions,
+ however many of the functions it provides are *"API Private"*.
+
+ In this case, the *"API Private"* functions are prefixed with a single underscore.
+
+
Imports
~~~~~~~
Module imports inside BuildStream are done with relative ``.`` notation
-Good::
+**Good**::
from .context import Context
-Bad::
+**Bad**::
from buildstream.context import Context
@@ -122,128 +582,540 @@ This makes things clear when reading code that said functions
are not defined in the same file but come from utils.py for example.
-Policy for private symbols
-~~~~~~~~~~~~~~~~~~~~~~~~~~
-Private symbols are expressed via a leading ``_`` single underscore, or
-in some special circumstances with a leading ``__`` double underscore.
+Instance Variables
+~~~~~~~~~~~~~~~~~~
+It is preferred that all instance state variables be declared as :ref:`private symbols
+<contributing_public_and_private>`, however in some cases, especially when the state
+is immutable for the object's life time (like an ``Element`` name for example), it
+is acceptable to save some typing by using a publicly accessible instance variable.
+
+It is never acceptable to modify the value of an instance variable from outside
+of the declaring class. In other words, the class which exposes an instance variable
+is the only one in control of the value of this variable.
+
+* If an instance variable is public and must be modified; then it must be
+ modified using a :ref:`mutator <contributing_accessor_mutator>`
+
+* Ideally for better encapsulation, all object state is declared as
+ :ref:`private instance variables <contributing_public_and_private>` and can
+ only be accessed by external classes via public :ref:`accessors and mutators
+ <contributing_accessor_mutator>`
+
+.. note::
+
+ In some cases, we may use small data structures declared as objects for the sake
+ of better readability, where the object class itself has no real supporting code.
+
+ In these exceptions, it can be acceptable to modify the instance variables
+ of these objects directly, unless they are otherwise documented to be immutable.
+
+
+.. _contributing_accessor_mutator:
+
+Accessors and Mutators
+~~~~~~~~~~~~~~~~~~~~~~
+An accessor and mutator, are methods defined on the object class to access (get)
+or mutate (set) a value owned by the declaring class, respectively.
+
+An accessor might derive the returned value from one or more of its components,
+and a mutator might have side effects, or delegate the mutation to a component.
+
+Accessors and mutators are always :ref:`public <contributing_public_and_private>`
+(even if they might have a single leading underscore and are considered
+:ref:`API Private <contributing_public_api_surface>`), as their purpose is to
+enforce encapsulation with regards to any accesses to the state which is owned
+by the declaring class.
+
+Accessors and mutators are functions prefixed with ``get_`` and ``set_``
+respectively, e.g.::
+
+ class Foo():
+
+ def __init__(self):
+
+ # Declare some internal state
+ self._count = 0
+
+ # get_count()
+ #
+ # Gets the count of this Foo.
+ #
+ # Returns:
+ # (int): The current count of this Foo
+ #
+ def get_foo(self):
+ return self._count
+
+ # set_count()
+ #
+ # Sets the count of this Foo.
+ #
+ # Args:
+ # count (int): The new count for this Foo
+ #
+ def set_foo(self, count):
+ self._count = count
+
+.. attention::
+
+ We are aware that Python offers a facility for accessors and
+ mutators using the ``@property`` decorator instead. Do not use
+ the ``@property`` decorator.
+
+ The decision to use explicitly defined functions instead of the
+ ``@property`` feature is rather arbitrary, and there is not much
+ technical merit to preferring one technique over the other.
+ However as :ref:`discussed below <contributing_always_consistent>`,
+ it is of the utmost importance that we do not mix both techniques
+ in the same code base.
+
+
+.. _contributing_abstract_methods:
+
+Abstract Methods
+~~~~~~~~~~~~~~~~
+In BuildStream, an *"Abstract Method"* is a bit of a misnomer and does
+not match up to how Python defines abstract methods, we need to seek out
+a new nomanclature to refer to these methods.
-Before understanding the naming policy, it is first important to understand
-that in BuildStream, there are two levels of privateness which need to be
-considered.
+In Python, an *"Abstract Method"* is a method which **must** be
+implemented by a subclass, whereas all methods in Python can be
+overridden.
-These are treated subtly differently and thus need to be understood:
+In BuildStream, we use the term *"Abstract Method"*, to refer to
+a method which **can** be overridden by a subclass, whereas it
+is **illegal** to override any other method.
-* API Private
+* Abstract methods are allowed to have default implementations
- A symbol is considered to be *API private* if it is not exposed in the *public API*.
+* Subclasses are not allowed to redefine the calling signature
+ of an abstract method, or redefine the API contract in any way
- Even if a symbol does not have any leading underscore, it may still be *API private*
- if the containing *class* or *module* is named with a leading underscore.
+* Subclasses are not allowed to override any other methods.
-* Local private
+The key here is that in BuildStream, we consider it unacceptable
+that a subclass overrides a method of it's parent class unless
+the said parent class has explicitly given permission to subclasses
+to do so, and outlined the API contract for this. No surprises
+are allowed.
- A symbol is considered to be *local private* if it is not intended for access
- outside of the defining *scope*.
- If a symbol has a leading underscore, it might not be *local private* if it is
- declared on a publicly visible class, but needs to be accessed internally by
- other modules in the BuildStream core.
+Error Handling
+~~~~~~~~~~~~~~
+In BuildStream, all non recoverable errors are expressed via
+subclasses of the ``BstError`` exception.
+This exception is handled deep in the core in a few places, and
+it is rarely necessary to handle a ``BstError``.
-Ordering
-''''''''
-For better readability and consistency, we try to keep private symbols below
-public symbols. In the case of public modules where we may have a mix of
-*API private* and *local private* symbols, *API private* symbols should come
-before *local private* symbols.
+Raising Exceptions
+''''''''''''''''''
+When writing code in the BuildStream core, ensure that all system
+calls are wrapped in a ``try:`` block, and raise a descriptive ``BstError``
+of the appropriate class explaining what exactly failed.
-Symbol naming
-'''''''''''''
-Any private symbol must start with a single leading underscore for two reasons:
+Always include the original system call error is formatted into
+your new exception, and that you use the python ``from`` semantic
+to retain the original call trace, example::
-* So that it does not bleed into documentation and *public API*.
+ try:
+ os.utime(self._refpath(ref))
+ except FileNotFoundError as e:
+ raise ArtifactError("Attempt to access unavailable artifact: {}".format(e)) from e
-* So that it is clear to developers which symbols are not used outside of the declaring *scope*
-Remember that with python, the modules (python files) are also symbols
-within their containing *package*, as such; modules which are entirely
-private to BuildStream are named as such, e.g. ``_thismodule.py``.
+Enhancing Exceptions
+''''''''''''''''''''
+Sometimes the ``BstError`` originates from a lower level component,
+and the source of the error did not have enough context to create
+a good and informative summary of the error for the user.
+In these cases it is necessary to handle the error and raise a new
+one, e.g.::
-Cases for double underscores
-''''''''''''''''''''''''''''
-The double underscore in python has a special function. When declaring
-a symbol in class scope which has a leading underscore, it can only be
-accessed within the class scope using the same name. Outside of class
-scope, it can only be accessed with a *cheat*.
+ try:
+ extracted_artifact = self._artifacts.extract(self, cache_key)
+ except ArtifactError as e:
+ raise ElementError("Failed to extract {} while checking out {}: {}"
+ .format(cache_key, self.name, e)) from e
-We use the double underscore in cases where the type of privateness can be
-ambiguous.
-* For private modules and classes
+Programming errors
+''''''''''''''''''
+Sometimes you are writing code and have detected an unexpected condition,
+or a broken invariant for which the code cannot be prepared to handle
+gracefully.
- We never need to disambiguate with a double underscore
+In these cases, do **not** raise any of the ``BstError`` class exceptions.
-* For private symbols declared in a public *scope*
+Instead, use the python ``assert`` statement, e.g.::
- In the case that we declare a private method on a public object, it
- becomes ambiguous whether:
+ assert utils._is_main_process(), \
+ "Attempted to save workspace configuration from child process"
- * The symbol is *local private*, and only used within the given scope
+This will result in a ``BUG`` message with the stack trace included being
+logged and reported in the frontend.
- * The symbol is *API private*, and will be used internally by BuildStream
- from other parts of the codebase.
- In this case, we use a single underscore for *API private* methods which
- are not *local private*, and we use a double underscore for *local private*
- methods declared in public scope.
+BstError parameters
+'''''''''''''''''''
+When raising ``BstError`` class exceptions, there are some common properties
+which can be useful to know about:
+* **message:** The brief human readable error, will be formatted on one line in the frontend
-Documenting private symbols
-'''''''''''''''''''''''''''
-Any symbol which is *API Private* (regardless of whether it is also
-*local private*), should have some documentation for developers to
-better understand the codebase.
+* **detail:** An optional detailed human readable message to accompany the **message** summary
+ of the error. This is often used to recommend the user some course of action, or to provide
+ additional context about the error.
-Contrary to many other python projects, we do not use docstrings to
-document private symbols, but prefer to keep *API Private* symbols
-documented in code comments placed *above* the symbol (or *beside* the
-symbol in some cases, such as variable declarations in a class where
-a shorter comment is more desirable), rather than docstrings placed *below*
-the symbols being documented.
+* **temporary:** Some errors are allowed to be *temporary*, this attribute is only
+ observed from child processes which fail in a temporary way. This distinction
+ is used to determine whether the task should be *retried* or not. An error is usually
+ only a *temporary* error if the cause of the error was a network timeout.
-Other than this detail, follow the same guidelines for documenting
-symbols as described below.
+* **reason:** A machine readable identifier for the error. This is used for the purpose
+ of regression testing, such that we check that BuildStream has errored out for the
+ expected reason in a given failure mode.
-Documenting BuildStream
------------------------
+Documenting Exceptions
+''''''''''''''''''''''
+We have already seen :ref:`some examples <contributing_class_order>` of how
+exceptions are documented in API documenting comments, but this is worth some
+additional disambiguation.
+
+* Only document the exceptions which are raised directly by the function in question.
+ It is otherwise nearly impossible to keep track of what exceptions *might* be raised
+ indirectly by calling the given function.
+
+* For a regular public or private method, your audience is a caller of the function;
+ document the exception in terms of what exception might be raised as a result of
+ calling this method.
+
+* For an :ref:`abstract method <contributing_abstract_methods>`, your audience is the
+ implementor of the method in a subclass; document the exception in terms of what
+ exception is prescribed for the implementing class to raise.
+
+
+.. _contributing_always_consistent:
+
+Always be consistent
+~~~~~~~~~~~~~~~~~~~~
+There are various ways to define functions and classes in Python,
+which has evolved with various features over time.
+
+In BuildStream, we may not have leveraged all of the nice features
+we could have, that is okay, and where it does not break API, we
+can consider changing it.
+
+Even if you know there is a *better* way to do a given thing in
+Python when compared to the way we do it in BuildStream, *do not do it*.
+
+Consistency of how we do things in the codebase is more important
+than the actual way in which things are done, always.
+
+Instead, if you like a certain python feature and think the BuildStream
+codebase, then propose your change on the `mailing list
+<https://mail.gnome.org/mailman/listinfo/buildstream-list>`_. Chances
+are that we will reach agreement to use your preferred approach, and
+in that case, it will be important to apply the change unilaterally
+across the entire codebase, such that we continue to have a consistent
+code base.
+
+
+Avoid tail calling
+~~~~~~~~~~~~~~~~~~
+With the exception of tail calling with simple functions from
+the standard python library, such as splitting and joining lines
+of text and encoding/decoding text; always avoid tail calling.
+
+**Good**::
+
+ # Variables that we will need declared up top
+ context = self._get_context()
+ workspaces = context.get_workspaces()
+
+ ...
+
+ # Saving the workspace configuration
+ workspaces.save_config()
+
+**Bad**::
+
+ # Saving the workspace configuration
+ self._get_context().get_workspaces().save_config()
+
+**Acceptable**::
+
+ # Decode the raw text loaded from a log file for display,
+ # join them into a single utf-8 string and strip away any
+ # trailing whitespace.
+ return '\n'.join([line.decode('utf-8') for line in lines]).rstrip()
+
+When you need to obtain a delegate object via an accessor function,
+either do it at the beginning of the function, or at the beginning
+of a code block within the function that will use that object.
+
+There are several reasons for this convention:
+
+* When observing a stack trace, it is always faster and easier to
+ determine what went wrong when all statements are on separate lines.
+
+* We always want individual lines to trace back to their origin as
+ much as possible for the purpose of tracing the history of code
+ with *git blame*.
+
+ One day, you might need the ``Context`` or ``Workspaces`` object
+ in the same function for another reason, at which point it will
+ be unacceptable to leave the existing line as written, because it
+ will introduce a redundant accessor to the same object, so the
+ line written as::
+
+ self._get_context().get_workspaces().save_config()
+
+ Will have to change at that point, meaning we lose the valuable
+ information of which commit originally introduced this call
+ when running *git blame*.
+
+* For similar reasons, we prefer delegate objects be accessed near
+ the beginning of a function or code block so that there is less
+ chance that this statement will have to move in the future, if
+ the same function or code block needs the delegate object for any
+ other reason.
+
+ Asides from this, code is generally more legible and uniform when
+ variables are declared at the beginning of function blocks.
+
+
+Vertical stacking of modules
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+For the sake of overall comprehensiveness of the BuildStream
+architecture, it is important that we retain vertical stacking
+order of the dependencies and knowledge of modules as much as
+possible.
+
+For instance, the ``Source`` objects are owned by ``Element``
+objects in the BuildStream data model, and as such the ``Element``
+will delegate some activities to the ``Source`` objects in it's
+possesion. The ``Source`` objects should however never call functions
+on the ``Element`` object, nor should the ``Source`` object itself
+have any understanding of what an ``Element`` is.
+
+If you are implementing a low level utility layer, for example
+as a part of the ``YAML`` loading code layers, it can be tempting
+to derive context from the higher levels of the codebase which use
+these low level utilities, instead of defining properly stand alone
+APIs for these utilities to work: Never do this.
+
+Unfortunately, unlike other languages where include files play
+a big part in ensuring that it is difficult to make a mess; Python,
+allows you to just call methods on arbitrary objects passed through
+a function call without having to import the module which defines
+those methods - this leads to cyclic dependencies of modules quickly
+if the developer does not take special care of ensuring this does not
+happen.
+
+
+Use less arguments in methods
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+When creating an object, or adding a new API method to an existing
+object, always strive to keep as much context as possible on the
+object itself rather than expecting callers of the methods to provide
+everything the method needs every time.
+
+If the value or object that is needed in a function call is a constant
+for the lifetime of the object which exposes the given method, then
+that value or object should be passed in the constructor instead of
+via a method call.
+
+
+Avoid transient state on instances
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+At times, it can be tempting to store transient state that is
+the result of one operation on an instance, only to be retrieved
+later via an accessor function elsewhere.
+
+As a basic rule of thumb, if the value is transient and just the
+result of one operation, which needs to be observed directly after
+by another code segment, then never store it on the instance.
+
+BuildStream is complicated in the sense that it is multi processed
+and it is not always obvious how to pass the transient state around
+as a return value or a function parameter. Do not fall prey to this
+obstacle and pollute object instances with transient state.
+
+Instead, always refactor the surrounding code so that the value
+is propagated to the desired end point via a well defined API, either
+by adding new code paths or changing the design such that the
+architecture continues to make sense.
+
+
+Refactor the code base as needed
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+Especially when implementing features, always move the BuildStream
+codebase forward as a whole.
+
+Taking a short cut is alright when prototyping, but circumventing
+existing architecture and design to get a feature implemented without
+re-designing the surrounding architecture to accommodate the new
+feature instead, is never acceptable upstream.
+
+For example, let's say that you have to implement a feature and you've
+successfully prototyped it, but it launches a ``Job`` directly from a
+``Queue`` implementation to get the feature to work, while the ``Scheduler``
+is normally responsible for dispatching ``Jobs`` for the elements on
+a ``Queue``. This means that you've proven that your feature can work,
+and now it is time to start working on a patch for upstream.
+
+Consider what the scenario is and why you are circumventing the design,
+and redesign the ``Scheduler`` and ``Queue`` objects to accommodate for
+the new feature and condition under which you need to dispatch a ``Job``,
+or how you can give the ``Queue`` implementation the additional context it
+needs.
+
+
+Adding core plugins
+-------------------
+This is a checklist of things which need to be done when adding a new
+core plugin to BuildStream proper.
+
+
+Update documentation index
+~~~~~~~~~~~~~~~~~~~~~~~~~~
+The documentation generating scripts will automatically pick up your
+newly added plugin and generate HTML, but will not add a link to the
+documentation of your plugin automatically.
+
+Whenever adding a new plugin, you must add an entry for it in ``doc/source/core_plugins.rst``.
+
+
+Bump format version
+~~~~~~~~~~~~~~~~~~~
+In order for projects to assert that they have a new enough version
+of BuildStream to use the new plugin, the ``BST_FORMAT_VERSION`` must
+be incremented in the ``_versions.py`` file.
+
+Remember to include in your plugin's main documentation, the format
+version in which the plugin was introduced, using the standard annotation
+which we use throughout the documentation, e.g.::
+
+ .. note::
+
+ The ``foo`` plugin is available since :ref:`format version 16 <project_format_version>`
+
+
+Add tests
+~~~~~~~~~
+Needless to say, all new feature additions need to be tested. For ``Element``
+plugins, these usually need to be added to the integration tests. For ``Source``
+plugins, the tests are added in two ways:
+
+* For most normal ``Source`` plugins, it is important to add a new ``Repo``
+ implementation for your plugin in the ``tests/testutils/repo/`` directory
+ and update ``ALL_REPO_KINDS`` in ``tests/testutils/repo/__init__.py``. This
+ will include your new ``Source`` implementation in a series of already existing
+ tests, ensuring it works well under normal operating conditions.
+
+* For other source plugins, or in order to test edge cases, such as failure modes,
+ which are not tested under the normal test battery, add new tests in ``tests/sources``.
+
+
+Extend the cachekey test
+~~~~~~~~~~~~~~~~~~~~~~~~
+For any newly added plugins, it is important to add some new simple elements
+in ``tests/cachekey/project/elements`` or ``tests/cachekey/project/sources``,
+and ensure that the newly added elements are depended on by ``tests/cachekey/project/target.bst``.
+
+One new element should be added to the cache key test for every configuration
+value which your plugin understands which can possibly affect the result of
+your plugin's ``Plugin.get_unique_key()`` implementation.
+
+This test ensures that cache keys do not unexpectedly change or become incompatible
+due to code changes. As such, the cache key test should have full coverage of every
+YAML configuration which can possibly affect cache key outcome at all times.
+
+See the ``tests/cachekey/update.py`` file for instructions on running the updater,
+you need to run the updater to generate the ``.expected`` files and add the new
+``.expected`` files in the same commit which extends the cache key test.
+
+
+Protocol buffers
+----------------
+BuildStream uses protobuf and gRPC for serialization and communication with
+artifact cache servers. This requires ``.proto`` files and Python code
+generated from the ``.proto`` files using protoc. All these files live in the
+``buildstream/_protos`` directory. The generated files are included in the
+git repository to avoid depending on grpcio-tools for user installations.
+
+
+Regenerating code
+~~~~~~~~~~~~~~~~~
+When ``.proto`` files are modified, the corresponding Python code needs to
+be regenerated. As a prerequisite for code generation you need to install
+``grpcio-tools`` using pip or some other mechanism::
+
+ pip3 install --user grpcio-tools
+
+To actually regenerate the code::
+
+ ./setup.py build_grpc
+
+
+Documenting
+-----------
BuildStream starts out as a documented project from day one and uses
sphinx to document itself.
+This section discusses formatting policies for editing files in the
+``doc/source`` directory, and describes the details of how the docs are
+generated so that you can easily generate and view the docs yourself before
+submitting patches to the documentation.
+
+For details on how API documenting comments and docstrings are formatted,
+refer to the :ref:`documenting section of the coding guidelines
+<contributing_documenting_symbols>`.
+
Documentation formatting policy
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
The BuildStream documentation style is as follows:
-* Titles and headings require two leading empty lines above them. Only the first word should be capitalized.
+* Titles and headings require two leading empty lines above them.
+ Only the first word in a title should be capitalized.
- * If there is an ``.. _internal_link`` anchor, there should be two empty lines above the anchor, followed by one leading empty line.
+ * If there is an ``.. _internal_link:`` anchor, there should be two empty lines
+ above the anchor, followed by one leading empty line.
* Within a section, paragraphs should be separated by one empty line.
-* Notes are defined using: ``.. note::`` blocks, followed by an empty line and then indented (3 spaces) text.
+* Notes are defined using: ``.. note::`` blocks, followed by an empty line
+ and then indented (3 spaces) text.
+
+ * Other kinds of notes can be used throughout the documentation and will
+ be decorated in different ways, these work in the same way as ``.. note::`` does.
-* Code blocks are defined using: ``.. code:: LANGUAGE`` blocks, followed by an empty line and then indented (3 spaces) text. Note that the default language is `python`.
+ Feel free to also use ``.. attention::`` or ``.. important::`` to call special
+ attention to a paragraph, ``.. tip::`` to give the reader a special tip on how
+ to use an advanced feature or ``.. warning::`` to warn the user about a potential
+ misuse of the API and explain it's consequences.
+
+* Code blocks are defined using: ``.. code:: LANGUAGE`` blocks, followed by an empty
+ line and then indented (3 spaces) text. Note that the default language is `python`.
* Cross references should be of the form ``:role:`target```.
- * To cross reference arbitrary locations with, for example, the anchor ``_anchor_name``, you must give the link an explicit title: ``:ref:`Link text <anchor_name>```. Note that the "_" prefix is not required.
+ * Explicit anchors can be declared as ``.. _anchor_name:`` on a line by itself.
+
+ * To cross reference arbitrary locations with, for example, the anchor ``anchor_name``,
+ always provide some explicit text in the link instead of deriving the text from
+ the target, e.g.: ``:ref:`Link text <anchor_name>```.
+ Note that the "_" prefix is not used when referring to the target.
Useful links:
-For further information, please see the `Sphinx Documentation <http://www.sphinx-doc.org/en/master/usage/restructuredtext/basics.html>`_.
+For further information, please see the `Sphinx Documentation
+<http://www.sphinx-doc.org/en/master/usage/restructuredtext/basics.html>`_.
Building Docs
@@ -312,32 +1184,6 @@ the ``man/`` subdirectory, which will be automatically included
in the buildstream distribution.
-Documenting conventions
-~~~~~~~~~~~~~~~~~~~~~~~
-We use the sphinx.ext.napoleon extension for the purpose of having
-a bit nicer docstrings than the default sphinx docstrings.
-
-A docstring for a method, class or function should have the following
-format::
-
- """Brief description of entity
-
- Args:
- argument1 (type): Description of arg
- argument2 (type): Description of arg
-
- Returns:
- (type): Description of returned thing of the specified type
-
- Raises:
- (SomeError): When some error occurs
- (SomeOtherError): When some other error occurs
-
- A detailed description can go here if one is needed, only
- after the above part documents the calling conventions.
- """
-
-
Documentation Examples
~~~~~~~~~~~~~~~~~~~~~~
The examples section of the documentation contains a series of standalone
@@ -419,30 +1265,8 @@ regenerate them locally in order to build the docs.
command: build hello.bst
-Protocol Buffers
-----------------
-BuildStream uses protobuf and gRPC for serialization and communication with
-artifact cache servers. This requires ``.proto`` files and Python code
-generated from the ``.proto`` files using protoc. All these files live in the
-``buildstream/_protos`` directory. The generated files are included in the
-git repository to avoid depending on grpcio-tools for user installations.
-
-
-Regenerating code
-~~~~~~~~~~~~~~~~~
-When ``.proto`` files are modified, the corresponding Python code needs to
-be regenerated. As a prerequisite for code generation you need to install
-``grpcio-tools`` using pip or some other mechanism::
-
- pip3 install --user grpcio-tools
-
-To actually regenerate the code::
-
- ./setup.py build_grpc
-
-
-Testing BuildStream
--------------------
+Testing
+-------
BuildStream uses pytest for regression tests and testing out
the behavior of newly added components.
@@ -495,6 +1319,7 @@ with::
Alternatively, any IDE plugin that uses pytest should automatically
detect the ``.pylintrc`` in the project's root directory.
+
Adding tests
~~~~~~~~~~~~
Tests are found in the tests subdirectory, inside of which
@@ -521,8 +1346,9 @@ Tests that run a sandbox should be decorated with::
and use the integration cli helper.
-Measuring BuildStream performance
----------------------------------
+
+Measuring performance
+---------------------
Benchmarking framework