On Fri, Feb 28, 2025 at 1:45 AM Dmitri Shilov <dshi...@cthru.xyz> 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 Tomek > > > +1 Alin > > > +1 Tiago > > > +1 TimK > > > +1 Lup > > > +1 Filipe > > > +1 Sebastien > > +1 Nathan > +1 Dmitri > +1 Matteo > > > > > > > 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 > > > +1 TimK > > > +1 Lup > > > +1 Filipe > > > +1 Sebastien > > +1 Nathan > +1 Dmitri > +1 Matteo > > > > > 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 > > > +1 TimK > > > +1 Lup > > > +1 Filipe > > > +1 Sebastien > > +1 Nathan > +1 Dmitri > +1 Matteo > > > > > > > 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 > > > +1 TimK (better to have the 'Motivation/Background' section, while > > > simplifying the rest. In my view, commit messages should address the > > > 'What', whereas PR documents should elaborate more on the 'Why'.) > > > +1 Lup > > > +1 Filipe > > > +1 Sebastien > > +1 Nathan > +1 Dmitri > +1 Matteo > > > > > 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 > > > +1 TimK (Regarding the commit message header, I recommend using the > > > style adopted by the Angular Community, which is widely accepted. > > > <type>(<scope>): <short summary>) > > > +1 Lup > > > +1 Filipe (let's make sure we have example for this on docs and also > on PR bot) > > > +1 Sebastien > > +1 Nathan > +1 Dmitri > > +1 Matteo > > > > > 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 > > > +1 TimK > > > +1 Lup > > > +1 Filipe > > > +1 Sebastien > > +1 Nathan > +1 Dmitro > > +1 Matteo > > > > > 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).*" > > > -1 TimK (I'd like say "should" instead of "must".) > > > 0 Lup. It depends? Smaller PRs can include a Doc Commit. When I add a > new > > > Arch + Board (e.g. StarPro64), the PR will include a link to my > article > > > that explains the new code. Then I prepare another PR for the User > Docs. > > > -1 Filipe (Don't see any problem in having documentation on same PR, > in fact I think it makes things easier.) > > > -1 separate commits in same pr > > -1 Nathan: Similar to others' comments. Also, could be same PR but > > separate commits? > 0 Dmitri > -1 Matteo: Documentation should be required, in the same PR as the change. No separate PRs. > > > > > 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 > > > +1 TimK > > > +1 Lup > > > +1 Filipe > > > +1 Sebastien > > +1 Nathan > +1 Dmitri > +1 Matteo > > > > > > > 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 > > > +1 TimK > > > +1 Lup > > > +1 Filipe > > > +1 Sebastien > > +1 Nathan > +1 Dmitri > 0 Matteo > > > > > > > 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 > > > +1 TimK > > > +1 Lup > > > +1 Filipe > > > +1 Sebastien > > +1 Nathan > +1 Dmitri > +1 Matteo > > > > > > > 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 > > > +1 TimK > > > +1 Lup > > > +1 Filipe > > > +1 Sebastien also some mandatory documentation on how to fix the > build after the breaking change. rust has cargo fix. Python has 2to3. > > +1 Nathan > +1 Dmitri > +1 Matteo > > > > > > > 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. > > > -1 TimK ("at least 2 independent positive reviews", may be too high > bar > > > we set.) > > > +1 Lup > > > +1 Filipe > > > +1 Sebastien > > 0 Nathan. Tiago summarized my proposal above. However, I am OK with > > whatever the community decides on this. > 0 Dmitri > +1 Matteo: Agree that pure documentation changes can have 1 reviewer > > > > > > > 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 > > > +1 TimK > > > +1 Lup > > > +1 Filipe > > > -1 Sebastien No. Reviewers must not be from same organization as > coder. Otherwise there is no independence. > > ?? Nathan: Reviews from same organization are nice to have, but > > shouldn't count toward number of reviews needed. > 0 Dmitri > +1 Matteo > > > > > > > 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 > > > +1 TimK > > > +1 Lup > > > +1 Filipe > > > 0 Sebastien > > +1 Nathan > 0 Dmitri > > +1 Matteo > > > > > 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 > > > +1 TimK (However, I would prefer the maintainer to perform a 'squash > > > merge' by default. In the case of a significant or breaking PR change, > > > we could consider a 'rebase merge'. > > > On a second thought, why does GitHub provide the 'Squash' option?) > > > +1 Lup > > > +1 Filipe > > > +1 Sebastien > > +1 Nathan. We should guard our master branches from direct pushes. See > https://github.com/apache/infrastructure-asfyaml?tab=readme-ov-file#branchpro > +1 Dmitri > +1 Matteo > > > > > > > 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 > > > +1 TimK > > > +1 Lup > > > +1 Filipe (item 5 clashes with voting item 8 discussion) Sebastien: > I'm ok with a single PR that contains separate commits for code and doc. > > > +1 Sebastien > > +1 Nathan > +1 Dmitri > -1 Matteo: #5 PR with new features should not require docs in a separate commit. It's difficult to make changes based on review and still keep the two commits (code and docs) separate when squashing. One commit for both should be fine. > > > > > > > 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. > > > 0 TimK > > > 0 Lup. I don't think this is priority right now? We can tweak the > guideline > > > later. > > > 0 Filipe > > > -1 Sebastien NERVER EVER let untrusted and unverified code in without > a review even if the process is slowed down. If developer WANTS his code > merged the burden is on them to get other reviewers involved so merge > becomes possible. > > -1 Nathan. I think I suggested Lazy Consensus initially, but after > > reading the discussions and comments about this, I think it is better > > to let PR authors to request reviews and keep their PR alive. > 0 Dmitri > -1 Matteo
