Initial import filtering implementation

src/west/app/project.py

@@ -17,6 +17,7 @@
 import sys
 import textwrap
 from time import perf_counter
+from typing import List as ListType
 from urllib.parse import urlparse
 
 from west.configuration import Configuration
@@ -962,7 +963,7 @@ def do_run(self, args, _):
         else:
             self.update_some()
 
-    def init_state(self, args):
+    def init_state(self, args: argparse.Namespace):
         # Helper for initializing instance state in response to
         # command line args and configuration files.
 
@@ -977,7 +978,7 @@ def init_state(self, args):
         self.sync_submodules = config.getboolean('update.sync-submodules',
                                                  default=True)
 
-        self.group_filter: List[str] = []
+        self.group_filter: ListType[str] = []
 
         def handle(group_filter_item):
             item = group_filter_item.strip()

src/west/manifest.py

@@ -224,6 +224,7 @@ class _import_map(NamedTuple):
     name_blocklist: List[str]
     path_blocklist: List[str]
     path_prefix: str
+    if_: Dict[str, Any]
 
 def _is_imap_list(value: Any) -> bool:
     # Return True if the value is a valid import map 'blocklist' or
@@ -233,6 +234,15 @@ def _is_imap_list(value: Any) -> bool:
             (isinstance(value, list) and
              all(isinstance(item, str) for item in value)))
 
+def _is_imap_if(value: Any) -> bool:
+    # Return True if the value is a valid import map 'if:' condition.
+    # This doesn't mean that the import should be done! It just means
+    # that the value of the 'if:' in the manifest file is well-formed.
+
+    return (isinstance(value, dict) and
+            ((len(value) == 1 and 'config-enabled' in value) or
+             not value))
+
 def _imap_filter(imap: _import_map) -> ImapFilterFnType:
     # Returns either None (if no filter is necessary) or a
     # filter function for the given import map.
@@ -339,6 +349,10 @@ class _import_ctx(NamedTuple):
     # Bit vector of flags that modify import behavior.
     import_flags: 'ImportFlag'
 
+    # Configuration passed to the top-level manifest instance in the
+    # import hierarchy.
+    config: Optional[Configuration]
+
 def _imap_filter_allows(imap_filter: ImapFilterFnType,
                         project: 'Project') -> bool:
     # imap_filter(project) if imap_filter is not None; True otherwise.
@@ -1273,7 +1287,14 @@ def __init__(self, *,  # All arguments are keyword-only.
 
         If *source_data* is given:
 
-            - You cannot pass *config*.
+            - Since west v1.1, you can pass *config* if you want to
+              influence an "import: if: config-enabled: <option>"
+              behavior in the data. If you do not, the <option>'s
+              value will be considered false and no import will
+              be done.
+
+              Prior to west v1.1, passing *config* would raise an
+              exception.
 
             - Manifest imports will fail unless you pass *importer*
               or ignore them with *import_flags*.
@@ -1695,8 +1716,6 @@ def _top_level_init(self, source_data, topdir, topdir_abspath,
             raise ValueError('neither topdir nor source_data were given')
         if topdir and source_data:
             raise ValueError('both topdir and source_data were given')
-        if source_data and config:
-            raise ValueError('both source_data and config were given')
         if not _flags_ok(import_flags):
             raise ValueError(f'bad import_flags {import_flags:x}')
 
@@ -1749,7 +1768,8 @@ def get_option(option, default=None):
                            current_data=current_data,
                            current_repo_abspath=current_repo_abspath,
                            project_importer=project_importer,
-                           import_flags=import_flags)
+                           import_flags=import_flags,
+                           config=config)
 
     def _recursive_init(self, ctx: _import_ctx):
         # Set up any required state that we need while recursively
@@ -2033,6 +2053,9 @@ def _import_map_from_self(self, imp: Dict) -> None:
         # We therefore need to compose them during the recursive import.
 
         imap = self._load_imap(imp, f'manifest file {self.abspath}')
+        if not self._imap_if_is_true(imap):
+            return
+
         imap_filter = _compose_imap_filters(self._ctx.imap_filter,
                                             _imap_filter(imap))
         path_prefix = self._ctx.path_prefix / imap.path_prefix
@@ -2162,9 +2185,15 @@ def _load_project(self, pd: Dict, url_bases: Dict[str, str],
                 'has no remote or url and no default remote is set')
 
         # The project's path needs to respect any import: path-prefix,
-        # regardless of self._ctx.import_flags. The 'ignore' type flags
-        # just mean ignore the imported data. The path-prefix in this
-        # manifest affects the project no matter what.
+        # regardless of self._ctx.import_flags or whether the import
+        # 'if:' condition is met.
+        #
+        # The 'ignore' type flags just mean ignore the imported data,
+        # and the 'if:' condition decides whether we try to load the
+        # imported data.
+        #
+        # The path-prefix value in this manifest affects the project
+        # no matter what.
         imp = pd.get('import', None)
         if isinstance(imp, dict):
             pfx = self._load_imap(imp, f'project {name}').path_prefix
@@ -2191,10 +2220,11 @@ def _load_project(self, pd: Dict, url_bases: Dict[str, str],
             groups = []
 
         if imp and groups:
-            # 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?".
+            # There isn't a sensible way to handle this case. We can't
+            # decide whether or not to import because we don't know if
+            # the project is active or not until after all group filters
+            # are resolved, which won't happen until we're done resolving
+            # the manifest.
             self._malformed(
                 f'project {name}: "groups" cannot be combined with "import"')
 
@@ -2328,6 +2358,11 @@ def _import_map_from_project(self, project: Project,
                                  imp: Dict) -> None:
         imap = self._load_imap(imp, f'project {project.name}')
 
+        if not self._imap_if_is_true(imap):
+            _logger.debug('project {project}: import "if:" ({imap.if_}) '
+                          'is false; skipping the import')
+            return
+
         _logger.debug(f'resolving import {imap} for {project}')
 
         imported = self._import_content_from_project(project, imap.file)
@@ -2430,7 +2465,8 @@ def _load_imap(self, imp: Dict, src: str) -> _import_map:
                           path_allowlist,
                           name_blocklist,
                           path_blocklist,
-                          copy.pop('path-prefix', ''))
+                          copy.pop('path-prefix', ''),
+                          if_=copy.pop('if', {}))
 
         # Check that the value is OK.
         if copy:
@@ -2452,9 +2488,29 @@ def _load_imap(self, imp: Dict, src: str) -> _import_map:
             self._malformed(f'{src}: bad import path-prefix '
                             f'{ret.path_prefix}; expected str, not '
                             f'{type(ret.path_prefix)}')
+        elif not _is_imap_if(ret.if_):
+            self._malformed(f'{src}: bad import if condition '
+                            f'{ret.if_}; expected dict with config-enabled '
+                            'key, or an empty dict')
 
         return ret
 
+    def _imap_if_is_true(self, imap: _import_map) -> bool:
+        # Return true if the import map's "if" condition says we should
+        # perform the import.
+        if not imap.if_:
+            return True
+
+        config = self._ctx.config
+
+        if config is None:
+            # If this happens, we're loading the top level Manifest
+            # from data and the caller wants us to treat the option
+            # as though it is unset.
+            return False
+
+        return config.getboolean(imap.if_['config-enabled'])
+
     def _add_project(self, project: Project) -> bool:
         # Add the project to our map if we don't already know about it.
         # Return the result.

tests/conftest.py

@@ -228,6 +228,70 @@ def west_init_tmpdir(repos_tmpdir):
     config.read_config()
     return west_tmpdir
 
+@pytest.fixture
+def config_tmpdir(tmpdir):
+    # Fixture for running from a temporary directory with
+    # environmental overrides in place so all configuration files
+    # live inside of it. This makes sure we don't touch
+    # the user's actual files.
+    #
+    # We also set ZEPHYR_BASE (to avoid complaints in subcommand
+    # stderr), but to a spurious location (so that attempts to read
+    # from inside of it are caught here).
+    #
+    # Using this makes the tests run faster than if we used
+    # west_init_tmpdir from conftest.py, and also ensures that the
+    # configuration code doesn't depend on features like the existence
+    # of a manifest file, helping separate concerns.
+    system = tmpdir / 'config.system'
+    glbl = tmpdir / 'config.global'
+    local = tmpdir / 'config.local'
+
+    os.environ['ZEPHYR_BASE'] = str(tmpdir.join('no-zephyr-here'))
+    os.environ['WEST_CONFIG_SYSTEM'] = str(system)
+    os.environ['WEST_CONFIG_GLOBAL'] = str(glbl)
+    os.environ['WEST_CONFIG_LOCAL'] = str(local)
+
+    # Make sure our environment variables (as well as other topdirs)
+    # are respected from tmpdir, and we aren't going to touch the
+    # user's real files.
+    start_dir = os.getcwd()
+    tmpdir.chdir()
+
+    try:
+        assert config._location(config.ConfigFile.SYSTEM) == str(system)
+        assert config._location(config.ConfigFile.GLOBAL) == str(glbl)
+        td = tmpdir / 'test-topdir'
+        td.ensure(dir=True)
+        (td / '.west').ensure(dir=True)
+        (td / '.west' / 'config').ensure(file=True)
+        assert config._location(config.ConfigFile.LOCAL) == str(local)
+        assert (config._location(config.ConfigFile.LOCAL,
+                                 topdir=str(td)) ==
+                str(local))
+        td.remove(rec=1)
+        assert not td.exists()
+
+        assert not local.exists()
+
+        # All clear: switch to the temporary directory and run the test.
+        yield tmpdir
+    finally:
+        # Go back to where we started, for repeatability of results.
+        os.chdir(start_dir)
+
+        # Clean up after ourselves so other test cases don't know
+        # about this tmpdir. It's OK if test cases deleted these
+        # settings already.
+        if 'ZEPHYR_BASE' in os.environ:
+            del os.environ['ZEPHYR_BASE']
+        if 'WEST_CONFIG_SYSTEM' in os.environ:
+            del os.environ['WEST_CONFIG_SYSTEM']
+        if 'WEST_CONFIG_GLOBAL' in os.environ:
+            del os.environ['WEST_CONFIG_GLOBAL']
+        if 'WEST_CONFIG_LOCAL' in os.environ:
+            del os.environ['WEST_CONFIG_LOCAL']
+
 #
 # Helper functions
 #

tests/test_config.py

@@ -21,66 +21,11 @@
 ALL = config.ConfigFile.ALL
 
 @pytest.fixture(autouse=True)
-def config_tmpdir(tmpdir):
-    # Fixture for running from a temporary directory with
-    # environmental overrides in place so all configuration files
-    # live inside of it. This makes sure we don't touch
-    # the user's actual files.
-    #
-    # We also set ZEPHYR_BASE (to avoid complaints in subcommand
-    # stderr), but to a spurious location (so that attempts to read
-    # from inside of it are caught here).
-    #
-    # Using this makes the tests run faster than if we used
-    # west_init_tmpdir from conftest.py, and also ensures that the
-    # configuration code doesn't depend on features like the existence
-    # of a manifest file, helping separate concerns.
-    system = tmpdir / 'config.system'
-    glbl = tmpdir / 'config.global'
-    local = tmpdir / 'config.local'
-
-    os.environ['ZEPHYR_BASE'] = str(tmpdir.join('no-zephyr-here'))
-    os.environ['WEST_CONFIG_SYSTEM'] = str(system)
-    os.environ['WEST_CONFIG_GLOBAL'] = str(glbl)
-    os.environ['WEST_CONFIG_LOCAL'] = str(local)
-
-    # Make sure our environment variables (as well as other topdirs)
-    # are respected from tmpdir, and we aren't going to touch the
-    # user's real files.
-    start_dir = os.getcwd()
-    tmpdir.chdir()
-
-    try:
-        assert config._location(SYSTEM) == str(system)
-        assert config._location(GLOBAL) == str(glbl)
-        td = tmpdir / 'test-topdir'
-        td.ensure(dir=True)
-        (td / '.west').ensure(dir=True)
-        (td / '.west' / 'config').ensure(file=True)
-        assert config._location(LOCAL) == str(local)
-        assert config._location(LOCAL, topdir=str(td)) == str(local)
-        td.remove(rec=1)
-        assert not td.exists()
-
-        assert not local.exists()
-
-        # All clear: switch to the temporary directory and run the test.
-        yield tmpdir
-    finally:
-        # Go back to where we started, for repeatability of results.
-        os.chdir(start_dir)
-
-        # Clean up after ourselves so other test cases don't know
-        # about this tmpdir. It's OK if test cases deleted these
-        # settings already.
-        if 'ZEPHYR_BASE' in os.environ:
-            del os.environ['ZEPHYR_BASE']
-        if 'WEST_CONFIG_SYSTEM' in os.environ:
-            del os.environ['WEST_CONFIG_SYSTEM']
-        if 'WEST_CONFIG_GLOBAL' in os.environ:
-            del os.environ['WEST_CONFIG_GLOBAL']
-        if 'WEST_CONFIG_LOCAL' in os.environ:
-            del os.environ['WEST_CONFIG_LOCAL']
+def autouse_config_tmpdir(config_tmpdir):
+    # Since this module tests west's configuration file features,
+    # adding autouse=True to the config_tmpdir fixture saves typing
+    # and is less error-prone than using it below in every test case.
+    pass
 
 def cfg(f=ALL, topdir=None):
     # Load a fresh configuration object at the given level, and return it.