Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Feature] 为 RTT 建立 maintainer 机制 #9903

Open
unicornx opened this issue Jan 10, 2025 · 19 comments · May be fixed by #9913
Open

[Feature] 为 RTT 建立 maintainer 机制 #9903

unicornx opened this issue Jan 10, 2025 · 19 comments · May be fixed by #9913
Assignees
Labels
Doc This PR/issue related with documents

Comments

@unicornx
Copy link
Contributor

unicornx commented Jan 10, 2025

Describe problem solved by the proposed feature

参考其他开源项目,建立maintainer负责机制:
好处:

  • 让每一位社区开发者明确知道 RTT 代码仓库中的代码是有人维护的,提交 PR 时也方便找到对应的 maintainer 进行 review
  • 对于一个没有 maintainer 愿意负责的代码,RTT 仓库有义务及时进行清理,目前 RTT 仓库中存在了太多的历史包袱,这不仅导致 RTT 的代码仓库过于臃肿,譬如 bsp 部分就有 2.3 G 的大小。由于没有建立维护机制,这些无人看护的代码反过来反而导致一些应该可以进行清理和优化的工作难以开展。因为开发者很难对所有涉及的代码,特别是 bsp 部分进行完整的测试。
  • 建立 maintainer 机制,对于开源贡献者也是一种荣誉激励。便于社区的良性发展。

Describe your preferred solution

建个maintainer的文件,再加一个脚本,可以根据 patch 涉及修改的文件找到对应的 maintainer。

剩下的就需要社区一起来推动了,愿意贡献的人可以主动报名承担 maintainer 的角色。

基于 maintainer 的信息,可以后继展开以下工作:

  • 通过 PR 自动找到适合的 reviewer
  • 找到那些无人认领的模块代码,特别是 bsp 部分,进行适当的清理。
  • ...... (待补充)

Describe possible alternatives

No response

@unicornx unicornx added the Doc This PR/issue related with documents label Jan 10, 2025
@unicornx unicornx self-assigned this Jan 10, 2025
@unicornx
Copy link
Contributor Author

我会先提交一个 rfc 的 pr,建个 maintainer的文件,再加一个脚本,可以根据 patch 涉及修改的文件找到对应的 maintainer。

@supperthomas
Copy link
Member

[GITHUB] PR代码reviewer分发机制
https://club.rt-thread.org/ask/article/fd8064c41022e458.html
希望有帮助。

@supperthomas
Copy link
Member

@supperthomas
Copy link
Member

目前BSP大小:2.3G
以下是git clone下下来之后的大小:

du -h --max-depth=1 | sort -hr
3.3G    .
2.3G    ./bsp
905M    ./.git
53M     ./components
22M     ./documentation
4.7M    ./libcpu
1.9M    ./examples
752K    ./src
692K    ./.github
664K    ./tools
176K    ./include
20K     ./.gitee
12K     ./.hooks
12K     ./.devcontainer

BSP下面的大小:

root@codespaces-079fc4:/workspaces/rt-thread/bsp# du -h --max-depth=1 | sort -hr
2.3G    .
660M    ./stm32
646M    ./nxp
127M    ./renesas
102M    ./hc32
67M     ./microchip
64M     ./nuvoton
52M     ./hpmicro
46M     ./nrf5x
41M     ./gd32
35M     ./n32
33M     ./Infineon
32M     ./allwinner
30M     ./at32
28M     ./essemi
23M     ./efm32
22M     ./wch
22M     ./bouffalo_lab
21M     ./apm32
19M     ./rockchip
16M     ./ht32
15M     ./samd21
9.4M    ./fujitsu
8.2M    ./msp432e401y-LaunchPad
8.1M    ./airm2m
7.4M    ./raspberry-pi
7.1M    ./phytium
7.1M    ./mm32l3xx
7.1M    ./mm32f103x
7.0M    ./tm4c123bsp
6.2M    ./tm4c129x
6.1M    ./yichip
6.0M    ./xplorer4330
6.0M    ./tae32f5300
6.0M    ./simulator
5.9M    ./synwit
5.9M    ./rv32m1_vega
5.8M    ./mm32f327x
5.6M    ./mm32l07x
5.3M    ./lm4f232
5.3M    ./lm3s8962
5.2M    ./lm3s9b9x
4.6M    ./avr32
4.4M    ./mm32
4.3M    ./hc32l136
4.2M    ./hk32
3.7M    ./zynqmp-a53-dfzu2eg
3.7M    ./ti
3.7M    ./smartfusion2
3.7M    ./hc32l196
3.5M    ./n32g452xx
3.4M    ./ft32
3.4M    ./frdm-k64f
3.2M    ./rx
3.2M    ./bluetrum
3.2M    ./acm32
2.9M    ./Vango
2.8M    ./maxim
2.7M    ./cvitek
2.6M    ./apollo2
2.5M    ./fm33lc026
2.2M    ./raspberry-pico
2.2M    ./loongson
2.2M    ./core-v-mcu
2.1M    ./tkm32F499
2.0M    ./CME_M7
1.8M    ./rm48x50
1.5M    ./qemu-vexpress-a9
1.5M    ./hifive1
1.5M    ./at91
1.4M    ./ESP32_C3
1.3M    ./ft2004
1.2M    ./sparkfun-redv
1.1M    ./zynqmp-r5-axu4ev
1.1M    ./nv32f100x
1.1M    ./amebaz
836K    ./ck802
832K    ./nuclei
668K    ./w60x
528K    ./asm9260t
472K    ./sam7x
464K    ./dm365
456K    ./thead-smart
444K    ./k210
376K    ./tms320c6678
368K    ./bf533
360K    ./m16c62p
352K    ./upd70f3454
328K    ./juicevm
324K    ./allwinner_tina
308K    ./mini2440
292K    ./beaglebone
260K    ./qemu-virt64-riscv
240K    ./k230
224K    ./x86
204K    ./qemu-virt64-aarch64
136K    ./pic32ethernet
136K    ./mini4020
132K    ./synopsys
116K    ./mipssim
112K    ./bm3803
60K     ./sep6200
56K     ./nios_ii
52K     ./wh44b0
48K     ./taihu
28K     ./microblaze

@unicornx
Copy link
Contributor Author

目前BSP大小:2.3G

谢谢,我原来说有 6.5G 是不对的(算错了),我已经更正了 issue 原文的描述。

@unicornx
Copy link
Contributor Author

https://github.com/RT-Thread/rt-thread/blob/master/.github/CODEOWNERS

感觉这个已经有 maintainer 的雏形了,@SuperThomas 对我提的这个 issue 有没有什么进一步的建议和想法?

@supperthomas
Copy link
Member

https://github.com/RT-Thread/rt-thread/blob/master/.github/CODEOWNERS

感觉这个已经有 maintainer 的雏形了,@SuperThomas 对我提的这个 issue 有没有什么进一步的建议和想法?

可以把codeownner完善一下,感觉能达到需求,这个唯一不太好的地方就是里面的必须是有rtthread. reviewer权限的人才行,(不过reviewer也只能加这些人)。想法挺好的,可以一起维护codeownner即可。其他的人员可以通过assign即可

@unicornx
Copy link
Contributor Author

unicornx commented Jan 11, 2025

@supperthomas 有几个问题:

可以把codeownner完善一下,感觉能达到需求

是否需要另外新建一个 Maintainer 文件来保存 maintainer 和其负责的文件的 mapping 关系。CODEOWNERS 似乎也已经包含的这部分信息,本来我想象中的 Maintainer 文件除了 CODEOWNERS 中的信息,还会记录一些其他的信息,譬如 maintainer 的名字,联系邮箱地址等,一个例子就是类似 linux 下的 https://github.com/torvalds/linux/blob/master/MAINTAINERS。不过如果够用的话,我也不介意就直接使用 CODEOWNERS 。

这个唯一不太好的地方就是里面的必须是有rtthread. reviewer权限的人才行,(不过reviewer也只能加这些人)。

是否可以认为以后注册了 maintainer 的人我们都可以给他 rtthread.reviewer 的权限,这样就能自动被列出在 pr 的 reviewer list 上呢?

其他的人员可以通过assign即可

你的意思是不是说:如果上面这个想法做不到的话,我这里可以提供一个脚本,根据另一个 Maintainer 文件信息(包含了maintainer 和其负责的文件的 mapping 关系),再加上 pr 中实际修改的文件列出 maintainter 的 github id,这样有rtthread.reviewer 权限的人(譬如您或者 @Rbb666 等)就可以通过手动 assign 的方式把这个 pr 指派给他?我比较不确定的是这种手动指派的人是否也是需要有什么权限?我的意思是说例如 pr 中下图中是可以根据 “任意的合法” github id 指派吗?我这里好像不行,不过也许是我的权限不够高。
image

@supperthomas
Copy link
Member

reviewer只有对仓库有权限的用户才行。assigned可以任意用户。其他的你看,还需要什么,可以加功能。我意思是之前codeowner有的功能就可以省一些工作量

@Rbb666
Copy link
Member

Rbb666 commented Jan 14, 2025

这份PR初步实现了提交PR后,CI机器人帮助@负责模块的用户协助review,不依赖是否有仓库权限: #9913

@BernardXiong
Copy link
Member

后续需要尽可能把一些BSP的文件移除去,保持有限的几个bsp才行。

@Rbb666 可以首先基于stm32来做个样本,先把stm32的移除,至少得让stm32 hal库都放外面才是,太大了。

@Rbb666
Copy link
Member

Rbb666 commented Jan 14, 2025

后续需要尽可能把一些BSP的文件移除去,保持有限的几个bsp才行。

@Rbb666 可以首先基于stm32来做个样本,先把stm32的移除,至少得让stm32 hal库都放外面才是,太大了。

嗯嗯,ST以及其他家的HAL库逐渐查分成软件包形式的,之前满老师有做过一个样本:#8907

@unicornx
Copy link
Contributor Author

unicornx commented Jan 20, 2025

这份PR初步实现了提交PR后,CI机器人帮助@负责模块的用户协助review,不依赖是否有仓库权限: #9913

经过社区讨论后,打算复用 #9913 实现本 issue。具体的方案如下:

不采用 label 方式记录 maintainer 信息,而是在 RT-Thread 主仓下新建一个 MAINTAINERS.json 文件,文件中用于记录 maintainer 的相关信息,具体格式的例子如下:

{
    "tag": "stm32l496-st-nucleo",
    "path": "bsp/stm32/stm32l496-st-nucleo", 
    "owner": "Li Tao (supperthomas) <[email protected]>, Zhang Bingru (Rbb) <[email protected]>"
},
{
    "tag": "stm32",
    "path": "bsp/stm32",
    "owner": "Li Tao (supperthomas) <[email protected]>, Zhang Bingru (Rbb) <[email protected]"
},
{
    "tag": "kernel: memory",
    "path": "kernel",
    "owner": "Li Tao (supperthomas) <[email protected]>, Zhang Bingru (Rbb) <[email protected]"
},
......
  • tag: 用于描述一个关联
  • path:用于描述该关联涉及的文件所在目录,路径相对于 RTT 主仓根目录,目前仅支持到目录,后续是否拓展更多格式看试用效果。
  • owner:用于描述该关联的 maintainers,可以有多个,用 "," 分开。每个负责人(maintainer)的描述格式是 ”Name (Github-ID) “

CI 自动识别 PR 中改动涉及的文件,然后根据 MAINTAINERS.json 中建立的关联找到文件关联的 maintainers。提取出 maintainers 的 Github-ID 后在 PR 中填写 comment,并 @ 相关 maintainers。这样仓库的 owner 就可以根据相关信息手动 assign reviewer 了。

comment 的格式如下:

@superthomas 
Tag: stm32l496-st-nucleo
Please take a review of this tag

@unicornx
Copy link
Contributor Author

unicornx commented Jan 20, 2025

经过社区讨论后,打算复用 #9913 实现本 issue。具体的方案如下:
......

我对以上方案有几个问题,欢迎讨论和补充:

  • 我建议该方案需要兼容现有的 CODEOWNERS 机制。或者说现有的 CODEOWNERS 建议保留或者说新的 MAINTAINERS 机制是对现有 CODEOWNERS 机制的保留。因为现有 CODEOWNERS 可以认为优先级更高,因为它可以自动 assign reviewers,当然这些可以自动 assign 的 reviewers 相对少一些,并要具备对 github 仓库的写权限。而 MAINTAINERS 机制对 reviewers 的权限要求没有,但需要仓库的管理员根据 comment 中的 AT 信息手动 assign。

  • 对于 MAINTAINERS 文件中文件目录的子集和超集问题,我记得满老师 @mysterywolf 会上提了一下,但我笔记上感觉说不清楚,请满老师 @mysterywolf 补充。具体的 json 例子就是:

    {
        "tag": "BSP",
        "path": "bsp", 
        "owner": "A (a) <[email protected]>"
    },
    {
        "tag": "stm32 BSP",
        "path": "bsp/stm32/",
        "owner": "B (b) <[email protected]>"
    },

    如果改动了 bsp/stm32 下的文件,那么 reviewrs 的人 A 和 B 都有,是只要有一个 approve 就可以,还是说 B 为主,A 为辅?

  • PR 更新了,譬如 review 后重新提交,会怎么处理?会重新触发 feat: CI script assigns PR reviews based on the list of maintainers #9913 的处理吗?

@kurisaW
Copy link
Contributor

kurisaW commented Jan 20, 2025

@unicornx
Copy link
Contributor Author

@kurisaW 没看明白你的描述 :(,要不直接举几个例子吧:

  • case 1:第一次提交,新建 pr 后涉及 maintainer A 和 maintainer B,CI 机器人新增一条评论提及 A 和 B。修改后再次 commit,此时涉及 maintainer A,maintainer B 和 maintainer C。CI 机器人是什么行为?我期望是:原评论保留,同时新增一个评论,仅提及 C。

  • case 2:第一次提交,新建 pr 后涉及 maintainer A 和 maintainer B,CI 机器人新增一条评论提及 A 和 B。修改后再次 commit,此时涉及 maintainer A。CI 机器人是什么行为?我期望是:原评论保留,同时不会新增评论即可。

  • case 3:第一次提交,新建 pr 后涉及 maintainer A 和 maintainer B,CI 机器人新增一条评论提及 A 和 B。修改后再次 commit,此时涉及 maintainer A 和 maintainer C。CI 机器人是什么行为?我期望是:原评论保留,但会新增一个评论,仅提及 C。

  • case 4:第一次提交,新建 pr 后涉及 maintainer A 和 maintainer B,CI 机器人新增一条评论提及 A 和 B。修改后再次 commit,此时仅涉及 maintainer C。CI 机器人是什么行为?我期望是:原评论保留,但会新增一个评论,仅提及 C。

基本上涉及的 reviewer,我觉得只增不减的样子。

@kurisaW
Copy link
Contributor

kurisaW commented Jan 21, 2025

@unicornx 我测试了一下实际效果,但是发现了一些问题。

首先明确CI机制,PR的commit修改的文件具备tag属性;同时一个人同时可能会看护多个TAG属性。

因此我对你上面的例子再做出一些扩展说明:

case 1:第一次提交,新建 pr 后涉及到 TAG1(kernel)的修改 ,对应 maintainer A 和 maintainer B,CI 机器人新增一条评论提及 A 和 B。修改后再次 commit,此时涉及到 TAG2(libcpu)的修改,而 TAG2 对应 maintainer A,maintainer B 和 maintainer C。对于 maintainer A/B/C 来说,这里的文件维护职责归属于他们三个,所以应该再次新增一条评论(提及A、B、C三人),这里虽然说前面因为 TAG1, A和B已经被提到了一次,但是这两次修改所对应的看护职责不同(第一次为 TAG1,第二次为 TAG2),是否应该为此容许评论两次的存在?

  • 如果容许的话,下面是演示效果:
# 第一次commit
---------------------------------
@maintainer A @maintainer B
Tag: kernel
Please take a review of this tag
---------------------------------

# 第二次commit
---------------------------------
@maintainer A @maintainer B @maintainer C
Tag: libcpu
Please take a review of this tag
---------------------------------
  • 如果不容许,提示的效果如下(但是这种情况下,maintainer A/B 看起来会误以为他负责的仅仅是kernel,而实际上负责的是kernel和libcpu):
# 第一次commit
---------------------------------
@maintainer A @maintainer B
Tag: kernel
Please take a review of this tag
---------------------------------

# 第二次commit
---------------------------------
@maintainer C
Tag: libcpu
Please take a review of this tag
---------------------------------

这两种情况您会更倾向哪一种(我本人觉得第一种职责还是更能明确),或者有更好的想法?

@unicornx
Copy link
Contributor Author

unicornx commented Jan 21, 2025

这两种情况您会更倾向哪一种(我本人觉得第一种职责还是更能明确),或者有更好的想法?

目前存在几个比较麻烦的事情:

  • 无法约束 pr 提交人保证每个 pr 只涉及一个模块(这里就是 tag)
  • 无法约束 pr 提交人员保证每个 pr 的每次提交自行确保 rebase

所以我的建议是如果我们在每次触发 ci 检查时,无论这个 pr 中有多少个 commit,我们都能得到这个 pr 修改的全集。

这样我们就可以确保 ci 产生的 最新一条评论 中覆盖了这个pr 所涉及的所有 模块,也就覆盖了所有的 maintainer。

@kurisaW
Copy link
Contributor

kurisaW commented Jan 23, 2025

我这里提交了一份最新的PR:#9913

该CI自动化脚本可自动识别PR修改的文件列表,并根据仓库根目录下的MAINTAINERS文件,映射审核团所维护的path,从而分配审查

MAINTAINERS文件可参考:

下面是该脚本的具体功能说明:

  • 审核方式由github bot自动评论@提及,并且保证仅保留一份最新的机器人评论;
  • 支持动态检测PR内所有被修改、删除、增加的文件;支持被修改文件的递归目录检测;并根据每次最新PR所变更的所有文件内容的追踪,生成实时的审核内容及审查人任命;
  • 支持查看最新PR的审查情况,可根据CI机器人评论的Current Review Status部分查看;
  • 支持自动捕获被任命的评论信息(审核者需要评论 LGTM/lgtm 来告知审查完毕),CI会同步bot机器人的评论:Current Review Status更新审查状况;
  • 支持手动点击🔄 刷新状态来重新运行CI,以此来获取最新的审查情况;
  • 支持一键展开被修改的文件详情,方便定位审查内容。

下面是一个演示效果,也可以通过下方链接查看实际效果:

需要注意的是,运行这个脚本需要熊大帮忙添加一个token,要求具备下面这几个权限:

      issues: write
      pull-requests: write
      contents: read

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Doc This PR/issue related with documents
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants