Initial import filtering implementation #656
Conversation
The mypy process is warning that we can't type check functions whose arguments aren't typed. Typing the argument in this case leads to an error that should be fixed. Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
This is a more convenient way to construct manifests so we might as well have it around. Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
It will be convenient to manipulate configuration files in a safe way from outside of test_config.py. To do this, make the config_tmpdir global to all test modules by migrating it to conftest.py. We still want to keep that fixture as an autouse in test_config.py, so add a shim to keep that as-is. Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
The more I think about it, the more convinced I am personally that combining 'import:' with 'groups:' is not a sensible thing to do semantically. Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
This feature provides a way to prevent west from performing an import
depending on a condition.
There are a variety of use cases for this:
- CI system maintainers want to be able to define an 'import' that
contains repositories that contain test cases and which should be
easy for developers to fetch with 'west update', but which are not
distributed to users by default (by 'turning off' the import
somehow)
- Similarly, but for projects which provide optional features that can
be safely ignored unless the user takes an explicit action to enable
the import. The main use case here isn't limiting visibility of
the feature, but rather limiting the storage size and network
bandwidth required to initialize a basic workspace
Provide this feature by extending the 'import: <map>' syntax with a
new key, 'if':
projects:
- name: foo
import:
if:
<condition>
Where the value of <condition> is itself a map, so that we can add
features to it over time. In this commit, we add the infrastructure
for the 'if' key and one simple way to write <condition>:
import:
if:
config-enabled: foo.bar
Here, the import will be resolved if and only if the west
configuration option 'foo.bar' is defined and set to a true boolean
value (see the python configparser docs for all the ways to write a
boolean option; "west config foo.bar true" is one way to set this up).
The configuration option can be set in any configuration file: local,
global, or system. This makes it easier for users who will want the
import performed but regularly set up workspaces in various places to
set it up permanently, without having to enable foo.bar every time
they set up a workspace, before the first "west update".
Design notes for the record:
- I was originally trying to do something like "import: if:
group-enabled: bsim", but that runs into possibilities for
self-contradiction, because a project that is imported later than
you can disable a group, so you don't really know if a group is
enabled or not until you are done parsing the manifest.
- It thus made sense to me to define a syntax for conditional
imports that doesn't rely on groups at all. Configuration options
are independent of groups and are set once and for all by the time
we are loading the manifest, so it seemed like a good place to
start.
- People are still rapidly ideating new ways to do import
filtering for different use cases, so a one-size-fits-all solution
seems unlikely. Making a flexible syntax that we can extend later
therefore seemed more future-proof. In the future, we can do things
like:
import:
if:
env: SOME_ENV_VAR=1
or:
import:
if:
anyOf:
config-enabled: foo.bar
env: SOME_ENV_VAR=1
etc. There seems to be plenty of room to grow.
Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
This comment was marked as resolved.
This comment was marked as resolved.
The flexibility looks great to avoid painting ourselves in a corner but I'm not sold on the first,
That contradiction and potential remedies were discussed in #543. One of the conclusions was: this contradiction stems from the fact that import filtering and group filtering are two very different things. So you seem to have made them very different indeed in this PR based on the description (I haven't read the code yet, I believe this is still at the "design" phase). Aren't they too different now? Let's take for instance bsim issue zephyrproject-rtos/zephyr#57198 which looks like it motivated this (57198 is one week old whereas #543 is 1-2 years old). Can you describe in detail the west configuration that would solve 57198 with this PR? (Ignoring of course other later attempts to solve 57198). More specifically: would enabling or disabling A "combo switch" is the main suggestion I made in #543. Long story short: import filtering and group filtering are kept distinct, happening at distinct phases and they don't talk to each other. BUT they just happened to look at the same filter keywords for UI convenience. This gives import filtering some sort of "precedence" resolving self-contradicting configurations when "creative" users try to break manifests. That precedence is only because import filtering runs first; again both filters would not interact with each other. |
@marc-hb It is my understanding that we'd only use the import filter in that case and remove the "babblesim" group filter, as it would otherwise just be redundant. |
If I read this rightly, it means that an additional west command is required to enable a group, but doesn't that then mean it can't be enabled by a custom west manifest? E.g. if someone wants to setup a manifest which imports zephyr and gets babblesim, they wouldn't be able to do that from their custom west.yml unless they ran a |
|
Fair points @marc-hb and @nordicjm . Today @carlescufi @aescolar and I met and discussed an alternative approach I will prototype today. |
|
Closing this for now since we're going another way |
Related issue: #543
This feature provides a way to prevent west from performing an import
depending on a condition.
There are a variety of use cases for this:
CI system maintainers want to be able to define an 'import' that
contains repositories that contain test cases and which should be
easy for developers to fetch with 'west update', but which are not
distributed to users by default (by 'turning off' the import
somehow)
Similarly, but for projects which provide optional features that can
be safely ignored unless the user takes an explicit action to enable
the import. The main use case here isn't limiting visibility of
the feature, but rather limiting the storage size and network
bandwidth required to initialize a basic workspace
Provide this feature by extending the
import: <map>syntax with anew key, 'if':
Where the value of
<condition>is itself a map, so that we can addfeatures to it over time. In this commit, we add the infrastructure
for the 'if' key and one simple way to write
<condition>:Here, the import will be resolved if and only if the west
configuration option 'foo.bar' is defined and set to a true boolean
value (see the python configparser docs for all the ways to write a
boolean option; "west config foo.bar true" is one way to set this up).
The configuration option can be set in any configuration file: local,
global, or system. This makes it easier for users who will want the
import performed but regularly set up workspaces in various places to
set it up permanently, without having to enable foo.bar every time
they set up a workspace, before the first "west update".
Design notes for the record:
I was originally trying to do something like "import: if:
group-enabled: bsim", but that runs into possibilities for
self-contradiction, because a project that is imported later than
you can disable a group, so you don't really know if a group is
enabled or not until you are done parsing the manifest.
It thus made sense to me to define a syntax for conditional
imports that doesn't rely on groups at all. Configuration options
are independent of groups and are set once and for all by the time
we are loading the manifest, so it seemed like a good place to
start.
People are still rapidly ideating new ways to do import
filtering for different use cases, so a one-size-fits-all solution
seems unlikely. Making a flexible syntax that we can extend later
therefore seemed more future-proof. In the future, we can do things
like: