-
Notifications
You must be signed in to change notification settings - Fork 6.8k
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
base: main
Are you sure you want to change the base?
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 |
---|---|---|
@@ -0,0 +1,11 @@ | ||
# This file is a YAML whitelist of property names that are allowed to | ||
# bypass the underscore check in bindings. These properties are exempt | ||
# from the rule that requires using '-' instead of '_'. | ||
# | ||
# The file content can be as shown below: | ||
# - propname1 | ||
# - propname2 | ||
# - ... | ||
|
||
- mmc-hs200-1_8v | ||
- mmc-hs400-1_8v |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,6 +22,7 @@ | |
import shutil | ||
import textwrap | ||
import unidiff | ||
import yaml | ||
|
||
from yamllint import config, linter | ||
|
||
|
@@ -36,6 +37,28 @@ | |
import list_boards | ||
import list_hardware | ||
|
||
sys.path.insert(0, os.path.join(str(Path(__file__).resolve().parents[2]), | ||
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'm surprised that so much needs to be added in this file |
||
'scripts/dts/python-devicetree/src')) | ||
from devicetree import edtlib | ||
|
||
|
||
# Let the user run this script as ./scripts/ci/check_compliance.py without | ||
# making them set ZEPHYR_BASE. | ||
ZEPHYR_BASE = os.environ.get('ZEPHYR_BASE') | ||
if not ZEPHYR_BASE: | ||
ZEPHYR_BASE = str(Path(__file__).resolve().parents[2]) | ||
# Propagate this decision to child processes. | ||
os.environ['ZEPHYR_BASE'] = ZEPHYR_BASE | ||
|
||
# Initialize the propertys name whitelist | ||
BINDINGS_PROPERTYS_WL = None | ||
with open(Path(__file__).parent / 'bindings_propertys_name_wl.yaml', 'r') as f: | ||
wl = yaml.safe_load(f.read()) | ||
if wl is not None: | ||
BINDINGS_PROPERTYS_WL = set(wl) | ||
else: | ||
BINDINGS_PROPERTYS_WL = {'',} | ||
|
||
logger = None | ||
|
||
def git(*args, cwd=None, ignore_non_zero=False): | ||
|
@@ -335,32 +358,75 @@ class DevicetreeBindingsCheck(ComplianceTest): | |
path_hint = "<zephyr-base>" | ||
|
||
def run(self, full=True): | ||
dts_bindings = self.parse_dt_bindings() | ||
|
||
for dts_binding in dts_bindings: | ||
self.required_false_check(dts_binding) | ||
bindings_diff, bindings = self.get_yaml_bindings() | ||
|
||
def parse_dt_bindings(self): | ||
# 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') | ||
|
||
for binding in bindings: | ||
self.check(binding, self.check_yaml_property_name) | ||
self.check(binding, self.required_false_check) | ||
|
||
@staticmethod | ||
def check(binding, callback): | ||
while binding is not None: | ||
callback(binding) | ||
binding = binding.child_binding | ||
|
||
def get_yaml_bindings(self): | ||
""" | ||
Returns a list of dts/bindings/**/*.yaml files | ||
Returns a list of 'dts/bindings/**/*.yaml' | ||
""" | ||
from glob import glob | ||
BINDINGS_PATH = 'dts/bindings/' | ||
bindings_diff_dir, bindings = set(), [] | ||
|
||
dt_bindings = [] | ||
for file_name in get_files(filter="d"): | ||
if 'dts/bindings/' in file_name and file_name.endswith('.yaml'): | ||
dt_bindings.append(file_name) | ||
for file_name in get_files(filter='d'): | ||
if BINDINGS_PATH in file_name: | ||
p = file_name.partition(BINDINGS_PATH) | ||
bindings_diff_dir.add(os.path.join(p[0], p[1])) | ||
|
||
return dt_bindings | ||
for path in bindings_diff_dir: | ||
yamls = glob(f'{os.fspath(path)}/**/*.yaml', recursive=True) | ||
bindings.extend(yamls) | ||
|
||
def required_false_check(self, dts_binding): | ||
with open(dts_binding) as file: | ||
for line_number, line in enumerate(file, 1): | ||
if 'required: false' in line: | ||
self.fmtd_failure( | ||
'warning', 'Devicetree Bindings', dts_binding, | ||
line_number, col=None, | ||
desc="'required: false' is redundant, please remove") | ||
bindings = edtlib.bindings_from_paths(bindings, ignore_errors=True) | ||
return list(bindings_diff_dir), bindings | ||
|
||
def check_yaml_property_name(self, binding): | ||
""" | ||
Checks if the property names in the binding file contain underscores. | ||
""" | ||
for prop_name in binding.prop2specs: | ||
if '_' in prop_name and prop_name not in BINDINGS_PROPERTYS_WL: | ||
better_prop = prop_name.replace('_', '-') | ||
print(f"Required: In '{binding.path}', " | ||
f"the property '{prop_name}' " | ||
f"should be renamed to '{better_prop}'.") | ||
self.failure( | ||
f"{binding.path}: property '{prop_name}' contains underscores.\n" | ||
f"\tUse '{better_prop}' instead unless this property name is from Linux.\n" | ||
"Or another authoritative upstream source of bindings for " | ||
f"compatible '{binding.compatible}'.\n" | ||
"\tHint: update 'bindings_propertys_name_wl.yaml' if you need to " | ||
"override this check for this property." | ||
) | ||
|
||
def required_false_check(self, binding): | ||
raw_props = binding.raw.get('properties', {}) | ||
for prop_name, raw_prop in raw_props.items(): | ||
if raw_prop.get('required') is False: | ||
self.failure( | ||
f'{binding.path}: property "{prop_name}": ' | ||
"'required: false' is redundant, please remove" | ||
) | ||
|
||
class KconfigCheck(ComplianceTest): | ||
""" | ||
|
@@ -1802,16 +1868,6 @@ def _main(args): | |
# The "real" main(), which is wrapped to catch exceptions and report them | ||
# to GitHub. Returns the number of test failures. | ||
|
||
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 | ||
|
||
# The absolute path of the top-level git directory. Initialize it here so | ||
# that issues running Git can be reported to GitHub. | ||
global GIT_TOP | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,91 @@ | ||
#!/usr/bin/env python3 | ||
|
||
# Copyright (c) 2025 James Roy <[email protected]> | ||
# SPDX-License-Identifier: Apache-2.0 | ||
|
||
# This script is used to replace the properties containing | ||
# underscores in the binding. | ||
|
||
|
||
import argparse | ||
import sys | ||
import os | ||
from pathlib import Path | ||
|
||
import yaml | ||
|
||
|
||
# bindings base | ||
BINDINGS_PATH = ["dts/bindings/"] | ||
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. Probably want this to be a list of Path objects 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. Strings list are convenient for command lines to pass parameters, see line 84 |
||
BINDINGS_PROPERTYS_WL = { | ||
"mmc-hs400-1_8v", | ||
"mmc-hs200-1_8v", | ||
} | ||
|
||
|
||
def parse_cmd_args(): | ||
parse = argparse.ArgumentParser(allow_abbrev=False) | ||
parse.add_argument("--path", nargs="+", help="User binding path directory") | ||
return parse.parse_args() | ||
|
||
|
||
def get_yaml_binding_paths() -> list: | ||
"""Returns a list of 'dts/bindings/**/*.yaml'""" | ||
from glob import glob | ||
|
||
bindings = [] | ||
for path in BINDINGS_PATH: | ||
yamls = glob(f"{os.fspath(path)}/**/*.yaml", recursive=True) | ||
bindings.extend(yamls) | ||
return bindings | ||
|
||
|
||
def replace_bindings_underscores(binding_paths): | ||
""" | ||
Replace all property names containing underscores in the bindings. | ||
""" | ||
for binding_path in binding_paths: | ||
with open(binding_path, encoding="utf-8") as f: | ||
yaml_binding = yaml.safe_load(f) | ||
properties = yaml_binding.get("properties", {}) | ||
for prop_name in properties: | ||
if '_' in prop_name and prop_name not in BINDINGS_PROPERTYS_WL: | ||
_replace_underscores(binding_path, prop_name) | ||
|
||
|
||
def _replace_underscores(binding_path, prop_name): | ||
"""Replace implementation""" | ||
with open(binding_path, "r+", encoding="utf-8") as f: | ||
lines = f.readlines() | ||
for i, rowline in enumerate(lines): | ||
line = rowline.split(":")[0].strip() | ||
if prop_name == line and "#" not in rowline: | ||
new_key = prop_name.replace("_", "-") | ||
if new_key != line: | ||
lines[i] = rowline.replace(line, new_key) | ||
f.seek(0) | ||
f.writelines(lines) | ||
|
||
|
||
def check_valid_cmd_path(): | ||
""" | ||
Checks whether the given command line argument is a valid path. | ||
""" | ||
for path in BINDINGS_PATH: | ||
path = Path(path) | ||
if not path.exists() or not path.is_dir(): | ||
raise FileNotFoundError(f"No such directory: '{path}'") | ||
|
||
|
||
if __name__ == '__main__': | ||
args = parse_cmd_args() | ||
|
||
if args.path is not None: | ||
BINDINGS_PATH.extend(args.path) | ||
|
||
check_valid_cmd_path() | ||
|
||
bindings = get_yaml_binding_paths() | ||
replace_bindings_underscores(bindings) | ||
|
||
sys.exit(0) |
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.
Probably can work around this using
ruff
- @pdgendt is fairly familiar with that.