Skip to content

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

Merged
merged 1 commit into from
Jan 17, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 3 additions & 24 deletions doc/build/kconfig/tips.rst
Original file line number Diff line number Diff line change
Expand Up @@ -876,31 +876,10 @@ For a Kconfig symbol that enables a driver/subsystem FOO, consider having just
usually be clear in the context of an option that can be toggled on/off, and
makes things consistent.

Style
=====

Header comments and other nits
==============================

A few formatting nits, to help keep things consistent:

- Use this format for any header comments at the top of ``Kconfig`` files:

.. code-block:: none

# <Overview of symbols defined in the file, preferably in plain English>
(Blank line)
# Copyright (c) 2019 ...
# SPDX-License-Identifier: <License>
(Blank line)
(Kconfig definitions)

- Format comments as ``# Comment`` rather than ``#Comment``

- Put a blank line before/after each top-level ``if`` and ``endif``

- Use a single tab for each indentation

- Indent help text with two extra spaces

See :ref:`coding_style` for style guidelines.

Lesser-known/used Kconfig features
**********************************
Expand Down
69 changes: 66 additions & 3 deletions doc/contribute/contributor_expectations.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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`.
Copy link
Collaborator

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


- 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
Copy link
Collaborator

Choose a reason for hiding this comment

The 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:
Copy link
Collaborator

Choose a reason for hiding this comment

The 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 ?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

The 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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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
Expand All @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is already covered and is a duplication, see https://docs.zephyrproject.org/latest/contribute/contributor_expectations.html#defining-smaller-prs.

Maybe it is important enough that it should be mentioned twice, my worry is that our guidelines and docs are growing organically and in a negative way, i.e. you might be adding something here that conflicts with somewhere else mentioned in some other part of the docs.

introduced by the PR. The documentation changes must use the proper
terminology as present in the existing pages, and must be written in American
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.

Expand Down
2 changes: 2 additions & 0 deletions doc/contribute/documentation/guidelines.rst
Original file line number Diff line number Diff line change
Expand Up @@ -700,6 +700,8 @@ Cross-referencing C documentation
Visual Elements
***************

.. _doc_images:

Images
======

Expand Down
50 changes: 49 additions & 1 deletion doc/contribute/guidelines.rst
Original file line number Diff line number Diff line change
Expand Up @@ -536,12 +536,28 @@ reference manuals, etc.
Coding Style
============

.. note::
Coding style is enforced on any new or modified code, but contributors are
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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but this is covered by linux already, in details, I do not think we need to repeat this here:

https://kernel.org/doc/html/latest/process/coding-style.html#naming

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not explicit some said, so it was made explicit here.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm in favor of incremental additions to our style guide that eventually make reference the Linux style guide obsolete. While unlikely, Linux can change their style guidelines at any time.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need to just fork the original linux style into our docs, call them zephyr style guidelines and never look back again.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kernel.org/doc/html/latest/process/coding-style.html#naming

the more I look at this, the more I think we should just go our own and stop referencing this from Linux. It looks more of a rant than guidelines in some parts.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kernel.org/doc/html/latest/process/coding-style.html#naming

the more I look at this, the more I think we should just go our own and stop referencing this from Linux. It looks more of a rant than guidelines in some parts.

Yes, totally, and I wanted to point this out the other day. It seriously feels very dated in the way it's written, to the point that it's not even reflecting the values we try to promote via our Code of Conduct, for example... Like, doing something wrong being refered to as being a "shooting offense"... Really?! So yeah if there's something where we can clearly state "we're not Linux" maybe it can start with coding style :) big +1 from me

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those are very good arguments. I've never before read the Linux coding guidelines like that, but you are absolutely right.

* The line length is 100 columns or fewer. In the documentation, longer lines
for URL references are an allowed exception.
Comment on lines 561 to 562
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given there are quite many places where line length is over 100 columns, does the rule "try to align with style in nearby code" apply? I.e. we're now saying we're not expecting contributors to fix non-conforming code/files, so do we want them to introduce potentially oddly/inconsistently wrapped content to existing files?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Up to others to comment on this, but for me this question really falls on the "case by case" and "common sense" criteria of the reviewers. I don't think rewriting the text or complicating it further will help solve all the corner cases like this one.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New code should comply with the requirements

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In principle I do not think this text needs changing, but ping me if you disagree @kartben.

* Add braces to every ``if``, ``else``, ``do``, ``while``, ``for`` and
Expand All @@ -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:

* CMake
Copy link
Member

@nashif nashif Jan 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agree with the couple of guidelines listed here, however, i think this is just touching the surface and we should be adding guidelines for each area and try to be complete and comprehensive and not just list those that someone happened to flag in some recent PR.

if we want to have some rules and guidelines for cmake files, we should spend the time to do this proper and call this done, if not, one second after this PR is merged we might be dealing with something new that would block another PR for ages, i.e. someone might not like the use for SET() or IF() and would prefer set() and if() or pefer a different way of quoting, i.e. set(my_path "${CMAKE_SOURCE_DIR}/include") instead of wrong set(my_path ${CMAKE_SOURCE_DIR}/include) etc. etc.

I propose to split this out and submit a PR for adding cmake style guidelines that goes beyond the above, ideally follow some work done in the cmake community itself and adopt such rules or use some large projects depending on cmake (Qt, KDE, ...) and borrow the rules from there where it makes sense.

Do that once, make sure the rules are as complete as possible, get this in and we do not have to come back to this topic again, unless there is something new and significant.

To support the above, here is a draft. #83713

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the draft PR and for the proposal. If we go this way though, we should probably then open two more PRs, one for Devicetree and one for Kconfig. And we should also decide whether we want the coding style to be in a single place (as it is in this PR) or in each respective section (as you propose with your draft CMake PR).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the draft PR and for the proposal. If we go this way though, we should probably then open two more PRs, one for Devicetree and one for Kconfig. And we should also decide whether we want the coding style to be in a single place (as it is in this PR) or in each respective section (as you propose with your draft CMake PR).

I like having one location for coding style - a single location provides is easier to find for new contributors (and for seasoned contributors to reference in PR comments).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can always cross reference them and still list them in one place. i.e. we reference linux styles and do not have them directly in the same page, we can do this with others. As each style guide for each components grows, it will all be one page and become unreadable/maintainable.
Having saud that, I do not case where they end up, as long as they are complete and more than just the result of cherrypicking.


* Indent with spaces, indentation is two spaces.
* Don't use space between commands (e.g. ``if``) and the corresponding opening
bracket (e.g. ``(``).

* Devicetree

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will leave this for a a second pass at it, don't want to extend the scope right now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a clear example of being too pick. Having a new line on that part makes easy to ready IMHO.
Why do we need this kind of rules? What we gain with that?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a clear example of being too pick. Having a new line on that part makes easy to ready IMHO. Why do we need this kind of rules? What we gain with that?

The files in that PR are a complete mess, compare that with other board files, it's completely at odds with that and doesn't even use the same layout in the same file, changing multiple times. It does not look easy to read

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From your pr:

Because makes it more readable.

So to answer:

Why do we need this kind of rules? What we gain with that?

We need it so people i.e. like you in your PR don't come up with your own styles completely at odds with what is standard in zephyr, we don't need every board or every subsystem to have their own coding styles which add nothing

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can either of you provide some stats perhaps? if most of the current code complies with one approach or other perhaps we can include it in the list. Otherwise I would rather not tread there to avoid overloading this PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I understand it, @nandojve does not want us to introduce guidelines that discourage empty lines between properties within a node.

I agree. Grouping properties using newlines is common practice in DTS, both in Zephyr and elsewhere.

I don't currently see anything in the content of this PR for introducing such guidelines, so perhaps we can leave it out of this PR - and address it separately, if anybody desires?

Copy link
Collaborator

@nordicjm nordicjm Dec 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can either of you provide some stats perhaps? if most of the current code complies with one approach or other perhaps we can include it in the list. Otherwise I would rather not tread there to avoid overloading this PR.

Out of 886 board dts files in zephyr, the following have spaces in aliases:

boards/mediatek/mt8196/mt8196_adsp.dts line 107
boards/microchip/mec1501modular_assy6885/mec1501modular_assy6885.dts line 25
boards/mxchip/az3166_iotdevkit/az3166_iotdevkit.dts line 19
boards/mxchip/az3166_iotdevkit/az3166_iotdevkit.dts line 23
boards/others/serpente/serpente.dts line 30

4 out of 886 = 0.4%

Copy link
Member

@nandojve nandojve Dec 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you put your script here? It seems to be too selective.

&usart2 {
status = "okay";
current-speed = <115200>;
pinctrl-0 = <&usart2_default>;
pinctrl-names = "default";
};

&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 ?

zephyr/MAINTAINERS.yml

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.

Copy link
Collaborator

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

* Kconfig

* Line length of 100 columns or fewer.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Line length of 100 columns or fewer.

Copy link
Member Author

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?

Copy link
Collaborator

@kartben kartben Dec 27, 2024

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.

Copy link
Collaborator

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.

Copy link
Member Author

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.

* Indent with tabs, except for ``help`` entry text which should be placed at
one tab plus two extra spaces.
* Leave a single empty line between option declarations.
* Use Statements like ``select`` carefully, see
:ref:`kconfig_tips_and_tricks` for more information.
* Format comments as ``# Comment`` rather than ``#Comment``
* Insert an empty line before/after each top-level ``if`` and ``endif``

Use these coding guidelines to ensure that your development complies with the
project's style and naming conventions.
Expand Down
1 change: 1 addition & 0 deletions doc/project/project_roles.rst
Original file line number Diff line number Diff line change
Expand Up @@ -350,6 +350,7 @@ Release Activity
Merge Criteria
++++++++++++++

* All :ref:`pr_requirements` must be met.
* Minimal of 2 approvals, including an approval by the designated assignee.
* Pull requests should be reviewed by at least a maintainer or collaborator of
each affected area; Unless the changes to a given area are considered trivial
Expand Down
Loading