Hi, > 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 > 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 > 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 > 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 > 5. PR must adhere to description requirements. > Proper description in PR according to template is mandatory, fill in > all required fields or change is auto-rejected until fixed / updated. > For code changes build and runtime logs are mandatory to prove code > was tested on at least one real world hardware. > > PR TEMPLATE / EXAMPLE: > > Summary > 1. Why change is necessary (fix, update, new feature)? > 2. What functional part of the code is being changed? > 3. How does the change exactly work (what will change and how)? > 4. Related NuttX Issue reference if applicable. > 5. Related NuttX Apps Issue / Pull Request reference if applicable. > 6. Related NuttX Documentation Pull Request reference if applicable. > > Impact > 1. New feature added? Existing feature changed? > 2. User (will user need to adapt to change)? NO / YES (please describe if yes). > 3. Build (will build process change)? NO / YES (please descibe if yes). > 4. Hardware (will arch(s) / board(s) / driver(s) change)? NO / YES > (please describe if yes). > 5. Documentation (is update required / provided)? NO / YES (please > describe if yes). > 6. Security (any sort of implications)? NO / YES (please describe if yes). > 7. Compatibility (backward/forward/interoperability)? NO / YES (please > describe if yes). > 8. Anything else to consider? > > Testing > > 1. I confirm that changes are verified on local setup and works as > intended NO / YES. > 2. Build Host(s): OS (Linux,BSD,macOS,Windows,..), CPU(Intel,AMD,ARM), > compiler(GCC,CLANG,version), etc. > Target(s): arch(sim,RISC-V,ARM,..), board:config, etc. > 3. Testing logs before change: > runtime / build logs before change goes here > 4.Testing logs after change: > runtime / build logs after change goes here > 5. (optional) How to repeat. You can also provide steps on how to > reproduce problem and verify the change if not obvious from test logs. > > Optional PR remarks: > 1. This PR introduces only one functional change. > 2. I have updated all required description fields above. > 3. My PR adheres to Contributing Guidelines and Documentation (git > commit message, coding standard, testing etc). > 4. My PR is still work in progress (not ready for review). > 5. My PR is ready for review and can be safely merged into a codebase. +1 > 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 > 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 > 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 -1. 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 comitters' lives easier. Alternative writing would be: " "Changes must come with with 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 > 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 > 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 > 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 > 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 > 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: 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. > 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 > 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 > 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 > 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 > 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: 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 10:18, Tomek CEDRO <to...@cedro.info> escreveu: > My responses below :-) > > On Mon, Feb 24, 2025 at 1:57 PM Tomek CEDRO <to...@cedro.info> wrote: > > 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 > > > 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 > > > 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 > > > 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 > > > 5. PR must adhere to description requirements. > > Proper description in PR according to template is mandatory, fill in > > all required fields or change is auto-rejected until fixed / updated. > > For code changes build and runtime logs are mandatory to prove code > > was tested on at least one real world hardware. > > > > PR TEMPLATE / EXAMPLE: > > > > Summary > > 1. Why change is necessary (fix, update, new feature)? > > 2. What functional part of the code is being changed? > > 3. How does the change exactly work (what will change and how)? > > 4. Related NuttX Issue reference if applicable. > > 5. Related NuttX Apps Issue / Pull Request reference if applicable. > > 6. Related NuttX Documentation Pull Request reference if applicable. > > > > Impact > > 1. New feature added? Existing feature changed? > > 2. User (will user need to adapt to change)? NO / YES (please describe > if yes). > > 3. Build (will build process change)? NO / YES (please descibe if yes). > > 4. Hardware (will arch(s) / board(s) / driver(s) change)? NO / YES > > (please describe if yes). > > 5. Documentation (is update required / provided)? NO / YES (please > > describe if yes). > > 6. Security (any sort of implications)? NO / YES (please describe if > yes). > > 7. Compatibility (backward/forward/interoperability)? NO / YES (please > > describe if yes). > > 8. Anything else to consider? > > > > Testing > > > > 1. I confirm that changes are verified on local setup and works as > > intended NO / YES. > > 2. Build Host(s): OS (Linux,BSD,macOS,Windows,..), CPU(Intel,AMD,ARM), > > compiler(GCC,CLANG,version), etc. > > Target(s): arch(sim,RISC-V,ARM,..), board:config, etc. > > 3. Testing logs before change: > > runtime / build logs before change goes here > > 4.Testing logs after change: > > runtime / build logs after change goes here > > 5. (optional) How to repeat. You can also provide steps on how to > > reproduce problem and verify the change if not obvious from test logs. > > > > Optional PR remarks: > > 1. This PR introduces only one functional change. > > 2. I have updated all required description fields above. > > 3. My PR adheres to Contributing Guidelines and Documentation (git > > commit message, coding standard, testing etc). > > 4. My PR is still work in progress (not ready for review). > > 5. My PR is ready for review and can be safely merged into a codebase. > > +1: However I understand this PR template is separate from the rule > and will be updated / voted independently. > > > 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 > > > 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 > > > 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: 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. > > > 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 > > > 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 > > > 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 > > > 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 > > > 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 > > > 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: Although I think 3 should be default to increase cross-checks. > > > 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 > > > 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 > > > 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 > > > 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 > > > 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: 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 :-) > > > -- > CeDeROM, SQ7MHZ, http://www.tomek.cedro.info >
