edtlib: Revert a regression affecting property filters at the child-binding level and bellow #80030
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This unit test was added specifically to cover a regression reported by the CI while working on [1]. Further work on related issues [2] showed that: - [1] and [2] are dead end: we need to first rethink how bindings (and especially child-bindings) are initialized - the inclusion mechanism supported by Zephyr deserves more systematic testing in edtlib if we want to work with confidence The approach we choose is to: - revert all changes made in [1] - from there, systematically add unit tests as we address the issues we identified (or the additional features we need) one after the other [1] edtlib: fix last modified semantic in included property specs [2] edtlib: Preserve paths of properties from included child bindings See also: zephyrproject-rtos#65221, zephyrproject-rtos#78095 This reverts commit 33bb3b6. Signed-off-by: Christophe Dufaza <[email protected]>
This unit test was added to cover the change introduced by [1]. Further work on related issues [2] showed that the chosen approach is dead end. We're reverting all changes made in [1]. [1] edtlib: fix last modified semantic in included property specs [2] edtlib: Preserve paths of properties from included child bindings See also: zephyrproject-rtos#65221, zephyrproject-rtos#78095 This reverts commit 70eaa61. Signed-off-by: Christophe Dufaza <[email protected]>
Use-case "B includes I includes X": - X is a base binding file, specifying common properties - I is an intermediary binding file, which includes X without modification nor filter - B includes I, filtering the properties it chooses to inherit with an allowlist or a blocklist Check that the properties inherited from X via I are actually filtered as B intends to, up to the grandchild-binding level. Signed-off-by: Christophe Dufaza <[email protected]>
[1] was introduced to get more valuable answers from the PropertySpec.path API, which is supposed to tell in which file the property's specification was "last modfied". Further work on related issues [2] showed that the approach chosen in [1] is dead end: we need to first rethink how bindings (and especially child-bindings) are initialized. [1] edtlib: fix last modified semantic in included property specs [2] edtlib: Preserve paths of properties from included child bindings See also: zephyrproject-rtos#65221, zephyrproject-rtos#78095 This reverts commit b3b5ad8. Signed-off-by: Christophe Dufaza <[email protected]>
ghost
self-requested a review
ghost
approved these changes
Oct 18, 2024
ghost
added
Regression
Contributor
Author
|
@decsny , just to keep you updated:
Thanks. Fixes #79948. |
decsny
approved these changes
Nov 5, 2024
decsny
requested review from
benediktibk,
fabiobaltieri,
gmarull,
henrikbrixandersen,
mmahadevan108,
nordicjm,
pdgendt and
tejlmand
fabiobaltieri
approved these changes
Nov 6, 2024
This was referenced Dec 30, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In #65221 we tried to get more valuable answers from the
PropertySpec.pathAPI, which is supposed to tell in which binding file a property's specification was last modified.Working on related issues (#78095) later showed that the chosen approach is dead end: we need to first rethink how bindings (and especially child-bindings) are initialized.
In the meantime, we have to tackle other concerns we've accidentally stumbled upon (#79948):
BfiltersIincludesX", the properties inherited fromXviaI, at the child-binding level and bellow, won't be filtered through theproperty-allowlistorproperty-blocklistinB: this is a regression introduced by edtlib: fix last modified semantic in included property specs #65221, which a revert will fixindependently of edtlib: fix last modified semantic in included property specs #65221, any use case where "BfiltersIfiltersX" will likely end with unexpected results (I'll open a separate PR with unit tests for these related issues that the revert does not address)[Edit: reverting #65221 will also address "
BfiltersIfiltersX", I'll instead add the complementary unit tests to #79900 once this is merged.]Hence the rationale for this PR:
Thanks.