search

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.37k stars 6.35k forks source link

Process: replacing CODEOWNERS with MAINTAINERS #38725

Open mbolivar-nordic opened 2 years ago

mbolivar-nordic commented 2 years ago

This issue tracks replacing the CODEOWNERS file with the MAINTAINERS file.

The CODEOWNERS file and MAINTAINERS files contain duplicate information. The labeler.yml file would also be obsoleted by using a MAINTAINERS file in combination with a script from @nashif which adds labels as well.

Issues to sort out and current status:

How do we handle "non-maintainer" code owners?

Issue summary: CODEOWNERS and MAINTAINERS serve different but related purposes today, and many people who belong in CODEOWNERS do not belong in MAINTAINERS as it is currently defined.

Status

Current thinking is to define a new "sub-areas" concept, that lets us split up areas large enough to deserve a maintainer -- like "the I2C subsystem" -- into smaller components -- like "the nRF TWIM I2C driver".

We want to extend the MAINTAINERS YAML schema to allow us to define sub-areas and their "owners". Details are still TBD. @stephanosio has agreed to work on this syntax.

Details

One file, or many?

Issue summary: do we want to continue to put everything in a single MAINTAINERS file, or do we want to have various files throughout the tree that let us distribute sub-area owners into related directories?

For example, should we have the nRF TWIM driver owner in MAINTAINERS, or in a new drivers/i2c/SUB-MAINTAINERS file (exact name of the "sub-area" files is up for bikeshedding).

Current status The current consensus of the group is towards a single MAINTAINERS file with a new "sub-areas" syntax (TBD) for tracking individual components of a subsystem or area and their owners. Multiple "sub-MAINTAINERS" files throughout the tree were proposed but a supermajority of participants did not want to go this way, mainly because it seemed easier to search through a single file than many.

How to do the move?

Issue summary: Delete CODEOWNERS and move forward, or audit CODEOWNERS to make sure we have coverage in MAINTAINERS?

Current status: delete and move forward, but then how do we ensure that MAINTAINERS.yml contains all collaborators from the CODEOWNERS file?

Details

*  @stephanosio: As per https://github.com/zephyrproject-rtos/zephyr/issues/38725#issuecomment-1055570100, there is no point in having duplicate information at two different places.

What other metadata do we want?

Issue summary having a YAML file for describing components like individual drivers opens up opportunities to maintain additional useful metadata about those components. What do we want to add here?

Current status TBD, but some ideas are:

nashif commented 2 years ago

new syntax proposal for the MAINTAINER file

- area: ARC arch
  collaborators:
  - abrodkin
  - evgeniy-paltsev
  - IRISZZW
  files:
  - arch/arc/
  - include/arch/arc/
  labels:
  - 'area: ARC'
  maintainers:
  - ruuddw
  status: maintained
  type: subsystem
- area: ARM arch
  collaborators:
  - carlocaione
  - galak
  - MaureenHelm
  - stephanosio
  - bbolen
  files:
  - arch/arm/
  - arch/arm/core/offsets/
  - include/arch/arm/aarch32/
  - include/arch/arm/
  - tests/arch/arm/
  labels:
  - 'area: ARM'
  maintainers:
  - ioannisg
  status: maintained
  type: subsystem
mbolivar-nordic commented 2 years ago

Process WG minutes:

evgeniy-paltsev commented 2 years ago

@abrodkin @evgeniy-paltsev @ruuddw - we need to copy ARC-related entries from CODEOWNERS to MAINTAINERS - as I remember we didn't maintain MAINTAINERS file for us and didn't add proper info to it.

mbolivar-nordic commented 2 years ago

Process WG minutes:

mbolivar-nordic commented 2 years ago

Process WG minutes:

galak commented 2 years ago

So if I understand what manifest.py#L425 is doing, this is not what I was asking for. I'm looking for something that would provide a class and functions that scripts that want to parse the MAINTAINERs files could be build around.

See:

https://github.com/zephyrproject-rtos/zephyr/blob/9cb94df57a940a4d6afc1d2202a1bbaa565420ec/scripts/ci/get_twister_opt.py#L128

mbolivar-nordic commented 2 years ago

I'm looking for something that would provide a class and functions that scripts that want to parse the MAINTAINERs files could be build around.

OK, yeah, that is also supported in the same file, just lower down (in the Manifest class).

So you're basically asking for a library with generic parsing and validation support that is reusable elsewhere, right?

nashif commented 2 years ago

@galak when we discussed the various files, the unification and reuse of code idea was primarily around the way we parse the wildcards for paths in CODEOWNERS, MAINTAINERS.yml and tags.yaml. The yaml syntax can vary and I do not see how we can verify the syntax in a generic way.

mbolivar-nordic commented 2 years ago

Process WG minutes:

mbolivar-nordic commented 2 years ago

Process WG:

nashif commented 2 years ago

@mbolivar-nordic lets put this on the agenda tomorrow, there are things blocking this that need to be discussed.

nashif commented 2 years ago
  • Anas: need to figure out "sub-maintainers" files for instances of things (like individual boards or drivers). E.g. I2C could have a sub-maintainer file with Erwan for STM, Carles for Nordic, etc. Until we have this solution it's going to be a problem to remove it. This is the only remaining blocker. Should try to sort it out by the end of the year.

So, the boards is going to be an easy one, we just add maintainers list per board in the board yaml file we already have, a quick PoC shows we can even get those populated from the existing CODEOWNERs file and then integrate it in the script that does the labeling and adding reviewers.

The challenge is how to deal with everything else, like drivers. We can solve multiple problems with one solution I guess, the most extreme approach would be to have metadata files per subsystem where we can list all the information needed about instances, their maintainers, how they are tested and how they relate to other areas in Zephyr, so for example for I2C, this could look like this:

- name: Designware I2C Driver
  maintainers:
    - nashif
    - teburd
  type: driver
  subsystem: i2c
  files:
    - Kconfig.dw
    - i2c_dw.*
    - i2c_dw_registers.h
  tests:
    - tests/drivers/i2c
  platforms:
    - rpi_pico
    - intel_s1000_crb
- name: ESP32 I2C Driver
  maintainers:
    - ...
  type: driver
  subsystem: i2c
  files:
    - Kconfig.esp32
    - i2c_esp32.c
  tests:
    - tests/drivers/i2c
  platforms:
    - esp32
...
...

Based on the information above and without the need for too much parse magic we can:

The main challenge of the above is how we get there and how we continue to maintain those files and keep them in sync, I guess CI can enforce this in different way, similar to how enforce this with CODEOWNER now, if you add a file or a new driver that is not part of this "database", you will be asked to add it....

mbolivar-nordic commented 2 years ago
  • know what platforms are using this driver, there are probably other way to know this, ie.. DTS but this is as static as it can get and does not require us to deal with DTS parsing and matching drivers to platforms

There is another way that does rely on DTS that I think would be better than "platforms": devicetree compatibles.

Instead of:

  platforms:
    - foo
    - bar

I suggest using:

  dtcompatibles:
    - vnd,foo
    - vnd,bar

I.e. a list of compatibles that this driver supports.

I also think there should be a documented top-level kconfig option for enabling the driver.

mbolivar-nordic commented 2 years ago

Process WG:

hakehuang commented 2 years ago

I.e. a list of compatibles that this driver supports.

@nashif , how could we find the mapping from given file to dts defines? using scripts to check the fild compatbile defines? but it is only existing in .c file, what if we change the .h file? or common file?

nashif commented 2 years ago

@nashif , how could we find the mapping from given file to dts defines? using scripts to check the fild compatbile defines? but it is only existing in .c file, what if we change the .h file? or common file?

a question for @mbolivar-nordic I guess

erwango commented 2 years ago

There is another way that does rely on DTS that I think would be better than "platforms": devicetree compatibles.

@mbolivar-nordic I guess you don't think about dealing with dts at each CI run (need to compile all dts for each target and take into account tests/samples in which targets are modified by overlays ...). Is the plan to generate, via actions, a file similar to the one mentioned by Anas periodically ?

mbolivar-nordic commented 2 years ago

@nashif , how could we find the mapping from given file to dts defines? using scripts to check the fild compatbile defines? but it is only existing in .c file, what if we change the .h file? or common file?

a question for @mbolivar-nordic I guess

I'm planning on picking this back up at next week's process meeting so I am looking back at this. I'm sorry @hakehuang , but I don't understand your question. Could you please give me some example?

@mbolivar-nordic I guess you don't think about dealing with dts at each CI run (need to compile all dts for each target and take into account tests/samples in which targets are modified by overlays ...).

I don't know what you mean here either, @erwango. The example from @nashif is a list of drivers. Surely the compatibles that a driver handles do not change with overlays being applied by tests/samples, so I don't get what you're asking about.

hakehuang commented 2 years ago

I'm planning on picking this back up at next week's process meeting so I am looking back at this. I'm sorry @hakehuang , but I don't understand your question. Could you please give me some example?

my question is how could we find the target application when only .h source is changed? or subsystem code is changed, which does not mapping to all supporting platform for given test application.

mbolivar-nordic commented 2 years ago

.h source where? Sorry, could you please give me an example of some real files in zephyr?

hakehuang commented 2 years ago

.h source where? Sorry, could you please give me an example of some real files in zephyr?

for example if I modify the 'zephyr\subsys\net\l2\ethernet\gptp\gptp_md.h', all impacted platfroms shall be built against, and how can I get the mapping relationship?

mbolivar-nordic commented 2 years ago

Process WG:

erwango commented 2 years ago

I don't know what you mean here either, @erwango. The example from @nashif is a list of drivers. Surely the compatibles that a driver handles do not change with overlays being applied by tests/samples, so I don't get what you're asking about.

My point was filling and keeping such file is tedious. Can't it be automated ?

mbolivar-nordic commented 2 years ago

Can't it be automated ?

Not in any way that I've discovered.

mbolivar-nordic commented 2 years ago

for example if I modify the 'zephyr\subsys\net\l2\ethernet\gptp\gptp_md.h', all impacted platfroms shall be built against, and how can I get the mapping relationship?

OK, so if some file foo.c uses something from 'zephyr\subsys\net\l2\ethernet\gptp\gptp_md.h', you want to know that you need to rebuild foo.c when gptp_md.h changes, right?

This was the original question though:

@nashif , how could we find the mapping from given file to dts defines? using scripts to check the fild compatbile defines? but it is only existing in .c file, what if we change the .h file? or common file?

This original question seems to be about something different, not the same as the example. What "dts defines" do you mean?

hakehuang commented 2 years ago

OK, so if some file foo.c uses something from 'zephyr\subsys\net\l2\ethernet\gptp\gptp_md.h', you want to know that you need to rebuild foo.c when gptp_md.h changes, right?

yes.

mbolivar-nordic commented 2 years ago

Process WG:

Where to track module maintainers?

canopennode
civetweb
cmsis
edtt
fatfs
hal_altera
hal_atmel
hal_cypress
hal_espressif
hal_gigadevice
hal_infineon
hal_microchip
hal_nordic
hal_nuvoton
hal_nxp
hal_openisa
hal_quicklogic
hal_rpi_pico
hal_silabs
hal_st
hal_stm32
hal_telink
hal_ti
hal_xtensa
libmetal
liblc3codec
littlefs
loramac-node
lvgl
lz4
mbedtls
mcuboot
mipi-sys-t
nanopb
net-tools
nrf_hw_models
open-amp
openthread
segger
sof
tflite-micro
tinycbor
tinycrypt
TraceRecorderSource
trusted-firmware-m
tf-m-tests
psa-arch-tests
zcbor
zscilib

Template entry in MAINTAINERS

"West project: WEST_YML_PROJECT_NAME":
    status: ...
    maintainers:
        - github-user-name-1
        - ...
    collaborators:
        - ...
    files:
        - [optional list]
    labels:
        - [optional list]
nashif commented 2 years ago

for example if I modify the 'zephyr\subsys\net\l2\ethernet\gptp\gptp_md.h', all impacted platfroms shall be built against, and how can I get the mapping relationship?

OK, so if some file foo.c uses something from 'zephyr\subsys\net\l2\ethernet\gptp\gptp_md.h', you want to know that you need to rebuild foo.c when gptp_md.h changes, right?

if gptp_md.h changes, you do not need to know anything about DTS, you just run the tests that verify this specific subsystem, and nothing else, and the tests should be aware of which platforms to run on. I think this is orthogonal to the issue of a driver being changed which requires us to identify the hardware to build and run on.

mbolivar-nordic commented 2 years ago
  • @mbolivar-nordic to propose initial maintainers for each project in the above list, send a PR to MAINTAINERS

https://github.com/zephyrproject-rtos/zephyr/pull/44614

mbolivar-nordic commented 2 years ago

Process WG:

danieldegrasse commented 2 years ago

@nashif Just wanted to call out a specific use case where CODEOWNERS falls short for boards: the faze board, which was contributed by @simonguinot and @mbittan, is a board designed and maintained by Seagate. They should be pulled in as code owners on any file changes that relate to that board, but GitHub will not request their review (see #44843, where review had to be manually requested). According to the GitHub docs:

The people you choose as code owners must have read permissions for the repository. When the code owner is a team, that team must be visible and it must have write permissions, even if all the individual members of the team already have write permissions directly, through organization membership, or through another team membership.

In the case of Zephyr, It seems that contributors who are not members of Zephyr on GitHub won't be added to the review list for files they are the code owner of.

mbolivar-nordic commented 2 years ago

Moving status to on hold since it's on my plate but I haven't touched it yet to add I2C driver maintainers. Will move back to In Progress when I have a PR.

stephanosio commented 2 years ago

https://github.com/zephyrproject-rtos/zephyr/pull/46373 and https://github.com/zephyrproject-rtos/zephyr/pull/46380 implemented a script and workflow to automatically assign the reviewers, assignee and labels based on the content of the MAINTAINERS.yml.

With that, we are now a step closer to replacing CODEOWNERS with MAINTAINERS.yml; we just need to make sure that the MAINTAINERS.yml is not missing any entries from the CODEOWNERS.

mbolivar-nordic commented 2 years ago

Process WG:

We'll put this on hold until next week's meeting at the latest, or whenever @nashif has a chance to comment either on Discord or in this issue.

mbolivar-nordic commented 2 years ago

Process WG:

mbolivar-nordic commented 2 years ago

Process WG: revisit next week

marc-hb commented 2 years ago

it's harder to follow changes to different files. if I want to look up changes to assign them different GitHub roles, it's harder to get a bird's eye view.

git log -p '*/MAINTAINERS' shows changes to all MAINTAINERS files across the tree. Not much harder.

What I find missing in the giant linux/MAINTAINERS approach is... a bird's eye view. No way to zoom in and out. Searching is impractical when trying to match a path: which subdir to look for? linux/MAINTAINERS itself asks to try subdirs one by one. I suspect linux/MAINTAINERS a write-only database that is never read; readers always use the script.

Good luck trying to do this sort of filtering with a single giant file: git log --oneline --stat *bluetooth*CMakeLists.txt (it shows changes in drivers/, subsys/, tests/,...)

mbolivar-nordic commented 2 years ago

Process WG: moved back to on hold; @stephanosio is busy

marc-hb commented 1 year ago

Will it be possible to unsubscribe from some pull requests? Not sure what the current status is now but "zephyrbot" keeps re-adding you after every force-push. I guess because a force-push can include new changed files but that's not the most common case.

dleach02 commented 1 year ago

I've run into a situation where an SOC/Board was contributed to Zephyr from a different business unit in NXP. Now that person wants to be the maintainer of the relevant files and be automatically pulled into PR reviews if those files are touched. He initially had updated the codeowners file to associate with the files but that apparently doesn't completely work unless he gets put into the MAINTAINERS file. I'm going to do that but now I'm getting too many collaborators at the top level. Which seems to imply needing some hierarchy in the MAINTAINERS file.

keith-zephyr commented 2 months ago

@nashif - suggests moving CODEOWNERS content into the MAINTAINERS file. @dleach02 what is the burden of keeping CODEOWNERS? @nashif duplication of information and the file isn't maintained. @fabiobaltieri GitHub uses CODE OWNERS for assigning reviewers. Zephyr bot only uses MAINTAINERS @jfischer-no Github uses CODEOWNER to display the "owner shield" on PRs. @nashif - CODEOWNERS hasn't been maintained. @nashif to take ownership for removing CODEOWNERS.