Hi, What is the goal of lazy consensus? Faster merge? This is what we are > trying to prevent I think.
Sorry Sebastien, it seems you didn't read my considerations. It isn't about merging faster. Is about merging even after calling for help. If we have a feature (likely a new feature) that doesn't affect anyone and it's pending review for some time, and it didn't have 4 reviewers after *X* weeks (I proposed two weeks, If you find it too fast, we can propose more time. This is not the point), why not merge it if it has at least one independent reviewer? It happens especially for supporting some specific peripheral that nobody is using *yet.* Again, I think we should discuss situations when a PR may be eligible for a *Lazy Consensus*. It's about defining what can or not be eligible. PS: Sebatien, nothing you were worried about would be eligible. Thinks broke because they affect the whole system and this would be something not eligible for *Lazy Consensus *at all. Em ter., 11 de fev. de 2025 às 11:33, Sebastien Lorquet < sebast...@lorquet.fr> escreveu: > What is the goal of lazy consensus? Faster merge? This is what we are > trying to prevent I think. > > 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 increased > >> from 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 and > >>> breakages. > >>> > >> 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 on > >>>> Docs) that mentions the rule of the commit being as small as it can be > >>> *without > >>>> breaking 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 :D > >>> > >>> > >>>> About 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> PR > >>> updated > >>>> the 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 changes > >>> depend > >>>> on each other); > >>>> - It affects only a single chip (and it's expected that no one other > >>> than > >>>> the manufacturer or the developer that submitted it is willing to > review > >>>> it); > >>>> > >>>> *In this specific case, if the PR adheres to the other guidelines > >>> (proper > >>>> testing, 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 > >>> >
