Hi! * if after a week the pull request does not have the required approvals, > the issue is brought on the mailing list for further discussion.
Particularly, I don't like the idea of "lazy consensus" too. I think we can try to actively ask for more reviewers (in the list and on GH). I dont understand one thing. Why reviewer needs to be a committer from > start? I think someone should be reviewer in the first place and after > some time when things work fine a committer may be granted for the > reviewer :-) This could also motivate for reviews to be then promoted > to committer..? :- I bet this is more like a "tool" thing than a rule. I also would like to be able to ask for a developer (who isn't part of of the PMC neither a committer) to review that code. * 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". I prefer to keep all the related discussion on GH: if this is the tool we use, I think we should keep concentrating it there. If we "follow the rules", have at least 4 commits, tests on QEMU and real HW, I don't think it's necessary to bring it to the list. If it generated a discussion there, yes, we should bring it here to enable more people to participate (but the technical arguments should be primarily done on GH) Best regards! Em qui., 6 de fev. de 2025 às 13:43, Sebastien Lorquet <sebast...@lorquet.fr> escreveu: > Hello Alan, and Tomek, > > Is this a good rule? > > I have the feeling that any pull request could come with unexpected > problems, and if no one reviews it, it does not mean that the problems > are going away. > > As suggested by anchao in previous emails, this is what happened in the > spinlock issue. No one was interested, it was merged, and boom. Then, it > was brought to the list and then investigated. > > So based on this experience (it worked well) I would suggest: > > * if after a week the pull request does not have the required approvals, > the issue is brought on the mailing list for further discussion. > > I think that quality is more important than quantity. A blocked pull > request is less important than a broken release. > > And if a pull request is too complex for being reviewed by anyone, it is > possibly a bad one that should not be merged. > > What do you think? > > Sebastien > > > On 06/02/2025 16:08, Alan C. Assis wrote: > > Hi Tomek, > > > > You missed that rule: > > > > * If one person approves a PR and no other reviews or objections after a > > week, the person who approved the PR can assume a "lazy consensus" and go > > ahead and merge. > > > > BR, > > > > Alan > > > > On Thu, Feb 6, 2025 at 12:46 AM Tomek CEDRO <to...@cedro.info> 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 > >> > >> -- > >> CeDeROM, SQ7MHZ, http://www.tomek.cedro.info > >> >
