I promise you, if there is a loophole in the process, it will be exploited.
Better start strict and keep some room to adapt if we find problems, than allowing an absence of review from the very start.
I suggest no lazy consensus by default for now, with a case by case option to resolve blocked situations.
Sorry, I will not easily let go.Please, no automatic merges of badly reviewed changes. Specifically after some dead time, when everyone has forgotten what the problematic code is about.
This is exactly what has proven problematic in the past. Sebastien. On 11/02/2025 15:21, Tiago Medicci Serrano wrote:
Hi, just an additional comment: Lazy consensus is exactly what lead to absence of any testing and breakages. This *Lazy consensus *is just about the timeframe and the lack of 4 reviewers. The PR should be compliant with all other requirements (description, testing, documentation etc). My proposal for this *Lazy consensus:* 14. Number of minimum required code review votes should be increasedfrom 2 to 4. This will ensure cross-checks and filter out faulty changes.14.1 A PR may be eligible to be merged under the concept of *Lazy consensus* with the following conditions: - It affects only a single chip or board; - It didn't get the minimum of 4 reviewers after two weeks; - It was reviewed by at least one independent reviewer; - It adheres to all other conditions. The PR's author should: - After a week without any reviewers, send an e-mail to the mailing list asking for more people to review it; - Explain why the PR can't be split into smaller PRs (if applicable); - Ask for the independent reviewer to merge it after two weeks without any other reviewers; The (required) independent reviewer is responsible for checking if the PR matches the Lazy Consensus conditions and merging it. Em ter., 11 de fev. de 2025 às 11:05, Tiago Medicci Serrano < tiago.medi...@gmail.com> escreveu:Hi! I agree with almost every single point from Sebastien and Tomek: Lazy consensus is exactly what lead to absence of any testing andbreakages.This isn't entirely true. *Lazy consensus *is very bad if it affects a lot of people, but the Wi-Fi support for all Espressif chips was added with it (huge PR, few reviewers, doesn't affect any existing configuration). That's why I claim we should have exceptions. We talked about "lazy merge" already and there were opposing voices(including me). The safe default and current updated approach is NOT to merge unless author can prove its useful, working, tested, and top priority does not break stuff. Good cross-check reviews / verification is important.. even if it takes more time than expected. We should value quality more over rushing code changes. This is where all recent discussion came from. Am I wrong?I include myself on that too: I was opposed to it. But I've changed my mind about it when it meets this very specific conditions: 1. Code that affects only a single chip/vendor implementation; 2. Are hard to review because can't be split into smaller PRs; And, even the procedure for that should be well-documented: 1. PR should initially be treated according the general rules (4 independent reviewers); 2. After a week without enough reviewers, a call should be made on the mailing list, explaining why the PR can't be split into smaller PRs; 3. After two weeks without any reviewers, we could merge if the above conditions are met and we have at least one independent reviewer; See, I prefer it to let PRs without any reviewers because we don't have enough people to review it (or we are not interested in that). It prevents people from forking the project just to be able to develop their stuff: *we* *really would not like that*. The PR's author is still responsible for fixing some bugs if found in the future. Please, consider this situation. Em ter., 11 de fev. de 2025 às 10:35, Tomek CEDRO <to...@cedro.info> escreveu:On Tue, Feb 11, 2025 at 12:40 PM Tiago Medicci Serrano <tiago.medi...@gmail.com> wrote:Two points that seem to be missing in the current *[VOTE] NuttX Contributing Guidelines update 202502.* thread: About commits... We were discussing splitting some changes into different PRs, adding the commit's message and description, etc but I couldn't find anything(even onDocs) that mentions the rule of the commit being as small as it can be*withoutbreaking the build*.Thanks Tiago! We want to clearly state that breaking changes are not welcome :-) True, we already require that each PR should be as small as possible and change only one functional thing at time so we avoid too-many-things-at-once bundles that are hard to analyze and verify.. but also cannot break things and sometimes bundles are exceptionally acceptable.. I thought this is obvious.. but maybe we should add this point :DAbout required reviewers... I have a new proposal about "lazy consensus". Currently, nothing being voted refers to it, but it may apply to one specific situation: there are huge PRs that can't be broken on smaller parts and affect only a single chip or board. For instance, this <https://github.com/apache/nuttx/pull/12004> PRupdatedthe wireless drivers for ESP32-S3. As can be seen, only a single independent reviewer reviewed it. And it makes sense that no one was willing to review it: - It's big (and can't be split into smaller PRs because the changesdependon each other); - It affects only a single chip (and it's expected that no one otherthanthe manufacturer or the developer that submitted it is willing to review it); *In this specific case, if the PR adheres to the other guidelines(propertesting, logs, documentation etc), we could rely on a minimum of 1 (one) independent reviewer and, after two weeks, merge it.*This way we should merge all stalled PRs with only one review right away :-P We talked about "lazy merge" already and there were opposing voices (including me). The safe default and current updated approach is NOT to merge unless author can prove its useful, working, tested, and top priority does not break stuff. Good cross-check reviews / verification is important.. even if it takes more time than expected. We should value quality more over rushing code changes. This is where all recent discussion came from. Am I wrong? In case of pull 12004, I agree this is big, maybe should be split into smaller PRs (i.e. why remove RMT support when updating WIFI?), maybe build and runtime logs could be attached, but it was clearly stated all these changes needs to be bundled not to break stuff, so it was accepted, exceptional situations like this happen, but require additional caution and testing. Still additional reviews can be easily requested, right? It may take more time to verify but we will have solid code base :-) I have ESP32S3 now. I can get others if needed. For instance I recently also bough popular rpi-pico(-2-w) just to see it does not build. I am open to vendors providing free devkits for testing support. For instance STM is known to share their devkits for free, maybe Espressif could provide some new stuff too? :-) To sum up, we want to add additional point like this? Please update if necessary :-) Pull Requests should be as small as possible and focused on only one functional change. Different functional changes should be provided in separate Pull Requests. Remember that breaking changes are not welcome. Pull Requests must not break overall build, runtime, and compatibility, especially for other components. When changes must be bundled together in order to maintain functionality and self-compatibility, exception can be made, and this must be clearly stated there is no other way. -- CeDeROM, SQ7MHZ, http://www.tomek.cedro.info
