-
Notifications
You must be signed in to change notification settings - Fork 7.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
tests: mcuboot: add "depends_on: mcuboot" #79924
tests: mcuboot: add "depends_on: mcuboot" #79924
Conversation
butok
commented
Oct 16, 2024
- Adds "depends_on: mcuboot" to the test_mcuboot testcase.yaml.
- Adds "supported: mcuboot" to the board yaml files, found in the testcase.yaml platform_allow list.
- Avoid appending of the platform_allow list for new platforms.
- Adds "depends_on: mcuboot" to the test_mcuboot testcase.yaml. - Adds "supported: mcuboot" to the board yaml files, found in the testcase.yaml platform_allow list. - Avoid appending of the platform_allow list for new platforms. Signed-off-by: Andrej Butok <andrey.butok@nxp.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No let's not add a tag that has nothing to do with the boards
Hi @nordicjm
Nothing wrong. This the supported board feature. This is the way how avoid growing platform_allow list for mcuboot specific samples/tests that was blocked by @nashif here #78944 So @nashif and @nordicjm please provide your solution and unblock any of PRs. |
He said there that you are adding a lot of boards to a sysbuild test, which adds a strain to the CI because it then has to build all of them. You're doing the same thing here, so I'm not sure why you think if that got rejected that this would be accepted? |
Yes, this is the goal:
|
If I understand @nashif's block on #78944, the issue there is that we are attempting to use a sample as a test, right? For this PR though, the goal is to expand the test coverage of the MCUBoot test. Personally I would love if we had a better solution than adding I'm unclear what the better solution here would be. The goal of the mcuboot test is to provide a contained testcase that verifies the platform can boot an image with MCUBoot, and run an image upgrade. Why would we want to limit the platform coverage for that testcase? |
I agree this will help a lot. and simply the tracking problem. |
I find supported/depends_on a better solution than platform_allow. It has a better scaling - 1 entry in platform yaml vs adding platform to every allow list. |
Thank you. The proposed approach is clean, |
For what it's worth, I also don't understand what's the issue with increasing coverage of the sample, even if it's not used for testing, what is wrong with making sure the sample works on all those platforms? It's pretty important actually for vendors since users will go to the sample to see how to use something and try out the board. And if there is some uncaught issue related to the sample only for some platform, that we don't know about, because somebody blocked adding coverage of more platforms to that sample over some philosophy and an aesthetic distaste for the new content in the yaml file, that's a problem. |
Hi @nordicjm |
just because someone made a mistake and added something in a board yaml file, does not make it right and does not make it a rule. tinycrypt does not make any sense in a board as a supported feature for example, same for NVS probably. need to clean this.
Isn't like evey other board in zephyr potentially bootable with mcuboot? Are we going to have majority of boards adding this? I welcome the removal of the long |
Thank you @nashif for your feedback, looking forward for your final decision. @hakehuang, as the chair of the Zephyr Testing Working Group, could you also give your feedback:
|
Hi @nashif
|
The core issue here (as far as I understand) is that twister's filtering step attempts to run the CMake phase of the build, to generate a One option I believe we considered in the past was allowing twister to filter based on a "base" build, without overlays. This introduces a new set of issues though, because some tests do need board overlays pulled in to satisfy the filter settings. |
Why does filtering on slot0 and slot1 partitions existing not work or has this not been tried? E.g.
|
Twister runs the cmake configuration phase in order to run filtering, right? If we run the cmake configuration phase for mcuboot on a board that does not define flash partitions in devicetree, it will fail. For example: |
That is not a test in zephyr and that's not the test being changed here, I made this changes:
And ran it:
So excluding that platform, seems like this works? Although 41 boards seems like a low number |
Using the |
Thanks for looking at this more- I took another look here, and it looks like we can indeed use the filter. What I was missing is that we now default to using "filter stages" for the CMake filtering stage when possible- filter stages will not run sysbuild. The issue I expected to find was that when we ran sysbuild and tried to generate a DTS for MCUBoot, the CMake filtering stage would fail. However, since we use filter stages this is not the case. Given that, I think we should make a change like so: diff --git a/tests/boot/test_mcuboot/testcase.yaml b/tests/boot/test_mcuboot/testcase.yaml
index 3de1f70426a..fd6a61c21ce 100644
--- a/tests/boot/test_mcuboot/testcase.yaml
+++ b/tests/boot/test_mcuboot/testcase.yaml
@@ -11,46 +11,6 @@ common:
- "Swapped application booted on (.*)"
tests:
bootloader.mcuboot:
+ sysbuild: true
tags: mcuboot
- platform_allow:
- - frdm_k22f
- - frdm_k64f
- - frdm_k82f
- - frdm_ke17z
- - frdm_ke17z512
- - rddrone_fmuk66
- - twr_ke18f
- - twr_kv58f220m
- - frdm_mcxn947/mcxn947/cpu0
- - lpcxpresso55s06
- - lpcxpresso55s16
- - lpcxpresso55s28
- - lpcxpresso55s36
- - lpcxpresso55s69/lpc55s69/cpu0
- - mimxrt1010_evk
- - mimxrt1015_evk
- - mimxrt1020_evk
- - mimxrt1024_evk
- - mimxrt1040_evk
- - mimxrt1050_evk
- - mimxrt1060_evk
- - mimxrt1062_fmurt6
- - mimxrt1064_evk
- - mimxrt1160_evk/mimxrt1166/cm7
- - mimxrt1170_evk/mimxrt1176/cm7
- - vmu_rt1170/mimxrt1176/cm7
- - mimxrt595_evk/mimxrt595s/cm33
- - mimxrt685_evk/mimxrt685s/cm33
- - nrf52840dk/nrf52840
- - rd_rw612_bga
- - nucleo_wba55cg
- - frdm_mcxw71
- integration_platforms:
- - frdm_k64f
- - nrf52840dk/nrf52840
- bootloader.mcuboot.assert:
- tags: mcuboot
- platform_allow:
- - b_u585i_iot02a
- extra_configs:
- - CONFIG_ASSERT=y
+ filter: dt_nodelabel_enabled("slot0_partition") and dt_nodelabel_enabled("slot1_partition") and dt_nodelabel_enabled("boot_partition")
I mean what is the alternative here? We want to build (and execute, if hardware is available) MCUBoot on the platforms that support it. It is a high CI load, but I don't really see another way to validate this. Maybe we only test one platform per flash driver? Not sure how else to narrow things down. |
Would be up to @stephanosio and @nashif if having this run 851 tests is something that is wanted (to put it into perspective, when CI runs for a PR and the tests are split up and ran on node, this is equivalent to having over 1 more complete sub-job). The CI for this PR for example, https://github.com/zephyrproject-rtos/zephyr/actions/runs/11365562284/job/31621002358 shows 736 jobs for that and 7 sub-jobs, so this would add more builds to CI that one full sub-job there has |
Twister .yaml should allow building/running tests/samples on all platforms that support those tests/samples. Proposal: |
Samples should not be treated as tests.
It is actually pretty clear: "Although being able to run a sample successfully does verify the functionality is working as expected, this should not replace dedicated and comprehensive tests with full coverage to be submitted into the tests/ folder. In a sample, the only thing you test is buildability and, in some cases, if a sample is performing as expected, i.e. you are testing the sample, not the subsystem it builds on top." "The tests should verify the default configuration of the sample and any optional features documented in the sample’s README file. Sample should never be used to test functionality of the subsystem or module the sample belongs to." "Use platform_allow to limit test to the platforms the sample was actually verified on. Those platforms should be listed in the sample’s README file." https://docs.zephyrproject.org/latest/samples/sample_definition_and_criteria.html#sample-criteria |
Hi @kartben OK, lets find a compromise. We can rephrase it as Twister .yaml should allow building/running tests on all platforms that support those tests. Also, this PR adds "depend on" to the test (not sample). |
Ping @nashif |
Hi @nashif could you clarify a board.yaml usage. According to https://docs.zephyrproject.org/latest/build/dts/design.html#example-remaining-work : Does it mean that we must avoid "depends_on" and use DTS "filter" instead? => Close this PR? |
I like this approach. I find the "platforms_allowed" to be a sledge hammer approach when we have the ability to basically have flags to drive things. For the overloaded CI, that is what the integration_platforms is for. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
using filter is the right(*) approach, supported
in the board yaml file is reserved for HW features and should be kept this way.
So one of the changes suggested by @nordicjm or @danieldegrasse could be applied.
(*) It is not optimal yet, but this how are things are right now, how twister selects platforms in CI and when/how filters are evaluated etc is subject to change and we are looking at different options.
No common conclusion. Closing. |
@butok Still open. Is that on purpose ? |