zephyrproject-rtos / zephyr

Primary Git Repository for the Zephyr Project. Zephyr is a new generation, scalable, optimized, secure RTOS for multiple hardware architectures.
https://docs.zephyrproject.org
Apache License 2.0
10.43k stars 6.39k forks source link

Refine the maintainer role #60316

Open keith-zephyr opened 1 year ago

keith-zephyr commented 1 year ago

Action item from the 2023 F2F meeting.

Define the maintainer role in a way that it becomes clear that it goes beyond just reviews. Ensure no stagnation in some of the areas.

First step is to survey the current maintainers and gather data about responsibilities maintainers are currently fulfilling.

keith-zephyr commented 1 year ago

Summary from Process WG

Items to query from maintainers:

General consensus that multiple maintainers per area will help the project. This should not be done at the expense of quality - co-maintainers must agree to same set of responsibilities.


@kartben - Items to capture from survey. How much time is spent doing maintenance?
Can we support co-maintaining? Is this done now, what's the format look like. @fabiobaltieri - It would be good to set expectations for time to respond to PRs and RFCs that overlap. @fabiobaltieri - would like to see 2 names per area - but the project largely assumes one maintainer. zephyr-bot doesn't handle multiple maintainers. This has lead to PRs stalling because assignee hasn't responded, but a maintainer has approved. @fabiobaltieri - We should encourage maintainers/collaborators to mark status on github (OOO for instance). @keith-zephyr - can we do multiple assignees?
@fabiobaltieri - this could allow a CI check that blocks merging until an assignee approves. @fabiobaltieri - can we realistically expect to get multiple maintainers in all areas? @kartben - hardly any areas have multiple maintainers. @fabio - this is largely because of push by @nashif . @kartben - we need some sort of backup for maintainers that are offline. Collaborators doesn't really cover this. @gregshue - we have maintainers for sample applications. Is the role of sample application maintainers different than maintainers in tree? This is managed a module to Zephyr. Would like the project to have same requirements as the in tree. @gregshue - mcuboot as an example. Has a folder upstream that maintains compatibility with Zephyr main. mcuboot maintainer has to verify compatibility. The CI does build mcuboot as part of some samples. @gregshue - should modules have a responsibility to verify compatibility with latest Zephyr release and/or Zephyr main? @dleach02 - modules are part of the Zephyr ecosystem. We need to identify maintainers for each module so that modules are tested against Zephyr. Releases check some key modules are compatible. If there is no maintainer, the module will bitrot and likely should be identified for removal. @fabiobaltieri - Sees responsibilities are the same for subsystem vs module. @gregshue - Not all code in modules is relevant to Zephyr. Some support other RTOS - maintainers are responsible for portions that integrate with Zephyr. @gregshue - wants consistency between in tree and modules

@fabiobaltieri - can we have 2 maintainers with every area? @nashif - yes this a good plan. But we can't have too many in a given area. This is supported by the tooling. @dleach02 - do collaborators help offload maintainers? @fabiobaltieri - generally no. The assignee comes from the maintainer. @dleach02 - collaborators should be helping review. What's the point of collaborators if they aren't sufficient to merge? @nashif - maintainer must approve otherwise there's no difference between maintainer/collaborator. Typically there should be one or more collaborators to approve plus the maintainer. @dleach02 - does CI prevent merging without maintainers approval? @nashif - No. Release team generally knows which areas should have a maintainer review and wait on a case by case basis. @fabiobaltieri - Can we use multiple assignees, so that the release team can have more rigid rules about merging? @nashif - need to be careful about diffusion of responsibility if we use multiple assignees. @nashif - there is probably not consensus among current maintainers what their responsibilities. @nashif - shouldn't force co-maintainers. Anyone designated as a maintainer needs to go through the same qualification/ responsibility sign-off. @fabiobaltieri - Can we capture some metrics to help determine if maintainers/collaborators are effective? @fabiobaltieri - metrics could also help identify good candidates for maintainers/collaborators. @kartben - how do we make the survey results actionable?

fabiobaltieri commented 1 year ago

Hey I'm thinking it may be interesting to also ask the maintainers what they think about to tooling that they have available to get the job done. I suspect that the general vibe is that the current tooling is somewhat inadequate and inefficient, thinking that maybe this could be a good start to thinking about putting together some project tooling to make the maintainers life easier and the general process more efficient. That may also help in having a more uniform experience from a user perspective.

For example, IIUC we still can't give a person a link to get a list of the PR they're blocking. I'm imagining a situation where we could build something to aggregate the PR and commit data and obtain:

The usual disclaimer here is that the project mission is to build an RTOS, so it's not ideal to put energy into building development tools instead, but some proper monitoring here may help taking the right decisions.

gregshue commented 1 year ago

It looks like we already have a number of explicit requirements upon module maintainers, along with several recommendations/expectations. I found the following statements related to module maintainers (emphasis mine).

From https://docs.zephyrproject.org/latest/contribute/external.html:

The second way of integrating external source code into the project is to import the whole or parts of the third-party open source project into a separate repository, and then include it under the form of a module. With this approach the code is considered as being developed externally, and thus it is not automatically subject to the requirements of the previous section. ... Regardless of the mode of integration, external source code that is integrated in Zephyr requires regular ongoing maintenance. The submitter of the proposal to integrate external source code must therefore commit to maintain the integration of such code for the foreseeable future.

From Modules:

  • the contributor of the original changes in Zephyr is required to submit the corresponding changes that are required in module repositories, to ensure that Zephyr CI on the pull request with the original changes, as well as the module integration testing are successful.
  • the module owner has the overall responsibility for synchronizing and testing the module codebase with the zephyr main tree. This includes occasional advanced testing of the module’s codebase in addition to the testing performed by Zephyr’s CI. The module owner is required to fix issues in the module’s codebase that have not been caught by Zephyr pull request CI runs. ... Changes to the main branch of a module repository, including synchronization with upstream code base, may only be applied via pull requests. These pull requests shall be verifiable by Zephyr CI and mergeable. ... This policy also allows to run Zephyr CI, git lint, identity, and license checks directly on the set of changes that are to be brought into the module repository.

From Modules - Testing Requirements:

  • Certain external projects provide test suites that reside in the upstream testing infrastructure but are written explicitly for Zephyr. These tests may (but are not required to) be part of the Zephyr test framework. ...
  • shall be built and executed (for example in QEMU) as part of twister runs in pull requests that introduce changes in module repositories. ...

(edited to add) The purpose of integration testing is not to provide functional verification of the module; this should be part of the testing framework of the external project.

[Note] New modules, that are candidates for being included in the Zephyr default manifest, shall provide some level of integration testing.

From Modules - Twister (Test Runner):

  • To execute both tests and samples available in modules, the Zephyr test runner (twister) should be pointed to the directories containing those samples and tests. This can be done by specifying the path to both samples and tests in the zephyr/module.yml file.
nashif commented 1 year ago

It looks like we already have a number of explicit requirements upon module maintainers, along with several recommendations/expectations.

I am not really sure I am following. How does any of that relate to the maintainer role? This is general expecations from any contributor. There is nothing special about being a maintainer of a module.

gregshue commented 1 year ago

Yeah, you missed it. Let's try this again.

From (https://docs.zephyrproject.org/latest/contribute/external.html):

The submitter of the proposal to integrate external source code must therefore commit to maintain the integration of such code for the foreseeable future.

From Submitting changes to modules:

Once the module is merged, the revision will need to be changed either by the submitter or by the maintainer to the commit hash of the module which reflects the changes.

From Zephyr Project Roles - Maintainer:

Maintainers have the following rights and responsibilities, in addition to those listed for Contributors and Collaborators:

  • Right to set the overall architecture of the relevant subsystems or areas of involvement.
  • Right to make decisions in the relevant subsystems or areas of involvement, in conjunction with the collaborators and submitters.
  • Responsibility to convey the direction of the relevant subsystem or areas to the TSC
  • Responsibility to ensure all contributions of the project have been reviewed within reasonable time.
  • Responsibility to enforce the code of conduct.

From Contributing to Zephyr modules (emphasis mine):

Module owner: Each module shall have a module code owner. Module owners will have the overall responsibility of the contents of a Zephyr module repository. In particular, a module owner will:

  • coordinate code reviewing in the module repository
  • be the default assignee in pull-requests against the repository’s main branch
  • request additional collaborators to be added to the repository, as they see fit
  • regularly synchronize the module repository with its upstream counterpart following the policies described in Synchronizing with upstream
  • be aware of security vulnerability issues in the external project and update the module repository to include security fixes, as soon as the fixes are available in the upstream code base
  • list any known security vulnerability issues, present in the module codebase, in Zephyr release notes.

The module roles list does not include a "maintainer" role. These responsibilities fall to the required role of "module owner" and are assigned to initial module submitter. When looking at the rest of the responsibilities it seems "module owner" == "maintainer (module)".

From Maintaining the module codebase(emphasis mine):

The responsibility for keeping the module codebase up to date is shared between the contributor of such updates in Zephyr and the module owner. In particular: ...

  • the module owner has the overall responsibility for synchronizing and testing the module codebase with the zephyr main tree. This includes occasional advanced testing of the module’s codebase in addition to the testing performed by Zephyr’s CI. The module owner is required to fix issues in the module’s codebase that have not been caught by Zephyr pull request CI runs.

The above paragraphs identifies several specific maintenance activities (integration testing, advanced testing, issue fixing, regular synchronization with upstream counterpart, update Zephyr release notes) are assigned specifically for the module owner to perform.

As you see, the current Zephyr documentation already lists many things special about being a maintainer (owner) of a module. ;-)

nashif commented 1 year ago

As you see, the current Zephyr documentation already lists many things special about being a maintainer (owner) of a module. ;-)

just like a platform maintainer or an architecture maintainer or a driver maintainer each are "special" in their own way. This does change the overlall maintainer role definition and we do not need to go into different types of maintainers.

keith-zephyr commented 1 year ago

Summary from 2023-07-27

@nashif and @kartben to finalize survey based on notes below. @gregshue to draft language to include module maintainer responsibilities to general maintainer responsibilities.


@nashif and @kartben started working on maintainer survey. @nashif - to provide survey link @dleach02 - suggest follow on questions to see if maintainer actually understands role @keith-zephyr - suggestions for "How you monitor PRs". Github GUI: https://github.com/pulls/review-requested, scripts using Github CLI, other scripting using REST API @nashif - "Assigned" button from review-requested dashboard provides a better interface for maintainers specifically @gregshue - There are already responsibilities documented. Should we check them for additional questions to the survey. @gregshue - maintenance of modules and in tree subsystems need to apply uniformly. Module documentation already enumerates responsibilities that should be pulled into the general maintainer responsibilities. @gregshue - "Maintainers should ensure the code is secure". @gregshue to refine this language on this issue. @dleach02 - consolidating this info will allow us to de-dup some of the module documentation.

keith-zephyr commented 1 year ago

@teburd - how to handle disagreements between co-maintainers? @keith-zephyr Use defined escalation path (Arch WG and TSC). @fabiobaltieri - don't need a different process for maintainer disagreements vs PR author and maintainer disagreement. @teburd - an alternative would be primary/secondary maintainer @dleach02 - even with single maintainer, there needs to be an escalation path to potentially overrule the maintainer @nashif - there is the potential for multiple maintainers causing delays when they don't agree @fabiobaltieri - with multiple assignees, we need define the requirements to merge a PR. e.g. only one maintainer needed, not both

gregshue commented 1 year ago

Proposed changes to the Maintainer role description:

Change:

A Maintainer is a Collaborator who is also responsible for knowing, directing and anticipating the needs of a given zephyr source code area.

to

A Maintainer is a Collaborator who is also responsible for knowing, directing and anticipating the needs of a given zephyr source code area. The zephyr source code area covers in-tree and out-of-tree code integrated with Zephyr.

Add the following bullets:

keith-zephyr commented 1 year ago

@nashif - Waiting on @kartben to return from OOO before sending out the survey. @gregshue - proposed changes to change repository/external code to source code area. @nashif - does not agree with language for "code area". @gregshue - is the module owner responsible for fixes? @nashif - external modules are not maintained by Zephyr. @gregshue Module owner is already identified as the maintainer for code integrated with Zephyr. @nashif - we need agreement on source code area. @nashif - responsibilities proposed are too specific and detailed. Changes effectively repeat contribution guidelines into the maintainer responsibilities. @nashif - in favor of leaving module responsibiliies in the module area only. @nashif - module owner is not necessarily the same as the module maintainer. e.g. embedTLS module maintainer isn't a contributor/owner of the embedTLS source. @nashif - secure guidelines are a separate activity from maintainer responsibilities @nashif - problem is that areas are not maintained or not getting enough attention @keith-zephyr - what are the gaps? @nashif - overloaded maintainers, maintainers not aware of their responsibilities. Coverage for maintainers OOO. Maintainers not always driving PRs to mergable state. Need mechanisms for assigned maintainers for orphaned areas. @fabiobaltieri - can we do something at the project level to make maintainership more efficient. Adding responsibilities could just add to the burden. @keith-zephyr - propose tabling this issue until maintainer survey completed @gregshue - maintainer yaml file includes maintainers under modules and for code in the tree. We may need a new term if the responsibilities are different.

keith-zephyr commented 1 year ago

Process WG summary from 2023-08-02 Move this issue to on-hold until the maintainer survey is completed.

keith-zephyr commented 1 year ago

Process WG action items

@kartben - to provide link of the final maintainer survey to @kestewart for review.


@kartben presented a draft survey to the Process WG. @henrikbrixandersen - suggests adding a free-form text to respondents to expand on the questions. @gregshue - What's the difference between "embedded software" and "firmware" developers? From "What is your role" question. @henrikbrixandersen - agrees with collapsing the two options. @henrikbrixandersen - suggests a separate question regarding time spent on non-technical aspects for Zephyr. @nashif - this is already covered by the meeting attendance question. @kartben - suggests changing time commitment to hours spent instead of percentage. @henrikbrixandersen - what about surveying collaborators? @nashif - this is covered by the general developer survey. @jfischer-no - suggests other challenges for maintainers includes reviewing open issues. @henrikbrixandersen - suggests expanding question about asking for more maintainers and collaborators @nashif - more maintainers is meant to cover OOO gaps when primary maintainer not available

@nashif @henrikbrixandersen - Send survey to the existing maintainer mailing list. Need to sanitize the mailing list and make sure it's up to date.

@nashif suggests asking for maintainer name as an optional response.

keith-zephyr commented 7 months ago

@kartben, @nashif - maintainer survey was completed. What are the next steps here? Is this something you want to bring back up to the Process WG?

keith-zephyr commented 3 months ago

@nashif and @kartben to discuss and come up with next steps (if any).