Hi all, My votes (with notes/commentary below):
1. +1 2. +0.5: See "Note On #2" below. 3. +1 4. +1 5. +1 6. +1 7. +1 8. +1 9. -1: See "Note On #9" below. 10. -0.5: See "Note On #10" below. 11. +1 12. +1 13. +0.5: See "Note On #13" below. 14. -1: See "Note On #14" below. 15. +1. 16. +1. 17. +1. "Note On #2": Sometimes we may need to override the Contributing Guidelines in special circumstances. Consider, for example, how nxstyle has given false positives on occasion, such as when we have to relax our capitalization rules because of 3rd party code we're interfacing with. So, I would suggest that while PRs _must_ adhere to the requirements, there should also be a way to relax this requirement if appropriate due to legitimate reasons. "Note on #9": It is probably not possible/feasible for a commit author to verify that a change doesn't break anything for anyone else. They probably don't have all the kinds of hardware in the world. Though I agree that we should require testing on real hardware, we should be very careful how we phrase the requirement for testing. Remember: We want to encourage contributions, so we have to make sure our expectations are within the realm of what's reasonably feasible. "Note On #10": I recommend to modify this rule: "Breaking changes require a discussion on the mailing list, followed by a [VOTE], and must have at least 3 +1 and no -1 to be allowed. Breaking changes that don't follow this process are not welcome. This is anything that alters......" Item #12 addresses this, and I voted +1 on item #12. "Note On #13": I agree that we need apps/ostest from more than one different architecture -- we should make a note here that if the commit author doesn't have different kinds of hardware available, they should be able to request testing from other members of the community. Everyone who performs testing should add their test results in a comment in the PR, and once there are enough successful tests, the PR can be merged. "Note On #14": We should not have a requirement of 4 reviewers for every change, but we _should_ require more reviewers for sensitive changes, and allow fewer reviewers for low risk changes. Examples of low risk: changes to documentation, changes in some obscure driver or board that affects very few people; these should be okay with just 1 reviewer. Examples of medium to high risk: changes to sched, changes to platform independent code that is heavily used, changes to low level arch; these should require more reviewers, and I would suggest that 3 reviewers should be enough. I am concerned that if we set the bar too high, PRs will get stuck for a long time and we'll accumulate a large number of PRs that will fall by the wayside. So we should try to strike a good balance. Cheers, Nathan On Thu, Feb 13, 2025 at 7:57 AM Filipe Cavalcanti <filipe.cavalca...@espressif.com.invalid> wrote: > > Hi all, here's my update on items 18 and 19 recently added. > > 18: +1 > Following suggestions from my colleagues, please also add documentation to > this. > > 19: +1 > Makes sense to me. I strongly support this as we don't have that many people > to review and approve. > We can't stop going forward, but this item can be reviewed in the future, > when more people are working in Nuttx. > > ________________________________ > From: Tiago Medicci Serrano <tiago.medi...@gmail.com> > Sent: Thursday, February 13, 2025 9:24 AM > To: dev@nuttx.apache.org <dev@nuttx.apache.org> > Subject: Re: [VOTE] NuttX Contributing Guidelines update 202502. > > [External: This email originated outside Espressif] > > My votes: > > 18. 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. > > +1. Documentation of a new feature should be provided in the same > PR. Documentation is provided in the `nuttx` repository, so if the PR > changes something that affects the current documentation, it should be > treated at the same PR. > > 19. 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 (no > > kernel/libs/upper-half drivers etc); > > - It implements a new feature (or app) that doesn't introduce > > any breaking changes or backward incompatibility; > > - It didn't get the minimum of 4 reviewers after two weeks (to > > be discussed); > > - At least one independent reviewer reviewed it; > > - It adheres to all other conditions. > > The PR's author should: > > - After a week (to be discussed) 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. > > +1. NuttX needs more features (new features) and more board/chip support. > New features don't break any existing implementation and may be eligible > for merging after at least one independent reviewer. > Some notes on that taking *Zephyr* - our main competitor- as an example. In > the last month, Zephyr had almost 400 authors and more than 2K commits ( > https://github.com/zephyrproject-rtos/zephyr/pulse/monthly), but the PRs > that implement new features don't require 4 reviewers to be merged. For > example: > > - https://github.com/zephyrproject-rtos/zephyr/pull/85525 > - https://github.com/zephyrproject-rtos/zephyr/pull/85374 > - https://github.com/zephyrproject-rtos/zephyr/pull/85342 > - https://github.com/zephyrproject-rtos/zephyr/pull/85284 > > NuttX had 57 authors and 344 commits ( > https://github.com/apache/nuttx/pulse/monthly). This is 7x less > authors/commits. We have limitations on what we can do trusting only on > people. Exceptions like *19* should exist otherwise *NuttX will die as a > distribution*. We should really focus our attention on automated testing > (than being to rigid depending only on people for new features). Zephyr, > with 7x more authors, doesn't rely on people that much. We really need more > features and chip/board support and *Lazy Consensus* is important here. > > Em qui., 13 de fev. de 2025 às 05:15, Sebastien Lorquet < > sebast...@lorquet.fr> escreveu: > > > you might be right, or not, I cant tell. > > > > To be honest, I find this specific rule to be very complex. it's not > > easy to understand its effect (I did not apparently) > > > > so it should be reworded at least. > > > > I suggest still not to integrate it as-is. > > > > Sebastien > > > > > > On 12/02/2025 19:21, Tiago Medicci Serrano wrote: > > > Hi, > > > > > > Again, Sebastien, read it *carefully*. It seems you are not willing to do > > > it. > > > > > > It doesn't bypass anything: > > > > > > - either the submitter still cares and will yell at people to get it > > >> approved > > > > > > * My proposal says explicitly about this:* > > > > > > The PR's author should: > > >> - After a week (to be discussed) without any reviewers, send > > an > > >> e-mail to the mailing list asking for more people to review it; > > > > > > *Continuing:* > > > > > > I dont want to see unsupervised auto commits. > > > > > > > > >> The conditions listed in this point (no breakage, execution of tests, > > >> etc) HAVE TO be verified. > > > > > > Neither do I! That's why there is the following rule: > > > > > > *The (required) independent reviewer* is responsible for > > checking > > >> if the PR matches the *Lazy Consensus* conditions and merging it > > >> > > > *THE INDEPENDENT REVIEWER *(not the PR's author) is responsible for > > > checking and merging it. It requires *AT LEAST* one independent reviewer. > > > > > > So, if we were not able to have 4 reviewers *and* already asked for help. > > > Then we can try to check if it's *eligible*. > > > > > > Em qua., 12 de fev. de 2025 às 15:10, Alan C. Assis <acas...@gmail.com> > > > escreveu: > > > > > >> The goal is not to automatically merge PR, but only to avoid that some > > PRs > > >> that don't reach the maximum number of reviews be allowed to be merged. > > >> > > >> Typically, those who are most eager to impose new rules are not the > > same as > > >> those who are subject to them! > > >> > > >> BR, > > >> > > >> Alan > > >> > > >> > > >> > > >> > > >> > > >> > > >> On Wed, Feb 12, 2025 at 2:55 PM Sebastien Lorquet <sebast...@lorquet.fr > > > > > >> wrote: > > >> > > >>> Again, I will vote against this. Not that it matters if a majority > > wants > > >>> to approve it. > > >>> > > >>> This is a bypass of all other rules we're trying to enforce. > > >>> > > >>> If such situation arise, there are two cases: > > >>> > > >>> - either the submitter still cares and will yell at people to get it > > >>> approved > > >>> > > >>> - either it will be dropped entirely. > > >>> > > >>> I dont want to see unsupervised auto commits. > > >>> > > >>> The conditions listed in this point (no breakage, execution of tests, > > >>> etc) HAVE TO be verified. > > >>> > > >>> Sebastien > > >>> > > >>> > > >>> On 12/02/2025 17:14, Tiago Medicci Serrano wrote: > > >>>> Hi! > > >>>> > > >>>> Thanks, Tomek ;) > > >>>> > > >>>> So, rewriting 19: > > >>>> > > >>>> *19.* 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 (no > > >>>> kernel/libs/upper-half drivers etc); > > >>>> - It implements a new feature (or app) that doesn't > > >> introduce > > >>> any > > >>>> breaking changes or backward incompatibility; > > >>>> - It didn't get the minimum of 4 reviewers after two weeks > > >>> (to be > > >>>> discussed); > > >>>> - At least one independent reviewer reviewed it; > > >>>> - It adheres to all other conditions. > > >>>> The PR's author should: > > >>>> - After a week (to be discussed) 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 qua., 12 de fev. de 2025 às 11:05, Tomek CEDRO <to...@cedro.info> > > >>>> escreveu: > > >>>> > > >>>>> On Wed, Feb 12, 2025 at 2:13 PM Tiago Medicci Serrano > > >>>>> <tiago.medi...@gmail.com> wrote: > > >>>>>> Hi! > > >>>>>> Tomek, there is an important missing point about 19 (that is part of > > >> my > > >>>>>> proposal): > > >>>>>> > > >>>>>> 19. "Lazy consensus" is a situation where PR is auto-merged into the > > >>>>>>> upstream when not enough reviews are done in predefined time (i.e. > > >>>>>>> there are no minimum required positive reviews within two weeks > > >> time). > > >>>>>>> PR should initially be treated according the general rules (4 > > >>>>>>> independent reviewers); 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; > > >>>>>>> After two weeks without any reviewers, we could merge if the above > > >>>>>>> conditions are met and we have at least one independent reviewer. > > >> This > > >>>>>>> may solve situation when 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. > > >>>>>> It's missing that a PR has to be *eligible *for requiring it and > > this > > >>>>>> includes some specific situations: > > >>>>>> - It should not affect more than one chip/board; > > >>>>>> - It applies to new features and apps that have no associated > > >> breaking > > >>>>>> changes and backward compatibility; > > >>>>>> - It requires at least one independent reviewer; > > >>>>>> - It requires an extensive testing section and proper documentation. > > >>>>>> > > >>>>>> These are *mandatory *conditions and we can vote it without it (this > > >>> was > > >>>>>> part of my considerations about *Lazy Consensus*). I would be > > against > > >>>>>> proposition 19 it if these requirements are missing. > > >>>>>> > > >>>>>> Can you please update proposition 19 and reconsider your vote based > > >> on > > >>>>> the > > >>>>>> requirements? > > >>>>> Hey there Tiago :-) > > >>>>> > > >>>>> This voting is to identify general rules that we all agree on > > already. > > >>>>> These points are supposed to be extract from our discussions. If I > > >>>>> missed some key points please add them here folks! For proposed rules > > >>>>> with doubts (0 or -1 votes) we should discuss more and update maybe > > >>>>> even finally reject them. > > >>>>> > > >>>>> On weekend we will gather and sum up the current vote results. Then > > >>>>> discussion will follow, where we may make clarifications, update > > >>>>> doubted rules, and vote again. The goal is to have common agreement > > on > > >>>>> the rules update. What we want to add and what form this is all up to > > >>>>> us. No one wants to enforce anything or create a monster that will > > >>>>> make our work harder, just a tool to help in review and code quality > > >>>>> improvement :-) > > >>>>> > > >>>>> If I provided incomplete information on proposition 19 then I am > > >>>>> sorry! Tiago you are the author, please send updated proposition 19 > > >>>>> text and note the old one is cancelled :-) > > >>>>> > > >>>>> Thanks!! :-) > > >>>>> > > >>>>> -- > > >>>>> CeDeROM, SQ7MHZ, http://www.tomek.cedro.info > > >>>>> > >
