Skip to content

Conversation

@mbolivar-nordic
Copy link
Contributor

@mbolivar-nordic mbolivar-nordic commented Jan 4, 2023

Fixes: #53506

This tree-wide change cleans up existing property names which use _ instead of -, and adds a CI check to avoid new ones from creeping in. This requires a little bit of prep work to add a helper function to edtlib to make the check easier to write.

I found a few issues in check_compliance.py while I was adding this that I took the time to fix while I was here.

@mbolivar-nordic mbolivar-nordic added area: Devicetree area: Devicetree Binding PR modifies or adds a Device Tree binding area: Devicetree Tooling PR modifies or adds a Device Tree tooling treewide 🧹 labels Jan 4, 2023
@mbolivar-nordic mbolivar-nordic changed the title Start looking at DT bindings from check_compliance Enforce standard conventions in devicetree bindings Jan 4, 2023
@mbolivar-nordic mbolivar-nordic force-pushed the dt-bindings-checks branch 2 times, most recently from 04e7331 to f39e62b Compare January 13, 2023 22:03
@mbolivar-nordic mbolivar-nordic changed the title Enforce standard conventions in devicetree bindings Enforce - as word separator in devicetree property names Jan 13, 2023
@mbolivar-nordic mbolivar-nordic marked this pull request as ready for review January 13, 2023 22:11
@mbolivar-nordic mbolivar-nordic added this to the v3.3.0 milestone Jan 14, 2023
@mbolivar-nordic
Copy link
Contributor Author

mbolivar-nordic commented Jan 20, 2023

At least the compliance check works

That's a relief. And what do you know, the offending properties (which appeared as part of the rebase) are legitimate. I added a configuration file we can use to override the check on a property-by-property basis, with initial contents set to allow existing OK properties.


logger = None

# This ends up using the current repository if ZEPHYR_BASE is unset.
Copy link
Member

Choose a reason for hiding this comment

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

But we already have a default for ZEPHYR_BASE:

global ZEPHYR_BASE
ZEPHYR_BASE = os.environ.get('ZEPHYR_BASE')
if not ZEPHYR_BASE:
# Let the user run this script as ./scripts/ci/check_compliance.py without
# making them set ZEPHYR_BASE.
ZEPHYR_BASE = str(Path(__file__).resolve().parents[2])
# Propagate this decision to child processes.
os.environ['ZEPHYR_BASE'] = ZEPHYR_BASE

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But we already have a default for ZEPHYR_BASE:

Oh, I didn't see that this function was changing global variables. That's fun.

I still need this at module level so that check_compliance: re-work DevicetreeBindingsCheck later on in the PR works, so I'll just refactor this patch to remove the lines you mention as well.

Copy link
Member

Choose a reason for hiding this comment

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

But you did not remove those lines, did you?

@carlescufi
Copy link
Member

You wrote:

  1. compliance.yml happens to run the script from ZEPHYR_BASE, which
    is what the tests mostly seem to want anyway

But that's only true upstream. Downstream this is run from the root of the repo that the checks need to be run on.

@mbolivar-nordic
Copy link
Contributor Author

But that's only true upstream. Downstream this is run from the root of the repo that the checks need to be run on.

You're saying that we are running the zephyr/.github/workflows/compliance.yml file from non-zephyr repositories in downstreams?

If so, I'm having trouble understanding if you're asking for a change, though. The documented path_hint behavior is

    path_hint:
      The path the test runs itself in.  This is just informative and used in
      the message that gets printed when running the test.

and this PR is making the implementation match this documentation, so I don't see any changes that need doing, except maybe removing the "just informative" part. Am I missing something?

@mbolivar-nordic mbolivar-nordic removed this from the v3.3.0 milestone Jan 30, 2023
@mbolivar-nordic
Copy link
Contributor Author

@carlescufi ping, can you please explain what the changes you are requesting are? See my previous comment.

@fabiobaltieri
Copy link
Member

@carlescufi ping? It'd be cool to move forward with this.

@henrikbrixandersen
Copy link
Member

This needs rebasing.

@cfriedt
Copy link
Member

cfriedt commented Apr 20, 2023

@mbolivar-nordic - please rebase and resolve conflicts. I'll approve again

@github-actions
Copy link

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@henrikbrixandersen
Copy link
Member

Removing the stale label as I hope this will be picked up again.

@github-actions
Copy link

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@github-actions github-actions bot added the Stale label Aug 20, 2023
@carlescufi
Copy link
Member

@mbolivar-nordic apologies for the huge delay. Your patches make sense, in general, I will add a comment in the code itself.

@carlescufi
Copy link
Member

You're saying that we are running the zephyr/.github/workflows/compliance.yml file from non-zephyr repositories in downstreams?

No, not the same exact workflow, but a very similar one. See here:
https://github.com/nrfconnect/sdk-nrf/blob/7b40e24932178c3686fcd3dee71796b7b7962dc7/.github/workflows/compliance.yml#L78

@github-actions github-actions bot removed the Stale label Aug 29, 2023
@github-actions
Copy link

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@github-actions github-actions bot added the Stale label Oct 29, 2023
@github-actions github-actions bot closed this Nov 13, 2023
@cfriedt
Copy link
Member

cfriedt commented Nov 17, 2023

This would be a good thing to redo before lts3

@henrikbrixandersen
Copy link
Member

This would be a good thing to redo before lts3

Agreed. Would love to see this happen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Require recommended word separator (-) in devicetree property names

8 participants