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
