cmake: modules: west: Add function to verify blobs

[pdgendt]

Add a cmake helper function to verify if blobs have a valid checksum.

[nordicjm]

Not having used this feature, I'm not sure I understand the purpose of this, they are hosted on git repos right, so git deals with ensuring the files have been correctly checked out, why is this needed?

[pdgendt]

Not having used this feature, I'm not sure I understand the purpose of this, they are hosted on git repos right, so git deals with ensuring the files have been correctly checked out, why is this needed?

Not really, you define a list of files in zephyr/module.yml that a module can download using west blobs fetch. This is a rather simple command that will download the files if the sha256 checksum has a mismatch (but will not validate after downloading).
However, running a west build does not care about the checksum of those files, this PR adds a helper function that can do that for you.

[tejlmand]

Not blocking because of feature itself, but because west is an optional tool.

The ability to use west to fetch binary blobs is a great help / convenience to users, but users are also allowed to download such blobs manually (or using a custom tool).

Therefore we should not rely on west for verifying a blob's SHA, also current implementation proposal does not present how callers of this function will invoke it.

From: https://docs.zephyrproject.org/latest/develop/modules.html#binary-blobs

Binary blobs are fetched using west blobs. If west is not used, they must be downloaded and verified manually.

Another minor observation, it seems to me that PR will make it harder to locally develop a bin-blob and test it with Zephyr before releasing it the public, as the west_blobs_verify() will fail the build.

[pdgendt]

@tejlmand thanks for the review, see my comments below:

Not blocking because of feature itself, but because west is an optional tool.

The ability to use west to fetch binary blobs is a great help / convenience to users, but users are also allowed to download such blobs manually (or using a custom tool).

I agree, but users aren't forced to call this function, it CAN be used if west is known to be the build tool.

Therefore we should not rely on west for verifying a blob's SHA, also current implementation proposal does not present how callers of this function will invoke it.

I've added examples in the function description? It can be used in CMakelists.txt:

# SPDX-License-Identifier: Apache-2.0

cmake_minimum_required(VERSION 3.20.0)

find_package(Zephyr REQUIRED HINTS $ENV{ZEPHYR_BASE})
project(hello_world)

include(west)

west_blobs_verify()

target_sources(app PRIVATE src/main.c)

Another minor observation, it seems to me that PR will make it harder to locally develop a bin-blob and test it with Zephyr before releasing it the public, as the west_blobs_verify() will fail the build.

I don't see why this should be the case? A local file can either be omitted from the module.yml file entirely or the sha256 checksum can be calculated locally and updated in module.yml.

[pdgendt]

@tejlmand I've added a check if WEST wasn't set to WEST-NOTFOUND and print a warning.

[tejlmand]

I agree, but users aren't forced to call this function, it CAN be used if west is known to be the build tool.

which was exactly the reason for my question.
Because if called from a (downstream) sample's CMakeLists.txt file. then correct other assumptions can be made by sample author.

But someone could also call those functions elsewhere, as example from a Zephyr module and thus suddenly enforce west as a tool, which is mostly my concern.

A local file can either be omitted from the module.yml file entirely or the sha256 checksum can be calculated locally and updated in module.yml.

Of course users can, just like they can comment out logic inside west_blobs_verify() functions.
But should really be possible to just touch code in question, just like when modifying a source file.

[pdgendt]

I agree, but users aren't forced to call this function, it CAN be used if west is known to be the build tool.

which was exactly the reason for my question. Because if called from a (downstream) sample's CMakeLists.txt file. then correct other assumptions can be made by sample author.

But someone could also call those functions elsewhere, as example from a Zephyr module and thus suddenly enforce west as a tool, which is mostly my concern.

I see, is the latest addition, where WEST-NOTFOUND is checked, sufficient to cover this case?

A local file can either be omitted from the module.yml file entirely or the sha256 checksum can be calculated locally and updated in module.yml.

Of course users can, just like they can comment out logic inside west_blobs_verify() functions. But should really be possible to just touch code in question, just like when modifying a source file.

I don't really follow here, what would you like to see changed?

[tejlmand]

I don't really follow here, what would you like to see changed?

it was more a comment that users testing locally may just omit the file from module.yml or update its SHA.
I just tried to exemplify why such comment was really not useful to users working with the bin-blobs, as users can do anything (such as modify code) to get around build failures, but they shouldn't really have to resort to such kind of things.

Users should be able to replace a local blob with a version of their choice (for example for debug purposes) without doing extra modification to the workspace, just like users can make local modification to source files.

[tejlmand]

I see, is the latest addition, where WEST-NOTFOUND is checked, sufficient to cover this case?

Now that the general principle of west being optional, let me take a look at the code itself.

[tejlmand]

Thanks for this proposal.

I do see some positive in the proposal, but we should also be a little careful.
Just like users may choose to mix / use a SHA on a project which is different from the SHA in the west manifest file (without warning / errors), then users are also allowed to use a custom bin blob (older / newer).

Users are responsible for running west blobs, just like they must remember to run west update.

That said, I know how annoying it is to things not working due to wrong code, such as a wrong bin blob.
So i'm positive to the general idea, but has some comments.

Upstream Zephyr modules currently have 91 bin blobs defined.
The whole idea behind the blobs mechanism was that users should only fetch blobs from the vendors they need, thus having binary_blobs_verify() failing a build because one blob is missing is a no-go.
What can be done instead is to only check SHAs of the bin-blobs actually present, that is testing against M, instead of A.

Another use-case I see is the ability for a module to verify that a specific blob is present before continuing to the build stage, and for this we can adjust the call a bit, like this:

binary_blobs_verify(PATHS <file> [REQUIRED])

where the REQUIRED indicates that verify should fail if file is missing (Current behavior).

But if REQUIRED is omitted, then only bin blobs present are verified.

This will allow a module's CMake code to do:

binary_blobs_verify(PATHS img/bluetooth/firmware/cy_ble_stack_controller.a REQUIRED)

before:

zephyr/modules/hal_infineon/bless/CMakeLists.txt

Lines 20 to 23 in 4204ca9

	zephyr_link_libraries(
	${bless_blobs_dir}/COMPONENT_BLESS_CONTROLLER/cy_ble_stack_controller.a
	${bless_blobs_dir}/COMPONENT_BLESS_CONTROLLER/cy_ble_stack_manager.a
	)

[pdgendt]

Thanks @tejlmand for the suggestions, I've updated the PR.

The current implementation allows you to specify paths for binary blobs, but if omitted simply verifies all the blobs. The MODULE argument can limit that to a single module.

[pdgendt]

Marking as DNM until I have been able to test the module name.

[pdgendt]

@tejlmand Removed the DNM label as it seems to work with module contexts.

One limitation is that zephyr_verify_blobs currently can't be called from an application's cmake context.

[tejlmand]

Much better.

Generally still not fond of the MODULE being able to break a build when files unrelated to current build is not present or having wrong SHA.

Thus i'm much more in favor of a check like:

    zephyr_blob_verify(FILES gecko/platform/radio/rail_lib/autogen/librail_release/librail_efr32xg${GECKO_SERIES_NUMBER}_gcc_release.a)
    add_prebuilt_library(librail      gecko/platform/radio/rail_lib/autogen/librail_release/librail_efr32xg${GECKO_SERIES_NUMBER}_gcc_release.a)

Ref: https://github.com/zephyrproject-rtos/hal_silabs/blob/11ab59175a5ded618680a7692dbaae8f4fbd6325/gecko/CMakeLists.txt#L92C1-L92C145

over:

    zephyr_blob_verify(MODULE silabs)
    add_prebuilt_library(librail      gecko/platform/radio/rail_lib/autogen/librail_release/librail_efr32xg${GECKO_SERIES_NUMBER}_gcc_release.a)

But as the most specific call available for fetching blobs is west blobs fetch <MODULE>, then it is accepted to support a zephyr_blobs_verify(MODULE <module>) and let the caller decide between the two options.

[pdgendt]

@tejlmand thanks for the effort, I've updated the PR

As per the suggestions I feel that the MODULE argument should be required, it is now possible to pass ${ZEPHYR_CURRENT_MODULE_NAME} anyway.

[pdgendt]

I would also like to add a test case, but I'm not sure where to add this.

[fabiobaltieri]

Hi, the CI failure has been fixed upstream, if you rebase it should pass.

[tejlmand]

thanks for the effort and patience 👍

[pdgendt]

@carlescufi you mind taking a look at this PR, as you've introduced the blobs infrastructure? Thanks