'import-group-filters' top level manifest key

[mbolivar-nordic]

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

[aescolar]

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

[carlescufi]

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

[mbolivar-nordic]

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

[aescolar]

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

[marc-hb]

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

• Test fixed group filter semantics #481 (comment)

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]

@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]

@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]

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

[mbolivar-nordic]

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