>
> 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
> 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
> 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
> 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
> 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
> 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
> 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
