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 >
