edtlib: fix last modified semantic in included property specs

[dottspina]

This PR follows issue #65135 .

Problem description

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 file that includes I1, modifying the specification for property x
• B is a top-level binding that includes I2, and specifies an additional property p

When enumerating B properties, we expect the PropertySpec.path API to answer:

• y was last modified by I1
• x was last modified by I2
• p was last modified by B

However, it will answer all properties were last modified by B.

The first commit in this PR is a unit test for this simple use-case: if picked alone, it should fail.

Analysis

The issue originates in the Binding constructor that will eventually initialize all (merged) property specs on its own behalf (self):

for prop_name in self.raw.get("properties", {}).keys():
    self.prop2specs[prop_name] = PropertySpec(prop_name, self)

Since PropertySpec.path is simply a convenience for PropertySpec.binding.path, all specifications will have the same path.

Or, put another way, we can't have per-specification paths without per-specification Binding instances:

• for each included YAML file a (top-level) binding includes: initialize specifications for the included properties on behalf of a new Binding instance that represents the included file contents (once the possible filters like "property-allowlist" have been applied, and taking care of the last modified semantic)
• eventually, update the (top-level) binding specifications with the properties it adds or modifies

Proposed solution(s)

After going around in circles a bit, I found Binding._load_raw() was where the relevant steps were happening:

• it's called for each included YAML file, and only for included files (not for top-level bindings, which just use yaml.load())
• it's there that Binding._merge_includes() turns recursive, and we must preserve the recursive nature of bindings initialization (we want each intermediate Binding instance to completely describe the properties it claims to specify)

Hence the approach implemented in this PR:

• hijack _load_raw() to initialize the included bindings and register them into prop2specs
• patch _merge_includes() to take into account filters are now applied in _load_raw()
• patch the end of Binding constructor to update prop2specs only with the properties the top-level binding adds or modifies

I was initially afraid this would introduce extra rereads of a same YAML file, but there's none: included files are still red once per top-level binding that will (recursively) include them (but now they're red when initializing their own Binding instances).

With these changes, the added unit test will pass without breaking the existing ones (which proves nothing more).

I understand touching bindings initialization in edtlib requires great care: non obvious side-effects could break Zephyr in its everyday use, e.g. causing incorrect DTS files, making it impossible to build applications (my first attempt at fixing actually did).

This is why I also suggested, in an humorous tone, that the right fix might as well be something like bellow:

     path:
       The file where this property was defined. In case a binding includes
-      other bindings, this is the file where the property was last modified.
+      other bindings, this is the file that specifies the top-level binding.

Which wouldn't really suit me (dtsh), though: I'd would very much like being able to show which YAML binding file defines (or modifies) which property.

So, could anyone familiar with edtlib:

• help me achieve a proper/better patch, that can be confirmed to not mess with normal EDT models initialization
• or suggest an alternative approach (to retrieve the origin of a property) that does not require to patch the library

Thanks.

[mbolivar-ampere]

LGTM, might need to retry CI. Thanks!

[mbolivar-ampere]

@dottspina the CI failure is unrelated to your PR. I think this should fix it: #66001

Once that is merged, you should be able to rebase and we can get this in.

[dottspina]

@dottspina the CI failure is unrelated to your PR. I think this should fix it: #66001

Once that is merged, you should be able to rebase and we can get this in.

Yes, this seems clearly related. I was just wondering if this PR was the only one affected.
Thanks for your prompt fix.

[mbolivar-ampere]

@dottspina the likely fix is merged, can you please rebase?

[dottspina]

@dottspina the likely fix is merged, can you please rebase?

Argh, you were right to be less optimistic than me: the CI still complains about the deprecated label property in dts/bindings/test/vnd,i2c.yaml.

Just to be sure: we're talking about commit b1532ce ?

Thanks.

[mbolivar-ampere]

Yes, I was expecting b1532ce to resolve this issue :(.

Looks like this will take some more investigation -- perhaps this PR is actually causing this to show up on its own?

[dottspina]

perhaps this PR is actually causing this to show up on its own?

@mbolivar-ampere , yes it is: I ran the twister tests on the main branch, and this error does not show up.
I think this PR breaks the property-blocklist filter in vnd,i2c.yaml.

I'll investigate a bit further and let you know.
Thanks.

[dottspina]

Bellow is a test-case the CI was failing to build:

compatible: "vnd,i2c"

include:
  - name: i2c-controller.yaml
    property-blocklist:
      - label

properties:
  label:
    type: string

where "label" is defined (and deprecated) in base.yaml, included by i2c-controller.yaml.

The issue (in this PR) was that the Binding object initialization for i2c-controller.yaml did not apply the "property-blocklist" from the top-level binding when including base.yaml: hence the deprecated "label" property in vnd,i2c.yaml.

I've update the PR to:

• pass the "property-allowlist" and "property-bocklist" filters from an including binding to the included ones
• merge these filters with those with which the binding would have called _load_raw()

With these changes the "label" property from base.yaml is properly blocked, and thus no more deprecated in vnd,i2c.yaml: this fixes the CI for this PR [*].

I also added a unit test (the third commit) to cover what happens when the filters set by a top-level binding should be applied to the files it will indirectly include (like in the test-cases that were causing the CI to fail).

@mbolivar-ampere , I've tested without commit b1532ce, and it looks like the issue you address there wasn't actually blocking this PR: perhaps you have more time than it first seemed to rework the tests with the "label" property deprecation in mind.

Thanks.

[*] When I run twister with -T tests/lib/devicetree, I get a few errors whose message I'm not sure to understand: Warning (unit_address_format): /memory@00000000: unit name should not have leading 0s.
They are not triggered by the CI because they happen only for platforms (qemu_xtensa[_mmu]) that it does not test with. I don't think these are related to this PR, but the messages really puzzle me ;-)

[github-actions]

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.

[dottspina]

Please e.g. @mbolivar-ampere or @gmarull , would you mind re-opening this ?

While I understand the underlying issue has absolutely no incidence on Zephyr's use of the edtlib library, this is a bug for DTSh which aims to help beginners understand Devicetree: a tool that says that the wakeup-source property is defined by the bindings for the "nordic,nrf52-flash-controller" compatible does not.

Since I already need to repackage edtlib (see e.g. #5e803eb), I'm thinking of applying this patch there, but would really prefer a code review: DTSh relying on a broken library won't help beginners either.

Thanks.