Skip to content
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

'import-group-filters' top level manifest key #660

Conversation

mbolivar-nordic
Copy link
Contributor

@mbolivar-nordic mbolivar-nordic commented May 9, 2023

Abandoned

@tejlmand has pointed out that this feature, if implemented, means that west manifest --resolve is no longer possible to implement. The semantics in this proposed feature would make the set of enabled/disabled groups context-specific, differing at different points in time when resolving imports. This in turn makes it impossible to write a single "equivalent" manifest file. We want to continue to support west manifest --resolve and believe the extra complexity and poorly specified semantics points to this idea being a nonstarter.

(TL;DR it would use too much complexity budget and doesn't really make sense)


This is the agreed upon approach for how we're going to handle the babblesim module in zephyr/west.yml (#656, #652, #653, #543).

The new 'import-group-filters' feature 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 the 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'.

Implementing this feature exposed a bug in the current group filter handling. The new implementation does not have this bug.

Fixes: #663
Fixes: #652

@mbolivar-nordic mbolivar-nordic force-pushed the import-group-filter branch 3 times, most recently from cca4db6 to a47f555 Compare May 9, 2023 05:57
@aescolar
Copy link
Member

aescolar commented May 9, 2023

Thanks @mbolivar-nordic I'm certainly happy with what the PR is trying to do.

@carlescufi
Copy link
Member

After giving this a cursory review, I believe this to be the right solution for #543

@mbolivar-nordic
Copy link
Contributor Author

I'm polishing this up and will push when I've got something that doesn't introduce regressions

There's a field whose initialization procedure differs enough
from the comment describing it that it's worth clarifying.

Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
The filename attribute no longer exists. Instead, we have a better
str() result for a ManifestImportFailed. Use it instead.

Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
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>
This is a more convenient way to construct manifests, so we might as
well have it around and use it to save some typing.

Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
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>
@mbolivar-nordic mbolivar-nordic changed the title [RFC] 'import-group-filters' top level manifest key 'import-group-filters' top level manifest key May 16, 2023
@mbolivar-nordic mbolivar-nordic marked this pull request as ready for review May 16, 2023 22:11
Add initial coverage for "import-group-filters: false" as well
as some other related improvements.

Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
Improve error handling to avoid dumping stack.

Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
@aescolar
Copy link
Member

@mbolivar-nordic is this ready to give it a spin?

@marc-hb marc-hb added the Partial imports Incomplete or changing imports are much more complicated than you think label May 23, 2023
@marc-hb
Copy link
Collaborator

marc-hb commented May 23, 2023

First impressions, sorry for the late reply.

Interesting proposition, simple enough but seems to do the job.

I think this change requires thorough test coverage BEFORE merge, glad to see you're already working on the tests. Another, less obvious way to "test" this is to... update the documentation. Documentation is often enough to find unexpected design gaps or bugs on the user interface side. So I don't think this should be merged before documentation updates have started.

Funny I wrote this in 2021 in

I expected group filters to be used only in top-level manifests not meant to be imported further [...] Including something excluded is a double negation [...]

Back to the description:

The new 'import-group-filters' feature is a TOP level manifest key that takes a boolean.

Emphasis mine, I initially missed "top". I know, it's in the title but I still missed it :-)

Maybe the description / commit messages / comments / documentation / etc. should use the English keywords "TOP" and "LEAF" more because these are really key relevant concepts. Yet they're mentioned too rarely or briefly and they tend to be lost in long discussions IMHO. I'm afraid past discussions have tended to focus on local/toy examples while ommiting a more global view of the greater import tree picture(s) which are closer to the users view.

Please correct me but one simple way to describe the long lived limitation that this is trying to resolve: projects in a groups could only be LEAF projects. They could not import anything further down.

So in other words this proposition makes non-leaf projects able to join groups too, it effectively allows "hierarchical groups" and not just groups of "leaves" as before? If correct then this should be part of the updated documentation because such high-level description(s) would help users decide whether they want to switch to this "new import paradigm" or stick with the older one.

Please correct me again: what the new paradigm removes is the ability for every manifest except the top one to perform any sort of filtering (which was likely a design mistake in the first place). That's the "price" to pay for "hierarchical groups" (better name wanted).

I usually stay away from pompous buzzwords like "paradigm" but in this case I'm afraid it's appropriate. The unassuming name import-group-filters: looks like an implementation detail which is... a bit deceiving? I don't have a better name (it describes exactly what it does) but that name feels too low level compared to the big conceptual switch it triggers. I think this switch deserves a "bigger looking" name in at least the documentation, something like "West group filtering 2.0", ok that one sucks too but for different reasons; you get the idea.

BTW this will roughly double the required amount of testing in this import+groups area, won't it?

So is the intention to switch the main, upstream zephyr/west.yml to this new import and filtering paradigm?

BTW: when this new attribute is found anywhere but at the TOP, it will be silently ignored, correct?

After switching the main, upstream zephyr/west.yml to this new paradigm, then the (non-leaf) bsim project can be made part of the babblesim group which is disabled by default? And this will restore the ability to west update only what I need and fix zephyrproject-rtos/zephyr#57198 ?

aescolar
aescolar previously approved these changes May 24, 2023
@aescolar
Copy link
Member

aescolar commented May 24, 2023

@marc-hb , @mbolivar-nordic would better answer, but as he is in a different timezone I'll do my best:

So in other words this proposition makes non-leaf projects able to join groups too

yes

Please correct me again: what the new paradigm removes is the ability for every manifest except the top one to perform any sort of filtering (which was likely a design mistake in the first place).

It prevents group filters from propagating "upwards" in the manifests hierarchy. A manifest can still set a group filter that affects its own projects or the project it imports, but a child manifest group filters does not affect the parent manifest group filters.

So is the intention to switch the main, upstream zephyr/west.yml to this new import and filtering paradigm?
After switching the main, upstream zephyr/west.yml to this new paradigm, then the (non-leaf) bsim project can be made part of the babblesim group which is disabled by default? And this will restore the ability to west update only what I need and fix zephyrproject-rtos/zephyr#57198 ?

Yes to all, with

--- a/west.yml
+++ b/west.yml
@@ -15,6 +15,8 @@
 # information.
 
 manifest:
+  version: "1.1"
+
   defaults:
     remote: upstream
 
@@ -23,6 +25,7 @@ manifest:
       url-base: https://github.com/zephyrproject-rtos
 
   group-filter: [-babblesim]
+  import-group-filters: false
 
   #
   # Please add items below based on alphabetical order
@@ -30,6 +33,8 @@ manifest:
     - name: bsim
       repo-path: babblesim-manifest
       revision: 908ffde6298a937c6549dbfa843a62caab26bfc5
+      groups:
+        - babblesim
       import:
         path-prefix: tools
     - name: canopennode

BTW: when this new attribute is found anywhere but at the TOP, it will be silently ignored, correct?

@mbolivar-nordic better answer this.

@aescolar
Copy link
Member

aescolar commented May 24, 2023

@mbolivar-nordic
It seems it is not there yet? :( ( https://github.com/aescolar/zephyr/commits/bsim_new_west )

west init -m git@github.com:aescolar/zephyr.git zp --mr bsim_new_west 
cd zp
west update
west config manifest.group-filter -- "+babblesim"
west update <- fail (all west commands fail now)
 File "west/src/west/manifest.py", line 2504, in _import_content_from_project
    content = self._ctx.project_importer(project, path)
  File "west/src/west/app/project.py", line 1051, in update_importer
    assert not project.groups
AssertionError

or

west init -m git@github.com:aescolar/zephyr.git zp2 --mr bsim_new_west  
cd zp2
west config manifest.group-filter -- "+babblesim"
west update <- fail (all west commands fail now)

@marc-hb
Copy link
Collaborator

marc-hb commented May 31, 2023

Why is #664 preferred now? Please don't close PRs or issues without a single word...

@mbolivar-nordic
Copy link
Contributor Author

Why is #664 preferred now? Please don't close PRs or issues without a single word...

I updated the PR description when I closed it with rationale

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Partial imports Incomplete or changing imports are much more complicated than you think
Projects
None yet
4 participants