Skip to content
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

treewide: Use hyphens instead of underscores for property separation #83352

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

rruuaanng
Copy link
Collaborator

This inherits from proposal #53502, with the main work as shown in the title. For details, see #53506.

@rruuaanng
Copy link
Collaborator Author

I have a lot of work to catch up on right now, like CI checks and other underlying changes (maybe due to a lack of focus).

@rruuaanng rruuaanng force-pushed the bindings-style branch 8 times, most recently from f765f9a to debb691 Compare December 25, 2024 07:11
@rruuaanng
Copy link
Collaborator Author

rruuaanng commented Dec 25, 2024

I wrote another PR which adds the devicetree coding style to the contributor guide.
(it provides a link to the upstream Linux kernel devicetree coding style.)

#83379

@rruuaanng
Copy link
Collaborator Author

I agree, I will separate the huge DTS commit into another PR later, it makes my force push very slow.

@rruuaanng
Copy link
Collaborator Author

rruuaanng commented Jan 9, 2025

@cfriedt The advancement of this proposal requires that all sub-PRs be merged and this PR be processed last. Because CI will throw a warning, we need to leave it to the end to solve it. They appear to have dozens of commits that modify the bindings and DTS between different subsystems.

@rruuaanng rruuaanng added the DNM This PR should not be merged (Do Not Merge) label Jan 9, 2025
@rruuaanng
Copy link
Collaborator Author

I believe I need to open this issue to the community, as it is a large-scale task. Given my current workload, I’m unable to dedicate my full attention to it. I will focus solely on handling this PR.

@cfriedt
Copy link
Member

cfriedt commented Jan 9, 2025

@cfriedt The advancement of this proposal requires that all sub-PRs be merged and this PR be processed last. Because CI will throw a warning, we need to leave it to the end to solve it. They appear to have dozens of commits that modify the bindings and DTS between different subsystems.

Call me skeptical (hopeful?), but I think that there is probably a way to a) activate CI checks only when a DT binding has changed, and b) find one binding to change that does not require changing the entire tree all at once.

@mbolivar
Copy link
Contributor

mbolivar commented Jan 9, 2025

The final step is to add an @mbolivar sign-off to the commit, specifically for the edtlib and CI checker sections. For these, I have referred to his implementation, and this needs to be accompanied by his personal authorization.

@rruuaanng thanks again for taking this on. Just to clarify, you don't need my personal authorization to add my signed-off-by: to any commits that you create, as long as I already added the signed-off-by: myself and opened a pull request with that commit to upstream zephyr.

When I added my signed-off-by: and created a PR, I was certifying for legal purposes (see the DCO docs) that I was able to submit the code under the appropriate license (apache 2 or other file-specific license) to the project at the time I opened the PR.

You are free to use the code under that license however you want, as long as you comply with the license's terms. You don't need any additional permission from me. The only thing you have to do is make sure that my signed-off-by: lines stay on any commits you submit yourself if they are adapted from my work.

@rruuaanng
Copy link
Collaborator Author

rruuaanng commented Jan 10, 2025

@cfriedt The advancement of this proposal requires that all sub-PRs be merged and this PR be processed last. Because CI will throw a warning, we need to leave it to the end to solve it. They appear to have dozens of commits that modify the bindings and DTS between different subsystems.

Call me skeptical (hopeful?), but I think that there is probably a way to a) activate CI checks only when a DT binding has changed, and b) find one binding to change that does not require changing the entire tree all at once.

Oh, I almost forgot. Currently, the binding style checks are skipped when no changes are made to the bindings. Let's wait and see.

        # If no bindings are changed, skip this check.
        try:
            subprocess.check_call(['git', 'diff', '--quiet', COMMIT_RANGE]
                                  + bindings_diff)
            nodiff = True
        except subprocess.CalledProcessError:
            nodiff = False
        if nodiff:
            self.skip('no changes to bindings were made')

Edit

If this operation needs to be implemented, I will need to remove the edtlib check during the build, otherwise, it won't pass the build and will cause the CI to fail.

    def _check_with_underscores(self, prop_name):
        # Allow special case property names
        # The content of this set needs to be consistent with the
        # content of 'scripts/ci/bindings_propertys_name_wl.yaml'
        wl = {'mmc-hs200-1_8v', 'mmc-hs400-1_8v'}

        if '_' in prop_name and prop_name not in wl:
            _err(f"'{prop_name}' should use hyphens instead of "
                 "underscores in the property name. "
                 f"The property names at '{self.edt.dts_path}' "
                 f"and '{self.binding_path}' must also be updated.")

That is, I did not need to make any changes to edtlib.

@rruuaanng
Copy link
Collaborator Author

The final step is to add an @mbolivar sign-off to the commit, specifically for the edtlib and CI checker sections. For these, I have referred to his implementation, and this needs to be accompanied by his personal authorization.

@rruuaanng thanks again for taking this on. Just to clarify, you don't need my personal authorization to add my signed-off-by: to any commits that you create, as long as I already added the signed-off-by: myself and opened a pull request with that commit to upstream zephyr.

When I added my signed-off-by: and created a PR, I was certifying for legal purposes (see the DCO docs) that I was able to submit the code under the appropriate license (apache 2 or other file-specific license) to the project at the time I opened the PR.

You are free to use the code under that license however you want, as long as you comply with the license's terms. You don't need any additional permission from me. The only thing you have to do is make sure that my signed-off-by: lines stay on any commits you submit yourself if they are adapted from my work.

I hope I haven't misunderstood. This is my current commit message:

    dts: bindings: Change the property names in the bindings

    Change the property names in the bindings and DTS to use
    hyphens(-) for separation instead of underscores(_).

    Signed-off-by: James Roy <[email protected]>
    Signed-off-by: Martí Bolívar <[email protected]>

This commit is based on your work, so I should change it to the following:

    dts: bindings: Change the property names in the bindings

    Change the property names in the bindings and DTS to use
    hyphens(-) for separation instead of underscores(_).

    Signed-off-by: Martí Bolívar <[email protected]>

But for content I have written myself, I only need to use my own signature:

    scripts: Add binding migration script

    This script replaces all underscore(_)
    separation in a binding with hyphens(-).

    Signed-off-by: James Roy <[email protected]>

Bindings now use hyphens(-) instead of
underscores(_) as property separators.

Signed-off-by: James Roy <[email protected]>
This script replaces all underscore(_)
separation in a binding with hyphens(-).

Signed-off-by: James Roy <[email protected]>
@zephyrbot zephyrbot added area: Continuous Integration Release Notes To be mentioned in the release notes labels Jan 10, 2025
@rruuaanng rruuaanng removed the DNM This PR should not be merged (Do Not Merge) label Jan 10, 2025
@rruuaanng rruuaanng marked this pull request as draft January 10, 2025 01:14
@rruuaanng
Copy link
Collaborator Author

rruuaanng commented Jan 10, 2025

CI throw

 561f94309a80cfa060b1f38e896f273adb4ade2c: author email (James Roy <[email protected]>) needs to match one of the signed-off-by entries.
Compliance error, check for error messages in the "Run Compliance Tests" step

Okay, it looks like only the signature issue remains. It seems like there might be a misunderstanding on my part? I might need to keep the original commit message.

from https://docs.zephyrproject.org/latest/contribute/guidelines.html#developer-certification-of-origin-dco

If you are altering an existing commit created by someone else, you must add your Signed-off-by: line without removing the existing one.

Hmmm.... It seems that the development process description isn't very clear on this. I apologize for not understanding it even after reading it multiple times. Does "keep" mean adding two signatures? Like this:

    dts: bindings: Change the property names in the bindings

    Change the property names in the bindings and DTS to use
    hyphens(-) for separation instead of underscores(_).

    Signed-off-by: James Roy <[email protected]>
    Signed-off-by: Martí Bolívar <[email protected]>

@rruuaanng rruuaanng requested a review from decsny January 10, 2025 01:20
@cfriedt
Copy link
Member

cfriedt commented Jan 10, 2025

It's definitely saying something about the bottom commit

Screenshot 2025-01-09 at 10 23 01 PM

@cfriedt
Copy link
Member

cfriedt commented Jan 10, 2025

@rruuaanng - you should be able to do git rebase -i HEAD~3, enter r instead of pick for that commit, and then update the commit message to add your signed-off-by line.

Implement a check in the CI pipeline to enforce
that property names in device tree bindings do
not contain underscores.

Signed-off-by: Martí Bolívar <[email protected]>
Signed-off-by: James Roy <[email protected]>
@rruuaanng
Copy link
Collaborator Author

@rruuaanng - you should be able to do git rebase -i HEAD~3, enter r instead of pick for that commit, and then update the commit message to add your signed-off-by line.

Should I add two signatures? Like this

    Signed-off-by: James Roy <[email protected]>
    Signed-off-by: Martí Bolívar <[email protected]>

@kartben
Copy link
Collaborator

kartben commented Jan 10, 2025

@rruuaanng - you should be able to do git rebase -i HEAD~3, enter r instead of pick for that commit, and then update the commit message to add your signed-off-by line.

Should I add two signatures? Like this

    Signed-off-by: James Roy <[email protected]>
    Signed-off-by: Martí Bolívar <[email protected]>

Yes

@rruuaanng rruuaanng marked this pull request as ready for review January 10, 2025 05:14
@rruuaanng
Copy link
Collaborator Author

Sure, everything looks good with the CI, and we can make a decision on the proposal.

Comment on lines +438 to +440
"./scripts/migrate_bindings_style.py" = [
"I001", # https://docs.astral.sh/ruff/rules/unsorted-imports
]
Copy link
Member

Choose a reason for hiding this comment

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

Probably can work around this using ruff - @pdgendt is fairly familiar with that.

@@ -36,6 +37,28 @@
import list_boards
import list_hardware

sys.path.insert(0, os.path.join(str(Path(__file__).resolve().parents[2]),
Copy link
Member

Choose a reason for hiding this comment

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

I'm surprised that so much needs to be added in this file



# bindings base
BINDINGS_PATH = ["dts/bindings/"]
Copy link
Member

Choose a reason for hiding this comment

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

Probably want this to be a list of Path objects

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Strings list are convenient for command lines to pass parameters, see line 84

@decsny decsny dismissed their stale review January 10, 2025 14:43

the commits changing the tree dont seem to exist anymore

@mbolivar
Copy link
Contributor

This is my current commit message:

This is correct -- for commits based on mine that you altered or incorporated here, use both my and your signed-off-by-lines.

But for work that is yours alone, use just your signed-off-by.

@rruuaanng
Copy link
Collaborator Author

rruuaanng commented Jan 11, 2025

@decsny They are scattered into a task list, and you can give some help when you have time :)
#83741

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Coding Guidelines Coding guidelines and style area: Continuous Integration area: Devicetree Binding PR modifies or adds a Device Tree binding area: Devicetree Release Notes To be mentioned in the release notes treewide 🧹
Projects
Status: Todo
Development

Successfully merging this pull request may close these issues.

8 participants