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
