Import group filters, too #482

mbolivar-nordic · 2021-02-19T09:42:46Z

The west 0.9 documentation for group-filter states:

Only the top level manifest file’s manifest: group-filter:
value has any effect. The manifest: group-filter: values in any
imported manifests are ignored.

See Project Groups and Active Projects for details on this feature.

This turns out to have been a mistake, which we are reversing course on.

It's a mistake because all users who import a manifest which makes some projects inactive will have to also manually disable the same groups just to get the same default list of projects.

Example manifest fragments showing the issue:

   # my/west.yml
   manifest:
     projects:
       - name: upstream
         import: true

and:

   # upstream/west.yml
   manifest:
     group-filter: [-disabled-group]
     projects:
       - name: i-want-this
       - name: i-do-not-want-this
         groups:
         - disabled-group

As written, my/west.yml will include 'i-do-not-want-this' as an active project.

This kind of leaky faucet has proven to be an unintuitive and poor user experience. People expect to get the default projects without having to copy the group-filter for everything they import.

We can't reverse this behavior without pushing out a new schema version, however: manifest schema 0.9 is doomed to repeat this behavior.

What we can do is create a new schema version that does the right thing, and get the word out to users to upgrade as soon as we can.

This PR updates the behavior for the new schema version (0.10) after some prep work to keep things tidy.

Additional notes:

supersedes Test fixed group filter semantics #481 by implementing the new behavior, and including test cases for all relevant scenarios.
the original use case which showed the mistake is described in manifest: move homekit elsewhere nrfconnect/sdk-nrf#3884
the corresponding documentation update is in west 0.10.0 and 0.9.1 docs zephyr#32381

API modules like west.manifest are using the standard logging module. This module prints messages at warning severity or above to stderr by default. That prints messages twice when a west command configures its own logger, like this: $ west foo some warning message WARNING: some warning message The first line comes from the print to stderr done by the default handler. The second one is from a ProjectCommandLogHandler. Disable the first message, which we do not want, by registering a null handler for the top level. This results in just the ProjectCommandLogHandler output being shown, like this: $ west foo WARNING: some warning message Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>

It's annoying to have to specify projects lists in some cases when all you're trying to do is exercise something else. Though the number of situations where a manifest with no projects is useful may be limited, they nonetheless exist, so make it possible. This is a backwards incompatible change, so bump the schema version. Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>

Freezing, resolving, etc. a manifest file should result in output that reflects the input's group-filter value too, but it doesn't. Fix that. Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>

Change some things related to import map names: - rename the _import_ctx 'filter_fn' attribute to 'imap_filter', along with local variables using the same name - rename _filter_ok() to _imap_filter_allows() - rename _and_filters() to _compose_imap_filters() - rename _new_ctx() to _compose_ctx_and_imap() - adjust the debug logging when an import map filter rejects a project This helps disambiguate filtering on projects that is due to import map allowlists and blocklists versus project group filters. These are all internal names: there are no API changes here. Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>

Add more source code comments describing this internal class's purpose. Use kwargs for fields whenever initializing an instance. Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>

Move RawGroupType right before its only point of use. Move GroupFilterType and GroupsType to the top level list of type aliases, as prep work for using them in more places. Change the '#:' comment style to '#' while we're here; the '#:' style is reserved for comments which are also docstrings that end up in Sphinx, which doesn't apply here. Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>

marc-hb · 2021-02-19T18:10:15Z

Nit-picking the description of the bug:

# upstream/west.yml
   manifest:
     group-filter: [-opt-in-group]
     projects:
       - name: core-project
       - name: restricted-access-project
         groups:
         - opt-in-group

I find "I-want-this" confusing because it looks like "I" wrote it there. If "I" had written the upstream manifest then I would have not inflicted this problem to myself in the first place.

"I-do-not-want-this" is not a problem; people cloning upstream Zephyr routinely download stuff they don't need.

src/west/__init__.py

mbolivar-nordic · 2021-02-19T18:56:57Z

"I-do-not-want-this" is not a problem; people cloning upstream Zephyr routinely download stuff they don't need.

Guess we will have to agree to disagree on this one. As discussed offline, we designed this feature with opt-in groups for zephyr repositories that are not restricted access in mind also.

marc-hb · 2021-02-19T19:24:48Z

I understand but then it's really just a minor annoyance/bug. A "bug description" is easier to understand when it focuses on the worst case. Never mind; this is not even in a commit message.

carlescufi

Approved based on reviewing manifest: re-work group filter handling and its description, as well as the discussion in #481.

tejlmand

Looking good.

But when testing this, I used the following syntax in my test manifest:

manifest:
  version: 0.10

according to this: https://docs.zephyrproject.org/latest/guides/west/manifest.html#version

However, this results in the following error message which may not be easy for
everyone to relate to quoting.

$ west list
FATAL ERROR: can't load west manifest
  Malformed manifest file: /projects/github/ncs/nrf/west.yml
  Schema file: /home/tora/.local/lib/python3.8/site-packages/west/manifest-schema.yml
  Hint: invalid version 0.1; must be one of: 0.6.99, 0.7, 0.8, 0.9, 0.10

However this is working (with quoting):

manifest:
  version: '0.10'

would it be possible to improve user feedback in this situation, to make them
aware they would need to quote the version ?

Also the Zephyr example should be updated to use quoted from.

tejlmand · 2021-03-09T15:22:21Z

src/west/manifest.py

+                    'please upgrade to west 0.10 or later and '
+                    'update this manifest.')


small nit, when user sees this message he is already running a west version that supports the new behavior so you could just inform the user to update manifest, as there basically no reason to update west itself.

New schema version is introduced in this PR here: 0c9deda

when user sees this message he is already running a west version that supports the new behavior

Changed to:

_logger.warning( "providing deprecated group-filter semantics " "due to explicit 'manifest: version: 0.9'; " "for the new semantics, use " "'manifest: version: \"0.10\"' or later")

This will be required by a later patch which uses the value '0.9' explicitly to see if the user is requesting group-filter behavior that we're going to change. It is also just a good way to tighten up the error handling. The intent here is to reject '0.9.99' as an invalid schema version, to make sure nobody tries to do that. We're bumping the schema from 0.9 to 0.10 as part of the v0.10.0 development branch. Any unreleased master branches during that time will have to request schema version 0.10 instead of 0.9.99 or something like that. Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>

For schema version 0.10: Allow group-filter values from imported manifests to affect the top level manifest group filter. Simplify the results so that Manifest.group_filter is just a list of disabled groups. Manifests have the same precedence on the final group filter as they do on project definition: manifests which appear earlier in the import order have higher precedence on the group filter, just as they have higher precedence when it comes to whether a project definition comes from them or is ignored. Delay loading the manifest.group-filter value from the local configuration file until we actually need to know its value in is_active(). Preserve 0.9 semantics if that is explicitly requested, but warn if there are group filters anywhere. Add test cases for the updated group-filter behavior, and regression tests to ensure that 0.9 semantics are preserved when explicitly requested. Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>

west.manifest.SCHEMA_VERSION is already 0.10; no changes needed there. Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>

mbolivar-nordic · 2021-03-09T23:25:00Z

Great comments @tejlmand; now addressed.

would it be possible to improve user feedback in this situation,

This now prints:

  Hint: invalid version 0.1; must be one of: 0.6.99, 0.7, 0.8, 0.9, 0.10. Do you need to quote the value (e.g. "0.10" instead of 0.10)?

New features: - The name key in a project's "submodules" list is now optional. Bug fixes: - West now checks that the manifest schema version is one of the explicitly allowed values. The old behavior was just to check that the schema version was newer than the west version where the "manifest: version:" key was introduced. This incorrectly allowed invalid schema versions, like 0.8.2. Other changes: - A manifest file's "group-filter" is now propagated through an import. This is a change from how west v0.9.x handled this. In west v0.9.x, only the top level manifest file's group-filter had any effect; the group filter lists from any imported manifests were ignored. Starting with west v0.10.0, the group filter lists from imported manifests are also imported. The new behavior will take effect if "manifest: version:" is not given or is at least 0.10. The old behavior is still available in the top level manifest file only with an explicit "manifest: version: 0.9". See #482 for the motivation for this change and additional context. Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>

mbolivar-nordic added 6 commits February 18, 2021 11:17

manifest: fix group filter round trip

820a01b

Freezing, resolving, etc. a manifest file should result in output that reflects the input's group-filter value too, but it doesn't. Fix that. Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>

manifest: improve _import_ctx readability

323bd77

Add more source code comments describing this internal class's purpose. Use kwargs for fields whenever initializing an instance. Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>

mbolivar-nordic mentioned this pull request Feb 19, 2021

Test fixed group filter semantics #481

Closed

mbolivar-nordic requested review from tejlmand, carlescufi and marc-hb February 19, 2021 09:49

mbolivar-nordic mentioned this pull request Feb 19, 2021

west 0.10.0 and 0.9.1 docs zephyrproject-rtos/zephyr#32381

Merged

marc-hb reviewed Feb 19, 2021

View reviewed changes

src/west/__init__.py Show resolved Hide resolved

mbolivar-nordic mentioned this pull request Feb 22, 2021

doc: west: submodule 'name' is optional after west 0.9.0 zephyrproject-rtos/zephyr#32106

Closed

carlescufi approved these changes Mar 1, 2021

View reviewed changes

mbolivar-nordic mentioned this pull request Mar 1, 2021

West 0.9.1a1 #483

Merged

tejlmand reviewed Mar 9, 2021

View reviewed changes

mbolivar-nordic added 2 commits March 9, 2021 13:38

mbolivar-nordic force-pushed the import-group-filters branch from 949ed11 to f17faed Compare March 9, 2021 21:47

West 0.10.0a1

1de058b

west.manifest.SCHEMA_VERSION is already 0.10; no changes needed there. Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>

mbolivar-nordic force-pushed the import-group-filters branch from f17faed to 1de058b Compare March 9, 2021 23:23

tejlmand approved these changes Mar 10, 2021

View reviewed changes

carlescufi merged commit 55b8349 into zephyrproject-rtos:master Mar 10, 2021

marc-hb added the Partial imports Incomplete or changing imports are much more complicated than you think label Jan 5, 2023

marc-hb mentioned this pull request Oct 27, 2023

Allow import of optional projects from manifest file #696

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Import group filters, too #482

Import group filters, too #482

mbolivar-nordic commented Feb 19, 2021

marc-hb commented Feb 19, 2021

mbolivar-nordic commented Feb 19, 2021 •

edited

Loading

marc-hb commented Feb 19, 2021

carlescufi left a comment

tejlmand left a comment •

edited

Loading

tejlmand Mar 9, 2021 •

edited

Loading

mbolivar-nordic Mar 9, 2021

mbolivar-nordic commented Mar 9, 2021

		'please upgrade to west 0.10 or later and '
		'update this manifest.')

Import group filters, too #482

Import group filters, too #482

Conversation

mbolivar-nordic commented Feb 19, 2021

marc-hb commented Feb 19, 2021

mbolivar-nordic commented Feb 19, 2021 • edited Loading

marc-hb commented Feb 19, 2021

carlescufi left a comment

Choose a reason for hiding this comment

tejlmand left a comment • edited Loading

Choose a reason for hiding this comment

tejlmand Mar 9, 2021 • edited Loading

Choose a reason for hiding this comment

mbolivar-nordic Mar 9, 2021

Choose a reason for hiding this comment

mbolivar-nordic commented Mar 9, 2021

mbolivar-nordic commented Feb 19, 2021 •

edited

Loading

tejlmand left a comment •

edited

Loading

tejlmand Mar 9, 2021 •

edited

Loading