west: sign/rimage: use rimage config to find SOF directory

[dcpleung]

After removal of SOF from manifest, if rimage is in path, the build would fail because the gen-uuid-reg.py cannot be found anymore. To fix this, use the rimage config path to find the SOF directory since the rimage config path is a directory under SOF. This would allow rimage to again sign images to run on hardware.

[marc-hb]

This looks like a minor hack but still a hack: why is sof.git not in any manifest?

You can combine SOF with any moving Zephyr version of your choice using sof/west.yml + a "submanifest". Example:

thesofproject/sof@f777417

This was not theoretical, this submanifest example was used "for real" in CI for a long time.

I regularly get lost in west "import" flexibility, yet none is suitable here? Just wondering about the use case. I suspect using an "out-of-manifest" Zephyr module causes other complications beyond this one?

[marc-hb]

Since 1a780f9 this is dead code, no point keeping it.

EDIT: I mean dead code unless sof/west.yml downloads... SOF recursively :-D

EDIT2: correction: or, someone combines zephyr/west.yml with some submanifest that downloads SOF. Looks like I got lost again in west infinite possibilities...

[marc-hb]

I just remembered there is already west config rimage.path ... and also west sign --tool-path ....

Considering rimage has been merged back into sof a long time ago, maybe sof_src_dir could be automagically inferred from one of these when not found in the manifest, avoiding one extra configuration item.

If one new configuration item can really not be avoided, then west config ... is more flexible, IMHO more convenient and a new environment variable and more consistent with where rimage configuration already is.

My 2 cents.

[dcpleung]

Modified to use the rimage config path to find the SOF directory.

[marc-hb]

No issue with cmake_cache.get('RIMAGE_CONFIG_PATH'), but the logic still does not feel right. For a very long time this code has been:

try:
  to find SOF project in [zephyr/]west.yml
  ...
except: # not found, so assume that the manifest is sof/west.yml      
   ...

Now, SOF has been removed from zephyr/west.yml. But instead of changing the try clause, this PR changes the except clause?!

After this PR, the code looks like:

try:
  to find something that will never be found! # changed location, unchanged code
  ...
except:
  do something that might work in both zephyr/west.yml and sof/west.yml cases?

This PR might work because your new except clause might work in both cases. But it's weird and confusing now.

BTW don't attempt to preserve the try: except: at all costs. An if/then/else might be enough now.

[marc-hb]

Obviously: please test this PR in both zephyr/west.yml and sof/west.yml cases.

[dcpleung]

Change the discovery logic a bit to cover possible scenarios.

[dcpleung]

Twister does not take west config variable so we still need a way to specific where SOF is, which is via environment variable SOF_SRC_DIR.

[marc-hb]

Wow, this increases the number of ways to find the SOF git clone to four now! But we have only two different, known situations to deal with: zephyr/west.yml and sof/west.yml? Four different solutions for two situations?

This rimage area has alway been difficult to maintain because of the too many different combinations and possibilities; merely reading the (already too long) sign() method is enough to get the corresponding headache: can't touch anything because "you never know". Ideally, that sign method should support fewer variations, not more. In the meantime, each way to find the SOF git clone should at least tell in a source comment what specific situation it is supposed to deal with.

BTW do you know how having rimage in the path affects #99497 (comment) ?

Twister does not take west config variable so we still need a way to specific where SOF is, which is via environment variable SOF_SRC_DIR.

I don't get this: why does twister even need to know about west config? Both the environment variables and the west config approaches have the same advantage of "cutting through" the layer of indirections without them even knowing about it.
It's not like sof_src_dir is something dynamic that depends on the platform or test case.

[marc-hb]

SonarCloud is not far off: this entire versions_json_file.exists() test is repeated 3 times. This is calling for some new test function.

Speaking of functions, the length of this PR (assuming it is justified) is now calling for some new def get_sof_dir() function too that get just return as soon as it finds something.

[dcpleung]

The main reason for needing to find the SOF directory is simply the need to run the UUID generation script.

1. Having the SOF pulled in via sub-manifest has been working fine before we remove SOF from the the manifest. We are still doing this in our internal CI for testing so this method needs to stay.
2. The SOF main repo, on the other hand, is pulling Zephyr as a project. So we need to use the root manifest path to find the SOF tree.
3. Using west config to specify RIMAGE_IMAGE_PATH is mentioned in the documentation. So we have to keep this method to avoid breaking someone's workflow.
4. However, using west config to specify the config path does not work with twister. That config is never applied during twister builds. So we need another way to specify where to find SOF tree to enable running twister to build and test on hardware.

Whether rimage is in the path or specified via west config rimage.path, the results are the same: unable to run the UUID generation script, and thus failing the signing process.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud