**Disclaimer: This proposal is a draft right now and is put up for further discussion within the trustedfirmware.org community. Nothing is set in stone yet and it is expected to go under change as feedback from the community is incorporated.** = Goal of this proposal = As the developers community at trustedfirmware.org is growing, there is an increasing need to have work processes that are clearly documented, feel smooth and scale well. We think that there is an opportunity to improve the way the TF-A and TF-M projects are managed today. The aim of this document is to propose a set of rules, guidelines and processes to try and improve the way we work together as a community today. In particular, some of these goals are: - To make the code review process more explicit and clearly documented. - To set expectations for contributors and reviewers. - To establish a hierarchy of roles and their responsibilities. - To provide guidelines for new contributors. - To enlarge maintainership to developers outside of Arm. We hope that it will make it easier for people to contribute to the project, improve the time it takes to get patches merged in the tree, make the process more transparent, while reducing potential frictions and frustration that can arise in the community. = Definitions of roles, rights and responsibilities = We propose defining several roles that people contributing to the project can assume. Different roles come with different rights and responsibilities and are attributed based on the level of expertise and engagement in the project, the bandwidth, to name a few. == Contributor == Default role for anyone starting to contribute to the project. Contributors can: - Submit patches for review with an aim to get them merged upstream. - Review other people's patches. Although their valuable input is always welcome, it is not sufficient to get a patch approved and merged. Similarly, they can express concerns but these cannot veto a patch. See section "Criteria to get a patch merged" below. - Participate on the development mailing list. == Code Owner == Someone who looks after a particular module of the source tree (for example: the power management layer or a specific platform port.) The project keeps a list of code owners, their contact information along with the modules they manage. Code owners will get requested to review patches in the modules they own and they are responsible to do so in a timely manner. If several code owners are looking after the same module, each patch needs only approving by one of them (but more of them might do it if they wish). A code owner shall have a genuine interest in the said module and have enough bandwidth to deal with incoming reviews and general design discussions. == Maintainer == There are a number of maintainers assigned to the project. The project keeps a list of them along with their contact information. Maintainers don't necessarily need a deep domain expertise of all areas of the project, instead they look at the project from a high level perspective. They are expected to ensure that changes adhere to the project's quality criteria, to its architectural direction, to its threat model and so on. Code owners and maintainers are complementary roles. Unlike code owners, maintainers can approve any patch, no matter what module it concerns. They are responsible for ensuring patches have had enough overall review and adding their own approval in a timely manner. They also have merge rights, i.e. the ability to merge a patch in the tree once it has received all required approvals. Note that roles can be cumulative, in particular the same individual can be both a code owner and a maintainer. In such a scenario, the individual would be able to self-merge a patch solely affecting his module, having the authority to approve it from both a code owner and maintainer's point of view. = How to become a code owner? = Usually, the individual contributing a new major piece of code to the project naturally becomes the code owner for this module. Such a major contribution is expected to be accompanied by an update to the list of code owners. It is strongly frowned upon to contribute a major piece of code and not nominate yourself (or someone you know and work with) as the code owner for it, as then there is no one to look after this module with the right expertise. For existing code that already has a code owner (or several), if you wish to help and become an additional code owner for this module, you may propose your candidature. To do this, add yourself to the list of code owners and submit the resulting patch for review, adding all other code owners for this module as reviewers. At least 2 existing code owners (or 1 if there is a sole code owner) must approve it and none of them must veto it. = How to become a maintainer? = Maintainers are elected by their peers on a meritocracy basis. To be assigned as a maintainer, you need to satisfy the criteria listed in the code of conduct of the project. This typically covers things like: - Being an active member of the project for a minimum duration. - Having contributed a substantial number of non-trivial and high-quality patches and/or - Having reviewed a substantial number of non-trivial patches, preferably in the generic layer, with high-quality constructive feedback. - Behaving in a professional and polite way, with the best interests of the project at heart. - Showing a strong will to improve the project and to do the right thing, rather than going for the quick and easy path. - Participating in design discussions on the development mailing list. - Having appropriate bandwidth to deal with the workload. = Life Cycle of a Patch - From submission to integration in the tree = All the above roles play a part in the life cycle of a patch. This section outlines who needs to do what in order for a patch to be approved and merged. When posting a new patch for review, a contributor (and patch author) must choose his reviewers. A patch must be approved by: - At least 1 code owner for each module modified by the patch. - At least 1 maintainer. Thus the patch author should put as reviewers the right people to reach this target. They may add more, and they may also add some non-code owners if they think they might provide useful input (because of their past work and experience for example). Appropriate code owners might be found by looking at the list of code owners for the project. Similarly for maintainers. For additional non-code owners, it is also useful to run a "git blame" on the source code to find out who's been recently working on this area of the code. The patch author is responsible for addressing all review comments from all code owners and maintainers, until they approve the patch. Addressing a comment might mean 2 things, depending on the nature of the comment: - Questions are addressed by providing an answer. - Suggestions can be addressed in one of 3 ways: - Adopt it immediately and post the reworked version. - Defer it to a subsequent change. The patch author is then responsible for making sure it gets done at some point in the future. - Disagree and provide additional info to explain your approach. Once a patch has been approved by all relevant code owners and maintainers, it can be merged. It is important to note that the patch author is responsible for driving his review. In particular, they should not expect that people will review their patches if they don't request them to do so. On the other hand, once someone has been asked to review a patch, they should do so in a timely manner, or if they don't have the bandwidth to do it at this moment in time, they should say it explicitly. In this case, the patch author should look for another appropriate reviewer. = Patch Contribution Guidelines = Each project has some patch contribution guidelines that every contribution must adhere to. Some examples follow: - The code compiles without any warnings on all supported tool chains. - The code meets the project's code quality criteria. - The documentation has been updated accordingly. - The continuous integration (CI) system has been updated accordingly (or will shortly be after the code updates). - The code has been tested as per the testing requirements of the project. In particular, it passes all CI tests, does not introduce any regression and does not break existing code. In the case where the required tests have not been integrated in the CI just yet, additional targeted testing has been locally conducted and details of that (list of tests and results) have been made available. These apply as much to generic code as to platform code. = Platform Support Life Cycle = Maintaining a platform essentially consists in keeping up to date the following items: - Documentation. Platform documentation should typically describe the hardware, what features in the project are supported on this platform, how they are configured and how to use them. - Build support. The project should be able to build all supported combinations for the platform using the supported range of compilers. - Continuous integration (CI) system. Proper testing for this platform should be deployed in the CI and maintained over time. This is a lot of work and it would be unreasonable to expect that in an open source project all platforms are supported forever. The following stages of a platform's life cycle are proposed as a way of managing that. == Fully Supported == A fully supported platform needs to meet the following requirements: - It builds with all supported versions of toolchains (or there are tickets tracking known issues). - It builds all configurations supported by this platform (or there are tickets tracking known issues). - All supported configurations are tested in the CI and all tests pass. - The documentation is up to date. - There is (at least) one active code owner for this platform. == Limited Support == The platform is supported to some degree but not every criteria of a "fully supported' platform is satisfied. For example, some build configurations fail, or some tests fail, or the documentation is not up to date, or the platform does not have a fully functional, complete CI loop just yet. What does not work will be disabled in the CI to keep it healthy and useful to everyone. Note that if the failures are tracked through tickets, that is a valid condition to preserve the platform in "Fully supported" state while they are being investigated. == Orphan == The platform has no active code owner but otherwise satisfies all criteria of a "fully supported" platform. Orphan platforms will be moved to "Out of date" status when the build/testing breaks. == Out of date == Some elements are broken (build/tests/documentation). If the situation does not improve, it will become "deprecated" after some time. == Deprecated == The platform is no longer supported. It could be because it has reached its end of life or there is a lack of interest in the community. Deprecated platforms may be removed from the codebase in future == Exclusions and complementary notes == There are things we thought about but did not include in this proposal on purpose. These are listed below for completeness. - We will most likely need a deviation to the code review process for patches that need to be fast-tracked. This should only apply in some circumstances and will depend on the nature of the patch. - The code of conduct mentioned in the document is not defined yet. TF-A and TF-M might define different guidelines that fit them. - In the future, we'll most likely need to organize "key signing" type of events to enforce the legitimacy of maintainers and code owners to the tools, i.e. to prove that they really are who they claim to be and protect our tree from user identity spoofing. - The list of code owners and maintainers for the project might be provided in a file in the source tree. A common file for both is likely to be the more convenient solution. In the future, it might be worth developing some tools (or reusing existing ones if the license is compatible) to assist developers (for example suggesting specific code owners or maintainers based on the nature of the patch). - It is arguable whether it is a good idea to allow some patches to be entirely self-reviewed (in the case of an individual being both the relevant code owner and maintainer). Self-reviewing comes at risk, as it's usually easier to miss things when it's your own patch. A better alternative might be to mandate a different maintainer to approve the patch in this case. - For both TF-A and TF-M, the CI system is not quite ready to be extended by the community. Thus, some of the guidelines around adding the required level of testing in the CI cannot be applied right now. We hope the situation will quickly evolve, which is why it's been included in this proposal nonetheless.