samples: wifi: Add ESP32 to twister

[krish2718]

This helps us catch build issues like we saw in #59881.

[krish2718]

I guess the sample.yaml for the CI is still picked from main instead of from PR, else this should have failed.

[sylvioalves]

@krish2718 Adding esp32 in there will trigger sample build? If yes I wonder how that would work as esp32 requires binary blobs which are not available in repository by default. (I see that CI passed though..)

[krish2718]

@krish2718 Adding esp32 in there will trigger sample build? If yes I wonder how that would work as esp32 requires binary blobs which are not available in repository by default. (I see that CI passed though..)

CI passed because of the reason in my above comment. Doesn't CI do west blobs fetch hal_espressif ? If not we need to add that.

[sylvioalves]

@krish2718 Adding esp32 in there will trigger sample build? If yes I wonder how that would work as esp32 requires binary blobs which are not available in repository by default. (I see that CI passed though..)

CI passed because of the reason in my above comment. Doesn't CI do west blobs fetch hal_espressif ? If not we need to add that.

I have talked about that in the past and got a very dirty reply. CI should not test binary blobs at all. In case you want to follow this up, better to ask TSC recommendation.
We have been using our internal CI to test Wi-Fi/BLE related tests/samples that requires binary blobs, that's the reason we caught the #59881.

[krish2718]

@krish2718 Adding esp32 in there will trigger sample build? If yes I wonder how that would work as esp32 requires binary blobs which are not available in repository by default. (I see that CI passed though..)

CI passed because of the reason in my above comment. Doesn't CI do west blobs fetch hal_espressif ? If not we need to add that.

I have talked about that in the past and got a very dirty reply. CI should not test binary blobs at all. In case you want to follow this up, better to ask TSC recommendation.

Oh, I was in processing adding west blobs fetch to .github/workflows/twister.yaml :), lets see if anyone has objections.

[sylvioalves]

@krish2718 Adding esp32 in there will trigger sample build? If yes I wonder how that would work as esp32 requires binary blobs which are not available in repository by default. (I see that CI passed though..)

CI passed because of the reason in my above comment. Doesn't CI do west blobs fetch hal_espressif ? If not we need to add that.

I have talked about that in the past and got a very dirty reply. CI should not test binary blobs at all. In case you want to follow this up, better to ask TSC recommendation.

Oh, I was in processing adding west blobs fetch to .github/workflows/twister.yaml :), lets see if anyone has objections.

That would be really good, but I don't think it will be accepted. Please, tag me in there if you can so I can follow up.

[carlescufi]

Added the TSC label because, although technically this is not currently allowed, I see no reason no to introduce it.

[henrikbrixandersen]

By having west blobs fetch in CI like this we lose the ability to check that basic board configurations build without blobs?

[nashif]

This violates the guidelines about pulling binary blobs in zephyr mainline CI.
@carlescufi to add a link to the google doc documenting this and documentation to be updated with exact guideline.

[carlescufi]

This violates the guidelines about pulling binary blobs in zephyr mainline CI. @carlescufi to add a link to the google doc documenting this and documentation to be updated with exact guideline.

https://docs.google.com/document/d/1heqcv7dzGvM5rA9xpTMW3kyKJLpZsl2Gjsmr5eqBje8/edit#heading=h.a4tmntn7vwmi

[krish2718]

This violates the guidelines about pulling binary blobs in zephyr mainline CI.
@carlescufi to add a link to the google doc documenting this and documentation to be updated with exact guideline.

Thanks for the background and the doc it is helpful.

From the doc:

The question of whether to recommend this to the board must be decided by the TSC.

So, is this already done and the idea was rejected or is it still pending?

By having west blobs fetch in CI like this we lose the ability to check that basic board configurations build without blobs?

Drivers need these blobs to do even a basic build (at least for ESP), so, the question is how do we make sure changes in Zephyr do not break the driver (this happened with ESP32 recently and is the main motivation for this PR). I tried to limit west blobs fetch to only when its needed, but couldn't find a right place in CI flow to do that.

[sylvioalves]

@krish2718, those blobs are only needed when building BT or Wi-Fi samples.. other samples do not. Currently we have been using an internal CI to check PRs that breaks (as you mentioned).
One solution I could imagine considering blobs as is (not open source), would be creating some kind of mocking in hal_espressif. If CI is running, use mock functions to allow APIs and driver testing. I am not sure whether this is valid though.

[krish2718]

One solution I could imagine considering blobs as is (not open source), would be creating some kind of mocking in hal_espressif. If CI is running, use mock functions to allow APIs and driver testing. I am not sure whether this is valid though.

May be we should add a Ztest testsuite in tests/drivers to ensure basic functionality, can leverage the mocking abilities?

[carlescufi]

@krish2718 @sylvioalves this was discussed in yesterday's TSC. I will send a PR soon to update the documentation of binary blobs, specifying explicitly that you cannot use them in Zephyr's upstream CI. So please do find a workaround for this, we cannot and will not west fetch blobs in CI.

[krish2718]

Closing this PR based on above discussion, we can work on developing a Ztest for ESP separately.