cederom commented on code in PR #15950: URL: https://github.com/apache/nuttx/pull/15950#discussion_r1986520976
########## CONTRIBUTING.md: ########## @@ -1,133 +1,438 @@ -# Contributing to Apache NuttX RTOS +# Apache NuttX RTOS Contributing Guidelines -Hi! Thank you for your interest in contributing to Apache NuttX RTOS :-) +Thank you for your interest in contributing to Apache NuttX RTOS :-) -## 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. -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 +the world. Remember that any code change may affect different users and their +business. Please try to keep your contributions high quality and compatible. -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 +us preserve long term self-compatibility and maintenance of the project. +NuttX is maintained by a small team of volunteers from around the world. -### Coding Standard +This document is split into two main parts: -* You should follow [NuttX C Coding Standard](https://nuttx.apache.org/docs/latest/contributing/coding_style.html). + 1. Contribution Rules. + 2. Contribution Templates. -* Your code will be automatically checked by GitHub Continuous Integration - (CI) system. If you see the "check" step fails, it is possible that this - happens due to style errors. +If you need more information please read the +[Full Contributing Documentation](https://nuttx.apache.org/docs/latest/contributing/index.html) +or ask questions at our [Social Media channels](https://nuttx.apache.org/community). -* Note that we require you to solve these issues and adapt all modified files - even if you didn't introduce the problem yourself (this way, - every contribution gets us closer to compliance). -### Commits -* Each commit **must** contain a meaningful **commit message** that consist of: - * **topic** (mandatory). - * **description** (optional, separate with blank line from topic). +## 1. Contribution Rules. -* **Prefix** commit topic with a functional context - (i.e. `sched: Fixed compiler warning.`). +### 1.1. Goals. -* Provide only **signed commits** (`git commit -s`) with valid author - and email fields. +Presented rules are here to assure high qualiy of the code and documentation, +standardized pull requests processing, as well as long term self-compatibilty +and maintenance of the project. -* Valid commit message example: +Because every change may affect users, products, or services around the world, +rules apply equally to all authors, reviewers, committers and maintainters. - ``` - net/can: Add g_ prefix to can_dlc_to_len and len_to_can_dlc. +This is our Check-List for processing every incoming pull request. +Also to filter out breaking changes and handle them accordingly. - 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. - Signed-off-by: AuthorName <Valid@EmailAddress> - ``` +### 1.2. Requirements are mandatory. -* If you create a proper commit message as explained above, the first line - will be automatically used as pull-request title and the rest is added as - description. Use it as a starting point to describe your Pull-Request (PR). +Each Contribution is a Pull Request (PR) that consists of one or more git +commits (COMMIT). Both PRs and COMMITs **must** adhere to requirements +presented in the Contributing Guidelines or will be auto-rejected +until fixed. Special cases are possible and outlined in a dedicated rules. -### Pull Requests +### 1.3. Proper change description. -* Be sure to **fill in the pull-request report** with meaningful content. - Be very descriptive, take your time. Good explanation, as for someone who - can see the issue for the first time, will help understand the change and - improve the assessment process. Please provide short descriptions even for - a trivial change. +Proper description of change is mandatory. Description **must** contain +detailed explanation on: - * **Summary:** It is important to provide information on why change is - necessary, what it exactly does and how, if new feature shows up, - provide references (dependencies, similar problems and solutions), etc. + * what is the purpose of change + * why it is necessary + * what it does / adds / fixes + * how things are added / changed / fixed / updated + * what is the impact (build / runtime / api / what area) + * how it was tested (build host, compiler, target, logs) + * references (i.e. `nuttx` and `nuttx-apps` changes). + * dependencies (if change depends on other change). - * **Impact:** State how (where applicable) that change affects users, build - process, hardware, documentation, security, compatibility, etc. +Local code build and real world hardware runtime test logs +**must** be provided (for code related changes), at least short form. +For fixes, "before" and "after" logs comparison is welcome. - * **Testing:** Please provide details on how did you verify the change, - what Host was used for build (OS, CPU, compiler, ..), what Target was - used for verification (arch, board:config, ..), etc. - Providing build and runtime logs from before and after change is highly - appreciated. +Description can be single or 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. -* Introduce only one functional change per pull-request. +Good description when read once will make others understand your work. +This allows faster review, quick understanding of change history (what +changed, why, how), and eventual problem identification / fix in future. -* Provide finished and verified solutions. - Clearly mark Work-In-Progress pull requests not yet ready for assessment. +### 1.4. COMMIT and PR descriptions are equally important. -## References +Git commit messages are as important as PR descriptions. -### Example Pull Request Report +Git log provides offline descriptions of each change that is +git client / interface independent. -#### Summary + +### 1.5. COMMIT requirements. + +Proper git commit message according to template (see 2.1) is **mandatory**, +or change is auto-rejected until fixed. Build and runtime logs are +optional here if these are too long and already provided in the PR. + +Git commit message consists of: + + 1. Topic with functional area prefix, `:` mark, short self-explanatory + functional context, `.` mark. + 2. Blank line. + 3. Description on what is changed, how, and why. May use several + lines, short sentences, or bullet points (see 1.3). + 4. Blank line. + 5. Signature (created with `git commit -s`). + +Valid git commit 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. + +Signed-off-by: AuthorName <Valid@EmailAddress> +``` + + +### 1.6. COMMIT message mandatory fields. + +Each git commit message **must** contain these fields, or change is +auto-rejected until fixed: + + 1. topic. + 2. description. + 3. signature (`git commit -s`). + +Although this seems to repeat rule 1.5, it clearly filters out commits +with no topic, description, or signature. + + +### 1.7. PR requirements. + +1. Proper description (see 1.3) of PR according to template (see 2.3) + is **mandatory**. All fields are required or change is auto-rejected + until fixed. +2. For code changes build and runtime logs are **mandatory** to prove code + was tested on **at least one** real world hardware target. +3. Pull Requests should be as small as possible and focused on only one + functional change. +4. Different functional changes must be provided in a separate Pull Requests. +5. PR may contain several commits but every single commit included + must not break break overall build, runtime, and compatibility, + especially for other components. +6. PR that breaks build or runtime anyhow is considered a Breaking Change + (see 1.12), is not welcome, and requires special handling (see 1.13). +7. PR that introduces new feature must have Documentation included (see 1.8). +8. Work-in-progress PRs should be marked `[WIP]` tag and set as "draft". +9. When changes for a 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 + that this is not a Breaking Change. + +Note that small team of maintainers is processing hundreds of PRs every month. +Providing high quality COMMITs and PRs really helps a lot! + +Please provide only finished and verified solutions. Every PR code update +(`git push -f` to your branch that represents the PR) each time triggers big +GitHub CI machinery that builds / cross-compiles your code to various targets +on various build hosts. This is not only slow but also expensive. + + +### 1.8. Documentation requirements. + +Changes **must** come with a documentation update (where applicable). + +Documentation is part of the `nuttx` git repository. If code change is part +of `nuttx-apps` repository then separate PR in `nuttx` repository is required. +Otherwise documentation should come in the same PR as the code update. + +For maintenance reasons, 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. + +If change presents new functionality documentation **must** be provided +along with the code (not in the future). + +If change requires existing documentation update it must be contained +along with the code change (not in the future). + +Successful documentation build logs (shortcut) are welcome. + +This helps us keep documentation in sync with the code. + +See: +1. https://github.com/apache/nuttx/tree/master/Documentation +2. https://github.com/apache/nuttx/blob/master/INVIOLABLES.md + + +### 1.9. Zero Trust approach to user testing. + +We implement zero trust approach to user provided testing. + +It is the change author duty to provide real world hardware build and runtime +logs for **at least one** real world hardware device. + +Remember that any code change may break things for others, please avoid that. + +This helps us filter out untested code commits. + +See: https://github.com/apache/nuttx/blob/master/INVIOLABLES.md + + +### 1.10. Long term maintenance and self-compatibility. + +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 instead of breaking existing features. + +Breaking changes are avoided and planned towards next major release (see 1.12). + +See: https://github.com/apache/nuttx/blob/master/INVIOLABLES.md + + +### 1.11. Experimental features. + +Experimental code and incomplete or experimental features that does not impact +overall project self-compatibility in terms of Breaking Changes (see 1.12) +**must** be clearly marked with a `[EXPERIMENTAL]` tag in the git commit topic +and PR title that will propagate to Release Notes. + +See: https://github.com/apache/nuttx/blob/master/INVIOLABLES.md + + +### TODO: 1.12. Breaking changes not welcome. + +Breaking changes are not welcome. **We do not "break by design"**. Please +provide only high quality code and think about other NuttX users. + +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" or "enforced changes" ideologies. + +"Breaking change" is anything that alters Build / Kernel / Architecture / API, +alters both nuttx and nuttx-apps repo at the same time, breaks +build / runtime / api for a single or many boards / architectures / +applications, breaks self-compatibility, breaks maintenance, +breaks build / runtime compatibility with existing release code (packages) +both for nuttx and nuttx-apps, etc. + +Remember that any code change may impact other users and their business! + +Changes should provide maximum self-compatibility with existing solutions, +should not impact build and runtime in a negative way, or may require design +reconsiderations such as providing alternative non-invasive user selectable +solutions (see 1.10). + +When breaking changes are unavoidable, these may be accepted, but need prior +discussion and agreement of the community and special handling (see 1.13). + +See: https://github.com/apache/nuttx/blob/master/INVIOLABLES.md + + +### TODO: 1.13. Breaking changes handling process. + +This rule complements "Breaking changes not welcome" rule. + +We avoid breaking changes unless absolutely necessary and unavoidable +(i.e. security fix, broken code, etc). Special case considerations apply: + + 1. First reviewer that recognizes a breaking change should block + accidental merge with "Request Changes" mark and ask for discussion. + 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. + 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). + 6. Breaking changes **must** be verified on **various different real + world hardware architectures**, build and runtime logs are + **mandatory**. Help of the community is welcome. + 7. Breaking change requires at least 4 independent positive PR reviews + (see 1.16), all discussions resolved, and zero "request changes". + 8. Change must be well documented (buid / runtime test logs, pr, git + commit, documentation, release notes, etc) with clear notes on how to + fix the introduced problems. + 9. Breaking Change must be clearly marked with a `[BREAKING]` tag in the + git commit topic and PR title that will propagate to Release Notes. + +See: https://github.com/apache/nuttx/blob/master/INVIOLABLES.md + + +### 1.14. Breaking changes build and runtime test logs are mandatory. + +Breaking changes are special case where build and runtime test logs +(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 +that revealed on a real world hardware after change was already merged. + +Although this seems to repeat rule 1.13, it ensures real world hardware +verification and minimizes possible negative impact on various users. + +See: https://github.com/apache/nuttx/blob/master/INVIOLABLES.md + + +### 1.15. Reviews reuqirements. + +Before PR can be merged to the master repository it requires: + + * 2 or more independent positive reviews. + * 4 or more independent positive reviews for Breaking Changes (see 1.12). + +In future we plan to add "areas" (such as trivial typo fixes or documentation +only updates) that may require only one reviewer. + + +### 1.16. Reviews independence. + +PR reviews should come from independent organizations. +NuttX is an Open-Source independent and unbiased project. + +Each PMC Member, Committer, and Reviewer must report up-to-date Affiliation +for clear identification. Commits should contain business email where +applicable. + +When code comes from the same organization as positive review, then review +is not considered independent, but still may provide helpful insight. + +Self review is not allowed. + +See: https://github.com/apache/nuttx/blob/master/INVIOLABLES.md + + +### 1.17. Merge rules. + +1. Each change **must** be provided as PR that undergoes independent review + process (see 1.16). +2. Single company / organization commit, review, and merge is not allowed. +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. Review Comment: 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 :-) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
