Hi Tomek, thanks for taking the lead on summarizing the changes so that we can vote them I would propose that we refine the review requirement and demand less reviews for boards and areas that have limited scope while increasing the number for common areas that have a wider scope : ex arch, drivers, frameworks. In my opinion we should mandate HW testing and community discussion for NuttX core, scheduler or areas that impact everybody.
Best regards Alin ________________________________ Från: raiden00pl <raiden0...@gmail.com> Skickat: den 6 februari 2025 09:28 Till: dev@nuttx.apache.org <dev@nuttx.apache.org> Ämne: Re: NuttX Code Quality Improvement 2025Q1 > Perhaps I would limit this to larger commits only (and all affecting Build / > Kernel / common arch code of course). GitHub bot tells you the size of the > pull request, so we can draw some line according to that. And maybe it could > also automatically > Perhaps I would limit this to larger commits only (and all affecting Build / Kernel / common arch code of course). GitHub bot tells you the size of the pull request, so we can draw some line according to that. And maybe it could also automatically set the mandatory number of reviewers. This way simple changes/fixes/additions to BSPs or MCU drivers would be handled faster. +1. Not all PRs have the same importance and impact. For PR that has a high impact on other users, 4 reviewers make sense. For PR that has low impact, addressing new features or newly developed architectures this is overkill. I could list more cases where such an approach doesn't make sense considering the resources we have. We'll end up being flooded with PRs without approvals, which will discourage both contributors and reviewers. Using github labels in this case can help, but the automation of this is very limited by the tool. It's not really possible to create complicated dependencies for labels, or I haven't figured out how to do it. We can introduce a new label "4 approvals", but it's assignment will have to be done manually by the PR creator or reviewer. We could also add labels for individual chips and separate them from labels for code common to the architecture (like "Arch: arm" and "Arch: STM32"), this would greatly help with judging the importance of PR. Unfortunately, implementing this is a long and boring work. > By Architecture, do you mean common architecture code or even MCU specific code? I am not sure if describing every change to a single MCU on a mailing list is beneficial, I am afraid it will just flood the list and no one will read it. GitHub issues seem more suitable for this. +1. Informing about breaking changes in the common code of the architectures - YES. Informing about breaking changes in stable chips that are used by users (like stm32) - YES. All other cases - probably NO. czw., 6 lut 2025 o 08:45 Michal Lenc <michall...@seznam.cz> napisał(a): > Hi Tomek, > > I think most of the points make sense, just few notes to some of them. > > > * Number of required code reviews should be increased from 2 to 4. > > This will ensure cross-checks and filter out faulty changes. > > Perhaps I would limit this to larger commits only (and all affecting > Build / Kernel / common arch code of course). GitHub bot tells you the > size of the pull request, so we can draw some line according to that. > And maybe it could also automatically set the mandatory number of > reviewers. This way simple changes/fixes/additions to BSPs or MCU > drivers would be handled faster. > > > * If change touches Build / Kernel / Architecture it must be presented > > and discussed on the mailing list with the community first. PR may be > > created with clear indication it is for discussion and marked as draft > > not to be "accidentally merged". > > By Architecture, do you mean common architecture code or even MCU > specific code? I am not sure if describing every change to a single MCU > on a mailing list is beneficial, I am afraid it will just flood the list > and no one will read it. GitHub issues seem more suitable for this. > > > Also it will be nice to have monthly online meetings just to talk > > things over and stay connected and synchronized. It should bring > > community closer together :-) > > Yes! We have to find a proper channel for this though. I think there > were efforts to have meetings on our Discord channel, but many people > don't like the platform (for understandable reasons) so it didn't last > for long. > > Best regards, > > Michal > > On 2/6/25 04:45, Tomek CEDRO wrote: > > Hello world :-) > > > > We have long discussions over the last months NuttX code quality and > > self-compatibility degradation. I think open discussion in this area > > as for 2025Q1 can be constructive. Please join the discussion :-) > > > > After we have a good understanding and agreement on selected points we > > should update Contributing Guidelines both for developers and > > reviewers. > > > > * Each PR _must_ adhere to requirements presented in Contributing > > Guidelines or be auto-rejected, that is present what this change does, > > why it is necessary, what if fixes and how, what is the impact, how it > > was tested (build and runtime test logs mandatory!!). > > * Number of required code reviews should be increased from 2 to 4. > > This will ensure cross-checks and filter out faulty changes. > > * Reviews should come from independent organizations. > > * Single company commit, review, merge is not allowed. > > * Self commited code merge is not allowed. > > * Each commit message must adhere to above (i.e. mandatory and > > self-explanatory topic and body with description, both topic and > > description are mandatory, may be simple single sentence or bullet > > points) or it will be auto-rejected. Simple changes require simple > > description, complex changes more, potentially breaking changes or big > > changes require solid proof nothing gets broken. > > * Both PR description _and_ commit messages are important especially > > in case of problems. If something gets broken we will be able to > > understand what was the goal of change.. or just by reading the git > > log to know what changed why and how. Verification logs may be part of > > PR only as these will be too long for git log. > > * If change touches Build / Kernel / Architecture it must be presented > > and discussed on the mailing list with the community first. PR may be > > created with clear indication it is for discussion and marked as draft > > not to be "accidentally merged". > > * If change touches Build / Kernel / Architecture then build and > > runtime test (i.e. apps/ostest) logs from developer working on a real > > world hardware are mandatory! > > * If change touches Build / Kernel / Architecture build and qemu is > > not enough as it does not catch all problems visible on real world > > hardware as we saw recently. > > * We should implement zero trust approach to user provided testing. > > * It is commit author to provide real world hardware build and runtime > > logs that change does not break stuff. > > > > Also it will be nice to have monthly online meetings just to talk > > things over and stay connected and synchronized. It should bring > > community closer together :-) > > > > There were also other nice ideas, please join the discussion :-) > > > > Tomek > > >
