Okay so here goes the vote results :-)
Lets turn this thread now to a discussion that should end up with all
down voted points clarified and cast to vote again.
Another question right now do we want to add +1 points already to
Guidelines or wait for all list to clarify? Some fine tuning of the
text will be part of PR review :-)
Remember we do this to make our life easier and improve code quality /
stability / self-compatibility :-)
Things that are all voted +1 and we agree with no neutral or blocking votes:
1. Contributing Guidelines with hints for Reviewers.
2. PR and git commits must adhere to Guidelines or rejected.
3. Git commit messages as important as PR description.
4. Proper description details requirements.
6. Git commit message must adhere to description requirements.
7. Git commit message mandatory fields (topic, desctiption, signature).
13. Breaking changes build and runtime test logs are mandatory.
Things that need redefinition had 0 or -1 votes:
5. PR must adhere to description requirements (one 0).
8. Changes must come with documentation (1:-1 vs 11:+1).
9. Zero trust approach to user testing (1:-1 vs 11:+1).
10. Breaking changes not welcome. (5:-1, 1:0, 6:+1).
11. Respect for long term maintenance and self-compatibility (1:-1, 1:0, 11:+1).
12. Breaking changes handling process (1:0, 11:+1).
14. Minimum code reviews 2 -> 4 (5:-1 vs 7:+1).
15. Reviews must come from independent organizations (1:-1, 1:0, 10:+1).
16. Self company commit/review/merge not allowed (1:0, 11+1).
17. Self commit and merge not allowed (1:-1 vs 11:+1).
18. REDO: PR as small as possible and single change only, no bundles (1:0, 3+1).
19. REDO: Lazy Consensus (3:-1, 3:+1).
Last remarks - another vote of this kind will be performed on Google
Forms so we have results presented and counted automatically with a
list of remarks auto generated too.. xls file will be attached to
result vote.. is that okay? ;-)
Thanks everyone!! :-)
Tomek
### 1. Contributing Guidelines with hints for Reviewers.
1. In Contributing Guidelines we are adding additional section for
Reviewers in order to provide complementary set of rules that should
filter out breaking code as much as possible also on our side.
+1: 12
0: 0
-1: 0
### 2. PR and git commits must adhere to Guidelines or rejected.
2. Each PR (including git commits) _must_ adhere to requirements
presented in Contributing Guidelines or will be auto-rejected until
fixed / updated.
+1: 11
+0.5: 1
0: 0
-1: 0
Remarks:
* Nathan: Sometimes we may need to override the Contributing
Guidelines in special circumstances. Consider, for example, how
nxstyle has given false positives on occasion, such as when we have to
relax our capitalization rules because of 3rd party code we're
interfacing with. So, I would suggest that while PRs _must_ adhere to
the requirements, there should also be a way to relax this requirement
if appropriate due to legitimate reasons.
### 3. Git commit messages as important as PR description.
3. Git commit messages are as important as PR descriptions. These
provide in-code descriptions of each change and are git interface
independent.
+1: 12
0: 0
-1: 0
### 4. Proper description details requirements.
4. 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
where mandatory. 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: 12
0: 0
-1: 0
### 5. PR must adhere to description requirements.
5. Proper description in PR is mandatory, or change is auto-rejected
until fixed / updated. Build and real world hardware runtime logs are
mandatory.
+1: 11
0: 1
-1: 0
Remarks:
* Ville: 0 Description for PR and commit are a given, how is anyone
expected to understand what you are trying to do (and why), without a
description? That is what the commit text field is for.
### 6. Git commit message must adhere to description requirements.
6. Proper description in Git commit message 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.
+1: 12
0: 0
-1: 0
Remarks:
* Matteo: +1 definitely shouldn't put runtime logs in the commit description.
### 7. Git commit message mandatory fields (topic, desctiption, signature).
7. Each git commit message must consist of topic, description, and
signature, which are mandatory, or change is auto-rejected until fixed
/ updated. Topic consists of functional prefix, ":" mark, and short
self-explanatory context. Description is separated from topic with a
single blank line. Example already presented in Contributing
Guidelines.
+1: 12
0: 0
-1: 0
Remarks:
* Ville: +1 Though, why is the signature significant? I never
understood this point. I'm not arguing against making it mandatory,
I'm just genuinely curious.
* Takashi: (no vote) what's "signature" in this context?
### 8. Changes must come with documentation.
8. Changes must come with with documentation update where applicable.
If change presents new functionality a documentation must be provided
in the same PR (not in future). If change requires documentation
update it must be contained in the same PR (not in future). Successful
documentation build log shortcut is welcome.
+1: 11
0: 0
-1: 1
Remarks:
* Tomek: +1 This is a good rule, people often deliver code and
documentation changes which is good, but there are places where
documentation is out of sync so this should prevent code-doc desync
and update missing docs on code update.
* Tiago: +1 I heavily recommend sending the documentation at the same
PR. Considering that PRs are as atomic as possible, the documentation
should be tightly linked with it. It would not create merge issues (at
least not an additional workload) and having them on separate PRs may
lead to errors if left behind when backporting.
* Viile: +1 as long as a bullet point list "this is how you build&run
this target"-type of documentation is enough.
* Sebastien: +1 same PR but different commits maybe?
* Alin: -1 documentation should be provided at the same time as a
separate PR with the same name and {2/2} to indicate that they belong
to the same PR. For LTS documentation may cause
merge issues and increase the maintainers workload.
### 9. Zero trust approach to user testing.
9. We implement zero trust approach to user provided testing. It is
the commit author duty to provide real world hardware build and
runtime logs that must prove change does not break stuff for others.
+1: 11
0: 0
-1: 1
Remarks:
* Sebastien: +1 make sure tests are relevant to the function being
tested and provide useful coverage of the fix/feature.
* Ville: +1, How about we don't break things for others, period ? This
is the biggest issue IMO, changes are made to targets that don't need
to be changed (Sebastien's issues are of this type). Platforms that
are not being used/maintained by the person/organization pushing for
the change should NEVER be touched.
* Filipe: +1 with a consideration: Some changes might affect multiple
boards and it may not be possible for a user to test it all. I think
this is where input from reviewers come in by making sure, at least in
theory, that the change is ok to be accepted without further hardware
testing. I agree user should provide logs and test on hardware, but
there are limitations. "Zero trust" seems quite a stretch for me.
* Alan: +1 // this is something we need to improve, HW CI should
confirm if a PR is working.
* Tomek: +1: Strong +1 here, people must realize their changes may
impact others, so proper testing is required. We need to also provide
tools to make that testing easier (i.e. drunx).
* Nathan: -1 It is probably not possible/feasible for a commit author
to verify that a change doesn't break anything for anyone else. They
probably don't have all the kinds of hardware in the world. Though I
agree that we should require testing on real hardware, we should be
very careful how we phrase the requirement for testing. Remember: We
want to encourage contributions, so we have to make sure our
expectations are within the realm of what's reasonably feasible.
### 10. Breaking changes not welcome.
10. Breaking changes are not welcome. 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.
+1: 6
0: 1
-0.5: 1
-1: 4
Remarks:
* Nathan: -0.5 I recommend to modify this rule: "Breaking changes
require a discussion on the mailing list, followed by a [VOTE], and
must have at least 3 +1 and no -1 to be allowed. Breaking changes that
don't follow this process are not welcome. This is anything that
alters......" Item #12 addresses this, and I voted +1 on item #12.
* Tomek: +1 +1: This is a good general rule to avoid "break by
design". We do not want to change critical stuff that affects
different areas in a breaking way. Sure there will be exceptions where
breaking stuff may be unavoidable, but it must be well marked, noted,
discussed, agreed, and tested before merge. In most cases changes can
be made with alternative / complementary solutions that provide
compatibility or choice. New features are not breaking changes, and
should not break
existing stuff.
* Matteo: -1, breaking changes shouldn't be unwelcome so long as they
are discussed an agreed upon by the community via the mailing list
first. Some criteria on what is considered sufficient approval should
probably be made.
* Alan: -1 In some cases breaking previous compatibility is necessary
to evolution of the project, but these breaking need to be discussed
further in the
* Tiago: *-1* Breaking changes are not welcomed, but sometimes they
are necessary. I think the next item (11) already covers the care we
need with breaking changes.
* Ville: +1 Absolutely, at least without a consensus that the change
makes things clearly better. Not different, BETTER.
* Sebastien: +1 with allowing managed exceptions: if an api is
actually broken and must absolutely be changed, then it must be
explicitly documented in a log of breaking interface changes that is
not buried in the release notes.
* Raiden: -1 Broken API, broken features and any other broken code
should be removed even if it breaks some users code. Legacy code means
more work for maintainers, worse code quality and inconvenience for
users. It's always been like this in
NuttX, which is why NuttX's code is readable and easy to follow.
Changing this can be a huge downside for the project.
* Michal Lenc: 0 these are sometimes necessary.
### 11. Respect for long term maintenance and self-compatibility.
11. 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.
+1: 10
0: 1
-1: 1
Remarks:
* Raiden: -1 Not every interface in NuttX is equally mature. Until we
establish which API is stable and which is not, this change is bad.
Breaking a stable API should be avoided, breaking an unstable and
under development API should be allowed.
* Alan: 0 We don't have stable APIs in the kernel yet (only POSIX
userspace), if "long term maintenance" means LTS support and if this
impact our few contributors and reviewers, it needs to be discussed
further. Forcing contributors to create a single PR for each
modification in diferent areas of the kernel doesn't sound feasible.
Commits and PR need to have logic separation, if a Documentation/ is
about something that touches the current PR, it is fine to be in the
same commit.
* Tomek: +1: As above, breaking changes are not welcome and avoided,
but sometimes unavoidable, what required good discussion prior change.
We should keep self-compatibility between releases, breaking changes
if happen these should be clearly noted. It may happen that in
upstream master problems may occur, but these should be fixed before a
release, and releases should be self-compatible.
### 12. Breaking changes handling process.
12. Breaking changes _must_ be discussed prior introduction on the
dev@ mailing list. PR may be created with clear indication it is for
discussion and marked as draft not to be "accidentally merged".
+1: 11
0:1
-1: 0
Remarks:
* Sebastien: +1, let's do this official page of unavoidable breaking
changes, so we know what to do when updating from release A to release
B.
* Raiden: 0 Not all breaking changes are equal.
### 13. Breaking changes build and runtime test logs are mandatory.
13. 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.
+1: 11
+0.5: 1
0: 0
-1: 0
Remarks:
* Tomek: +1: Yes critical changes that are or may be breaking should
be tested on various hardware architectures and logs should be
provided from these tests by the author, that will help in review and
at least prove someone tested it.
* Nathan: +0.5 I agree that we need apps/ostest from more than one
different architecture -- we should make a note here that if the
commit author doesn't have different kinds of hardware available, they
should be able to request testing from other members of the community.
Everyone who performs testing should add their test results in a
comment in the PR, and once there are enough successful tests, the PR
can be merged.
### 14. Minimum code reviews 2 -> 4.
14. Number of minimum required code review votes should be increased
from 2 to 4. This will ensure cross-checks and filter out faulty
changes.
+1: 7
0:
-1: 5
* Remarks:
* Nathan: -1 We should not have a requirement of 4 reviewers for every
change, but we _should_ require more reviewers for sensitive changes,
and allow fewer reviewers for low risk changes. Examples of low risk:
changes to documentation, changes in some obscure driver or board that
affects very few people; these should be okay with just 1 reviewer.
Examples of medium to high risk: changes to sched, changes to platform
independent code that is heavily used, changes to low level arch;
these should require more reviewers, and I would suggest
that 3 reviewers should be enough. I am concerned that if we set the
bar too high, PRs will get stuck for a long time and we'll accumulate
a large number of PRs that will fall by the wayside. So we should try
to strike a good balance.
* Tomek: +1: This should ensure cross-checks and should mobilize community :-)
* Matteo: +1 maybe even 3 would be okay though, there seems to be a
shortage of reviewers.
* Alan: -1 If it was reviewed by other peers (2 or more) and the PR
still there waiting to be merge for more than 5 days after review, the
author (if he has commit write permission) could have the right to
merge it.
* Filipe: *-1* Are we even achieving 2 votes average? It seems like
this would keep PRs cooking for a long time waiting for votes. I would
keep at 2 or 3 at most. Sure this could change with time as more
people enter the project.
* Sebastien: +1 more importantly, not all from the same organization.
* Raiden: -1 Not all PRs are equal. For important PRs it's OK, but as
a requirement for all PRs - nope.
* Michal Lenc: -1 I would still apply it only for bigger changes.
### 15. Reviews must come from independent organizations.
15. Counting review votes should come from independent organizations.
There may be more than one review from a single organization, but
these will count as one vote.
+1: 10
0: 1
-1: 1
Remarks:
* Raiden: 0 Single company review should not be allowed and that's
enough. There are areas where 2 reviews from the same company has more
value than 2 reviews from the independent people with no knowledge
about a given area. The quality of the review is more important than
the company it comes from.
* Takashi: (no vote) how do you attribute people to companies/organizations?
* Tomek: +1: Cross-checks are supposed to work that way. We should
clarify affiliations of the committers.
### 16. Self company commit/review/merge not allowed.
16. Single company commit, review, merge is not allowed.
+1: 11
0: 1
-1: 0
Remarks:
* Alan: 0 If it was reviewed by other peers (2 or more) and the PR
still there waiting to be merge for more than 5 days after review, the
author (if he has commit write permission) could have the right to
merge it.
* Takashi: (no vote) how do you attribute people to companies/organizations?
### 17. Self commit and merge not allowed.
17. Self committed code merge is not allowed.
+1: 11
0: 0
-1: 1
Remarks:
* Tomek: +1: This rule obeys already but is not written down?
* Alan: -1 If it was reviewed by other peers (2 or more) and the PR
still there waiting to be merge for more than 5 days after review, the
author (if he has commit write permission) could have the right to
merge it.
* Ville: +1 and if this ever happens it should be severely punished.
### 18. PR as small as possible and single change only, no bundles.
18. ADDED DURING VOTE NEEDS REVOTE Pull Requests should be as small as
possible and focused on only one functional change. Different
functional changes should be provided in separate Pull Requests.
Remember that breaking changes are not welcome. Pull Requests must not
break overall build, runtime, and compatibility, especially for other
components. When changes 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.
+1: 3
0: 1
-1: 0
Remarks:
* Filipe: +1 Following suggestions from my colleagues, please also add
documentation to this.
* Tiago: +1. Documentation of a new feature should be provided in the
same PR. Documentation is provided in the `nuttx` repository, so if
the PR changes something that affects the current documentation, it
should be treated at the same PR.
* Michal Lenc: 0: Does this apply to the documentation as well
(discussed in a different thread for LTS)? I prefer to put the docs in
one PR instead of creating the second one. We have a documentation in
a single repository (compared to other projects like RTEMS for
example), so let's take the advantage of it. Otherwise we can split
the code and documentation into two repositories. But that's harder to
maintain. Also another example is the implementing a new peripheral
for some MCU. This implementation SHOULD come with another commit that
adds the support for it to BSP, otherwise the CI jobs for the
peripheral are useless, the new code is not build at all.
* Tomek: +1: New authors sometimes provide single PR with different
functional changes, that should split into different PRs, this should
keep things clean. Sometimes big PR is required to update single area
(i.e. chip drivers) then with good argumentation this should be
acceptable.
### 19. Lazy Consensus
19. ADDED DURING VOTE NEEDS REVOTE A PR may be *eligible* to be merged
under the concept of *Lazy consensus* with the following conditions:
- It affects only a single chip or board (no kernel/libs/upper-half
drivers etc);
- It implements a new feature (or app) that doesn't introduce any
breaking changes or backward incompatibility;
- It didn't get the minimum of 4 reviewers after two weeks (to be
discussed);
- At least one independent reviewer reviewed it;
- It adheres to all other conditions.
The PR's author should:
- After a week (to be discussed) without any reviewers, send an
e-mail to the mailing list asking for more people to review it;
- Explain why the PR can't be split into smaller PRs (if
applicable);
- Ask for the independent reviewer to merge it after two weeks
without any other reviewers;
*The (required) independent reviewer* is responsible for checking if
the PR matches the *Lazy Consensus* conditions and merging it.
+1: 3
0: 0
-1: 3
Remarks:
* Filipe: +1 Makes sense to me. I strongly support this as we don't
have that many people to review and approve. We can't stop going
forward, but this item can be reviewed in the future, when more people
are working in Nuttx.
* Tiago: +1. NuttX needs more features (new features) and more
board/chip support. New features don't break any existing
implementation and may be eligible for merging after at least one
independent reviewer. Some notes on that taking *Zephyr* - our main
competitor- as an example. In the last month, Zephyr had almost 400
authors and more than 2K commits
(https://github.com/zephyrproject-rtos/zephyr/pulse/monthly), but the
PRs that implement new features don't require 4 reviewers to be
merged. For example:
- https://github.com/zephyrproject-rtos/zephyr/pull/85525
- https://github.com/zephyrproject-rtos/zephyr/pull/85374
- https://github.com/zephyrproject-rtos/zephyr/pull/85342
- https://github.com/zephyrproject-rtos/zephyr/pull/85284
NuttX had 57 authors and 344 commits
(https://github.com/apache/nuttx/pulse/monthly). This is 7x less
authors/commits. We have limitations on what we can do trusting only
on people. Exceptions like *19* should exist otherwise *NuttX will die
as a
distribution*. We should really focus our attention on automated
testing (than being to rigid depending only on people for new
features). Zephyr, with 7x more authors, doesn't rely on people that
much. We really need more features and chip/board support and *Lazy
Consensus* is important here.
* Alan: +1 The goal is not to automatically merge PR, but only to
avoid that some PRs that don't reach the maximum number of reviews be
allowed to be merged.
* Tomek: -1: Still, sorry, I think this is too complex, contradicts
idea 14 whatever final form it will have, and may create a loophole
for "bad" code to enter the upstream that we are trying to fix right
now. For now I am on the other side of spectrum, a safe defaults,
safe-open didn't work well. I understand the reasons behind, maybe it
reveals something constructive that we need, maybe in another form,
maybe not now, for sure this idea needs more discussion :-)
* Sebastien: -1 To be honest, I find this specific rule to be very
complex. it's not easy to understand its effect (I did not
apparently). so it should be reworded at least. Again, I will vote
against this. This is a bypass of all other rules we're trying to
enforce. If such situation arise, there are two cases: - either the
submitter still cares and will yell at people to get it approved -
either it will be dropped entirely. I dont want to see unsupervised
auto commits. The conditions listed in this point (no breakage,
execution of tests, etc) HAVE TO be verified. I suggest still not to
integrate it as-is.
* Michal Lenc: -1 in case we don't require 4 reviewers for every PR,
but only for major and potentially breaking ones, otherwise +1.
--
CeDeROM, SQ7MHZ, http://www.tomek.cedro.info
