manifest: add 'import-group-filters:' support

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'.

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

src/west/manifest-schema.yml

@@ -35,6 +35,16 @@ mapping:
     required: false
     type: text
 
+  # The "import-group-filters" key is an optional boolean that defaults
+  # to 'true'. If it's 'false', then the group filter for this manifest
+  # will not depend on the group filters of any manifests it imports.
+  # If it's 'true', it will represent the combination of its own group
+  # filter and the group filters of imported manifests. Setting this to
+  # 'false' is the only way to combine 'import:' with 'groups:' in a project
+  # element.
+  import-group-filters:
+    type: bool
+
   # The "defaults" key specifies some default values used in the
   # rest of the manifest.
   defaults:

src/west/manifest.py

@@ -9,6 +9,7 @@
 
 import enum
 import errno
+import itertools
 import logging
 import os
 from pathlib import PurePosixPath, Path
@@ -52,7 +53,7 @@
 #: v1.0.x, so that users can say "I want schema version 1" instead of
 #: having to keep using '0.13', which was the previous version this
 #: changed.)
-SCHEMA_VERSION = '1.0'
+SCHEMA_VERSION = '1.1'
 # MAINTAINERS:
 #
 # - Make sure to update _VALID_SCHEMA_VERS if you change this.
@@ -121,7 +122,7 @@ class _defaults(NamedTuple):
 _EARLIEST_VER_STR = '0.6.99'  # we introduced the version feature after 0.6
 _VALID_SCHEMA_VERS = [
     _EARLIEST_VER_STR,
-    '0.7', '0.8', '0.9', '0.10', '0.12', '0.13',
+    '0.7', '0.8', '0.9', '0.10', '0.12', '0.13', '1.0',
     SCHEMA_VERSION
 ]
 
@@ -282,16 +283,6 @@ class _import_ctx(NamedTuple):
     # element.
     projects: Dict[str, 'Project']
 
-    # The current shared group filter. This is mutable state in the
-    # same way 'projects' is. Manifests which are imported earlier get
-    # higher precedence here too.
-    #
-    # This is done by prepending (NOT appending) any 'manifest:
-    # group-filter:' lists we encounter during import resolution onto
-    # this list. Since group-filter lists have "last entry wins"
-    # semantics, earlier manifests take precedence.
-    group_filter: GroupFilterType
-
     # The list of west command names provided by the manifest
     # repository itself. This is mutable state in the same way
     # 'projects' is. Manifests which are imported earlier get
@@ -1361,6 +1352,25 @@ def __init__(self, *,  # All arguments are keyword-only.
         self._raw_config_group_filter: Optional[str] = None
         # A helper attribute we use for schema version v0.9 compatibility.
         self._top_level_group_filter: GroupFilterType = []
+        # A temporary list of Manifest objects that we resolve
+        # while handling 'import:' statements in the current manifest
+        # data.
+        #
+        # We need this information because the final 'group_filter'
+        # attribute for this manifest will be either:
+        #
+        # - if 'self._import_group_filters' is false (default): the
+        #   concatenation of all the imported manifests' group filters,
+        #   (which we can compute by concatenating
+        #   self._imported[i].group_filter for each i), plus this
+        #   manifest's group-filter, or
+        #
+        # - otherwise: just this manifest data's group_filter.
+        #
+        # For simplicity, we keep track of the list either way. We
+        # delete this after resolving the current manifest to avoid
+        # keeping garbage Manifest objects around.
+        self._imported: List['Manifest'] = []
         # The manifest.path configuration option in the local
         # configuration file, as a Path.
         self._config_path: Optional[Path] = None
@@ -1404,6 +1414,9 @@ def __init__(self, *,  # All arguments are keyword-only.
 
         self._load_validated()
 
+        # Get rid of intermediate state.
+        del self._imported
+
     def get_projects(self,
                      # any str name is also a PathType
                      project_ids: Iterable[PathType],
@@ -1743,7 +1756,6 @@ def get_option(option, default=None):
             self._config_path = manifest_path
 
         return _import_ctx(projects={},
-                           group_filter=[],
                            manifest_west_commands=[],
                            imap_filter=None,
                            path_prefix=Path('.'),
@@ -1775,6 +1787,7 @@ def _load_validated(self) -> None:
         manifest_data = self._ctx.current_data['manifest']
 
         schema_version = str(manifest_data.get('version', SCHEMA_VERSION))
+        parsed_schema_version = parse_version(schema_version)
 
         # We want to make an ordered map from project names to
         # corresponding Project instances. Insertion order into this
@@ -1788,9 +1801,23 @@ def _load_validated(self) -> None:
         # Load data from "self:", including resolving any self imports.
         self._load_self(manifest_data)
 
-        # Load data from "group-filter:".
+        # Load data from "group-filter:" and perform the first
+        # initialization of self._disabled_groups. This may be updated
+        # later by self._final_group_filter().
         self._load_group_filter(manifest_data)
 
+        if 'import-group-filters' in manifest_data:
+            if parsed_schema_version < parse_version('1.1'):
+                # The only way this can happen is if the user explicitly
+                # set a schema version that is too old.
+                self._malformed(
+                    f'manifest schema version {schema_version}: '
+                    'this is too old to use with import-group-filters; '
+                    'to fix, use at least \'version: "1.1"\'')
+            self._import_group_filters = manifest_data['import-group-filters']
+        else:
+            self._import_group_filters = False                
+
         # Add this manifest's projects to the map, and handle imported
         # projects and group-filter values.
         url_bases = {r['name']: r['url-base'] for r in
@@ -1828,21 +1855,17 @@ def _load_validated(self) -> None:
 
                     self._projects_by_rpath[Path(p.abspath).resolve()] = p
 
-            # Update self.group_filter
-            #
-            # For schema version 0.10 or later, there's no point in
-            # overwriting these attributes for anything except the top
-            # level manifest: all the other ones we've loaded above
-            # during import resolution are already garbage.
-            #
-            # For schema version 0.9, we only want to warn once, at the
-            # top level, if the distinction actually matters.
-            self.group_filter = self._final_group_filter(schema_version)
+        # Update self.group_filter
+        self.group_filter = self._final_group_filter(schema_version)
 
         _logger.debug(f'loaded {loading_what}')
 
     def _load_group_filter(self, manifest_data: Dict[str, Any]) -> None:
-        # Update self._ctx.group_filter from manifest_data.
+        # Set the initial value for self._disabled_groups and
+        # (potentially self._top_level_group_filter) from
+        # manifest_data. This value might be further changed after
+        # we're done resolving all the imports in this manifest if
+        # self._import_group_filters is True.
 
         if 'group-filter' not in manifest_data:
             _logger.debug('group-filter: unset')
@@ -1855,7 +1878,9 @@ def _load_group_filter(self, manifest_data: Dict[str, Any]) -> None:
         group_filter = self._validated_group_filter('manifest', raw_filter)
         _logger.debug('group-filter: %s', group_filter)
 
-        self._ctx.group_filter[:0] = group_filter
+        for item in group_filter:
+            if item.startswith('-'):
+                self._disabled_groups.add(item[1:])
 
         if self._top_level:
             self._top_level_group_filter = group_filter
@@ -2016,16 +2041,14 @@ def _import_pathobj_from_self(self, pathobj_abs: Path,
         # - pathobj: same, but relative to self.repo_abspath as obtained
         #   from the import data
 
-        # Destructively merge imported content into self._ctx. The
-        # intermediate manifest is thrown away; we're just
-        # using __init__ as a function here.
         child_ctx = self._ctx._replace(
             current_abspath=pathobj_abs,
             current_relpath=pathobj,
             current_data=pathobj_abs.read_text(encoding='utf-8')
         )
         try:
-            Manifest(topdir=self.topdir, internal_import_ctx=child_ctx)
+            self._imported.append(Manifest(topdir=self.topdir,
+                                           internal_import_ctx=child_ctx))
         except RecursionError as e:
             raise _ManifestImportDepth(None, pathobj) from e
 
@@ -2063,7 +2086,8 @@ def _import_map_from_self(self, imp: Dict) -> None:
                 current_data=import_abs.read_text(encoding='utf-8')
             )
             try:
-                Manifest(topdir=self.topdir, internal_import_ctx=child_ctx)
+                self._imported.append(Manifest(topdir=self.topdir,
+                                               internal_import_ctx=child_ctx))
             except RecursionError as e:
                 raise _ManifestImportDepth(None, import_abs) from e
 
@@ -2193,13 +2217,14 @@ def _load_project(self, pd: Dict, url_bases: Dict[str, str],
         else:
             groups = []
 
-        if imp and groups:
+        if imp and groups and self._import_group_filters:
             # Maybe there is a sensible way to combine the two of these.
             # but it's not clear what it is. Let's avoid weird edge cases
             # like "what do I do about a project whose group is disabled
             # that I need to import data from?".
             self._malformed(
-                f'project {name}: "groups" cannot be combined with "import"')
+                f'project {name}: "groups" cannot be combined with "import" '
+                'unless you add "import-group-filters: false" to the manifest')
 
         userdata = pd.get('userdata')
 
@@ -2293,6 +2318,11 @@ def _import_from_project(self, project: Project, imp: Any):
 
         self._assert_imports_ok()
 
+        if not self.is_active(project):
+            _logger.debug(f'{project.name_and_path}: inactive, so ignoring '
+                          f'the import')
+            return
+
         self.has_imports = True
 
         imptype = type(imp)
@@ -2375,6 +2405,8 @@ def _import_data_from_project(self, project: Project, data: Any,
             raise _ManifestImportDepth(None, imap.file if imap else None) \
                 from e
 
+        self._imported.append(submanifest)
+
         # Patch up any extension commands in the imported data
         # by allocating them to the project.
         project.west_commands = _west_commands_merge(
@@ -2492,16 +2524,30 @@ def _check_names(self) -> None:
                 self._malformed(f'Invalid project name: {name}')
 
     def _final_group_filter(self, schema_version: str):
-        # Update self.group_filter based on the schema version.
+        # Decide the final self.group_filter based on the schema
+        # version and 'import-group-filters:' value.
 
         if schema_version == '0.9':
             # If the user requested v0.9.x group-filter semantics,
             # provide them, but emit a warning that can't be silenced
             # if group filters were used anywhere.
             #
-            # Hopefully no users ever actually see this warning.
+            # Hopefully no users ever actually see this warning,
+            # but either way, we only want to warn once, at the
+            # top level, if the distinction actually matters.
 
-            if self._ctx.group_filter:
+            if self._top_level and (self.group_filter or
+                                    any(bool(manifest.group_filter)
+                                        for manifest in self._imported)):
+                # Note that if any submanifest in self._imported has
+                # 'import-group-filters: false', and that submanifest
+                # imports but ignores some sub-sub-manifest's
+                # group-filter, we won't warn here about the
+                # sub-sub-manifest's group-filter.
+                #
+                # This seems like the right thing to do: the
+                # submanifest has taken explicit action to have an
+                # empty group-filter; let's respect that.
                 _logger.warning(
                     "providing deprecated group-filter semantics "
                     "due to explicit 'manifest: version: 0.9'; "
@@ -2512,11 +2558,13 @@ def _final_group_filter(self, schema_version: str):
                 self._legacy_group_filter_warned = True
 
             return self._top_level_group_filter
-
-        else:
+        elif self._import_group_filters:
             _update_disabled_groups(self._disabled_groups,
-                                    self._ctx.group_filter)
-            return [f'-{g}' for g in self._disabled_groups]
+                                    list(itertools.chain(
+                                        manifest.group_filter
+                                        for manifest in self._imported)))
+
+        return [f'-{g}' for g in self._disabled_groups]
 
 class _PatchedConfiguration(Configuration):
     # Internal helper class that fakes out manifest.path and manifest.file