From c47b1baaacacb44411048bc0c329176d84af3e89 Mon Sep 17 00:00:00 2001 From: Christophe Dufaza Date: Wed, 15 Nov 2023 05:23:20 +0100 Subject: [PATCH 1/3] edtlib: test "last modified" semantic for included property specs Make sure the property specs answered by the Binding.prop2specs API do not all claim (PropertySpec.path) they were last modified by the top-level binding. Signed-off-by: Christophe Dufaza --- .../tests/test-bindings-include/base.yaml | 7 +++++++ .../tests/test-bindings-include/modified.yaml | 7 +++++++ .../tests/test-bindings-include/top.yaml | 21 +++++++++++++++++++ .../python-devicetree/tests/test_edtlib.py | 12 +++++++++++ 4 files changed, 47 insertions(+) create mode 100644 scripts/dts/python-devicetree/tests/test-bindings-include/base.yaml create mode 100644 scripts/dts/python-devicetree/tests/test-bindings-include/modified.yaml create mode 100644 scripts/dts/python-devicetree/tests/test-bindings-include/top.yaml diff --git a/scripts/dts/python-devicetree/tests/test-bindings-include/base.yaml b/scripts/dts/python-devicetree/tests/test-bindings-include/base.yaml new file mode 100644 index 00000000000000..f564578b48d0b4 --- /dev/null +++ b/scripts/dts/python-devicetree/tests/test-bindings-include/base.yaml @@ -0,0 +1,7 @@ +# SPDX-License-Identifier: BSD-3-Clause + +properties: + x: + type: int + y: + type: int diff --git a/scripts/dts/python-devicetree/tests/test-bindings-include/modified.yaml b/scripts/dts/python-devicetree/tests/test-bindings-include/modified.yaml new file mode 100644 index 00000000000000..3d8130c1aedd0b --- /dev/null +++ b/scripts/dts/python-devicetree/tests/test-bindings-include/modified.yaml @@ -0,0 +1,7 @@ +# SPDX-License-Identifier: BSD-3-Clause + +include: base.yaml + +properties: + x: + required: true diff --git a/scripts/dts/python-devicetree/tests/test-bindings-include/top.yaml b/scripts/dts/python-devicetree/tests/test-bindings-include/top.yaml new file mode 100644 index 00000000000000..8fb9320676d515 --- /dev/null +++ b/scripts/dts/python-devicetree/tests/test-bindings-include/top.yaml @@ -0,0 +1,21 @@ +# SPDX-License-Identifier: BSD-3-Clause + +description: | + Top-level binding file for testing included property spec paths. + + base.yaml: specifies properties "x" and "y" + modified.yaml: includes base.yaml, modifies property "x" + top.yaml (this file): includes modified.yaml, specifies property "p" + + From the top-level binding, we expect: + - "x" was last modified in modified.yaml + - "y" was last modified in base.yaml + - "p" was last modified in top.yaml + +compatible: top-level + +include: modified.yaml + +properties: + p: + type: int diff --git a/scripts/dts/python-devicetree/tests/test_edtlib.py b/scripts/dts/python-devicetree/tests/test_edtlib.py index ca97fbcb9c2b07..151e61b16aa4fd 100644 --- a/scripts/dts/python-devicetree/tests/test_edtlib.py +++ b/scripts/dts/python-devicetree/tests/test_edtlib.py @@ -365,6 +365,18 @@ def test_include_filters(): assert set(child.prop2specs.keys()) == {'child-prop-1', 'child-prop-2', 'x', 'z'} # root level 'y' is blocked +def test_include_paths(): + '''Test "last modified" semantic for included bindings paths.''' + + fname2path = {'base.yaml': 'test-bindings-include/base.yaml', + 'modified.yaml': 'test-bindings-include/modified.yaml'} + + with from_here(): + top = edtlib.Binding('test-bindings-include/top.yaml', fname2path) + + assert 'modified.yaml' == os.path.basename(top.prop2specs["x"].path) + assert 'base.yaml' == os.path.basename(top.prop2specs["y"].path) + assert 'top.yaml' == os.path.basename(top.prop2specs["p"].path) def test_bus(): '''Test 'bus:' and 'on-bus:' in bindings''' From 1f0b3a02b58f42fef24905dbdfdbe05944125430 Mon Sep 17 00:00:00 2001 From: Christophe Dufaza Date: Wed, 15 Nov 2023 06:06:07 +0100 Subject: [PATCH 2/3] edtlib: fix "last modified" semantic for included property specs Although the PropertySpec.path attribute is documented as "the file where the property was last modified", all property specs in Binding.prop2specs will claim they were last modified by the top-level binding itself. Consider: - I1 is a base binding that specifies properties x and y - I2 is an "intermediate" binding that includes I1, modifying the specification for property x - B is a top-level bindings that includes I2, and specifies an additional property p When enumerating the properties of B, we expect the values of PropertySpec.path to tell us: - y was last modified by I1 - x was last modified by I2 - p was last modified by B However, the Binding constructor: - first merges all included bindings into the top-level one - eventually initializes specifications for all the defined properties As a consequence, all defined properties claim they were last modified by the top-level binding file. We should instead: - first, take into account their own specifications for the included properties - eventually update these specifications with the properties the top-level binding adds or modifies Signed-off-by: Christophe Dufaza --- .../src/devicetree/edtlib.py | 131 ++++++++++++++++-- 1 file changed, 116 insertions(+), 15 deletions(-) diff --git a/scripts/dts/python-devicetree/src/devicetree/edtlib.py b/scripts/dts/python-devicetree/src/devicetree/edtlib.py index 851706790b990f..088ef6e09967a2 100644 --- a/scripts/dts/python-devicetree/src/devicetree/edtlib.py +++ b/scripts/dts/python-devicetree/src/devicetree/edtlib.py @@ -163,7 +163,9 @@ class Binding: def __init__(self, path: Optional[str], fname2path: Dict[str, str], raw: Any = None, require_compatible: bool = True, - require_description: bool = True): + require_description: bool = True, + inc_allowlist: Optional[List[str]] = None, + inc_blocklist: Optional[List[str]] = None): """ Binding constructor. @@ -191,16 +193,36 @@ def __init__(self, path: Optional[str], fname2path: Dict[str, str], "description:" line. If False, a missing "description:" is not an error. Either way, "description:" must be a string if it is present in the binding. + + inc_allowlist: + The property-allowlist filter set by including bindings. + + inc_blocklist: + The property-blocklist filter set by including bindings. """ self.path: Optional[str] = path self._fname2path: Dict[str, str] = fname2path + self._inc_allowlist: Optional[List[str]] = inc_allowlist + self._inc_blocklist: Optional[List[str]] = inc_blocklist + if raw is None: if path is None: _err("you must provide either a 'path' or a 'raw' argument") with open(path, encoding="utf-8") as f: raw = yaml.load(f, Loader=_BindingLoader) + # Get the properties this binding modifies + # before we merge the included ones. + last_modified_props = list(raw.get("properties", {}).keys()) + + # Map property names to their specifications: + # - first, _merge_includes() will recursively populate prop2specs with + # the properties specified by the included bindings + # - eventually, we'll update prop2specs with the properties + # this binding itself defines or modifies + self.prop2specs: Dict[str, 'PropertySpec'] = {} + # Merge any included files into self.raw. This also pulls in # inherited child binding definitions, so it has to be done # before initializing those. @@ -224,10 +246,11 @@ def __init__(self, path: Optional[str], fname2path: Dict[str, str], # Make sure this is a well defined object. self._check(require_compatible, require_description) - # Initialize look up tables. - self.prop2specs: Dict[str, 'PropertySpec'] = {} - for prop_name in self.raw.get("properties", {}).keys(): + # Update specs with the properties this binding defines or modifies. + for prop_name in last_modified_props: self.prop2specs[prop_name] = PropertySpec(prop_name, self) + + # Initialize look up tables. self.specifier2cells: Dict[str, List[str]] = {} for key, val in self.raw.items(): if key.endswith("-cells"): @@ -291,18 +314,41 @@ def _merge_includes(self, raw: dict, binding_path: Optional[str]) -> dict: if isinstance(include, str): # Simple scalar string case - _merge_props(merged, self._load_raw(include), None, binding_path, - False) + # Load YAML file and register property specs into prop2specs. + inc_raw = self._load_raw(include, self._inc_allowlist, + self._inc_blocklist) + + _merge_props(merged, inc_raw, None, binding_path, False) elif isinstance(include, list): # List of strings and maps. These types may be intermixed. for elem in include: if isinstance(elem, str): - _merge_props(merged, self._load_raw(elem), None, - binding_path, False) + # Load YAML file and register property specs into prop2specs. + inc_raw = self._load_raw(elem, self._inc_allowlist, + self._inc_blocklist) + + _merge_props(merged, inc_raw, None, binding_path, False) elif isinstance(elem, dict): name = elem.pop('name', None) + + # Merge this include property-allowlist filter + # with filters from including bindings. allowlist = elem.pop('property-allowlist', None) + if allowlist is not None: + if self._inc_allowlist: + allowlist.extend(self._inc_allowlist) + else: + allowlist = self._inc_allowlist + + # Merge this include property-blocklist filter + # with filters from including bindings. blocklist = elem.pop('property-blocklist', None) + if blocklist is not None: + if self._inc_blocklist: + blocklist.extend(self._inc_blocklist) + else: + blocklist = self._inc_blocklist + child_filter = elem.pop('child-binding', None) if elem: @@ -313,10 +359,12 @@ def _merge_includes(self, raw: dict, binding_path: Optional[str]) -> dict: _check_include_dict(name, allowlist, blocklist, child_filter, binding_path) - contents = self._load_raw(name) + # Load YAML file, and register (filtered) property specs + # into prop2specs. + contents = self._load_raw(name, + allowlist, blocklist, + child_filter) - _filter_properties(contents, allowlist, blocklist, - child_filter, binding_path) _merge_props(merged, contents, None, binding_path, False) else: _err(f"all elements in 'include:' in {binding_path} " @@ -336,11 +384,17 @@ def _merge_includes(self, raw: dict, binding_path: Optional[str]) -> dict: return raw - def _load_raw(self, fname: str) -> dict: + + def _load_raw(self, fname: str, + allowlist: Optional[List[str]] = None, + blocklist: Optional[List[str]] = None, + child_filter: Optional[dict] = None) -> dict: # Returns the contents of the binding given by 'fname' after merging - # any bindings it lists in 'include:' into it. 'fname' is just the - # basename of the file, so we check that there aren't multiple - # candidates. + # any bindings it lists in 'include:' into it, according to the given + # property filters. + # + # Will also register the (filtered) included property specs + # into prop2specs. path = self._fname2path.get(fname) @@ -352,8 +406,55 @@ def _load_raw(self, fname: str) -> dict: if not isinstance(contents, dict): _err(f'{path}: invalid contents, expected a mapping') + # Apply constraints to included YAML contents. + _filter_properties(contents, + allowlist, blocklist, + child_filter, self.path) + + # Register included property specs. + self._add_included_prop2specs(fname, contents, allowlist, blocklist) + return self._merge_includes(contents, path) + def _add_included_prop2specs(self, fname: str, contents: dict, + allowlist: Optional[List[str]] = None, + blocklist: Optional[List[str]] = None) -> None: + # Registers the properties specified by an included binding file + # into the properties this binding supports/requires (aka prop2specs). + # + # Consider "this" binding B includes I1 which itself includes I2. + # + # We assume to be called in that order: + # 1) _add_included_prop2spec(B, I1) + # 2) _add_included_prop2spec(B, I2) + # + # Where we don't want I2 "taking ownership" for properties + # modified by I1. + # + # So we: + # - first create a binding that represents the included file + # - then add the property specs defined by this binding to prop2specs, + # without overriding the specs modified by an including binding + # + # Note: Unfortunately, we can't cache these base bindings, + # as a same YAML file may be included with different filters + # (property-allowlist and such), leading to different contents. + + inc_binding = Binding( + self._fname2path[fname], + self._fname2path, + contents, + require_compatible=False, + require_description=False, + # Recursively pass filters to included bindings. + inc_allowlist=allowlist, + inc_blocklist=blocklist, + ) + + for prop, spec in inc_binding.prop2specs.items(): + if prop not in self.prop2specs: + self.prop2specs[prop] = spec + def _check(self, require_compatible: bool, require_description: bool): # Does sanity checking on the binding. From 56f1637f7bbba111e58468d42c6332f5514a5a5e Mon Sep 17 00:00:00 2001 From: Christophe Dufaza Date: Tue, 5 Dec 2023 08:48:10 +0100 Subject: [PATCH 3/3] edtlib: test filters set by including bindings Make sure filters set by property-allowlist and property-blocklist in an including binding are recursively applied to included bindings. Signed-off-by: Christophe Dufaza --- .../tests/test-bindings-include/inc-base.yaml | 3 +++ .../tests/test-bindings-include/top-allows.yaml | 10 ++++++++++ .../tests/test-bindings-include/top-blocks.yaml | 10 ++++++++++ .../dts/python-devicetree/tests/test_edtlib.py | 17 +++++++++++++++++ 4 files changed, 40 insertions(+) create mode 100644 scripts/dts/python-devicetree/tests/test-bindings-include/inc-base.yaml create mode 100644 scripts/dts/python-devicetree/tests/test-bindings-include/top-allows.yaml create mode 100644 scripts/dts/python-devicetree/tests/test-bindings-include/top-blocks.yaml diff --git a/scripts/dts/python-devicetree/tests/test-bindings-include/inc-base.yaml b/scripts/dts/python-devicetree/tests/test-bindings-include/inc-base.yaml new file mode 100644 index 00000000000000..59dc45eab0dbbe --- /dev/null +++ b/scripts/dts/python-devicetree/tests/test-bindings-include/inc-base.yaml @@ -0,0 +1,3 @@ +# SPDX-License-Identifier: BSD-3-Clause + +include: base.yaml diff --git a/scripts/dts/python-devicetree/tests/test-bindings-include/top-allows.yaml b/scripts/dts/python-devicetree/tests/test-bindings-include/top-allows.yaml new file mode 100644 index 00000000000000..a4938eb0d67680 --- /dev/null +++ b/scripts/dts/python-devicetree/tests/test-bindings-include/top-allows.yaml @@ -0,0 +1,10 @@ +# SPDX-License-Identifier: BSD-3-Clause + +description: Test property-allowlist filters set by including bindings + +compatible: "top-allowlist" + +include: + - name: inc-base.yaml + property-allowlist: + - x diff --git a/scripts/dts/python-devicetree/tests/test-bindings-include/top-blocks.yaml b/scripts/dts/python-devicetree/tests/test-bindings-include/top-blocks.yaml new file mode 100644 index 00000000000000..787db223a939ee --- /dev/null +++ b/scripts/dts/python-devicetree/tests/test-bindings-include/top-blocks.yaml @@ -0,0 +1,10 @@ +# SPDX-License-Identifier: BSD-3-Clause + +description: Test property-blocklist filters set by including bindings. + +compatible: "top-blocklist" + +include: + - name: inc-base.yaml + property-blocklist: + - x diff --git a/scripts/dts/python-devicetree/tests/test_edtlib.py b/scripts/dts/python-devicetree/tests/test_edtlib.py index 151e61b16aa4fd..a81aa929370a07 100644 --- a/scripts/dts/python-devicetree/tests/test_edtlib.py +++ b/scripts/dts/python-devicetree/tests/test_edtlib.py @@ -378,6 +378,23 @@ def test_include_paths(): assert 'base.yaml' == os.path.basename(top.prop2specs["y"].path) assert 'top.yaml' == os.path.basename(top.prop2specs["p"].path) +def test_include_filters_included_bindings(): + '''Test filters set by including bindings.''' + fname2path = {'base.yaml': 'test-bindings-include/base.yaml', + 'inc-base.yaml': 'test-bindings-include/inc-base.yaml'} + + with from_here(): + top_allows = edtlib.Binding('test-bindings-include/top-allows.yaml', fname2path) + assert top_allows.prop2specs.get("x") + assert not top_allows.prop2specs.get("y") + + with from_here(): + top_blocks = edtlib.Binding('test-bindings-include/top-blocks.yaml', fname2path) + assert not top_blocks.prop2specs.get("x") + assert top_blocks.prop2specs.get("y") + + + def test_bus(): '''Test 'bus:' and 'on-bus:' in bindings''' with from_here():