On 27/02/2025 13:53, Filipe Cavalcanti 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
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
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
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
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
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
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
Note: Lup: web pages vanish. Documenting new code on your blog is cool but feel
insufficient for the future of the nuttx project, that should stay
self-contained. You cannot guarantee your website will always stay online.
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
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
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
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.
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
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.
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
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
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
Request=
s.
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
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.
Sebastien
________________________________
From: Lee, Lup Yuen <lu...@appkaki.com>
Sent: Thursday, February 27, 2025 8:41 AM
To: dev@nuttx.apache.org <dev@nuttx.apache.org>
Subject: Re: Re: [VOTE] ROUND2 NuttX Contributing Guidelines update 202502.
[External: This email originated outside Espressif]
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
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
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
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
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
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
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.
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
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
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
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
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
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
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
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
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
Request=
s.
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
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.
