`manifest.project-filter` #664

mbolivar-nordic · 2023-05-31T05:14:53Z

Take three at trying to provide an "import filtering" (#543) solution that will work to allow making the bsim project in zephyr/west.yml inactive, so you can west build etc without it cloned.

To test:

# set up a workspace without a bsim project any way you want. Example:
west init zephyrproject
cd zephyrproject

# exercise the new config option in a local workspce:
west list # error because bsim project has 'import' and is not cloned
west config manifest.project-filter -- -bsim
west list # works, and bsim project is not in the output (it's inactive)
west list bsim # works, prints the usual info for bsim

alternatively:

west config --global manifest.project-filter -- -bsim
west init zephyrproject
cd zephyrproject
west list # works

Add support for a new manifest.project-filter configuration option.

The option's value is a comma-separated list of regular expressions,
each prefixed with '+' or '-', like this:

    +re1,-re2,-re3

Project names are matched against each regular expression (re1, re2,
re3, ...) in the list, in order. If the entire project name matches
the regular expression, that element of the list either deactivates or
activates the project. The project is deactivated if the element
begins with '-'. The project is activated if the element begins with
'+'. (Project names cannot contain ',' if this option is used, so the
regular expressions do not need to contain a literal ',' character.)

If a project's name matches multiple regular expressions in the list,
the result from the last regular expression is used. For example,
if manifest.project-filter is:

    -hal_.*,+hal_foo

Then a project named 'hal_bar' is inactive, but a project named
'hal_foo' is active.

If a project is made inactive or active by a list element, the project
is active or not regardless of whether any or all of its groups are
disabled. (This is also now the only way to make a project that has no
groups inactive.)

Otherwise, i.e. if a project does not match any regular expressions in
the list, it is active or inactive according to the usual rules
related to its groups.

Within an element of a manifest.project-filter list, leading and
trailing whitespace are ignored. That means these example values
are equivalent:

    +foo,-bar
    +foo , -bar

The lists in the manifest.project-filter options in the system,
global, and local configuration files are concatenated together.
For example, on Linux, with:

    /etc/westconfig (system file):
    [manifest]
    project-filter = +foo

    ~/.westconfig (global file):
    [manifest]
    project-filter = -bar_.*

    <workspace>/.west/config (local file):
    [manifest]
    project-filter = +bar_local

Then the overall project filter when within the workspace
is:

    +foo,-bar_.*,+bar_local

This lets you set defaults in the system or global files that can be
overridden within an individual workspace as needed, without having to
maintain the defaults across multiple workspaces.

Fixes #653

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

Improve error handling to avoid dumping stack. Signed-off-by: Martí Bolívar <[email protected]>

We're not using the 'i' variable. Signed-off-by: Martí Bolívar <[email protected]>

The error handling in the _load_imap() function is not being tested. This was discovered by manually inspecting the HTML coverage data. Add statement coverage for the error handling. Signed-off-by: Martí Bolívar <[email protected]>

aescolar · 2023-05-31T11:05:50Z

Thanks @mbolivar-nordic, it is looking very nice so far.
A minor request: It would be nice if the manifest repo was not affected by the filter. Background: I expect users will very often want to do a -.*, or +.* (for example to ignore global config, or have a clean slate), but there is no purpose of having west ignoring the manifest repo, and on the other hand west ignoring would be unexpected (as west does not really manage it)

aescolar · 2023-05-31T11:20:47Z

Regarding the "default-inactive" feature. Would it make sense to add it right away?
(I guess the logic would be something like adding just before
https://github.com/zephyrproject-rtos/west/pull/664/files#diff-6ef1afe17ca985687d1222e9ece942bbf4fdca50c2baa784b1dfb3504b078acbR1730
something like

if  (ret == PFR.NONE) and (project.default_inactive)
   ret = PFR.INACTIVE

(apart from the extra changes to support that extra project key)

mbolivar-nordic · 2023-05-31T13:45:41Z

Would it make sense to add it right away?

I don't think so. There is still a fair bit of work to do to get the current feature set in a merge-able state and I think such an addition is not a blocker.

mbolivar-nordic · 2023-05-31T13:47:16Z

A minor request: It would be nice if the manifest repo was not affected by the filter

That's a bug, thanks

mbolivar-nordic · 2023-05-31T13:52:23Z

To be confirmed: west manifest --resolve desired behavior after this PR

carlescufi · 2023-05-31T14:53:29Z

For west commands that require the whole manifest to be resolved (e.g. resolve, freeze, etc), the proposed approach is to let west error out if the project-filter configuration option is set at all when invoking them. This is to avoid the local project filtering interfering with full resolution of the manifest.

Moving all the "work" of main() into WestApp.run() will help us in future patches where we need to set up logging in a way that requires internal state from that object. Signed-off-by: Martí Bolívar <[email protected]>

We've been avoiding this for a long time, but we can't any longer and it's time to bite the bullet. There are some chicken-and-egg problems in our argument parsing that can only be handled if we do some manual command line argument parsing to determine what the common options (like --verbose) are set to, and to determine what the command name (if any) is. For example, we would ideally like to know the verbosity level of the west command *before* we set up logging for the west.manifest module, so that we can set the log level for that module appropriately. Right now, we can't do that, because we load the manifest to figure out what the extension commands are, and from there delegate to argparse to count the '--verbose' options. By the time argparse runs, then, it's too late: we already loaded the manifest, and any log messages from west.manifest are lost. Enable solving this and other problems by writing our own, very limited, command line parser that just figures out the global options (west -hvVz) and the command name. We won't need all of this right away, but we might as well try to be complete from the start. This is prep work only; we'll put it to work in later patches. Signed-off-by: Martí Bolívar <[email protected]>

Right now if you run a zephyr extension like 'west build' outside of a workspace, argparse says: [...] invalid choice: ‘build’ [...] This is because argparse's subcommand parser doesn't seem to have any API to add a catch-all value for when the user provides an unknown command, so it expects that exactly the subcommands we told it about are available. This is confusing to users, and now that we have our own EarlyArgs parsed, we can do better by printing some west-specific help if we aren't in a workspace: usage: west [-h] [-z ZEPHYR_BASE] [-v] [-V] <command> ... west: unknown command "build"; do you need to run this inside a workspace? as well as if you are: usage: west [-h] [-z ZEPHYR_BASE] [-v] [-V] <command> ... west: unknown command "foo"; workspace /home/mbolivar/zp does not define this extension command -- try "west help" Signed-off-by: Martí Bolívar <[email protected]>

Commit 92c18ac ("main: move verbose manifest logging to project.py") did not achieve its purpose. We are loading the manifest long before we call the setup_logging() methods in each of the project.py classes, so any messages from the manifest class have long been discarded by the time that we enable them. Now that we have EarlyArgs in main.py, we can use that to centralize logging and enforce the following: - warnings and above for west APIs are emitted by default - info and above emitted with west -v - debug and above with west -vv or higher Fixes: zephyrproject-rtos#665 Signed-off-by: Martí Bolívar <[email protected]>

Put the first argument on the same line as the string "error:", to make it easier to grep. Signed-off-by: Martí Bolívar <[email protected]>

This will still fail or succeed in the same situations, but it is convenient to do it this way as prep work for future enhancements. Signed-off-by: Martí Bolívar <[email protected]>

In order to support the manifest.project-filter configuration option, we need to forbid certain characters from occurring in project names. Making that happen is a backwards incompatible change, however, so phase it in by emitting a warning. We will make it a hard error if manifest.project-filter is set later on. That way, users will only see an error if they are opting into the latest west for now, and other users will get a warning and have time to migrate their projects. Signed-off-by: Martí Bolívar <[email protected]>

This adds initial support for loading a 'manifest.project-filter' configuration option. This option is special in that its values in the system, global, and local configuration files are all considered at the same time, rather than just whichever one is defined at highest precedence. This is because the eventual purpose of this configuration option is to allow people to deactivate or activate projects using the configuration file system, and it seems nicer to allow people to progressively refine a filter instead of having to synchronize global settings that they always want applied into each workspace they have or may create. This patch doesn't actually do anything with the option values besides defining the internal representation, validating the options on the file system, and saving the results in the import context state that we carry around while importing projects. Applying the filter itself will come in future patches. See source code comments for details on behavior (these will eventually make their way into the documentation). Signed-off-by: Martí Bolívar <[email protected]>

Add support for this option. The option's value is a comma-separated list of regular expressions, each prefixed with '+' or '-', like this: +re1,-re2,-re3 Project names are matched against each regular expression (re1, re2, re3, ...) in the list, in order. If the entire project name matches the regular expression, that element of the list either deactivates or activates the project. The project is deactivated if the element begins with '-'. The project is activated if the element begins with '+'. (Project names cannot contain ',' if this option is used, so the regular expressions do not need to contain a literal ',' character.) If a project's name matches multiple regular expressions in the list, the result from the last regular expression is used. For example, if manifest.project-filter is: -hal_.*,+hal_foo Then a project named 'hal_bar' is inactive, but a project named 'hal_foo' is active. If a project is made inactive or active by a list element, the project is active or not regardless of whether any or all of its groups are disabled. (This is also now the only way to make a project that has no groups inactive.) Otherwise, i.e. if a project does not match any regular expressions in the list, it is active or inactive according to the usual rules related to its groups. Within an element of a manifest.project-filter list, leading and trailing whitespace are ignored. That means these example values are equivalent: +foo,-bar +foo , -bar Any empty elements are ignored. That means these example values are equivalent: +foo,,-bar +foo,-bar The lists in the manifest.project-filter options in the system, global, and local configuration files are concatenated together. For example, on Linux, with: /etc/westconfig: [manifest] project-filter = +foo ~/.westconfig: [manifest] project-filter = -bar_.* <workspace>/.west/config: [manifest] project-filter = +bar_local Then the overall project filter when within the workspace <workspace> is: +foo,-bar_.*,+bar_local This lets you set defaults in the system or global files that can be overridden within an individual workspace as needed, without having to maintain the defaults across multiple workspaces. Signed-off-by: Martí Bolívar <[email protected]>

Internal discussion at Nordic indicates that the semantics for 'west manifest --resolve' should probably be something along the lines of "resolve the manifest but ignore the value of manifest.project-filter, and print the resolved result". This is consistent with the way that these commands are not affected by the value of manifest.group-filter. However, we don't have a way to support this right now: west.manifest has no API for loading itself but with manifest.project-filter ignored. Such an API would be straightforward to add, but we don't have time for that right now, as we're under time pressure to add support for this option to resolve an issue during the zephyr v3.4 stabilization period. For now, work around the issue by just erroring out if the option is set at all, telling the user that they can get in touch with us if they need this. We'll let the level of noise that results be our guide in prioritizing this enhancement. Signed-off-by: Martí Bolívar <[email protected]>

Add test cases for behavior and matching rules. Signed-off-by: Martí Bolívar <[email protected]>

mbolivar-nordic · 2023-06-01T04:12:32Z

For west commands that require the whole manifest to be resolved (e.g. resolve, freeze, etc), the proposed approach is to let west error out if the project-filter configuration option is set at all when invoking them.

Note that this is just a temporary solution. We can add full support for these commands at a later date when the enhancement is required for a concrete use case. This differs from #660, which would have prevented us from implementing these commands at all.

aescolar

Thanks @mbolivar-nordic , it's looking good.
Just 3 minor nits, for which there is no need to hold this.

src/west/app/project.py

src/west/manifest.py

tejlmand · 2023-06-01T09:27:50Z

The lists in the manifest.project-filter options in the system, global, and local configuration files are concatenated together.

i'm not so sure on this.
west also supports manifest.group-filter, and one could with good reasons assume that both kind of filters would behave similar.

For example if I can have:

    /etc/westconfig (system file):
    [manifest]
    project-filter = +foo

    ~/.westconfig (global file):
    [manifest]
    project-filter = -bar

    <workspace>/.west/config (local file):
    [manifest]
    project-filter = +bar_local

why can I then not have similar for groups ?

    /etc/westconfig (system file):
    [manifest]
    group-filter = +foo

    ~/.westconfig (global file):
    [manifest]
    group-filter = -bar

    <workspace>/.west/config (local file):
    [manifest]
    group-filter = +bar_local

I know project-filter supports regex'es, but that's the only syntax difference between those filters, and not enough to justify such a behavioral difference.
I prefer consistency between those settings in this case.
(Feel free to propose group-filter to be concatenated in same fashion as project-filter)

carlescufi · 2023-06-01T10:30:53Z

@aescolar and myself discussed this comment with @tejlmand. The inconsistency in handling the aggregation of different config levels between group-filter and project-filter is noted, @mbolivar-nordic to decide if he wants to align group-filter to the current behavior of project-filter or the other way around.

carlescufi

Discussed together with @aescolar.

mbolivar-nordic · 2023-06-01T16:10:04Z

OK @tejlmand I will reverse course on the way that manifest.project-filter works and only use the value from the highest precedence configuration file as you suggest. Will send a follow up PR.

@aescolar I will address your nits.

Commit d842371 ("app: handle unexpected command name better") intentionally created a big error handling difference between extensions and built-in commands. As an unfortunate side effect, it lost the relevant "manifest import failure" error messages for _extension_ commands. This is especially ironic when the extension command is missing because the import failed, see example in zephyrproject-rtos#726. Error handling is generally very hard to test comprehensively and even more so in this complex "bootstrap" area. Rather than trying to refactor it once again, tweak the existing error messages to gently steer the user away from (potentially missing) extensions and towards built-in commands that provide a simpler and better error handling out of the box: they still show relevant, manifest error messages! Always have. Also recommand the -vv option which conveniently includes git error messages like "dubious ownership" since commit fcbb0a2 ("app: tweak logging on malformed manifest or configuration") from the same PR zephyrproject-rtos#664. Signed-off-by: Marc Herbert <[email protected]>

marc-hb · 2024-08-20T22:43:26Z

src/west/app/main.py

+    def handle_early_arg_errors(self, early_args):
+        # If early_args indicates we should error out, handle it
+        # gracefully. This provides more user-friendly output than
+        # argparse can do on its own.


It's more user-friendly... unless the manifest fails to import. Then the original, relevant error message is lost, see issue #726 and tentative mitigation #727

mbolivar-nordic added 7 commits May 16, 2023 12:06

manifest: clarify an internal attribute's initialization

08d7d00

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 <[email protected]>

commands: manifest: fix error path

33ebab6

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 <[email protected]>

tests: add Manifest.from_topdir() helper

72e669e

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 <[email protected]>

manifest: handle missing top level manifest file

79b07b7

Improve error handling to avoid dumping stack. Signed-off-by: Martí Bolívar <[email protected]>

manifest: remove dead stores

b06b279

We're not using the 'i' variable. Signed-off-by: Martí Bolívar <[email protected]>

tests: manifest: improve 'import: <map>' coverage

5c6d718

The error handling in the _load_imap() function is not being tested. This was discovered by manually inspecting the HTML coverage data. Add statement coverage for the error handling. Signed-off-by: Martí Bolívar <[email protected]>

mbolivar-nordic requested review from carlescufi, aescolar, tejlmand and marc-hb May 31, 2023 05:15

mbolivar-nordic mentioned this pull request May 31, 2023

bsim module must be cloned for west to work zephyrproject-rtos/zephyr#57198

Closed

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

marc-hb mentioned this pull request May 31, 2023

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

Closed

mbolivar-nordic added 10 commits May 31, 2023 19:48

app: move some code around

f6dc1ab

Moving all the "work" of main() into WestApp.run() will help us in future patches where we need to set up logging in a way that requires internal state from that object. Signed-off-by: Martí Bolívar <[email protected]>

app: tweak logging on malformed manifest or configuration

fd5a2e6

Put the first argument on the same line as the string "error:", to make it easier to grep. Signed-off-by: Martí Bolívar <[email protected]>

manifest: check project names as we go

1dff1ac

This will still fail or succeed in the same situations, but it is convenient to do it this way as prep work for future enhancements. Signed-off-by: Martí Bolívar <[email protected]>

manifest: add tests for manifest.project-filter

605903e

Add test cases for behavior and matching rules. Signed-off-by: Martí Bolívar <[email protected]>

mbolivar-nordic force-pushed the project-filter branch from 9fe3cb1 to 605903e Compare June 1, 2023 04:10

mbolivar-nordic marked this pull request as ready for review June 1, 2023 04:19

aescolar approved these changes Jun 1, 2023

View reviewed changes

src/west/app/project.py Show resolved Hide resolved

src/west/manifest.py Show resolved Hide resolved

src/west/manifest.py Show resolved Hide resolved

carlescufi approved these changes Jun 1, 2023

View reviewed changes

carlescufi merged commit 698e0e8 into zephyrproject-rtos:main Jun 1, 2023

aescolar mentioned this pull request Jun 1, 2023

RFC: manifest: Add finegrained group filters zephyrproject-rtos/zephyr#57225

Closed

This was referenced Jun 1, 2023

Support west manifest --resolve and --freeze with manifest.project-filter #667

Open

Follow up work for manifest.project-filter implementation #668

Closed

aescolar mentioned this pull request Aug 31, 2023

manifest: move optional modules to a submanifest and make them optional zephyrproject-rtos/zephyr#61505

Merged

marc-hb mentioned this pull request Aug 20, 2024

app: point "handle_unknown_command()" error message to "west -vv status" #727

Merged

marc-hb reviewed Aug 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`manifest.project-filter` #664

`manifest.project-filter` #664

mbolivar-nordic commented May 31, 2023 •

edited

Loading

aescolar commented May 31, 2023 •

edited

Loading

aescolar commented May 31, 2023 •

edited

Loading

mbolivar-nordic commented May 31, 2023

mbolivar-nordic commented May 31, 2023

mbolivar-nordic commented May 31, 2023

carlescufi commented May 31, 2023

mbolivar-nordic commented Jun 1, 2023

aescolar left a comment •

edited

Loading

tejlmand commented Jun 1, 2023

carlescufi commented Jun 1, 2023

carlescufi left a comment

mbolivar-nordic commented Jun 1, 2023

marc-hb Aug 20, 2024

manifest.project-filter #664

manifest.project-filter #664

Conversation

mbolivar-nordic commented May 31, 2023 • edited Loading

aescolar commented May 31, 2023 • edited Loading

aescolar commented May 31, 2023 • edited Loading

mbolivar-nordic commented May 31, 2023

mbolivar-nordic commented May 31, 2023

mbolivar-nordic commented May 31, 2023

carlescufi commented May 31, 2023

mbolivar-nordic commented Jun 1, 2023

aescolar left a comment • edited Loading

Choose a reason for hiding this comment

tejlmand commented Jun 1, 2023

carlescufi commented Jun 1, 2023

carlescufi left a comment

Choose a reason for hiding this comment

mbolivar-nordic commented Jun 1, 2023

marc-hb Aug 20, 2024

Choose a reason for hiding this comment

`manifest.project-filter` #664

`manifest.project-filter` #664

mbolivar-nordic commented May 31, 2023 •

edited

Loading

aescolar commented May 31, 2023 •

edited

Loading

aescolar commented May 31, 2023 •

edited

Loading

aescolar left a comment •

edited

Loading