twister: Not possible to build/run tests with binary blob dependencies in downstream trees

[jhedberg]

Describe the bug

Since upstream Zephyr CI has the policy of never fetching binary blobs, test configurations which require such blobs have been disabled in various ways. For example, the Silabs Bluetooth HCI driver depends on west blobs fetch hal_silabs in order to build. Because of this, Silabs boards have the following filters in their definition:

zephyr/boards/silabs/dev_kits/xg27_dk2602a/xg27_dk2602a.yaml

Lines 15 to 18 in 2aa31a3

	testing:
	ignore_tags:
	- net
	- bluetooth

To Reproduce

$ west blobs fetch hal_silabs
$ west twister -T ../zephyr/samples -s sample.bluetooth.peripheral_dis -p xg27_dk2602a -b -K -v

Expected behavior

The tests can be built.

Actual behavior

INFO    - 1 test scenarios (1 test instances) selected, 1 configurations skipped (1 by static filter, 0 at runtime).
INFO    - 0 of 1 test configurations passed (0.00%), 0 failed, 0 errored, 1 skipped with 0 warnings in 5.08 seconds
INFO    - 0 test configurations executed on platforms, 0 test configurations were only built.

Impact

It's not possible to automate blob-dependent builds with twister in a downstream tree.

Possible solutions

Twister currently has a -K (or --force-platform) switch to ignore any platform related filters. However, it doesn't seem to have anything to ignore tag-based filters. Having a way to ignore the tag filtering would be one possible way to alleviate the issue, but it doesn't seem particularly clean or scalable in the long run.

A perhaps better solution would be if we could include information in the board metadata (the yaml file) about blob dependencies, e.g. being able to enumerate which tags require blobs. Once this kind of information is available twister could by default filter out any tests that require blobs. There could then be a new command line switch to explicitly disable such filtering and assume that the blob dependencies have been fulfilled before running twister.

Yet another way (just brainstorming here) to indicate blob dependencies could be through Kconfig, i.e. something like:

config CONFIG_BT_HCI_SILABS
	depends on CONFIG_HAL_SILABS_MODULE_BLOBS

[jhedberg]

@nashif even though this got auto-assigned to you, I could try creating a fix myself, assuming we have some consensus on what kind of fix is most acceptable.

[jhedberg]

@ifyall @sylvioalves @erwango there are HCI drivers for your respective platforms which depend on west blobs fetch to build & run. Have you used some other kind of solution downstream to automate testing with twister, or you just don't use twister?

The whole platform_allow stuff in most of our Bluetooth tests and samples should IMO be removed so that we rely solely on the tags (like bluetooth), however there'd then need to be another way for upstream CI to filter out blob dependent configurations (see my solution suggestions above).

[sylvioalves]

@jhedberg, we do have a downstream CI to run Wi-Fi and BLE tests as all ESP32-based boards use ignore_tags. As you mentioned, we have a script to patch those ignore list to allow testing accordingly.
Another scenario that would also be interesting is to mock-up all blobs with sources files. That would allow blob-dependent vendor to get GitHub CI running Wi-Fi/BLE tests. Of course it would not allow running application but would cover twister testing at least.

[jhedberg]

Another scenario that would also be interesting is to mock-up all blobs with sources files. That would allow blob-dependent vendor to get GitHub CI running Wi-Fi/BLE tests. Of course it would not allow running application but would cover twister testing at least.

That's indeed an interesting solution which I hadn't thought of. I suppose it'd still require some kind of "awareness" of the build system to decide whether to build against the mock implementation or to link against the blobs.

[keithmwheeler]

@ifyall asked me to respond. Currently we are not performing any BLE tests; we're only checking for compilation errors with BLE apps so we're using west blobs fetch just to build. My team is currently working on enabling functional BLE tests; I can share our process when we are up and running.

[nashif]

@nashif even though this got auto-assigned to you, I could try creating a fix myself, assuming we have some consensus on what kind of fix is most acceptable.

sure, but IMO this is an enhancement, not a bug. sample.bluetooth.peripheral_dis does already have a limited list of allowed platforms anyways, so the tag ignoring is not the main issue here.

zephyr/boards/silabs/dev_kits/xg27_dk2602a/xg27_dk2602a.yaml

Lines 15 to 18 in 2aa31a3

	testing:
	ignore_tags:
	- net
	- bluetooth

this appears in many boards and is more or less what used to be a good way to not build and test networking/bluetooth for boards that did not support any of those subsystems, it is not being consistently used and we need a better way, so this might help at one level if we find a solution here.

a feature enabled by a board that requires binary blob and filtering on those is something new. I am still not sure how this will work in general and with twister in particular.
west blobs fetch .. should in this case flip some switch somewhere that tell us that we are able to build this feature and twister should look at this switch but then we are looking at mapping blobs to specific samples or tests, which will get very complicated...

[jhedberg]

sure, but IMO this is an enhancement, not a bug

Sure, makes sense.

sample.bluetooth.peripheral_dis does already have a limited list of allowed platforms anyways, so the tag ignoring is not the main issue here.

True, although the whole usage of platform_allow in Bluetooth tests and samples is a mistake IMO. I'll work to remove it for Zephyr 4.0 - it should be enough to rely on the bluetooth tag instead. It's possible we might still need platform_allow in a limited set of cases, but it should be an exception to the rule then (today almost all samples and tests use it). The only place where specifying platforms explicitly makes sense to me is integration_platforms.

this appears in many boards and is more or less what used to be a good way to not build and test networking/bluetooth for boards that did not support any of those subsystems, it is not being consistently used and we need a better way, so this might help at one level if we find a solution here.

The problem is that it's used for boards which do not support these features, but also for boards which do support them, but can't be built in CI due to blob dependencies. We already have the tags, and they should really be all that's needed to match up boards with samples & tests for 99% of the cases.

a feature enabled by a board that requires binary blob and filtering on those is something new. I am still not sure how this will work in general and with twister in particular.
west blobs fetch .. should in this case flip some switch somewhere that tell us that we are able to build this feature and twister should look at this switch but then we are looking at mapping blobs to specific samples or tests, which will get very complicated...

Today, twister makes two implicit assumptions: that both west update and west blobs fetch has been previously run as appropriate to make it possible to build and run whatever twister is instructed to do. I say it makes these assumptions because it doesn't have any built-in functionality to try to ensure or check the dependencies itself.

When considering what twister should or shouldn't do, you mentioned (I think it was on Discord) that west update and west blobs fetch are analogous to each other. The former is for code and the latter for blobs. west update doesn't flip any switches or set any flags to indicate that modules are up to date, so my current thinking is that west blobs fetch shouldn't either. It could of course, but this would then be a departure from the existing design philosophy. My proposal would instead be to add a command line switch to twister to control its assumption about the blobs availability, which would also make the upstream CI policy explicit rather than how it's now indirectly enforced by ignore_tags and platform_allow.

Whichever of the two approaches we take, we would still need to encode information about which features have blob dependencies. This could be on a very granular level (e.g. specifying exact Kconfig options and which module's blobs they depend on), or on a more high level by having each board list which tags it supports but only if the relevant blobs have been fetched. The less granular approach would be simpler, although it would mean that fetching blobs can't be automated on a general level. Downstream trees would instead need to have specific knowledge of relevant boards and which blobs they may require - I think this is an acceptable compromise.

Anyway, in practice we'd go from the following in the board yaml:

supported:
  - bluetooth

to something like:

supported-with-blobs:
  - bluetooth

Upstream CI with twister would simply ignore the supported-with-blobs list. Does that seem reasonable?

[erwango]

When considering what twister should or shouldn't do, you mentioned (I think it was on Discord) that west update and west blobs fetch are analogous to each other. The former is for code and the latter for blobs. west update doesn't flip any switches or set any flags to indicate that modules are up to date, so my current thinking is that west blobs fetch shouldn't either. It could of course, but this would then be a departure from the existing design philosophy. My proposal would instead be to add a command line switch to twister to control its assumption about the blobs availability, which would also make the upstream CI policy explicit rather than how it's now indirectly enforced by ignore_tags and platform_allow..

Note that if we support west lazy modules loading one day, we'd have the same need for west update as well.

[henrikbrixandersen]

We support for samples/tests to depend on particular modules (see e.g. samples/modules/*. Perhaps we could have something similar for depending on blobs?

[jhedberg]

We support for samples/tests to depend on particular modules (see e.g. samples/modules/*. Perhaps we could have something similar for depending on blobs?

Isn't the use case slightly different though? What we'd need e.g. for Bluetooth is not samples or tests that always depend on blobs, rather they depend on them only when built for a specific subset of platforms. Or to frame it slightly differently (and more generally), here the issue is that the underlying implementation of a Zephyr feature (which already has plenty of existing samples and tests) depends on blobs if you try to build it for some subset of platforms that support the feature.

[nashif]

True, although the whole usage of platform_allow in Bluetooth tests and samples is a mistake IMO. I'll work to remove it for Zephyr 4.0 - it should be enough to rely on the bluetooth tag instead. It's possible we might still need platform_allow in a limited set of cases, but it should be an exception to the rule then (today almost all samples and tests use it). The only place where specifying platforms explicitly makes sense to me is integration_platforms.

Let's hold on this, a better strategy and design is needed.

Right now our testing is targeted at devices and platforms, i.e. we focus on running a test on as many platforms as possible instead of focusing on testing and targeting features, possible with the least number of targets to verify this feature. So we bend backwards to make some tests build and run on each platform without any value added or coverage added to the original goal of testing. In short, we should be validating Zephyr features and focus on those, it is not about platforms and validating their capabilities.

There is also ther other side of the problem, CI. Most of the filters we have mostly go back to CI run-time and scope optimization deprioritizing the need someone might have to run some tests on target hardware (this issue).

Today, twister makes two implicit assumptions: that both west update and west blobs fetch has been previously run as appropriate to make it possible to build and run whatever twister is instructed to do. I say it makes these assumptions because it doesn't have any built-in functionality to try to ensure or check the dependencies itself.

No, twister does not need west update at all. twister expects a workspace to be setup and ready, whether it needs west update or blob that is up to you. If you want to test some vendor boards that depend on a hal, then you need to call west update, if you are just using native_sim and just testing a built-in functionality, you probably do not need west update.

Twister expects many things to be setup an ready and does not try to do this for you. If you do not have a toolchain installed, it does not pull the toolchain for you.

When considering what twister should or shouldn't do, you mentioned (I think it was on Discord) that west update and west blobs fetch are analogous to each other.

As with many similar issues, we should try always to go back and solve the issue at the core, and not in twister, which should be completely agnostic to all of this and should never care about blobs or anything else we might add in the future.

IMO this could also be solved on the west/kconfig side of things, because the problem first appears when you try to build a sample with west/cmake for a board that needs some blob, for example for bluetooth. IMO this should be solved at this level.

Given that we have all the blobs defined in the module.yaml file and we parse those yaml files already and generate Kconfigs for modules, we could do the same for blobs and have the Kconfig enabling the feature (bluetooth) depend a blob kconfig and the existence of such blob in the tree (y if it was fetched, n if it was not). You can in this case also issue a warning to the user telling them that a certain feature requires blobs and so on...

now you get:

-- Generating done
-- Build files have been written to: /home/nashif/zephyrproject/zephyr/build
-- west build: building application
ninja: error: '/home/nashif/zephyrproject/modules/hal/silabs/zephyr/blobs/gecko/protocol/bluetooth/bgstack/ll/lib/libbluetooth_controller_efr32xg27_gcc_release.a', needed by 'zephyr/zephyr_pre0.elf', missing and no known rule to make it
FATAL ERROR: command exited with status 1: /home/nashif/bin/cmake-3.21.1-linux-x86_64/bin/cmake --build /home/nashif/zephyrproject/zephyr/build

If we do that, some of the current filters limiting overall testing can be removed and we can think about inclusion and coverage strategies without having to change twister to be aware of blobs.

[jhedberg]

Today, twister makes two implicit assumptions: that both west update and west blobs fetch has been previously run as appropriate to make it possible to build and run whatever twister is instructed to do. I say it makes these assumptions because it doesn't have any built-in functionality to try to ensure or check the dependencies itself.

No, twister does not need west updateat all. twister expects a workspace to be setup and ready, whether it needs west update or blob that is up to you. If you want to test some vendor boards that depend on a hal, then you need to callwest update, if you are just using native_sim and just testing a built-in functionality, you probably do not need west update.

Twister expects many things to be setup an ready and does not try to do this for you. If you do not have a toolchain installed, it does not pull the toolchain for you.

We're in agreement on this point then. What I meant by "as appropriate" was "whatever is necessary to fulfill dependencies"

IMO this could also be solved on the west/kconfig side of things, because the problem first appears when you try to build a sample with west/cmake for a board that needs some blob, for example for bluetooth. IMO this should be solved at this level.

Given that we have all the blobs defined in the module.yaml file and we parse those yaml files already and generate Kconfigs for modules, we could do the same for blobs and have the Kconfig enabling the feature (bluetooth) depend a blob kconfig and the existence of such blob in the tree (y if it was fetched, n if it was not). You can in this case also issue a warning to the user telling them that a certain feature requires blobs and so on...

Ok, I didn't realize that there was this kind of linkage from module.yml to Kconfig already. In that case that does indeed sound like promising way to solve the issue. E.g. the Silabs Bluetooth HCI driver Kconfig definition would become something like:

config BT_SILABS_HCI
	bool "Silicon Labs Bluetooth interface"
	default y
	depends on DT_HAS_SILABS_BT_HCI_ENABLED
	depends on ZEPHYR_HAL_SILABS_MODULE_BLOBS
	...

That solves controlling enabling of a feature that requires blobs, but it still leaves open the question of how to filter for these in twister's test plan generation. E.g. with the above dependencies you'd be able to successfully build Bluetooth apps for Silabs platforms even if the blobs are not present, but since the HCI driver wasn't actually built (even though its DT node might have status = "okay";) the bt_enable() would fail at runtime due to device_is_ready(hci_dev) returning false.

[nashif]

look at ./scripts/zephyr_module.py, it does have some blob handling already, i.e. when blobs are fetched and in the tree, it generates:

menu "hal_silabs (/home/nashif/zephyrproject/modules/hal/silabs)"
osource "/home/nashif/zephyrproject/modules/hal/silabs/zephyr/Kconfig"
config ZEPHYR_HAL_SILABS_MODULE
        bool
        default y
        select TAINT_BLOBS
endmenu

so this can be extended and improved a bit to be specific and generate a HAL specfic blob kconfig...

[jhedberg]

so this can be extended and improved a bit to be specific and generate a HAL specfic blob kconfig..

@nashif I've proceeded with doing this as part of #77386. @tejlmand also helped create a fix to make CI pass with it here: #77476

Unfortunately, this doesn't seem to get me any closer to a solution for the problem described here, so I'm back to looking at other ways.

we should be validating Zephyr features and focus on those, it is not about platforms and validating their capabilities.

I understand and agree, but right now it looks like we're clearly misusing platform_allow to accomplish this. The documentation for this option states the following:

"Set of platforms that this test scenario should only be run for. Do not use this option to limit testing or building in CI due to time or resource constraints, this option should only be used if the test or sample can only be run on the allowed platform and nothing else."

That's exactly the opposite of what we're using it for (at least in the case of Bluetooth tests & samples). It seems to me like for CI purposes we'd need another test configuration option that's solely used to define which minimal set of upstream platforms are sufficient to get coverage of the feature in question, i.e. it wouldn't impact in any way which platforms the test can theoretically be built for and run on.

There is also ther other side of the problem, CI. Most of the filters we have mostly go back to CI run-time and scope optimization deprioritizing the need someone might have to run some tests on target hardware (this issue).

For me it's not even about running the tests right now. For starters I'd just like to build them (with twister). I guess what I'll do is to bravely remove the ignore_tags from these platforms, and then hope for the best that the upstream sample & tests definitions are themselves good enough to keep these configurations out of CI.

[jhedberg]

It seems to me like for CI purposes we'd need another test configuration option that's solely used to define which minimal set of upstream platforms are sufficient to get coverage of the feature in question, i.e. it wouldn't impact in any way which platforms the test can theoretically be built for and run on.

@nashif what are your thoughts if we'd add such a new option for tests, which would then allow us to remove most Bluetooth uses of platform_allow. The new option could be called e.g. coverage_platforms.

[PerMac]

It seems to me like for CI purposes we'd need another test configuration option that's solely used to define which minimal set of upstream platforms are sufficient to get coverage of the feature in question

I think this is already what integration_platform is for. I looked in the docs and found that only the functionality of integration_platforms is described, but not idea behind them. I recall that multiple times when we were discussing this functionality almost identical definition was used, i.e. a minimal platform scope to verify if a given test/sample works.

I think adding integration_platforms to all bluetooth tests (and probably to all zephyr tests in general) can be a good idea. In our downstream we have those in all yamls. To me, as a CI maintainer, it is extremely valuable, since it is the responsibility of test/sample owners to add those to a yaml. I don't have to guess some generic platform scope in CLI call of twister and then seeing that 90% is filtered out due to individual test requirements. Instead, maintainers, i.e. people who know what the test/sample is doing and should verify can decide on which platforms it should be evaluated when integration mode is on.

If you want to remove ignore_tags we need other mechanism to limit the CI. Adding integration_platforms section to all yamls with tags that appear in ignore_tags could IMO do the trick. integration_platforms would be a list of platforms that don't have a given tag in their ignore_tags section. I think this will result in the same scope when running in the integration mode (-G/--integration). However, the CI is not always running in the integration mode. E.g. in a weekly build we try to verify everything on everything. I think ignore_tags presence descope such platforms from weekly build, although I don't know if this is consistent with "build everything wherever it can be built".

[jhedberg]

I looked in the docs and found that only the functionality of integration_platforms is described, but not idea behind them.

I'm not sure what part of the documentation you looked at, but platform_allow is pretty clearly defined in https://docs.zephyrproject.org/latest/develop/test/twister.html#tests:

Set of platforms that this test scenario should only be run for. Do not use this option to limit testing or building in CI due to time or resource constraints, this option should only be used if the test or sample can only be run on the allowed platform and nothing else.

The above makes it pretty clear (IMO) that platform_allow is currently being misused.

I think adding integration_platforms to all bluetooth tests

Sure, that might indeed make sense. One thing that is missing from most Bluetooth samples and tests that I looked at is depends_on: bluetooth. That seems like an obvious thing to limit building and running to only platforms that actually declare supported: bluetooth.

Regarding weekly tests, you said:

in a weekly build we try to verify everything on everything.

Do you mean that the weekly build even ignores platform_allow?

[PerMac]

I agree with platform_allow, was commenting that I don't see a proper explanation of integration_platforms :)

Do you mean that the weekly build even ignores platform_allow?

No, I mean that if you decide to use some less restrictive filtration than platform_allow or this ignore_tags workarounds the weekly CI scope will most likely increase. The question is: what is the point of weekly run? Should it aim on veryfing everything on everything (with respect to filters)? I believe this was the original intend, but is no longer feasible as weekly builds are already way too long (>24h).
But all the above is a bit off topic. We had a chat with Anas and we agree that we need to rethink zephyr's CI, ideally to be more targeted and less "fishing with a net and seeing what ended up in scope".

[nashif]

I agree with platform_allow, was commenting that I don't see a proper explanation of integration_platforms :)

It is there https://docs.zephyrproject.org/latest/develop/test/twister.html#tests

integration_platforms: <YML list of platforms/boards>
This option limits the scope to the listed platforms when twister is invoked with the --integration option. Use this insteadof platform_allow if the goal is to limit scope due to timing or resource constraints.