- 
                Notifications
    
You must be signed in to change notification settings  - Fork 8.2k
 
doc: contribution guidelines: Clarify and extend #83117
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | 
|---|---|---|
| 
          
            
          
           | 
    @@ -87,27 +87,83 @@ Changes which require an RFC proposal include: | |
| Maintainers have the discretion to request that contributors create an RFC for | ||
| PRs that are too large or complicated. | ||
| 
     | 
||
| .. _pr_requirements: | ||
| 
     | 
||
| PR Requirements | ||
| *************** | ||
| 
     | 
||
| - Each commit in the PR must provide a commit message following the | ||
| :ref:`commit-guidelines`. | ||
| 
     | 
||
| - No fixup or merge commits are allowed, see :ref:`Contribution workflow` for | ||
| more information. | ||
| 
     | 
||
| - The PR description must include a summary of the changes and their rationales. | ||
| 
     | 
||
| - All files in the PR must comply with :ref:`Licensing | ||
| Requirements<licensing_requirements>`. | ||
| 
     | 
||
| - Follow the Zephyr :ref:`coding_style` and :ref:`coding_guidelines`. | ||
| - The code must follow the Zephyr :ref:`coding_style` and :ref:`coding_guidelines`. | ||
| 
     | 
||
| - PRs must pass all CI checks. This is a requirement to merge the PR. | ||
| - The PR must pass all CI checks, as described in :ref:`merge_criteria`. | ||
| Contributors may mark a PR as draft and explicitly request reviewers to | ||
| provide early feedback, even with failing CI checks. | ||
| 
     | 
||
| - When breaking a PR into multiple commits, each commit must build cleanly. The | ||
| - When breaking up a PR into multiple commits, each commit must build cleanly. The | ||
| CI system does not enforce this policy, so it is the PR author's | ||
| responsibility to verify. | ||
| 
         
      Comment on lines
    
      +112
     to 
      114
    
   
  There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Non-blocking - but this paragraph is now duplicated by the "Retain Bisectability" below.  | 
||
| 
     | 
||
| - Commits in a pull request should represent clear, logical units of change that are easy to review | ||
| and maintain bisectability. The following guidelines expand on this principle: | ||
| 
     | 
||
| 1. Distinct, Logical Units of Change | ||
| 
     | 
||
| Each commit should correspond to a self-contained, meaningful change. For example, adding a | ||
| feature, fixing a bug, or refactoring existing code should be separate commits. Avoid mixing | ||
| different types of changes (e.g., feature implementation and unrelated refactoring) in the same | ||
| commit. | ||
| 
     | 
||
| 2. Retain Bisectability | ||
| 
     | 
||
| Every commit in the pull request must build successfully and pass all relevant tests. This | ||
| ensures that git bisect can be used effectively to identify the specific commit that introduced | ||
| a bug or issue. | ||
| 
     | 
||
| 3. Squash Intermediary or Non-Final Development History | ||
| 
     | 
||
| During development, commits may include intermediary changes (e.g., partial implementations, | ||
| temporary files, or debugging code). These should be squashed or rewritten before submitting the | ||
| pull request. Remove non-final artifacts, such as: | ||
| 
         There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As partial implementations, ... should be squashed before submitting a zephyr PR, doesn't this exclude the possibility to cooperate on a PR ? Or does this mean that at every stage of development on a zephyr PR the commits should be squashed before submitting the update ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you can collaborate in a PR and mark it as a draft, when ready for review, the commits shall follow the guidelines. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the initial draft becomes too messy after some time, you can also close it and submit other, cleaner PR(s) to make the final review easier and faster. This is relatively common. Don't forget to link back to the initial one in case some reviewers have time for the long and messy story. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that what could be considered "partial implementation" are fixup type commits. While having some work presented in stages, for not only in draf, the commits should have rather reviewable quality, and should be squashed to one before merge. For example if you move something around file, or files, and then change the logic, it may aid reviewers to pick these changes in separate commits to see how the move happened, and then diff of the moved code. Having these separate on review makes sense, but on tree does not as the file is tested in CI as this was single-commit change, and probably neither "partial" commit can be removed without voiding that. On the other hand fixups do not help, because you when you pick one of commits in PR and then there is fixup for some logic, then you have a problem reviewing either commit: one is broken and second is incomplete without the first one, and does not give you full view of a change anyway. Let me throw here recent PR here #84058, where you can see a potential for two commit, for review time, change, where large block of text is remove and changed at the same time ;). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @de-nordic renaming and changing at the same time is a common enough use case, not always well handled by git, so maybe it is missing here. I agree it's MUCH easier to review when separate! Both before and AFTER merge. On the other hand, this PR is clearly insisting on reviewability and there are already recommendations here and elsewhere to have "atomic" commits. So hopefully the risk of misinterpretation is low? To avoid stalling this PR again, how about a follow-up one from you? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 
 Small follow-up submitted:  | 
||
| 
     | 
||
| * Temporary renaming of files that are later renamed again. | ||
| * Code that was rewritten or significantly changed in later commits. | ||
| 
     | 
||
| 4. Ensure Clean History Before Submission | ||
| 
     | 
||
| Use interactive rebasing (git rebase -i) to clean up the commit history before submitting the | ||
| pull request. This helps in: | ||
| 
     | 
||
| * Squashing small, incomplete commits into a single cohesive commit. | ||
| * Ensuring that each commit remains bisectable. | ||
| * Maintaining proper attribution of authorship while improving clarity. | ||
| 
     | 
||
| 5. Renaming and Code Rewrites | ||
| 
     | 
||
| If files or code are renamed or rewritten in later commits during development, squash or rewrite | ||
| earlier commits to reflect the final structure. This ensures that: | ||
| 
     | 
||
| * The history remains clean and easy to follow. | ||
| * Bisectability is preserved by eliminating redundant renaming or partial rewrites. | ||
| 
     | 
||
| 6. Attribution of Authorship | ||
| 
     | 
||
| While cleaning up the commit history, ensure that authorship attribution remains accurate. | ||
| 
     | 
||
| 7. Readable and Reviewable History | ||
| 
     | 
||
| The final commit history should be easy to understand for future maintainers. Logical units of | ||
| change should be grouped into commits that tell a clear, coherent story of the work done. | ||
| 
     | 
||
| - When major new functionality is added, tests for the new functionality shall | ||
| be added to the automated test suite. All API functions should have test cases | ||
| and there should be tests for the behavior contracts of the API. Maintainers | ||
| 
        
          
        
         | 
    @@ -133,6 +189,13 @@ PR Requirements | |
| - Changes to APIs must increment the API version number according to the API | ||
| version rules. | ||
| 
     | 
||
| - Documentation must be added and/or updated to reflect the changes in the code | ||
                
       | 
||
| introduced by the PR. The documentation changes must use the proper | ||
| terminology as present in the existing pages, and must be written in American | ||
                
      
                  carlescufi marked this conversation as resolved.
               
              
                Outdated
          
            Show resolved
            Hide resolved
         | 
||
| English. If you include images as part of the documentation, those must follow | ||
| the rules in :ref:`doc_images`. Please refer to :ref:`doc_guidelines` for | ||
| additional information. | ||
| 
     | 
||
| - PRs must also satisfy all :ref:`merge_criteria` before a member of the release | ||
| engineering team merges the PR into the zephyr tree. | ||
| 
     | 
||
| 
          
            
          
           | 
    ||
| Original file line number | Diff line number | Diff line change | 
|---|---|---|
| 
          
            
          
           | 
    @@ -700,6 +700,8 @@ Cross-referencing C documentation | |
| Visual Elements | ||
| *************** | ||
| 
     | 
||
| .. _doc_images: | ||
| 
     | 
||
| Images | ||
| ====== | ||
| 
     | 
||
| 
          
            
          
           | 
    ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| 
          
            
          
           | 
    @@ -536,12 +536,28 @@ reference manuals, etc. | |||||||||||||||||||||||||||||||||||||||||||||
| Coding Style | ||||||||||||||||||||||||||||||||||||||||||||||
| ============ | ||||||||||||||||||||||||||||||||||||||||||||||
| 
     | 
||||||||||||||||||||||||||||||||||||||||||||||
| .. note:: | ||||||||||||||||||||||||||||||||||||||||||||||
                
      
                  nordicjm marked this conversation as resolved.
               
              
                Outdated
          
            Show resolved
            Hide resolved
         | 
||||||||||||||||||||||||||||||||||||||||||||||
| Coding style is enforced on any new or modified code, but contributors are | ||||||||||||||||||||||||||||||||||||||||||||||
                
      
                  carlescufi marked this conversation as resolved.
               
              
                Outdated
          
            Show resolved
            Hide resolved
         | 
||||||||||||||||||||||||||||||||||||||||||||||
| not expected to correct the style on existing code that they are not | ||||||||||||||||||||||||||||||||||||||||||||||
| modifying. | ||||||||||||||||||||||||||||||||||||||||||||||
| 
     | 
||||||||||||||||||||||||||||||||||||||||||||||
| .. note:: | ||||||||||||||||||||||||||||||||||||||||||||||
| For style aspects where the guidelines don't offer explicit guidance or | ||||||||||||||||||||||||||||||||||||||||||||||
| permit multiple valid ways to express something, contributors should follow | ||||||||||||||||||||||||||||||||||||||||||||||
| the style of existing code in the tree, with higher importance given to | ||||||||||||||||||||||||||||||||||||||||||||||
| "nearby" code (first look at the function, then the same file, then | ||||||||||||||||||||||||||||||||||||||||||||||
| subsystem, etc). | ||||||||||||||||||||||||||||||||||||||||||||||
| 
     | 
||||||||||||||||||||||||||||||||||||||||||||||
| .. _Linux kernel coding style: | ||||||||||||||||||||||||||||||||||||||||||||||
| https://kernel.org/doc/html/latest/process/coding-style.html | ||||||||||||||||||||||||||||||||||||||||||||||
| 
     | 
||||||||||||||||||||||||||||||||||||||||||||||
| .. _snake case: | ||||||||||||||||||||||||||||||||||||||||||||||
| https://en.wikipedia.org/wiki/Snake_case | ||||||||||||||||||||||||||||||||||||||||||||||
| 
     | 
||||||||||||||||||||||||||||||||||||||||||||||
| In general, follow the `Linux kernel coding style`_, with the following | ||||||||||||||||||||||||||||||||||||||||||||||
| exceptions: | ||||||||||||||||||||||||||||||||||||||||||||||
| exceptions and clarifications: | ||||||||||||||||||||||||||||||||||||||||||||||
| 
     | 
||||||||||||||||||||||||||||||||||||||||||||||
| * Use `snake case`_ for code and variables. | ||||||||||||||||||||||||||||||||||||||||||||||
                
       | 
||||||||||||||||||||||||||||||||||||||||||||||
| * The line length is 100 columns or fewer. In the documentation, longer lines | ||||||||||||||||||||||||||||||||||||||||||||||
| for URL references are an allowed exception. | ||||||||||||||||||||||||||||||||||||||||||||||
                
       | 
||||||||||||||||||||||||||||||||||||||||||||||
| * Add braces to every ``if``, ``else``, ``do``, ``while``, ``for`` and | ||||||||||||||||||||||||||||||||||||||||||||||
| 
        
          
        
         | 
    @@ -554,6 +570,38 @@ exceptions: | |||||||||||||||||||||||||||||||||||||||||||||
| * Avoid using binary literals (constants starting with ``0b``). | ||||||||||||||||||||||||||||||||||||||||||||||
| * Avoid using non-ASCII symbols in code, unless it significantly improves | ||||||||||||||||||||||||||||||||||||||||||||||
| clarity, avoid emojis in any case. | ||||||||||||||||||||||||||||||||||||||||||||||
| * Use proper capitalization of nouns in code comments (e.g. ``UART`` and not | ||||||||||||||||||||||||||||||||||||||||||||||
| ``uart``, ``CMake`` and not ``cmake``). | ||||||||||||||||||||||||||||||||||||||||||||||
| 
     | 
||||||||||||||||||||||||||||||||||||||||||||||
| Beyond C code, the following coding style rules apply to other types of files: | ||||||||||||||||||||||||||||||||||||||||||||||
                
      
                  fabiobaltieri marked this conversation as resolved.
               
              
                Outdated
          
            Show resolved
            Hide resolved
                
      
                  carlescufi marked this conversation as resolved.
               
              
                Outdated
          
            Show resolved
            Hide resolved
         | 
||||||||||||||||||||||||||||||||||||||||||||||
| 
     | 
||||||||||||||||||||||||||||||||||||||||||||||
| * CMake | ||||||||||||||||||||||||||||||||||||||||||||||
                
       | 
||||||||||||||||||||||||||||||||||||||||||||||
| 
     | 
||||||||||||||||||||||||||||||||||||||||||||||
| * Indent with spaces, indentation is two spaces. | ||||||||||||||||||||||||||||||||||||||||||||||
| * Don't use space between commands (e.g. ``if``) and the corresponding opening | ||||||||||||||||||||||||||||||||||||||||||||||
| bracket (e.g. ``(``). | ||||||||||||||||||||||||||||||||||||||||||||||
| 
     | 
||||||||||||||||||||||||||||||||||||||||||||||
| * Devicetree | ||||||||||||||||||||||||||||||||||||||||||||||
                
      
                  carlescufi marked this conversation as resolved.
               
              
                Outdated
          
            Show resolved
            Hide resolved
         | 
||||||||||||||||||||||||||||||||||||||||||||||
| 
     | 
||||||||||||||||||||||||||||||||||||||||||||||
| * Indent with tabs. | ||||||||||||||||||||||||||||||||||||||||||||||
| * Follow the Devicetree specification conventions and rules. | ||||||||||||||||||||||||||||||||||||||||||||||
| * Use dashes (``-``) as word separators for node and property names. | ||||||||||||||||||||||||||||||||||||||||||||||
| * Use underscores (``_``) as word separators in node labels. | ||||||||||||||||||||||||||||||||||||||||||||||
                
      
                  carlescufi marked this conversation as resolved.
               
              
                Outdated
          
            Show resolved
            Hide resolved
         | 
||||||||||||||||||||||||||||||||||||||||||||||
| * Leave a single space on each side of the equal sign (``=``) in property | ||||||||||||||||||||||||||||||||||||||||||||||
| definitions. | ||||||||||||||||||||||||||||||||||||||||||||||
| * Don't insert empty lines before a dedenting ``};``. | ||||||||||||||||||||||||||||||||||||||||||||||
| * Insert a single empty line to separate nodes at the same hierarchy level. | ||||||||||||||||||||||||||||||||||||||||||||||
| 
     | 
||||||||||||||||||||||||||||||||||||||||||||||
                
       | 
||||||||||||||||||||||||||||||||||||||||||||||
| &usart2 { | |
| status = "okay"; | |
| current-speed = <115200>; | |
| pinctrl-0 = <&usart2_default>; | |
| pinctrl-names = "default"; | |
| }; | 
zephyr/boards/atmel/sam/sam_e70_xplained/sam_e70_xplained-common.dtsi
Lines 57 to 62 in dabeb9c
| &afec0 { | |
| status = "okay"; | |
| pinctrl-0 = <&afec0_default>; | |
| pinctrl-names = "default"; | |
| }; | 
Is this change only valid for the boards directory? Or we are looking only there because @nordicjm is a collaborator of that part.
BTW who is 57300 ?
Lines 594 to 602 in dabeb9c
| Board/SoC configuration: | |
| status: maintained | |
| maintainers: | |
| - tejlmand | |
| collaborators: | |
| - galak | |
| - nashif | |
| - nordicjm | |
| - "57300" | 
Nevermind, GH really allow a number be person.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As said, only for aliases, https://pastebin.com/tuv8VLsK
Is this change only valid for the boards directory? Or we are looking only there because @nordicjm is a collaborator of that part.
It's there because that's where board aliases are
        
          
              
                  carlescufi marked this conversation as resolved.
              
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
              
                  carlescufi marked this conversation as resolved.
              
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
              
                Outdated
          
        
      There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| * Line length of 100 columns or fewer. | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why? This makes sense in the context of Kconfig, because in Devicetree it is not enforced unless I am mistaken?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because I was under the impression that the enclosing section, "Coding style", already applies to everything? Why would this need to be repeated for Kconfig? And why is it not repeated for CMake, then?
[...] in Devicetree it is not enforced unless I am mistaken?
Oh is it not? I had no idea fwiw :) and I don't think that's what the text says in its current form either. If there are exceptions they need to be mentioned otherwise it reads as everything is 100 columns or fewer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reading this again it looks like "Coding Style" is probably meant to read "C coding style", and that you probably didn't mean to fold Kconfig/devicetree/CMake "under" this section as this has really nothing to do with C, and can mistakenly lead people like myself to then interpret "Coding guidelines" as being "General style guidelines" (and e.g. interpret the bullet "In the documentation, longer lines for URL references are an allowed exception." as applying to RST files, whereas I guess the intent was to apply to Doxygen documentation?)
Related(ish): The "Coding guidelines" section is pretty buried in the document.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What you say is correct and would benefit from a cleanup, but I am trying really really hard to limit the scope of this PR. If I start now rewriting the Coding Style section I will open a can of worms that I would rather leave closed in this iteration, we can then do another pass cleaning up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"must follow coding guidelines" contradicts what coding guidelines actually say -- the linked page says we "begin enforcement on a limited [and undocumented?] scope" as we're preparing to enter "Stage II", and stage I literally says that PR cannot be blocked when there are violations.
Talked to @nashif and it might be that this whole stage thingy is just obsolete. cc @keith-zephyr
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#84058