.. _contributing: Contributing ============ The development of Bao projects' components uses the Github ecosystem for contributions, code review, bug reporting, discussion of ideas or any other community development, or project management activities. Therefore, every :term:`contributor` should first create a Github account, if they do not have one already. If you plan to contribute a significant portion of code, such as an architecture or platform port, a new subsystem or feature, please consider creating a discussion thread, where the design and trade-offs can be first debated among the community and **maintainers**. We also recommend you to read our :ref:`licensing terms and the DCO claim ` before making any contribution. .. _contrib_roles: Roles ----- The project defines three essential types of roles in the development and contribution process: **developer**, **code owner**, and **maintainer**. A **developer** can be internal (if they are part of the project's core team) or are an outside :term:`contributor`. Anyone that contributes in any way to the project (e.g., code, reviews, issues, etc) is assigned to this role. A **code owner** must either be part of the core team or someone invited and marked as an outside collaborator by a **maintainer**. He is responsible for overseeing the development of one or multiple subsystems, that must have a proven record of deep understanding of that modules, specifically by having heavily contributed to their design and/or implementation. Besides, he must participate in code reviews that affect their assigned subsystems. A **maintainer** must always be part of the core team. He is the person ultimately responsible for everything in the repository, including code structure, quality, functionality, overall repository architecture, and interoperability. In all, making sure the repository adheres to the project quality standards, high-level goals and vision. They are also responsible for repository management tasks which include: * setting up :ref:`branch protection rules`, in particular for the ``main`` branch; * assigning subsystems to code-owners through the :ref:`.CODEOWNERS file`; * keep an updated :ref:`list of authors and contributors`; * coordinating and moderating code reviews; * final approval and merging of pull-requests (:term:`PR`); * coordinating, moderating and closing of issues; * setting up the repository's :ref:`CI pipeline via Github Actions`; * manage :ref:`topic branches` (e.g. delete stale ones); * maintain the overall organization of the repository; * inviting outside collaborators; A repository must always have at least one **maintainer**, that is implicitly the **code owner** of any subsystems that do not have any other. If a **maintainer** steps down, it should first nominate a successor, preferably out of the **code owners**. .. _contribution_workflow: Contribution Workflow --------------------- The project's development flow for all repositories is centered around the ``main`` branch. Any code contribution, be it internal or external, takes the form of a Github :term:`PR` to the ``main`` branch. The ``main`` branch is then tagged periodically according to the defined :ref:`versioning scheme`. .. _topic_branches: Internal Topic Branches *********************** For long-running developments, internal contributors may create topic branches in the repository itself. Topic branch names should be prefixed with by ``/`` (where type is one of the predetermined :ref:`branch types `), followed by a short descriptor written in lower snake case. For example, when developing a new feature the branch should be named ``feat/my_new_feature``. Once development on a topic branch ceases, either because it was merged to ``main`` or for some reason discarded, the branch must be deleted. **Maintainers** are ultimately responsible for deleting stale of merged topic branches. Submitting Commits ****************** If you are an external :term:`contributor` or do not have write permissions to the repository you wish to contribute to, first of all, you should `fork that repository `_. If you do have write privileges over the original repository, you carry out the development directly on it. Follow these steps: 1. Create a topic branch from the ``main`` branch; .. note:: It is possible to create a topic branch based on another branch. This may be necessary if the changes are dependent on that specific branch. However, if this occurs during **step 4** of the process, it is mandatory to make the target branch the one that the new branch is based on. 2. Make all the necessary commits and push your work to your remote fork. Make sure that all the introduced commits and the overall branch follow the :ref:`commit and PR guidelines`; 3. Make sure the branch is synced and can be merged or rebased on ``main`` without conflicts. If necessary, `rewrite the branch's history `_, by rebasing it on ``main``; 4. `Create a PR `_ targeting the original repository's ``main`` branch. In case the PR introduces a large set of modifications, to facilitate the review process, it is recommended to split it into multiple smaller PRs (i.e., :ref:`splitting large PRs `). Ultimately, the assignee will ask you to split the PR if they feel it is too large; 5. Patiently wait for reviews and be engaged when they arrive: * participate in the discussion with **reviewers**; * address any refactoring, fixes, or other modifications to your code contribution raised by the **reviewers'** comments. In doing so, add new commits to the existing pull request. If existing commits need to be modified, rewrite the history and force push them to maintain a clean linear history; * always reply to each inline comment/conversation topic opened by the reviewer and never mark it as resolved yourself. The **reviewer** must be the one to close that conversation in case they agree with the changes; * re-request a review from **reviewers'** after you have addressed all their concerns and modification requests; * if the **reviewers** are taking too long, try contacting the :term:`PR` assignee. .. _split_prs: Splitting Large PRs ################### Code reviews are typically a difficult and time-consuming process. To facilitate the reviewer's work, contributors should try to split their PRs into smaller ones if they feel that the PR introduces a large set of modifications. Nevertheless, the PR assignee decides if the PR is too large and may request splitting it. To split your large PR, a contributor should follow this process: * create a topic branch from the ``main`` branch and call it ``wip/``. The branch should be protected (:ref:`branch protection rules`). The ```` should be a short description of the feature, subsystem, or functionality being developed. External contributors should ask maintainers to create this branch for them, whom will become responsible for managing the ``wip`` branch and synchronizing with the external contributor; * this ``wip/`` should be the base branch of the overall feature to be submitted. For example, it can be a skeleton PR that only contains the infrastructure of the new functionality; * submit this base branch as a draft PR and mention on the PR description that it is the base branch for a big feature that will be submitted in multiple PRs; * continue to introduce the remaining components of the feature one PR at a time, ensuring that each PR is as self-contained as possible. * when each sub-feature PR is accepted and merged into the ``wip/`` branch and the feature is complete, the assignee is responsible to merge the ``wip/`` branch into ``main``. .. _chain_prs: Chain of PRs ############ A chain of PRs is a sequence of PRs that are dependent on each other. For example, a PR that introduces a new subsystem of a feature inherently depends on the PR that introduces the feature itself. Therefore, the PR that introduces the subsystem must be merged after. In case this happens, a contributor must chain the PRs, avoiding the merge of PRs that are dependent on others in the wrong order. To do so, the contributor must: * if you are the first to introduce the the chain, follow the typical PR submission process and introduce the first PR of the chain. The contributor should mention on the PR description that it is the first PR of a chain of PRs or, in the case you are following up another PR, mention in the PR description that dependency; * continue to introduce the remaining PRs of the chain one PR at a time, as pieces of a one-row puzzle, meaning that each PR should be dependent on the previous one. Therefore, when submitting the PR, make sure that the target branch corresponds to the previous PR of the chain; * as each PR is merged, github will automatically update the target branch of the next PR in the chain. For example, in the below diagram, if ``wip/base_feat`` is merged, github automatically changes the target branch of ``wip/sub_feat1`` to ``main``; * the contributor should mention on the PR description that it is part of a chain of PRs. Tag that PR dependency using a phrase such as "Depends on #"; * this process should not be used to create PRs that are dependent on two or more PRs. The below diagram provides a visual representation of the chain of PRs: [main] <- [wip/base_feat] <- [wip/sub_feat1] <- [wip/sub_feat2] Review Assignment ***************** A :term:`PR` must have at least one assignee and be approved by at least two **reviewers**. The assignee must be a **maintainer** which will be responsible for getting the :term:`PR` through, having the ultimate say on its acceptance and that must carry out the final merge. **Maintainers** must coordinate to choose among them the assignee as PRs arrive. One of the **reviewers** must also be a **maintainer** (which can coincide with the assignee). If a **code owner** exist for the code being submitted, at least one of them (for each of the files/subsystems) must review the code. **Code owners** will be automatically assigned as **reviewers** if the **maintainers** are correctly managing the :ref:`.CODEOWNERS file`. If there are not enough **reviewers**, the assignee is responsible for appointing a second **reviewer**. Preferably, a project's internal :term:`contributor`. They might also require and invite more **reviewers** if there is no consensus. Review Guide ************ As much as possible, code quality and enforced standards/guidelines will be automatically checked in the CI pipeline. **Reviewers** must be particularly attentive to the ones that are not addressed by these automated tools. The following are some tips all **reviewers** should take into account: * Make sure the code is readable, well commented (includes doxygen comments), and the :term:`PR` provide the appropriate/necessary documentation; * The code follows the project's coding guidelines as much as possible, especially those not automatically checked such as: * code organization, that is, are the files placed correctly? (e.g., architecture specific files in the *arch* directory); * naming conventions for files, functions, variables, types, etc. * The :term:`PR` complies with all the relevant standards mandated for the specific language or repo in question (e.g. :ref:`MISRA`); * There are no obscure binary blobs included in the :term:`PR`; * Understand the design and implementation decisions behind the :term:`PR`; Try to imagine how you'd go about implementing the same functionality, and engage in discussion when it does not match the proposed approach. Discuss the trade-offs of the various approaches. * Have a holistic view of the code in mind: * how do the modifications affect other subsystems and the maintainability and evolution of the code base?; * does the design follow the same philosophy of the over module, repo or project? Be it at the API, architecture, or implementation levels. * Be on the look out for bugs in both the implementation (e.g. precision loss or wrong operator precedence) and the semantic (does it correctly achieve the desired functionality). Try to reason about corner cases. * New files contain the necessary :ref:`license and copyright information`; * All the necessary requirement and traceability artifacts or tags are correctly added or updated; Review the code as much as possible by opening discussions and adding comments inline in the ``Files Changed`` tab of the :term:`PR`, and opening a review. When you are done click the ``Finish Review`` button and submit the review either by only commenting or requesting explicit changes. As the :term:`contributor` addresses your concerns mark each item as resolved. When you are happy with the current state with the :term:`PR` and agree it should be merged, add a final review with an explicit approval. Beware there might still be new commits after you have approved the :term:`PR` and you might need or be asked to review it again. Finally, although obvious, self-reviewing is prohibited. Final Approval and Merging ************************** The final approval of the :term:`PR` to ``main`` must be carried out by a **maintainer**. They should verify the following checklist, although some of it might be automatically checked and enforced by GitHub: * was reviewed by at least by two **reviewers**; * all review comments, suggestions or modification requests have been addressed; * passes all CI pipeline checks; * can be rebased on ``main`` without any conflict; The **maintainer** shall have as the main objective when integrating the :term:`PR` to maintain a linear git history. Therefore, it should preferably perform either a rebase of the :term:`PR` branch on ``main`` (or fast-forward merge if possible) or perform a squash merge if they deem necessary. If the :term:`PR` originates from an internal topic branch, the branch should be deleted. Finally, the **maintainer** should update any :term:`contributor`, :ref:`author and/or code owner files`, especially when new files are created. .. _commit_guidelines: Commit, Branch, and PRs Guidelines ---------------------------------- PRs: Commit History ******************* All contributions must be submitted via Github PRs. You should ensure that all commits in the history within the :term:`PR`: * have messages that follow the :ref:`conventional commit style` * introduce small, self-contained logical units of modifications/extensions and don't include irrelevant changes (typo or formatting fixes should be submitted in dedicated PRs); * are logically related (unrelated modifications or fixes must be addressed in a separate ``branch/pull-request``); * follow a logical order. That is, a commit that has a dependence on the modifications by a different commit of the same :term:`PR`, is after the former. * adhere to the project's coding guidelines for the targeted languages; * tag the necessary requirements; * introduce code that is readable and sufficiently commented/documented; * pass all base CI pipeline checks, by running them locally; * make sure your code works: test your code in as many targets as possible and write the needed automated tests; * introduces or updates the necessary documentation; * the branch can be rebased on ``main`` without conflicts; * the :ref:`appropriate license and copyright information` is present and updated; * make sure you have the rights to all the submitted code and that :ref:`all commits contain a sign-off message`, acknowledging the :ref:`DCO`; Commits: Structure, Format, and Sign-off **************************************** All commits follow a set of guidelines for the message structure and format. Moreover, they should all be sign-off by the contributor. .. _commits_style: Message Structure ################# The git commit messages must always contain a **header**, a **body**, and a **footer**. We follow the `Convential Commits `_ specification (with some slight deviations) that provides an easy set of rules for creating an explicit commit history. This leads to more readable messages that are self-explanatory through the project history. Each commit message has the following structure: .. code-block:: none [header] (): [body] [footer] **Message header** The commit message **header** has a special format that includes a **type**, a optional **scope**, and a **description**. The prefix ```` consists of a noun describing the type of commit. These nouns are pre-defined and described in the below table (:ref:`msg_format`). The *optional* prefix ```` may be provided after the type to identify the subsystem, architecture of platform affected by the changes. The ```` field follows immediately after the colon and space after the type/scope prefix. It must provide a short summary of the code changes. **Message body** The commit message **body** must be descriptive enough to address in the ```` at least the following points: * describes the introduced features, fixes, extensions, and refactoring; * provide a brief rationale for the chosen implementation or overall approach; * how are you sure it works, i.e., describe the tests and corner cases you ran; **Message footer** The ``