202503 Apache NuttX RTOS Contributing Guidelines update. #15950

cederom · 2025-03-07T05:30:00Z

This change comes from vast mailing list discussions on low PR/COMMIT
quality, breaking changes, code quality, long term self-compatibility
and maintenance, review problems, and how to improve things.
Contributing Guidelines updates vote history:
1. https://www.mail-archive.com/dev@nuttx.apache.org/msg12584.html
2. https://www.mail-archive.com/dev@nuttx.apache.org/msg12723.html

CONTRIBUTING.md

acassis · 2025-03-07T09:15:28Z

CONTRIBUTING.md

+For maintenance reasons, code and documentation should be split into
+two separate commits in the same PR. This helps backporting and separates


@cederom I suggest removing this requirement, as I have shown other project doesn't require it and even Linux that @jerpelea said requires it I looked the git history and them don't have this requirement.

@raiden00pl @xiaoxiang781216 @hartmannathan for awareness

All related change should come in one patch to avoiding the mismatch.

we will do as the majority decides

here is a Linux example
https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/Documentation/bpf?h=v6.14-rc5

The proposed rule reflects exactly what we've been doing for a while (same PR, separate commits). About adding it to the same (or to an individual commit), well, there is always that "gold" rule that each commit should aggregate the minimum buildable code necessary to it. That being said, this rule adheres to it: documentation is not mandatory to make the "code" buildable and should be split into different commits.

The same is valid for arch and board support: when a new feature is added, it might not be mandatory to have arch and board-code in the same commit (the arch code is buildable by itself). They can be split into different commits (in the same PR!). Documentation would be the third commit. Reverting it requires reverting all commits in the same PR.

@jerpelea @tmedicci I agree that is is nice to have documentation in a separated commit, but this should be suggested as good practice, but not as an enforced rule. Because even projects like Linux doesn't enforce it. Never be "plus royaliste que le roi"! ;-)

There is a difference between adding something new to the documentation and updating the documentation according to changes made to the code. I don't see a problem with a separate commit for doc in the first case, but in the second case this seems strange. Documentation describes the code, if the code changes then the documentation should also be changed otherwise the doc doesn't describe the current code state.

@cederom I suggest removing this requirement, as I have shown other project doesn't require it and even Linux that @jerpelea said requires it I looked the git history and them don't have this requirement.

@raiden00pl @xiaoxiang781216 @hartmannathan for awareness

Thanks, @acassis. I am okay with either option. Code and Documentation in same commit is OK with me, for git bisect purposes. I'm also OK with separating the commits but same PR.

We should choose one and standardize.

There is a difference between adding something new to the documentation and updating the documentation according to changes made to the code. I don't see a problem with a separate commit for doc in the first case, but in the second case this seems strange. Documentation describes the code, if the code changes then the documentation should also be changed otherwise the doc doesn't describe the current code state.

Exactly, it could happen very ofter: someone revert a commit and forgot the revert the other documentation commit, so the documentation will be out of sync.

It is advised that the code and documentation should be split into two
separate commits in the same PR. This helps backporting and separates
possible code and documentation build errors. Squashing code together with
documentation into a single commit should be avoided, but is acceptable.

How about this one? We may prefer to have doc and code commits separate but accept code+doc commits too? :-)

The revert argument is valid, although we use PR revert over github, but code PR may be reverted on nuttx-apps and skipped on nuttx where documentation resides, true. This is why having good PR descriptions is so important.

I can also understand the backporting argument.. but when the code is backported along with the documentation it should be also okay?

I also updated these two points (added "in the same PR"):

If change presents new functionality documentation must be provided
in the same PR along with the code (not in the future).

If change requires existing documentation update it must be contained
in the same PR along with the code change (not in the future).

tmedicci

One minor suggestion. Thanks @cederom !

CONTRIBUTING.md

tmedicci · 2025-03-07T13:13:55Z

CONTRIBUTING.md

+For maintenance reasons, code and documentation should be split into
+two separate commits in the same PR. This helps backporting and separates


The proposed rule reflects exactly what we've been doing for a while (same PR, separate commits). About adding it to the same (or to an individual commit), well, there is always that "gold" rule that each commit should aggregate the minimum buildable code necessary to it. That being said, this rule adheres to it: documentation is not mandatory to make the "code" buildable and should be split into different commits.

The same is valid for arch and board support: when a new feature is added, it might not be mandatory to have arch and board-code in the same commit (the arch code is buildable by itself). They can be split into different commits (in the same PR!). Documentation would be the third commit. Reverting it requires reverting all commits in the same PR.

hartmannathan

Thanks to everyone for your efforts!! Hope my review suggestions are helpful. Feel free to use them, modify them, or ignore them!!

hartmannathan · 2025-03-07T13:58:40Z

CONTRIBUTING.md

-## Guidelines
+If you haven't yet read
+[The Inviolable Principles of NuttX]( https://github.com/apache/nuttx/blob/master/INVIOLABLES.md)
+please do so right now.


Suggested change

please do so right now.

please do so first.

hartmannathan · 2025-03-07T13:59:09Z

CONTRIBUTING.md

-To help us successfully review your contribution, it is very
-important that you follow these guidelines.
+Please note that NuttX supports over 15 different architectures, 360+ boards,
+and 1600+ configurations, that are used in various project and products around


Suggested change

and 1600+ configurations, that are used in various project and products around

and 1600+ configurations, that are used in various projects and products around

hartmannathan · 2025-03-07T13:59:27Z

CONTRIBUTING.md

+Please note that NuttX supports over 15 different architectures, 360+ boards,
+and 1600+ configurations, that are used in various project and products around
+the world. Remember that any code change may affect different users and their
+business. Please try to keep your contributions high quality and compatible.


Suggested change

business. Please try to keep your contributions high quality and compatible.

businesses. Please try to keep your contributions high quality and compatible.

hartmannathan · 2025-03-07T14:02:21Z

CONTRIBUTING.md


-If you need more information please read the [Full Contributing Documentation](https://nuttx.apache.org/docs/latest/contributing/index.html).
+To help us smoothly process your contributions it is very important that you
+follow the guidelines. These rules are based on daily experiences and helps


Suggested change

follow the guidelines. These rules are based on daily experiences and helps

follow the guidelines. These rules are based on daily experiences and help

hartmannathan · 2025-03-07T14:05:36Z

CONTRIBUTING.md

+To help us smoothly process your contributions it is very important that you
+follow the guidelines. These rules are based on daily experiences and helps
+us preserve long term self-compatibility and maintenance of the project.
+NuttX is maintained by a small team of volunteers from around the world.


This is a good sentence, but I think it might need to move to elsewhere in the document, because it doesn't quite fit with the flow of this part of the text.

hmm, i used top-bottom approach.. did small rearrangement.. what place would be better or what better text? :-)

hartmannathan · 2025-03-07T14:39:22Z

CONTRIBUTING.md

+    This PR cannot be merged until all requests are resolved.
+ 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 solution is researched along the community.


Suggested change

4. Alternative non-breaking solution is researched along the community.

4. Alternative non-breaking solution is researched alongside the community.

hartmannathan · 2025-03-07T14:43:35Z

CONTRIBUTING.md

+ 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).


Suggested change

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

5. Breaking change after discussion / updates is voted on the mailing

list and requires at least 4 +1 binding votes with no -1 binding votes.

Any -1 binding vote is a [veto](https://apache.org/foundation/voting.html#Veto) and blocks the change.

NuttX PMC members cast binding votes. See [Apache Voting Process](https://apache.org/foundation/voting.html)

Apologies for my suggested text not being line-wrapped properly. I am doing this from mobile device!

hartmannathan · 2025-03-07T14:46:51Z

CONTRIBUTING.md

+(i.e. `apps/ostest`) for **more than one** different architecture is
+**mandatory**. Community support is welcome.
+
+QEmu tests does not count here as in the past it did not catch problems


Suggested change

QEmu tests does not count here as in the past it did not catch problems

QEmu tests **do not count** here as in the past they did not catch problems

(Also I'm unsure of QEmu; I have always seen it stylized as QEMU before.)

hartmannathan · 2025-03-07T14:49:41Z

CONTRIBUTING.md

+3. Merge of PRs with unresolved discussions and "change request" marks
+   is not allowed.
+3. Self committed code merge with or without review is not allowed.
+4. Direct push to master is not allowed.


(We should setup nuttx and apps repos to block direct push to master. That is something we can do ourselves; INFRA does not need to be involved in that.)

Definitely :-) I am kinda new here but this is not allowed on most repos that I know, not sure why it was enabled, but there may be a reason? We will switch it after rules are applied for sure :-)

hartmannathan · 2025-03-07T14:51:04Z

CONTRIBUTING.md

  I confirm that changes are verified on local setup and works as intended:
  * 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.
+  * you can also provide steps on how to reproduce problem and verify the change.


Suggested change

* you can also provide steps on how to reproduce problem and verify the change.

* you can also provide steps on how to reproduce the problem and verify the change.

tmedicci · 2025-03-07T19:17:09Z

Just to make sure: this file is not rendered into the documentation at https://nuttx.apache.org/docs/latest/index.html, right?

Still pending adding the same info at https://nuttx.apache.org/docs/latest/contributing/index.html (maybe creating a new page)

cederom · 2025-03-10T02:40:14Z

@tmedicci: Just to make sure: this file is not rendered into the documentation at https://nuttx.apache.org/docs/latest/index.html, right?

Still pending adding the same info at https://nuttx.apache.org/docs/latest/contributing/index.html (maybe creating a new page)

Yes, this is only the nuttx/CONTRIBUTING.md file update for now, but its formatting makes it possible to attach to the Documentation too, I can do that after we sort get the final form and commit :-)

cederom · 2025-03-10T02:41:00Z

Thank you folks for all the remarks and grammar suggestions! The update is pushed, please comment :-)

tmedicci · 2025-03-11T13:23:43Z

Thank you folks for all the remarks and grammar suggestions! The update is pushed, please comment :-)

Just re-reviewed: we are fine here. Can you mark the PR as ready-to-review, @cederom please?

cederom · 2025-03-11T18:04:30Z

@cederom: Thank you folks for all the remarks and grammar suggestions! The update is pushed, please comment :-)

@tmedicci: Just re-reviewed: we are fine here. Can you mark the PR as ready-to-review, @cederom please?

Done :-)

Please add more comments or resolve remaining discussions if all is fine folks :-)

Thanks!! :-)

simbit18 · 2025-03-12T09:43:22Z

Question for old PR before this Guidelines update.
For abandoned and very old PR, how will they be handled?
List filters -> is:pr is:open created:<2024-01-01
https://github.com/apache/nuttx/pulls?q=is%3Apr+is%3Aopen+created%3A%3C2024-01-01+state%3Aopen+

cederom · 2025-03-13T02:47:37Z

@simbit18: Question for old PR before this Guidelines update. For abandoned and very old PR, how will they be handled? List filters -> is:pr is:open created:<2024-01-01 https://github.com/apache/nuttx/pulls?q=is%3Apr+is%3Aopen+created%3A%3C2024-01-01+state%3Aopen+

Good question, probably we will ask for the PR/COMMIT description updates where necessary, but we will handle them as usual? If the PR is good right from start there is not much to be done except for code verification :-)

All this is to help everyone in the review process, set standards, and improve quality of the process / project. There are lots of good PRs already and overall is improving. There are not many active reviewers and still outnumbered. I think that long term self-compatibility is the goal and reason behind this update :-)

simbit18 · 2025-03-13T09:31:18Z

Hi @cederom I was referring to
Is a Pull Request open forever ? (PR is like diamonds !!! ) :)

Post Always Be Closing...Pull Requests
https://www.hanselman.com/blog/always-be-closingpull-requests

hartmannathan

Only found a couple of typos but otherwise LGTM, thanks to everyone!

hartmannathan · 2025-03-13T11:46:56Z

CONTRIBUTING.md


-* Provide only **signed commits** (`git commit -s`) with valid author
-  and email fields.
+The rules presented are here to ensure high qualiy code and documentation,


Suggested change

The rules presented are here to ensure high qualiy code and documentation,

The rules presented are here to ensure high quality code and documentation,

thank you fixed! :-)

hartmannathan · 2025-03-13T11:47:15Z

CONTRIBUTING.md

-* Provide only **signed commits** (`git commit -s`) with valid author
-  and email fields.
+The rules presented are here to ensure high qualiy code and documentation,
+standardized pull requests processing, as well as long term self-compatibilty


Suggested change

standardized pull requests processing, as well as long term self-compatibilty

standardized pull requests processing, as well as long term self-compatibility

thank you fixed! :-)

* This change comes from vast mailing list discussions on low PR/COMMIT quality, breaking changes, code quality, long term self-compatibility and maintenance, review problems, and how to improve things. * Contributing Guidelines updates vote history: 1. https://www.mail-archive.com/dev@nuttx.apache.org/msg12584.html 2. https://www.mail-archive.com/dev@nuttx.apache.org/msg12723.html Signed-off-by: Tomasz 'CeDeROM' CEDRO <tomek@cedro.info>

cederom · 2025-03-17T01:31:31Z

@acassis could you please review your remarks and close discussions if all is okay? :-)

cederom self-assigned this Mar 7, 2025

cederom marked this pull request as draft March 7, 2025 05:30

github-actions bot added Area: Documentation Improvements or additions to documentation Size: M The size of the change in this PR is medium labels Mar 7, 2025

This comment was marked as resolved.

Sign in to view

acassis reviewed Mar 7, 2025

View reviewed changes

tmedicci approved these changes Mar 7, 2025

View reviewed changes

hartmannathan reviewed Mar 7, 2025

View reviewed changes

cederom force-pushed the cederom-20250306-contributing-guidelines-update branch from 2a59298 to 973b133 Compare March 10, 2025 02:36

cederom force-pushed the cederom-20250306-contributing-guidelines-update branch from 973b133 to 8121b68 Compare March 11, 2025 17:59

cederom changed the title ~~[DRAFT] Apache NuttX RTOS Contributing Guidelines update.~~ 202503 Apache NuttX RTOS Contributing Guidelines update. Mar 11, 2025

cederom force-pushed the cederom-20250306-contributing-guidelines-update branch from 8121b68 to bef1ceb Compare March 11, 2025 18:00

cederom added this to Apache NuttX RTOS Mar 11, 2025

cederom marked this pull request as ready for review March 11, 2025 18:02

cederom force-pushed the cederom-20250306-contributing-guidelines-update branch from bef1ceb to 91e39a3 Compare March 11, 2025 19:27

lupyuen approved these changes Mar 13, 2025

View reviewed changes

jerpelea approved these changes Mar 13, 2025

View reviewed changes

simbit18 approved these changes Mar 13, 2025

View reviewed changes

hartmannathan approved these changes Mar 13, 2025

View reviewed changes

cederom force-pushed the cederom-20250306-contributing-guidelines-update branch from 91e39a3 to 9c598e1 Compare March 17, 2025 01:29

jerpelea approved these changes Mar 17, 2025

View reviewed changes

acassis approved these changes Mar 17, 2025

View reviewed changes

acassis merged commit b37a527 into apache:master Mar 17, 2025
39 checks passed

github-project-automation bot moved this to Done in Apache NuttX RTOS Mar 17, 2025

lupyuen mentioned this pull request Apr 9, 2025

[BREAKING]libc/elf: rename modlib to libelf #15765

Merged

		For maintenance reasons, code and documentation should be split into
		two separate commits in the same PR. This helps backporting and separates

	and 1600+ configurations, that are used in various project and products around
	and 1600+ configurations, that are used in various projects and products around

	business. Please try to keep your contributions high quality and compatible.
	businesses. Please try to keep your contributions high quality and compatible.

	follow the guidelines. These rules are based on daily experiences and helps
	follow the guidelines. These rules are based on daily experiences and help

	4. Alternative non-breaking solution is researched along the community.
	4. Alternative non-breaking solution is researched alongside the community.

	QEmu tests does not count here as in the past it did not catch problems
	QEmu tests do not count here as in the past they did not catch problems

	* you can also provide steps on how to reproduce problem and verify the change.
	* you can also provide steps on how to reproduce the problem and verify the change.

	The rules presented are here to ensure high qualiy code and documentation,
	The rules presented are here to ensure high quality code and documentation,

	standardized pull requests processing, as well as long term self-compatibilty
	standardized pull requests processing, as well as long term self-compatibility

202503 Apache NuttX RTOS Contributing Guidelines update. #15950

202503 Apache NuttX RTOS Contributing Guidelines update. #15950

Uh oh!

Conversation

cederom commented Mar 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as resolved.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

acassis Mar 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

acassis Mar 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tmedicci left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hartmannathan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tmedicci commented Mar 7, 2025

Uh oh!

cederom commented Mar 10, 2025

Uh oh!

cederom commented Mar 10, 2025

Uh oh!

tmedicci commented Mar 11, 2025

Uh oh!

cederom commented Mar 11, 2025

cederom commented Mar 7, 2025 •

edited

Loading

acassis Mar 7, 2025 •

edited

Loading

acassis Mar 7, 2025 •

edited

Loading

simbit18 commented Mar 12, 2025 •

edited

Loading