Hi, Voting again (my previous e-mail was discarded in the voting process):
1. Contributing Guidelines with hints for Reviewers. > We are adding additional section for Reviewers to Contributing > Guidelines in order to provide checklist and complementary set of > rules that should filter out breaking code as much as possible also on > our side. +1 Tomek +1 Alin +1 Tiago 2. PR and GIT COMMITS must adhere to Contributing Guidelines or rejected. > Each PR and GIT COMMIT **must** adhere to requirements presented in > Contributing Guidelines or will be auto-rejected until fixed / > updated. Both code authors and reviewers/committers must follow the > rules. Special cases are defined in a separate dedicated rules. +1 Tomek +1 Alin +1 Tiago 3. Git commit messages as important as PR description. > Git commit messages are as important as PR descriptions. These > provide in-code descriptions of each change and are git interface > independent. +1 Tomek +1 Alin +1 Tiago 4. Proper description details requirements. > Proper description of change is mandatory. Description must contain > explanation on what proposed change do, why it is necessary, what if > fixes, and how things are changed / fixed / updated, what is the > impact (build / runtime / api / what area), how it was tested. Local > code build and real world hardware runtime test logs must be provided > (for code related changes). Description can be single..several > sentences long or bullet points but enough for anyone to understand > change goals and details. Usually it will look similar for PR and git > commit message. +1 Tomek ( However I understand this PR template is separate from the rule and will be updated / voted independently.) +1 Alin +1 Tiago 6. Git commit message must adhere to description requirements. > Proper GIT COMMIT message according to template is mandatory, or > change is rejected until fixed / updates. Build and runtime logs are > optional here if these are too long and already provided in PR. Git commit message consists of: 1. Topic with functional name prefix, ":" mark, and short > self-explanatory context. > 2. Blank line > 3. Description on what is changed, how, and why. May use several > lines, short sentences, or bullet points. > 4. Blank line. > 5. Signature (created with `git commit -s`). > GIT COMMIT TEMPLATE / EXAMPLE: > net/can: Add g_ prefix to can_dlc_to_len and len_to_can_dlc. > Add g_ prefix to can_dlc_to_len and len_to_can_dlc to > follow NuttX coding style conventions for global symbols, > improving code readability and maintainability. > * you can also use bullet points. > * to note different thing briefly. +1 Tomek +1 Alin +1 Tiago 7. Git commit message mandatory fields (topic, desctiption, signature). > Each git commit message must consist of topic, description, and > signature (git commit -s), as presented in GIT COMMIT TEMPLATE, which > are mandatory, or change is auto-rejected until fixed / updated. +1 Tomek +1 Alin +1 Tiago 8. Changes must come with documentation. > Changes must come with with documentation update where applicable. For > maintenance reasons code and documentation should be split into two > separate PR with the same name marked [1/2 CODE] for code and [2/2 > DOC] for documentation. If change presents new functionality a > documentation must be provided along with the code (not in future). If > change requires documentation update it must be contained along with > the code (not in future). Successful documentation build log shortcut > is welcome. > See: https://github.com/apache/nuttx/blob/master/INVIOLABLES.md 0: Tomek (Having documentation in a single PR (same pr separate commit) is easier to perform and review, otherwise we may get out of code/doc sync? But if this is the only way and better for release manager then okay.) 0 Alin (Having documentation in a single PR (same pr separate commit) is easier to perform and review. The release process can use it) 0 Tiago. Yes, documentation should be provided, but I don't see any reason for splitting it into two different PRs. We keep our documentation in the same repository and - for the sake of traceability - it should be updated in the same PR (separate commit, not PR). We should make reviewers' and committers lives easier. Alternative writing would be: "*Changes must come with a documentation update where applicable. For* *maintenance reasons, code and documentation should be split into two* *commits in the same PR. If change presents new functionality, documentation* *must be provided along with the code (not in the future). If change* *requires a documentation update it must be contained along with the code* *(not in the future).*" 9. Zero trust approach to user testing. > We implement zero trust approach to user provided testing. It is the > commit author duty to provide real world hardware build and runtime > logs for at least one device. Remember that any code change may break > things for others, please avoid that. > See: https://github.com/apache/nuttx/blob/master/INVIOLABLES.md +1 Tomek +1 Alin +1 Tiago 10. Breaking changes not welcome. > Breaking changes are not welcome. We do not "break by design". When > unavoidable, breaking changes need prior discussion and agreement of > the community (see Breaking Changes handling rule). This is anything > that alters Build / Kernel / Architecture / API, alters both nuttx and > nuttx-apps repo at the same time, breaks build/runtime/api for single > or many boards/architectures/applications, breaks self-compatibility, > breaks build/runtime compatibility with existing release code > (packages) both for nuttx and nuttx-apps, etc. Because thousands of > users / companies and their projects / products depend on NuttX code, > we strongly prefer self-compatibility and long-term maintenance over > "change is good" ideologies. Any code change may impact other users > and their business, please keep that in mind. > See: https://github.com/apache/nuttx/blob/master/INVIOLABLES.md +1 Tomek +1 Alin +1 Tiago 11. Respect long term maintenance and self-compatibility > We respect long term maintenance and self-compatibility is our > ultimate goal. Alternative solutions and non-invasive approaches are > preferred that offers user a choice and compatibility. Breaking > changes are avoided, and planned towards next major release, see > Breaking Changes rule. > Experimental code that does not impact overall project > self-compatibility in terms of Breaking Changes should be clearly > marked [EXPERIMENTAL]. > See: https://github.com/apache/nuttx/blob/master/INVIOLABLES.md +1 Tomek +1 Alin +1 Tiago 13. Breaking changes build and runtime test logs are mandatory. > Breaking changes are special case where build and runtime test logs > (i.e. apps/ostest) from more than one different architecture is > **mandatory** . QEmu tests does not count here as it passed breaking > change that did not work on a real hardware. Community support is > desired. > See: https://github.com/apache/nuttx/blob/master/INVIOLABLES.md +1 Tomek +1 Alin +1 Tiago 14. Minimum code reviews. > Each PR requires at least 2 independent positive reviews, except > Breaking Changes where at least 4 positive independent organizations > reviews, are required before merge to the upstream. +1 Tomek( Although I think 3 should be default to increase cross-checks.) +1 Alin (minimum 3) +1 Tiago. I still prefer Nathan's proposal of creating "areas". Documentation and experimental features shouldn't require 2 reviewers. For the sake of simplicity, this rule works. Even 2 reviewers for documentation and experimental features are too restrictive. 15. Reviews independence. > PR Reviews should come from independent organizations. Each PMC > Member, Committer, and Reviewer must report up-to-date Affiliation for > clear identification. When code comes from the same organization as > positive review, then at least one independent review is necessary > (except Breaking Changes). Self review is not allowed. > See: https://github.com/apache/nuttx/blob/master/INVIOLABLES.md +1 Tomek +1 Alin +1 Tiago 16. Self company commit/review/merge not allowed * > Single company commit, review, merge is not allowed. Each PMC Member, > Committer, and Reviewer must report up-to-date Affiliation for clear > identification. > See: https://github.com/apache/nuttx/blob/master/INVIOLABLES.md +1 Tomek +1 Alin +1 Tiago 17. Merge rules. > Each change **must** be provided as PR that undergoes independent > review process. Self committed code merge with or without review is > not allowed, just as direct push to master, and will be punished. +1 Tomek +1 Alin +1 Tiago 18. PR as small as possible . > 1. Pull Requests should be as small as possible and focused on only > one functional change. > 2. Different functional changes must be provided in separate Pull Requests. > 3. PR may contain several commits but every single commit included > must not break break overall build, runtime, and compatibility, > especially for other components. > 4. PR that breaks build or runtime anyhow is considered a Breaking > Change, is not welcome and requires special considerations (see > Breaking Changes rule). > 5. PR that introduces a new feature must have Documentation included > in separate commit. > 6. When changes for dedicated function 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 and > this is not a Breaking Change. +1 Tomek +1 Alin +1 Tiago 19. Lazy Consensus. > A PR may be *eligible* to be merged under the concept of *Lazy > consensus* with the following conditions: > 1. It affects only a single chip or board (no kernel/libs/upper-half > drivers etc). > 2. It implements a new feature (or app) that doesn't introduce any > breaking changes or backward incompatibility. > 3. It didn't get the minimum reviewers after two weeks. > 4. At least one independent reviewer reviewed it. > 5. It adheres to all other Contributing Guide requirements conditions. > The PR's author should: > 1. After a week without any reviewers, send an e-mail to the mailing > list asking for more people to review it. > 2. Explain why the PR can't be split into smaller PRs (if applicable). > 3. After two weeks ask for the independent reviewer to merge if there > are no other reviews. The independent reviewer is responsible for > checking if the PR matches the *Lazy Consensus* conditions before > merging it. -1: Tomek (Considering we are leaving 2 reviewers as is (increased to 4 for breaking changes), lazy consensus may undermine quality, I think this point is not required anymore :-)) -1 Alin (we risk critical bugs or harmful code to slip as lazy consensus ) -1 Tiago. For the sake of simplicity, let's adopt rule 14 only and re-evaluate in the future. Em seg., 24 de fev. de 2025 às 11:42, Alin Jerpelea <jerpe...@gmail.com> escreveu: > I am sorry for the confusion > Please fill in your answers to this email followed by the voter's name to > avoid future confusion > > > 1. Contributing Guidelines with hints for Reviewers. > We are adding additional section for Reviewers to Contributing > Guidelines in order to provide checklist and complementary set of > rules that should filter out breaking code as much as possible also on > our side. > > +1 Tomek > +1 Alin > > 2. PR and GIT COMMITS must adhere to Contributing Guidelines or rejected. > Each PR and GIT COMMIT **must** adhere to requirements presented in > Contributing Guidelines or will be auto-rejected until fixed / > updated. Both code authors and reviewers/committers must follow the > rules. Special cases are defined in a separate dedicated rules. > > +1 Tomek > +1 Alin > > 3. Git commit messages as important as PR description. > Git commit messages are as important as PR descriptions. These > provide in-code descriptions of each change and are git interface > independent. > > +1 Tomek > +1 Alin > > 4. Proper description details requirements. > Proper description of change is mandatory. Description must contain > explanation on what proposed change do, why it is necessary, what if > fixes, and how things are changed / fixed / updated, what is the > impact (build / runtime / api / what area), how it was tested. Local > code build and real world hardware runtime test logs must be provided > (for code related changes). Description can be single..several > sentences long or bullet points but enough for anyone to understand > change goals and details. Usually it will look similar for PR and git > commit message. > > +1 Tomek ( However I understand this PR template is separate from the rule > and will be updated / voted independently.) > +1 Alin > > 6. Git commit message must adhere to description requirements. > Proper GIT COMMIT message according to template is mandatory, or > change is rejected until fixed / updates. Build and runtime logs are > optional here if these are too long and already provided in PR. > > Git commit message consists of: > 1. Topic with functional name prefix, ":" mark, and short > self-explanatory context. > 2. Blank line > 3. Description on what is changed, how, and why. May use several > lines, short sentences, or bullet points. > 4. Blank line. > 5. Signature (created with `git commit -s`). > > GIT COMMIT TEMPLATE / EXAMPLE: > > net/can: Add g_ prefix to can_dlc_to_len and len_to_can_dlc. > > Add g_ prefix to can_dlc_to_len and len_to_can_dlc to > follow NuttX coding style conventions for global symbols, > improving code readability and maintainability. > * you can also use bullet points. > * to note different thing briefly. > > +1 Tomek > +1 Alin > > 7. Git commit message mandatory fields (topic, desctiption, signature). > Each git commit message must consist of topic, description, and > signature (git commit -s), as presented in GIT COMMIT TEMPLATE, which > are mandatory, or change is auto-rejected until fixed / updated. > > +1 Tomek > +1 Alin > > 8. Changes must come with documentation. > Changes must come with with documentation update where applicable. For > maintenance reasons code and documentation should be split into two > separate PR with the same name marked [1/2 CODE] for code and [2/2 > DOC] for documentation. If change presents new functionality a > documentation must be provided along with the code (not in future). If > change requires documentation update it must be contained along with > the code (not in future). Successful documentation build log shortcut > is welcome. > > See: https://github.com/apache/nuttx/blob/master/INVIOLABLES.md > > 0: Tomek (Having documentation in a single PR (same pr separate commit) is > easier to perform and review, otherwise we may get out of code/doc > sync? But if this is the only way and better for release manager then > okay.) > 0 Alin (Having documentation in a single PR (same pr separate commit) is > easier to perform and review. The release process can use it) > > 9. Zero trust approach to user testing. > We implement zero trust approach to user provided testing. It is the > commit author duty to provide real world hardware build and runtime > logs for at least one device. Remember that any code change may break > things for others, please avoid that. > > See: https://github.com/apache/nuttx/blob/master/INVIOLABLES.md > > +1 Tomek > +1 Alin > > 10. Breaking changes not welcome. > Breaking changes are not welcome. We do not "break by design". When > unavoidable, breaking changes need prior discussion and agreement of > the community (see Breaking Changes handling rule). This is anything > that alters Build / Kernel / Architecture / API, alters both nuttx and > nuttx-apps repo at the same time, breaks build/runtime/api for single > or many boards/architectures/applications, breaks self-compatibility, > breaks build/runtime compatibility with existing release code > (packages) both for nuttx and nuttx-apps, etc. Because thousands of > users / companies and their projects / products depend on NuttX code, > we strongly prefer self-compatibility and long-term maintenance over > "change is good" ideologies. Any code change may impact other users > and their business, please keep that in mind. > > See: https://github.com/apache/nuttx/blob/master/INVIOLABLES.md > > +1 Tomek > +1 Alin > > 11. Respect long term maintenance and self-compatibility > We respect long term maintenance and self-compatibility is our > ultimate goal. Alternative solutions and non-invasive approaches are > preferred that offers user a choice and compatibility. Breaking > changes are avoided, and planned towards next major release, see > Breaking Changes rule. > Experimental code that does not impact overall project > self-compatibility in terms of Breaking Changes should be clearly > marked [EXPERIMENTAL]. > > See: https://github.com/apache/nuttx/blob/master/INVIOLABLES.md > > +1 Tomek > +1 Alin > > 12. Breaking changes handling process. > This rule complements "Breaking changes not welcome" rule. We avoid > breaking changes unless absolutely necessary and unavoidable (i.e. > security fix, broken code, etc), then special case considerations may > apply: > 1. First reviewer that recognizes a breaking change should block > accidental merge with "Request Changes" mark and ask for discussion. > 2. PR is marked as "Draft" to avoid accidental merge. > 3. Detailed discussion should follow both in PR AND dev@ Mailing List. > 4. Alternative non-breaking alternative solution is researched with > help of the community. > 5. Breaking change after discussion / updates is voted on the mailing > list, requires at least 4 +1 binding votes and single -1 binding vote > blocks the change (binding vote means PMC member). > 6. Breaking changes **must** be verified on various different real > world hardware architectures, build and runtime logs are > **mandatory**, help of the community is desired. > 7. Breaking change requires at least 4 independent organizations > positive PR reviews. > 8. Change must be well documented (buid/runtime test logs, pr, git > commit, documentation, release notes, etc). > 9. Change must be clearly marked with [BREAKING] mark (pr, git commit, > release notes, etc). > > See: https://github.com/apache/nuttx/blob/master/INVIOLABLES.md > > +1 Tomek > +1 Alin > > 13. Breaking changes build and runtime test logs are mandatory. > Breaking changes are special case where build and runtime test logs > (i.e. apps/ostest) from more than one different architecture is > **mandatory** . QEmu tests does not count here as it passed breaking > change that did not work on a real hardware. Community support is > desired. > > See: https://github.com/apache/nuttx/blob/master/INVIOLABLES.md > > +1 Tomek > +1 Alin > > 14. Minimum code reviews. > Each PR requires at least 2 independent positive reviews, except > Breaking Changes where at least 4 positive independent organizations > reviews, are required before merge to the upstream. > > +1 Tomek( Although I think 3 should be default to increase cross-checks.) > +1 Alin (minimum 3) > > 15. Reviews independence. > PR Reviews should come from independent organizations. Each PMC > Member, Committer, and Reviewer must report up-to-date Affiliation for > clear identification. When code comes from the same organization as > positive review, then at least one independent review is necessary > (except Breaking Changes). Self review is not allowed. > > See: https://github.com/apache/nuttx/blob/master/INVIOLABLES.md > > +1 Tomek > +1 Alin > > 16. Self company commit/review/merge not allowed * > Single company commit, review, merge is not allowed. Each PMC Member, > Committer, and Reviewer must report up-to-date Affiliation for clear > identification. > > See: https://github.com/apache/nuttx/blob/master/INVIOLABLES.md > > +1 Tomek > +1 Alin > > 17. Merge rules. > Each change **must** be provided as PR that undergoes independent > review process. Self committed code merge with or without review is > not allowed, just as direct push to master, and will be punished. > > +1 Tomek > +1 Alin > > 18. PR as small as possible . > 1. Pull Requests should be as small as possible and focused on only > one functional change. > 2. Different functional changes must be provided in separate Pull Requests. > 3. PR may contain several commits but every single commit included > must not break break overall build, runtime, and compatibility, > especially for other components. > 4. PR that breaks build or runtime anyhow is considered a Breaking > Change, is not welcome and requires special considerations (see > Breaking Changes rule). > 5. PR that introduces a new feature must have Documentation included > in separate commit. > 6. When changes for dedicated function 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 and > this is not a Breaking Change. > > +1 Tomek > +1 Alin > > 19. Lazy Consensus. > A PR may be *eligible* to be merged under the concept of *Lazy > consensus* with the following conditions: > 1. It affects only a single chip or board (no kernel/libs/upper-half > drivers etc). > 2. It implements a new feature (or app) that doesn't introduce any > breaking changes or backward incompatibility. > 3. It didn't get the minimum reviewers after two weeks. > 4. At least one independent reviewer reviewed it. > 5. It adheres to all other Contributing Guide requirements conditions. > > The PR's author should: > 1. After a week without any reviewers, send an e-mail to the mailing > list asking for more people to review it. > 2. Explain why the PR can't be split into smaller PRs (if applicable). > 3. After two weeks ask for the independent reviewer to merge if there > are no other reviews. The independent reviewer is responsible for > checking if the PR matches the *Lazy Consensus* conditions before > merging it. > > -1: Tomek (Considering we are leaving 2 reviewers as is (increased to 4 for > breaking changes), lazy consensus may undermine quality, I think this > point is not required anymore :-)) > -1 Alin (we risk critical bugs or harmful code to slip as lazy consensus ) >
