Recursive group filter imports are not working properly #663
Assignees
Labels
bug
Something isn't working
Comments
mbolivar-nordic
added a commit
to mbolivar-nordic/west
that referenced
this issue
May 16, 2023
Fixing the test case logic causes it to fail, so mark it xfail. This is bug zephyrproject-rtos#663. The test case test_group_filter_self_import() is incorrect, which conveniently enough provides steps to reproduce. The test case should read as follows (patch applies to f6f5cf6): diff --git a/tests/test_manifest.py b/tests/test_manifest.py index 14f2941..e934b71 100644 --- a/tests/test_manifest.py +++ b/tests/test_manifest.py @@ -2828,7 +2828,7 @@ def test_group_filter_imports(manifest_repo): sha2 = setup_project('project2', '[+gy,+gy,-gz]') v0_9_expected = ['+ga', '-gc'] - v0_10_expected = ['-ga', '-gb', '-gc', '-gw', '-gy', '-gz'] + v0_10_expected = ['-ga', '-gb', '-gc', '-gw', '-gz'] # # Basic tests of the above setup. In other words, west incorrectly concludes that group 'gy' is disabled in this scenario, when it should be enabled. The test creates the following layout, where ./mp/west.yml is the main manifest: ─────────────────────────────────────── File: ./mp/west.yml ─────────────────────────────────────── manifest: version: "0.10" group-filter: [+ga,-gc] projects: - name: project1 revision: ... import: true - name: project2 revision: ... import: true self: import: self-import.yml .. ─────────────────────────────────────── ─────────────────────────────────────── File: ./project1/west.yml ─────────────────────────────────────── manifest: group-filter: [-gw,-gw,+gx,-gy] ─────────────────────────────────────── ─────────────────────────────────────── File: ./project2/west.yml ─────────────────────────────────────── manifest: group-filter: [+gy,+gy,-gz] ─────────────────────────────────────── ─────────────────────────────────────── File: ./mp/self-import.yml ─────────────────────────────────────── manifest: group-filter: [-ga,-gb] ─────────────────────────────────────── The west docs say: In other words, let: - the submanifest resolved from self-import have group filter self-filter - the top-level manifest file have group filter top-filter - the submanifests resolved from import-1 through import-N have group filters filter-1 through filter-N respectively The final resolved group-filter value is then filter1 + filter-2 + ... + filter-N + top-filter + self-filter, where + here refers to list concatenation. - https://docs.zephyrproject.org/latest/develop/west/manifest.html Applying these rules, the final filter should be concatenated from ./project1/west.yml, ./project2/west.yml, ./mp/west.yml, ./mp/self-import.yml, in that order. Since neither ./mp/west.yml nor ./mp/self-import.yml have a group filter which affects gy, the result should be that it is enabled, since ./project2/west.yml enables it explicitly. Fix the test so it matches the expected behavior. We'll fix the implementation in a separate commit. Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
mbolivar-nordic
added a commit
to mbolivar-nordic/west
that referenced
this issue
May 16, 2023
This involves a rework of the group filter mechanism, which has the
side effect of fixing its behavior to match its documentation. We can
therefore drop the xfail on a related test.
This is a top level manifest key that takes a boolean. The default
value is 'true' and is west's current behavior. The behavior when
this value is false can be shown by this example:
manifest:
group-filter: [-foo]
import-group-filters: false
projects:
- name: proj
import: true
self:
import: submanifest.yml
Here, the final Manifest.group_filter attribute for the manifest
whose data are shown above will always be [-foo], regardless of what
whe imported manifest data from 'proj' or 'submanifest.yml' are.
The purpose of this extension is to allow the combination of 'groups'
and 'import':
- In previous versions of west, we could not combine the two, because
in pathological edge cases, we could end up resolving an import for a
group which was later disabled by an import that happened later on.
- With this new backwards-compatible enhancement to the manifest file
format, we can combine 'import:' with 'groups:' in a single project
whenever 'import-group-filters:' is 'false'. This is because we will
know the final 'group_filter' attribute for the entire Manifest at
the time we perform the import, and it cannot be changed later by
any imports we might perform.
Since this new behavior would break backwards compatibility if made the
default, we make it explicitly opt-in by requiring
'import-group-filters: false'.
Fixes: zephyrproject-rtos#663
Fixes: zephyrproject-rtos#652
Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
carlescufi
pushed a commit
that referenced
this issue
Jun 1, 2023
Fixing the test case logic causes it to fail, so mark it xfail. This is bug #663. The test case test_group_filter_self_import() is incorrect, which conveniently enough provides steps to reproduce. The test case should read as follows (patch applies to f6f5cf6): diff --git a/tests/test_manifest.py b/tests/test_manifest.py index 14f2941..e934b71 100644 --- a/tests/test_manifest.py +++ b/tests/test_manifest.py @@ -2828,7 +2828,7 @@ def test_group_filter_imports(manifest_repo): sha2 = setup_project('project2', '[+gy,+gy,-gz]') v0_9_expected = ['+ga', '-gc'] - v0_10_expected = ['-ga', '-gb', '-gc', '-gw', '-gy', '-gz'] + v0_10_expected = ['-ga', '-gb', '-gc', '-gw', '-gz'] # # Basic tests of the above setup. In other words, west incorrectly concludes that group 'gy' is disabled in this scenario, when it should be enabled. The test creates the following layout, where ./mp/west.yml is the main manifest: ─────────────────────────────────────── File: ./mp/west.yml ─────────────────────────────────────── manifest: version: "0.10" group-filter: [+ga,-gc] projects: - name: project1 revision: ... import: true - name: project2 revision: ... import: true self: import: self-import.yml .. ─────────────────────────────────────── ─────────────────────────────────────── File: ./project1/west.yml ─────────────────────────────────────── manifest: group-filter: [-gw,-gw,+gx,-gy] ─────────────────────────────────────── ─────────────────────────────────────── File: ./project2/west.yml ─────────────────────────────────────── manifest: group-filter: [+gy,+gy,-gz] ─────────────────────────────────────── ─────────────────────────────────────── File: ./mp/self-import.yml ─────────────────────────────────────── manifest: group-filter: [-ga,-gb] ─────────────────────────────────────── The west docs say: In other words, let: - the submanifest resolved from self-import have group filter self-filter - the top-level manifest file have group filter top-filter - the submanifests resolved from import-1 through import-N have group filters filter-1 through filter-N respectively The final resolved group-filter value is then filter1 + filter-2 + ... + filter-N + top-filter + self-filter, where + here refers to list concatenation. - https://docs.zephyrproject.org/latest/develop/west/manifest.html Applying these rules, the final filter should be concatenated from ./project1/west.yml, ./project2/west.yml, ./mp/west.yml, ./mp/self-import.yml, in that order. Since neither ./mp/west.yml nor ./mp/self-import.yml have a group filter which affects gy, the result should be that it is enabled, since ./project2/west.yml enables it explicitly. Fix the test so it matches the expected behavior. We'll fix the implementation in a separate commit. Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
|
Well, oops. This was actually a documentation bug, not a west bug, but I'm going to keep this open so that we can close it automatically when we turn off the xfail in the tests introduced by commit d0e6b9a. Earlier in the same section, we see:
Where the "import order" is defined as:
This means that That means that, instead of saying this:
The documentation should say:
Notice it currently goes from filter-1 to filter-N. It should go from filter-N to filter-1. |
mbolivar-nordic
added a commit
to mbolivar-nordic/west
that referenced
this issue
Jun 1, 2023
This reverts commit d0e6b9a. West was right, the docs are wrong/inconsistent. The commit I'm reverting was following the west documentation it referenced correctly, but the documentation itself was wrong (and in fact was inconsistent with earlier documentation on the same page). The basis for making commit d0e6b9a was that the docs currently say: In other words, let: - the submanifest resolved from self-import have group filter self-filter - the top-level manifest file have group filter top-filter - the submanifests resolved from import-1 through import-N have group filters filter-1 through filter-N respectively The final resolved group-filter value is then filter1 + filter-2 + ... + filter-N + top-filter + self-filter, where + here refers to list concatenation. But earlier in the same section, they also say: The resolved manifest has a group-filter value which is the result of concatenating the group-filter values in the top-level manifest and any imported manifests. Manifest files which appear earlier in the import order have higher precedence and are therefore concatenated later into the final group-filter. Where the "import order" is defined higher up as: Importing is done in this order: 1. Manifests from self-import are imported first. 2. The top-level manifest file’s definitions are handled next. 3. Manifests from import-1, …, import-N, are imported in that order. This means that import-1 happens **earlier** than import-N, and should therefore have **higher** precedence when it comes to computing the final group filter. But putting import-N **later** in the concanated group filter actually means import-1 has **lower** precedence. So the docs are inconsistent. To fix the docs so they are consistent and match west's current behavior, we should replace this: The final resolved group-filter value is then filter1 + filter-2 + ... + filter-N + top-filter + self-filter, where + here refers to list concatenation. with this: The final resolved group-filter value is then filter-N + ... + filter-1 + top-filter + self-filter, where + here refers to list concatenation. That is: - docs currently go from: filter-1 to filter-N - they should go from: filter-N to filter-1. Fixes: zephyrproject-rtos#663 Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
carlescufi
pushed a commit
that referenced
this issue
Jun 2, 2023
This reverts commit d0e6b9a. West was right, the docs are wrong/inconsistent. The commit I'm reverting was following the west documentation it referenced correctly, but the documentation itself was wrong (and in fact was inconsistent with earlier documentation on the same page). The basis for making commit d0e6b9a was that the docs currently say: In other words, let: - the submanifest resolved from self-import have group filter self-filter - the top-level manifest file have group filter top-filter - the submanifests resolved from import-1 through import-N have group filters filter-1 through filter-N respectively The final resolved group-filter value is then filter1 + filter-2 + ... + filter-N + top-filter + self-filter, where + here refers to list concatenation. But earlier in the same section, they also say: The resolved manifest has a group-filter value which is the result of concatenating the group-filter values in the top-level manifest and any imported manifests. Manifest files which appear earlier in the import order have higher precedence and are therefore concatenated later into the final group-filter. Where the "import order" is defined higher up as: Importing is done in this order: 1. Manifests from self-import are imported first. 2. The top-level manifest file’s definitions are handled next. 3. Manifests from import-1, …, import-N, are imported in that order. This means that import-1 happens **earlier** than import-N, and should therefore have **higher** precedence when it comes to computing the final group filter. But putting import-N **later** in the concanated group filter actually means import-1 has **lower** precedence. So the docs are inconsistent. To fix the docs so they are consistent and match west's current behavior, we should replace this: The final resolved group-filter value is then filter1 + filter-2 + ... + filter-N + top-filter + self-filter, where + here refers to list concatenation. with this: The final resolved group-filter value is then filter-N + ... + filter-1 + top-filter + self-filter, where + here refers to list concatenation. That is: - docs currently go from: filter-1 to filter-N - they should go from: filter-N to filter-1. Fixes: #663 Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
NOTE: the original analysis in the issue description was wrong. See #663 (comment) for details. Keeping this here for the record.
The test case test_group_filter_self_import() is incorrect, which conveniently enough provides steps to reproduce.
The test case should read as follows (patch applies to f6f5cf6 or today's main):
In other words, west incorrectly concludes that group
gyis disabled in this scenario, when it should be enabled.The test creates the following layout, where ./mp/west.yml is the main manifest:
From https://docs.zephyrproject.org/latest/develop/west/manifest.html#group-filters, we have the following rules:
Applying these rules, the final filter should be concatenated from ./project1/west.yml, ./project2/west.yml, ./mp/west.yml, ./mp/self-import.yml, in that order. Since neither ./mp/west.yml nor ./mp/self-import.yml have a group filter which affects
gy, the result should be that it is enabled, since ./project2/west.yml enables it explicitly.However, west doesn't do this.
The text was updated successfully, but these errors were encountered: