Project groups #454

mbolivar-nordic · 2020-11-24T04:57:43Z

The purpose of this pull request is to allow projects to declare groups they belong to, like this:

  manifest:
    # ...
    projects:
    - name: foo
      groups:
      - YouAreIt
    - name: bar
      groups:
      - YouAreIt
      - MeToo
    - name: baz

The above projects have these groups lists:

foo: ['YouAreIt']
bar: ['YouAreIt', 'MeToo']
baz: []

Paired with this is a new top-level update-groups key in the manifest, which lets you specify allowlists and blocklists for groups:

    manifest:
      # ...
      groups:
          - some-group
          - another-group
      no-groups:
          - not-this-one

By default, west update will no longer update any project whose group is blocked (if there is a no-groups blocklist), or not explicitly allowed (if there is a groups allowlist). Allowlists override blocklists: if a project is blocked and allowed, it will be updated. You can also configure allowlists and blocklists in the local configuration file, via new manifest.groups and manifest.no-groups configuration options.

In order to allow for one-time overrides, let the user specify additional groups to add to the allowlist via a new "--groups" argument to west update, and additional groups to add to the blocklist via "--no-groups".

mbolivar-nordic · 2020-11-24T04:59:08Z

@carlescufi the basics are working for me with some simple manual west update testing, so I'm pushing this to open it up for feedback on the CLI while I'm improving test coverage / fixing bugs.

I guess we'll probably want to add some configuration options so users can set a workspace default for the new west command line options.

tejlmand

still needs some discussions.
As example, what should be presented on west list which is used in build system for inclusion of Zephyr modules ?

A user could have done west update --group <some-group> just to see what's there, and then later simply do west update and no longer care for group.

How should build system handle <some-group> if it is also a Zephyr module.

This is still referring to the bootstrapper. Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>

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

mbolivar-nordic · 2020-12-08T05:25:09Z

Pushed an update with the following changes:

manifest: groups: and manifest: no-groups: instead of manifest: update-groups: ..., i.e. the manifest file allowlists and blocklists are at top level now
for parallelism, west update --groups and west update --no-groups (plural) now
the manifest will consult the manifest.groups and manifest.no-groups local configuration file options to extend these lists if there is a workspace
renamed should_update to is_updatable
west list no longer prints projects that aren't updatable unless you give --all
a stale comment in conftest.py was found and removed in a new cleanup commit
more test cases

tejlmand · 2020-12-10T15:27:32Z

src/west/manifest.py

+        if cfg_no_groups or manifest_no_groups:
+            cfg_no_groups = cfg_no_groups or []
+            manifest_no_groups = manifest_no_groups or []
+            self.no_groups = cfg_no_groups + manifest_no_groups


So if manifest contains:

groups: - groupA - groupB no-groups: - groupC

and user configures:

[manifest] # I want group C groups = groupC # I don't want group A and B no-groups = groupA,groupB

then the self.groups and self.no_groups will both contain:

['groupA', 'groupB', 'groupC']

I don't think this is the intention.

When fixing this, I think user defined group configs should have precedence over manifest defaults.

I don't think this is the intention.

It is the intention; what's the issue?

I have made a small manifest example with groups here:
https://github.com/tejlmand/zephyr/tree/testing/west_454_groups

The manifest projects are divided into three groups: groupA, groupB, groupC.

groupA and groupB has a manifest setup, and groupC is just default, as:
https://github.com/tejlmand/zephyr/blob/7f42bb41c77d4d6d48930b211ca65ca8da983a65/west.yml#L17-L21

manifest: groups: - groupA no-groups: - groupB

doing a west list I get:

$ west list manifest zephyr HEAD N/A cmsis modules/hal/cmsis 542b2296e6d515b265e25c6b7208e8fea3014f90 https://github.com/zephyrproject-rtos/cmsis

just as expected.

Now, I don't want groupA listed (or updated), so let me disable that:

$ west config manifest.no-groups groupA $ west list manifest zephyr HEAD N/A cmsis modules/hal/cmsis 542b2296e6d515b265e25c6b7208e8fea3014f90 https://github.com/zephyrproject-rtos/cmsis

Hooh, it's still there. As a user I want to be able to overrule the manifest file and disable a group.

Let me see if I can enable groupB.

$ west config manifest.groups groupB $ west list manifest zephyr HEAD N/A cmsis modules/hal/cmsis 542b2296e6d515b265e25c6b7208e8fea3014f90 https://github.com/zephyrproject-rtos/cmsis hal_atmel modules/hal/atmel 9f0a699a2f0253dc9688211ef696daa7b8fadb75 https://github.com/zephyrproject-rtos/hal_atmel

working as expected.

(Same happens for west update of course)

Also:

$ west help update ... advanced options: .... -G GROUPS, --no-groups GROUPS update as if GROUPS are in manifest.no-groups

Cool, let me try this:

$ west update -G groupA === updating cmsis (modules/hal/cmsis): HEAD is now at 542b229 DSP: Integrate CMSIS-DSP 1.8.0 (CMSIS 5.7.0)

Damn, groupA is still being updated.

Note, it's not only the commented line with this, but both those lines:
https://github.com/mbolivar-nordic/west/blob/project-groups/src/west/manifest.py#L1506
https://github.com/mbolivar-nordic/west/blob/project-groups/src/west/manifest.py#L1511

I believe the user should be able to overrule settings from west.yml or .west/config

Hooh, it's still there. As a user I want to be able to overrule the manifest file and disable a group.

+1 to that.

Damn, groupA is still being updated.

That's a bug IMHO. -G should take precedence to configs and manifest, since the user is overriding explicitly.

tejlmand · 2020-12-10T15:28:29Z

@mbolivar-nordic Thanks for the update.

tejlmand

A couple of small extra thoughts.
Feel free to ignore if you disagree.

src/west/app/project.py

Testing Signed-off-by: Torsten Rasmussen <Torsten.Rasmussen@nordicsemi.no>

tejlmand

Tested with:
https://github.com/tejlmand/zephyr/tree/testing/west_454_groups

where west.yml contains:

manifest:
  groups:
    - groupA
    - -groupB
    - -groupC

works.
Only groupA is listed:

$ west list
manifest     zephyr                       HEAD                                     N/A
cmsis        modules/hal/cmsis            542b2296e6d515b265e25c6b7208e8fea3014f90 https://github.com/zephyrproject-rtos/cmsis

Enabling groupB also works:

$ west config manifest.groups "groupB"
$ west list 
manifest     zephyr                       HEAD                                     N/A
cmsis        modules/hal/cmsis            542b2296e6d515b265e25c6b7208e8fea3014f90 https://github.com/zephyrproject-rtos/cmsis
hal_atmel    modules/hal/atmel            9f0a699a2f0253dc9688211ef696daa7b8fadb75 https://github.com/zephyrproject-rtos/hal_atmel

but enabling multiple groups fails:

$ west config manifest.groups "groupB,groupC"
$ west list 
manifest     zephyr                       HEAD                                     N/A
cmsis        modules/hal/cmsis            542b2296e6d515b265e25c6b7208e8fea3014f90 https://github.com/zephyrproject-rtos/cmsis

my .west/config:

$ west config -l
manifest.path=zephyr
manifest.groups="groupB,groupC"
zephyr.base=zephyr

mbolivar-nordic · 2020-12-16T21:09:48Z

manifest.groups="groupB,groupC"

That's the problem. If you manually edit it to

manifest.groups=groupB,groupC

Then it works.

The root cause seems to be that we are using the standard library configparser to read values, but the third party configobj library to write them. They aren't working the same way: configparser returns the value with the quotes, which don't get stripped, and end up unblocking the groups "groupB and groupC".

mbolivar-nordic · 2020-12-16T21:10:59Z

@tejlmand I'll need to think about the right way to fix this bug. This ties in to #149 because the current west.configuration API is stateful and awkward to use anyway.

mbolivar-nordic · 2020-12-17T18:56:52Z

Fixed it by dropping the configobj dependency completely. See commit log for further justification and details.

mbolivar-nordic · 2020-12-18T18:13:58Z

Now changed to OR together the project group allow status instead of ANDing it together as discussed offline.

tejlmand

Looks good to me.
Thanks for the good effort.

btw. I got some commented out lines deleted in my .west/config that I had kept for future reference when testing this 😵

tejlmand · 2020-12-18T21:06:58Z

src/west/manifest.py

+            blocked_groups = self._blocked_groups
+
+        # A project with groups is active if and only if any one of its
+        # groups are blocked.


Did you mean, groups are not blocked. ?

Did you mean, groups are not blocked. ?

Yes. I just deleted the comment since the docstring has all the details anyway. Thanks.

We currently don't have a way to group projects with extra semantic information. There is a use case for this in Zephyr; the ultimate goal is to allow manifests to not update certain groups of projects by default, but allow easy update of those groups if the user opts in. Start to enable this by letting a 'projects' item in a manifest file have a 'groups' key, which must contain a list of scalar values (str, int, or float). For example, in this manifest: manifest: # ... projects: - name: foo groups: - YouAreIt - name: bar groups: - YouAreIt - MeToo - name: baz The projects have these groups lists: - foo: ['YouAreIt'] - bar: ['YouAreIt', 'MeToo'] - baz: [] As a restriction to give us some room to grow into and generally keep things sane, 'projects' group values must not contain commas (,), colons (:), whitespace. Groups cannot start with a '-' either as that's how we'll be blocking them. The manifest data are converted to lists of str internally and associated with the west.manifest.Project objects. Projects are "active" when any of their groups are not blocked. Groups are allowed by default, but may be blocked using the manifest, like this: manifest: ... groups: [-thisisblocked,-this-too] Or via the .west/config workspace local configuration file, like this: [manifest] groups = -blocked1,-blocked2,allowed I.e. via commands like: $ west config manifest.groups -- -blocked1,-blocked2,allowed There is also an API routine is_active() for checking if a project is active, potentially with some extra blocklist and allowlist entries for groups obtained from elsewhere. For now, no commands are aware of this. We'll make commands respond differently to 'inactive' projects in the next patches. Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>

Add support in "west update" for skipping updates for any project that the Manifest object says is not active. In order to allow for overrides, let the user specify additional groups to add to the allowlist via a new "--group" argument, and additional groups to add to the blocklist via "--no-group". This is useful for debugging credential management and doing things like quickly syncing the entire potential workspace with a command like "west update --groups all". Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>

Add '-f {groups}' and '--all' options. Respect active projects by not printing inactive ones by default. This allows printing a project's groups, and hides non-updatable groups by default. Here we rely on the fact that groups may not have commas or whitespace to ensure that the printed-out groups are a single whitespace-free and comma separated string in the 'west list' output. For example, in this manifest: manifest: # ... projects: - name: foo groups: - foo-group-1 - foo-group-2 - name: bar path: path-for-bar - name: baz groups: - baz-group The output for "west list -f '{name} {groups} {path}'" is the following, with <SPC> standing for a space character: foo<SPC>foo-group-1,foo-group-2<SPC>foo bar<SPC><SPC>path-for-bar baz<SPC>baz-group<SPC>baz The empty string in the {groups} field for 'bar' can be picked up by tools like cut(1) to find the entire list of groups. E.g., this command $ west list -f '{name} {groups} {path} | cut -d' ' -f 2 will print: foo-group-1,foo-group-2 baz-group Additionally, we repurpose the old --all command line option (that used to print the location of the bootstrapped west repository back in the 0.5.x days) to print projects that would be skipped by 'west update' by default due to groups or no-groups lists. We're about to do something similar to several other commands, so structure the help in the epilog in a way that we'll take advantage of in subsequent patches. Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>

This change is similar to the one already done for west list: - don't print inactive projects by default - add an --all argument to include inactive projects Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>

I just noticed this doesn't fit on one line in the west help output. Let's fix that. Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>

Make it fit on one line in the west help output. Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>

This is currently failing because we are using configparser to read, but configobj to write. The two don't seem to be compatible. Setting an option to 'bar,baz' uses configobj, which writes the string "bar,baz" -- with quotes -- to the config file. Then when we read it back with configparser, we get the value '"bar,baz"' -- quotes are not stripped. This will need to be fixed. Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>

West has historically relied on the third-party configobj library for writing configuration files instead of using the standard library's configparser module. The reason why is that configobj has round-trip support for comments. However, the public reading API uses configparser. Up until now, we were assuming that the two were compatible for the simple purposes we needed, and indeed they've proven compatible "enough" that the different code paths on read vs. write haven't been an issue. This has become a problem now that we are introducing the manifest.groups configuration option, though, because its value contains commas. The configparser and configobj file formats have a semantic difference between these two options, though: [section] foo = "one,two" bar = one,two The difference is: - in configobj, 'foo' is the string "one,two" and 'bar' is the list ['one', 'two'] - in configparser, 'foo' is the string '"one,two"' and bar is the string 'one,two' Further, the configobj library automatically adds quotes around any string that contains commas to enforce this distinction. This is breaking round-trips, since: west config section.foo one,two # configobj writes "one,two" west config section.foo # configparser reads '"one,two"' Looking at it further, configobj development seems to have stalled in 2014, and the most significant user it claims in its documentation (IPython) has moved on to .py and .json configuration files. This isn't worth the hassle. Just drop the configobj dependency and use configparser everywhere. This will delete comments that users have added to their configuration files and may otherwise reorder sections, but having the round-trip semantics correct is more important than that. Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>

West has introduced support for group tags in: zephyrproject-rtos/west#454 This means that manifest files might start containing groups. Zephyr itself only requires west>=0.7.2 where groups are not supported but other Zephyr based projects might start using the group feature. When using a west version with group support, then only active projects will be processed as Zephyr modules. West versions without group support will consider all projects active. Signed-off-by: Torsten Rasmussen <Torsten.Rasmussen@nordicsemi.no>

West has introduced support for group tags in: zephyrproject-rtos/west#454 This means that manifest files might start containing groups. Zephyr itself only requires west>=0.7.2 where groups are not supported but other Zephyr based projects might start using the group feature. When using a west version with group support, then only active projects will be processed as Zephyr modules. West versions without group support will consider all projects active. Signed-off-by: Torsten Rasmussen <Torsten.Rasmussen@nordicsemi.no> (cherry picked from commit 0cafde6)

mbolivar-nordic requested review from tejlmand and carlescufi November 24, 2020 04:57

mbolivar-nordic force-pushed the project-groups branch 2 times, most recently from 9b1003f to 847c2a2 Compare November 24, 2020 05:04

tejlmand requested changes Dec 7, 2020

View reviewed changes

mbolivar-nordic force-pushed the project-groups branch from 847c2a2 to 49d14d6 Compare December 8, 2020 05:22

mbolivar-nordic added 2 commits December 7, 2020 21:22

tests/conftest.py: fix stale comment

ed22ece

This is still referring to the bootstrapper. Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>

west update: delete dead code

07b25e7

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

mbolivar-nordic force-pushed the project-groups branch from 49d14d6 to 1d1c7ec Compare December 8, 2020 05:22

mbolivar-nordic changed the title ~~[WIP] Project groups~~ Project groups Dec 8, 2020

mbolivar-nordic force-pushed the project-groups branch 3 times, most recently from 964a668 to 7155b6e Compare December 9, 2020 15:40

tejlmand requested changes Dec 10, 2020

View reviewed changes

tejlmand reviewed Dec 10, 2020

View reviewed changes

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

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

mbolivar-nordic force-pushed the project-groups branch from 7155b6e to bceb5b3 Compare December 11, 2020 03:17

mbolivar-nordic force-pushed the project-groups branch from bceb5b3 to 40d36d3 Compare December 16, 2020 07:00

tejlmand added a commit to tejlmand/zephyr that referenced this pull request Dec 16, 2020

west: zephyrproject-rtos/west#454 testing

dc206a0

Testing Signed-off-by: Torsten Rasmussen <Torsten.Rasmussen@nordicsemi.no>

tejlmand requested changes Dec 16, 2020

View reviewed changes

mbolivar-nordic force-pushed the project-groups branch from 9f3afd1 to 54a4f13 Compare December 18, 2020 18:04

tejlmand approved these changes Dec 18, 2020

View reviewed changes

mbolivar-nordic added 7 commits December 18, 2020 13:13

project.py: teach diff/status/forall about project groups

c1ce4e1

This change is similar to the one already done for west list: - don't print inactive projects by default - add an --all argument to include inactive projects Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>

west list: tweak help string

0e4a7e4

I just noticed this doesn't fit on one line in the west help output. Let's fix that. Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>

west config: tweak help string

98ad801

Make it fit on one line in the west help output. Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>

mbolivar-nordic force-pushed the project-groups branch from 54a4f13 to 4cd9f35 Compare December 18, 2020 21:21

mbolivar-nordic merged commit 36f3f91 into zephyrproject-rtos:master Dec 18, 2020

tejlmand mentioned this pull request Dec 21, 2020

modules: introducing MODULE_ROOT to allow glue code in Zephyr repo zephyrproject-rtos/zephyr#30904

Merged

tejlmand mentioned this pull request Jan 7, 2021

scripts: modules: support west group feature zephyrproject-rtos/zephyr#31146

Merged

tejlmand mentioned this pull request Jan 29, 2021

[nrf fromtree]: scripts: modules: support west group feature nrfconnect/sdk-zephyr#433

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Project groups #454

Project groups #454

mbolivar-nordic commented Nov 24, 2020 •

edited

mbolivar-nordic commented Nov 24, 2020 •

edited

tejlmand left a comment

mbolivar-nordic commented Dec 8, 2020 •

edited

tejlmand Dec 10, 2020

mbolivar-nordic Dec 11, 2020

tejlmand Dec 11, 2020

carlescufi Dec 11, 2020

tejlmand commented Dec 10, 2020

tejlmand left a comment •

edited

tejlmand left a comment

mbolivar-nordic commented Dec 16, 2020

mbolivar-nordic commented Dec 16, 2020

mbolivar-nordic commented Dec 17, 2020

mbolivar-nordic commented Dec 18, 2020

tejlmand left a comment

tejlmand Dec 18, 2020

mbolivar-nordic Dec 18, 2020

Project groups #454

Project groups #454

Conversation

mbolivar-nordic commented Nov 24, 2020 • edited

mbolivar-nordic commented Nov 24, 2020 • edited

tejlmand left a comment

Choose a reason for hiding this comment

mbolivar-nordic commented Dec 8, 2020 • edited

tejlmand Dec 10, 2020

Choose a reason for hiding this comment

mbolivar-nordic Dec 11, 2020

Choose a reason for hiding this comment

tejlmand Dec 11, 2020

Choose a reason for hiding this comment

carlescufi Dec 11, 2020

Choose a reason for hiding this comment

tejlmand commented Dec 10, 2020

tejlmand left a comment • edited

Choose a reason for hiding this comment

tejlmand left a comment

Choose a reason for hiding this comment

mbolivar-nordic commented Dec 16, 2020

mbolivar-nordic commented Dec 16, 2020

mbolivar-nordic commented Dec 17, 2020

mbolivar-nordic commented Dec 18, 2020

tejlmand left a comment

Choose a reason for hiding this comment

tejlmand Dec 18, 2020

Choose a reason for hiding this comment

mbolivar-nordic Dec 18, 2020

Choose a reason for hiding this comment

mbolivar-nordic commented Nov 24, 2020 •

edited

mbolivar-nordic commented Nov 24, 2020 •

edited

mbolivar-nordic commented Dec 8, 2020 •

edited

tejlmand left a comment •

edited