Test fixed group filter semantics

[mbolivar-nordic]

Superseded by #482

[mbolivar-nordic]

I'm sending this as test cases first so that we can agree on what we want to see before I implement it.

[marc-hb]

Some thoughts after a Slack discussion with Marti:

• Let's first get the "Doctor, it hurts when I do this" bit out of the way. I didn't realize group filters were meant for such a use case. I mean I didn't expect widely distributed and "importable" manifests to distribute "opt-in" projects disabled by default and especially not for projects with restricted access. I expected group filters to be used only in top-level manifests not meant to be imported further. I expected restricted access or niche projects to be referenced only in more specific manifests that would have to be imported specifically. So this problem seems very artificial to me. Including something excluded is a double negation and manifest: move homekit elsewhere nrfconnect/sdk-nrf#3884 looks like a permanent fix to me. Trying to overcome this and provide some useful feedback anyway:

• I would avoid the word "inheritance" if possible. It's scary and I hope the documentation for group filters will never have to refer to a page like this: https://en.wikipedia.org/wiki/Covariance_and_contravariance_(computer_science). Simple suggestion: "s/inherited/imported/ group-filters"

• Speaking of imports, it would be nice if the precedence rules for group filters somewhat match the ones for projects. Easier to remember but of course actual use cases matter more.

• Short of experimenting with actual use cases before implementing it (chicken/egg), good test coverage mimicking all anticipated use cases seems important.

• Good logging is important too because searching multiple manifest files to go and try to figure out the final filter combination will not be fun. Can hopefully be enhanced later.

• Should imported groups have a namespace/prefix based on the manifest they come from? In other words:

# my/west.yml
   group-filter: [+upstream.disabled-group]
   manifest:
     projects:
       - name: upstream
         import: true

or

# my/west.yml
   manifest:
     projects:
       - name: upstream
         import: true
         group-filter: [+disabled-group]

or both.

• Similar, non-mutually exclusive idea from Marti:

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

The last one would basically allow anyone to keep the simpler (IMHO) 0.9.0 behaviour.

Best of luck!

[carlescufi]

So this problem seems very artificial to me. Including something excluded is a double negation and nrfconnect/sdk-nrf#3884 looks like a permanent fix to me.

I disagree here. This problem is, in my opinion, as "natural" as they come, as it appeared in a real-life scenario of west usage.
We offer a single manifest (sdk-nrf/west.yml) as the entry point to an SDK. But we want that SDK to also be able to offer "extra" functionality via the inclusion of additional non-public repositories. So, instead of using multiple manifests, which is error-prone and complicates the lives of the user, the cleanest approach is to have a single (manifest) entry point to the SDK, with additional options to opt-in into the non-public repos.

That way all users do the same when initializing the SDK:

west init -m <url to SDK> --mr <revision>

and most will then simply do:

west update

without even having to worry about the presence of additional non-public repos in that manifest.
But those with access and knowledge can take the additional step:

west config manifest.group-filter +private
west update

In the same way, all SDK users import the main (and only) manifest entry point in their manifests for their own SDK-based applications, again without having to worry about the presence of non-public repos. Those who do care about them then do not need to do anything extra in their manifest, instead just using the same west config line from above.

[marc-hb]

So, instead of using multiple manifests, which is error-prone and complicates the lives of the user,

I have very little experience with this but I don't see why this must be error-prone and more complicated.

Typical software development involves a top-level executable that "imports"/ links to a tree of libraries/dependencies and the decisions about what gets included are generally centralized at the top-level. More importantly they don't involve (double) negations. There's rarely a need to combine operators up and down the tree to figure out the final result.

Off-topic sorry.

[carlescufi]

I have very little experience with this but I don't see why this must be error-prone and more complicated.

Having a single entry point means that the user never has to deal with the actual name of the manifest file during west init. Every single user of the SDK will only ever have to do:

west init -m <url to SDK> --mr <revision>

And then configure the group filter correctly based on their needs/access to non-public features.
If you start adding manifests, then suddenly people have to (or can) point to them:

west init -m <url to SDK>/west-non-public.yml --mr <revision>

but they also need to deal with group filters because they might not have access to all the repos listed in west-non-public.yml. Which means that they now need to deal with 2 (or more) manifests and also group filters, instead of just group filters. The same applies with using import: from their own manifests.

[marc-hb]

but they also need to deal with group filters because they might not have access to all the repos listed in west-non-public.yml

Right, a combination of the two schemes is always going to be more complicated than a single one of them :-) I didn't foresee any west-non-public.yml manifest, but a homekit.yml and other similar manifests. My assumption was that users never import manifests listing repos they don't have access to, that's what feels "unnatural" to me. This would admittedly require users with "advanced" needs (for instance users who need repos on more than one restricted server) to compose their top-level manifest themselves. I think it would make west manifests more similar to other manifests (I don't remember any other manifest supporting exclusions like this) and things simpler for everyone else. Never mind.

[mbolivar-nordic]

Hi @marc-hb and thanks for the very thoughtful feedback. Here are my responses.

I didn't realize group filters were meant for such a use case.

Noted -- however, that is indeed what they are meant for, so we need to proceed on that basis.

I would avoid the word "inheritance" if possible.

That's reasonable, and I've struck the word inherit from the pull request description.

Speaking of imports, it would be nice if the precedence rules for group filters somewhat match the ones for projects. Easier to remember but of course actual use cases matter more.

Yes, as discussed on Slack, I absolutely agree this must be consistent with the way that manifest import order already works. I will send a draft PR to the documentation so we can see the new semantics in black and white.

Short of experimenting with actual use cases before implementing it (chicken/egg), good test coverage mimicking all anticipated use cases seems important.

Yep, that is what I'm trying to do here. I will add some more test cases for the use cases which I am aware of.

However, we have to be practical about how much we can expect. To a first approximation, nobody reviews, they just use things when they come out and complain if they don't get what they want. That's totally fine and in fact is what happened here. We are adjusting based on that feedback.

Good logging is important too because searching multiple manifest files to go and try to figure out the final filter combination will not be fun. Can hopefully be enhanced later.

Yes, the PR which implements the changes will contain additional debug logging related to this change.

Should imported groups have a namespace/prefix based on the manifest they come from?

I don't think that's a good idea. The source of group's definition is not important, any more than the source of a project definition is important. We don't prefix a project's name with an identifier for the manifest that defined it for good reasons which also apply here.

Concretely, if I have this project in zephyr/west.yml:

name: foo
groups:
  - bar

I can run west config manifest.group-filter -- -bar to make foo inactive.

Now if I get that very same project via import, all of a sudden I have to run west config manifest.group-filter -- -zephyr.bar instead? That's not OK.

Why? For one thing, it breaks use cases where people change their manifest repository via west config manifest.path zephyr / west config manifest.path my-manifest-repo followed by west update. Such users would have to keep their manifest.group-filter values synced up with the current source of the groups they are interested in.

That makes no sense and would make me quite frustrated as a user.

Similar, non-mutually exclusive idea from Marti:

On Slack, I offered to add this and you said no. If you're changing your mind now and you would like to request it, I will make sure to implement it. Please confirm if that's the case.

[tejlmand]

Based on this observation: zephyrproject-rtos/zephyr#32106 (comment)
i'm not sure we can actually cut a 0.9.1 now.

[tejlmand]

Now if I get that very same project via import, all of a sudden I have to run west config manifest.group-filter -- -zephyr.bar instead? That's not OK.

Why? For one thing, it breaks use cases where people change their manifest repository via west config manifest.path zephyr / west config manifest.path my-manifest-repo followed by west update. Such users would have to keep their manifest.group-filter values synced up with the current source of the groups they are interested in.

Actually, this is a very interesting thought imo.
Users doing west config manifest.path zephyr / west config manifest.path my-manifest-repo are clearly advanced west users, and to advanced users such a feature / behavior could be useful.

Example:

west config manifest.group-filter -- -zephyr.bar   # This will not give me bar when I work downstream
west config manifest.group-filter -- +bar`         # This will give me bar when I work upstream

west config manifest.path my-manifest-repo
west update                                    # bar will not be included

west config manifest.path zephyr
west update                                    # bar will now be included

So a user switching between up- and downstream could actually benefit from not having to reconfigure groups when switching.

But I think the current proposal is simpler to describe and understand, and thus less risk of confusing users.
So I think KISS wins in this case.

[tejlmand]

@mbolivar-nordic

This commit adds tests for the expected behavior for the new schema
version and adds regression tests ensuring that explicitly requesting
schema version 0.9 produces the same results.

what will happen in this case:

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

and:

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

where upstream specifies 0.9 and downstream specifies 0.10.
(or the opposite order, upstream 0.10 and downstream 0.9)

[tejlmand]

Thinking a bit more on this:

# Schema version 0.10 will reverse that behavior: manifests which
# request schema version 0.10 will get Manifest.group_filter
# values that *ARE* inherited from imported manifests, by
# appending these values in import order.
#
# For compatibility, manifests which explicitly request a 0.9 schema
# version will get the old behavior.

Instead of doing what is described above, then what if we deprecate the key group-filter and introduce new-group-filter (TBD: find a better name).

That way, group-filter will keep its current behavior and new-group-filter will have new behavior, thus all the hassle of selecting behavior based on manifest schema version goes away. Two unique keys, where one was short lived.

[marc-hb]

That way, group-filter will keep its current behavior and new-group-filter will have new behavior, thus all the hassle of selecting behavior based on manifest schema version goes away. Two unique keys, where one was short lived.

That feels more complicated to me because now there is the possibility of using different keys at different import levels = a combinatorial explosion impossible to test practically.

What about simply changing the behavior unconditionally? Not looking at the manifest version except for printing a warning? In other words, treat this as a bug fix considering this was always the behavior intended in the first place? Note this is from the person who find this "opt-in" feature... weird? Yet I value the simplicity of just one weird behavior more than some complex matrix of possibilities.

Yes this would break "backwards compatibility" but backwards compatibility seems overrated here, the perfect is the enemy of the good. To be affected you would have to be:

• using 0.9 that was released a very short time ago,
• importing manifest(s) with group filters, a brand new feature that was released a very short time ago,
• importing a manifest with a negative, "opt-in" group filter

Isn't that a small set of use cases already? At this point there are two subsets of users left: those who use the opt-in group and those who don't. For non-users, this change will be a pure and sometimes eagerly expected bug fix. So only actual users will have the minor inconvenience of having to actually opt-in. Once they have, any west version will work for them.

[tejlmand]

What about simply changing the behavior unconditionally? Not looking at the manifest version except for printing a warning? In other words, treat this as a bug fix considering this was always the behavior intended in the first place?

I like that approach, let's see if others agree.

[mbolivar-nordic]

Superseded by #482

[mbolivar-nordic]

What about simply changing the behavior unconditionally? Not looking at the manifest version except for printing a warning? In other words, treat this as a bug fix considering this was always the behavior intended in the first place?

I like that approach, let's see if others agree.

I didn't end up going this way. The Manifest._finalize_group_filter helper which is responsible for deciding whether to give the new or legacy semantics ended up being quite short to write.

[marc-hb]

Linking to new "import filter" feature #543