[SKIP CI] Use west manifest in SOF

[aborisovich]

Implemented west tool along with its manifest to SOF

To use it please install: pip3 install --user -U west
For new build guides head to updated SOF project documentation Build with Zephyr.

Features

• You may now clone SOF project without git using west init -m <SOF_URL> command (well... after this PR is merged).
• If you already cloned SOF using git, run command west init -l <SOF_DIR> to use west
• You may also use xtensa-build-zephyr.py convenience script that will initialize west for you
• Executing command west update will cause to download SOF dependencies (previously submodules) and zephyr along with dependencies required for SOF platforms building and testing (except modules/audio/sof - we don't need another SOF copy!)
• If you are not using ./sof/scripts/xtensa-build-zephyr.py when building with Zephyr, you need to manually create a symlink for Zephyr:
  ln -s ./modules/audio/sof ./sof (run in ~/workspace/my_sof_workspace see directory structure example below)

Changes to GIT

• Submodules rimage and tomlc99 are now managed by west and were removed from git submodules
• rimage directory added to .gitignore

Changes to directory structure of SOF & Zephyr

• Due to west manifest specification we could not maintain existing directory layout where Zephyr is placed in SOF/zephyrproject directory. West manifest requires to have a .west workspace initialized one folder "up" in relation to west.yml manifest. New layout looks like the following (SOF placed side-by-side to Zephyr and its modules):
  

Changes to xtensa-build-zephyr.py

• "Clone-mode" (-c flag) had been removed and replaced by new "-u, --update" flag that clones zephyr with required dependencies using west manifest. It differs from old "clone mode" by the fact that if run it will not fail when components are already cloned but will update their revisions to those matching manifest. If you have active changes on the any component, revision will not be updated (but if you have a custom branch with no changes it will be changed to manifest revision).
• "Point-mode" had been completely removed as it is no longer possible to maintain this mode when west manifest expects (and defines!) directory where Zephyr project is located.
• "-z flag" had been removed as you can now set your custom Zephyr revision in the west.yml file.

Signed-off-by: Andrey Borisovich andrey.borisovich@intel.com

[lgirdwood]

@nashif @aborisovich I assume this is complementary to the SOF app PR from @nashif ?

[lgirdwood]

I think we also have something in the python/bash build scripts that check/report on submodules. This would need updated too.
Fwiw, the python/bash scripts should probably also print out that they are being deprecated for Intel/NXP and west should be used instead as part of this PR.

[aborisovich]

Right now it downloads *all Zephyr modules. Maybe it is a bit of an overkill (download takes ~10 min on my machine).
I will try to narrow down dependencies we need for building Intel ADSP.
I initially added all modules because Zephyr when you publish PR runs a ton of twister tests - for every CAVS/ACE board it builds ~10 zephyr applications. I do not know them all and do not know what modules/dependencies they have.
I think it is useful and mandatory to be able to run twister manually before publishing a PR
(it is done with command ./zephyr/scripts/twister -p <board_name>)

[nashif]

instead of blacklisting, you should be whitelisting I guess and only add the things you need. Start with only those:

      - hal_xtensa
      - mbedtls
      - mipi-sys-t

And then try and see if there are any other hidden deps you might want to add.

[nashif]

      import:
        name-whitelist:
          - mainline-app
          - lib2

[aborisovich]

Push update:

• Changed approach to cloning Zephyr as @nashif suggested - now we are not cloning all Zephyr dependencies except sof as module but clone only selected modules which are required to build Intel ADSP platforms and run twister scripts on them.
• Added pristine flag to west build command

[aborisovich]

accidental autoadd, will remove

[marc-hb]

@nashif @aborisovich I assume this is complementary to the SOF app PR from @nashif ?

I think they should be independent from each other. I recommend we adjust all CI(s) for the other one first, it's much less disruptive.

[marc-hb]

You don't need to delete find_west_workspace() yet. Keep it for now and make this disruptive PR smaller and easier to review.

Of course you need to remove the --clone feature but I don't see why you need to remove the --west-path option, it should not matter here? Again this should make this disruptive PR smaller hence easier to review, test and troubleshoot (and less disruptive for CI)

Removed "zephyr revision" or "-z" argument as it is no longer
needed and did not have actual implementation.

That's not correct, who would have added this option without testing it? I think we use this in CI now. I'm not saying this should be kept; I'm only saying the commit message is wrong.

Removed rimage submodule from git, added it to .gitignore.

Again you probably don't need to do this yet, a transition period is likely possible. There are still git submodule commands in CMakeLists.txt. It's bad and they should have been removed earlier anyway but for the moment they are still here so it's probably easier to have a transition period. The simpler is probably for west to download rimage in a different directory and update CMake to use either.

Removing the rimage submodule is also breaking non-Zephyr builds which are still being tested in CI.

[marc-hb]

I don't see the value of adding a bare layer of indirection on a mere west update. The purpose of this PR is to increase the direct usage of west and reduce the usage of this script.

[aborisovich]

The purpose of this PR is to increase the direct usage of west and reduce the usage of this script.

That is a very bold statement. I do not agree with it.
Usage of west is still more cumbersome that usage of this script.
Even when we move all platforms to Zephyr XTOS and integrate @nashif s PR6004 , the set of commands required to build a single platform (from scratch) is:

1. west update
2. west build build --build-dir build-<platform> -p always -b <board_name> sof/app
3. cmake -B ./build-rimage -S ./drivers.audio.firmware.converged/rimage
4. cmake --build ./build-rimage
5. west sign --build-dir build-mtl -t rimage --tool-path ./build-rimage/rimage --tool-data ./sof/rimage/config -- -k ./sof/keys/some_key.pem
6. ./build-mtl/zephyr/smex_ep/build/smex -l ./build-mtl/zephyr/zephyr.ldc ./build-mtl/zephyr/zephyr.elf

This is much longer than:
./scripts/xtensa-build-zephyr.py -c <platform>
That does this all for you. Maybe someday we will integrate it better so there are less commands to call but we are not there yet.

[marc-hb]

Good points, this script will still be used a lot.

OK, now I see this function is used as part of west init AND west update AND symlink, makes sense to me now.

[marc-hb]

Unresolving to keep @aborisovich's good summary visible.

At least two more things the script do:

• building sof-logger
• iterating on multiple platforms and "installing" all the files in the correct subdirectories:

build-sof-staging
├── tools
│   ├── cavstool.py
│   ├── cavstool_client.py
│   ├── cavstwist.sh
│   └── sof-logger
└── sof
    ├── sof-cnl.ldc
    ├── community
    │   ├── sof-apl.ri
    │   └── sof-cnl.ri
    └── sof-apl.ldc

[marc-hb]

Suggested change

	def west_update_submodules():
	def west_update_sof_rimage():

They're not "submodules" anymore here.

[marc-hb]

I'm not convinced of the value of updating rimage only. Why not update hal_xtensa and all other SOF dependencies at the same time?

[aborisovich]

Marc I think you misunderstood the content.
It does not update rimage only. It updates also tomlc99.
It is also going to update alsa-lib but I need to add it as @lgirdwood suggested.
The idea behind splitting two modes:
"init_mode" and "update_mode" is that there are platforms in SOF that do not use Zephyr.
When building those platforms, you do not need to wait that ~3 min to download Zephyr and its dependencies.
So the "init_mode" is supposed to clone only SOF dependencies: rimage, tomlc99 and alsa-lib.
Then "update_mode" does the same as "init_mode" but also it updates Zephyr dependencies.

[aborisovich]

I think my understanding on what this script does was also a bit wrong.
After all it is called xtensa-build_zephyr.py and should be used to build with zephyr only right?
I should not care for non-zephyr platforms/builds in this particular script.
I think I will remove "init_mode" completely.

[miRoox]

It seems to break our XTOS build. Will we completely move to Zephyr soon?

[marc-hb]

It seems to break our XTOS build.

As mentioned above I don't think it has too. This PR can add as much while removing less.

Will we completely move to Zephyr soon?

"Soon" yes but "now" is a bit too early still.

[marc-hb]

Please try to use a different location. Removing submodules and replacing them by something else in the same directory makes switching branches super painful, breaks git bisect etc.

[aborisovich]

I'm looking into it. However now I restored rimage as git submodule and it should be ok.
If west clones to very same directory where submodule is expected, clones very same revision of repo then git during command:
git submodule update --init --recursive
gives you only information of registering:

[aborisovich]

Ok I see a potencial bad scenario:

1. west update clones rimage and tomlc99 but does not registers them as submodules on branch A
2. end-user does not run git submodule update --init --recursive (because why would he/she)?
3. End-user checks out to branch B that have submodules registered to other SHA than branch A.
4. End-user runs platform build that fails with eg. image sining
5. End-user runs git status that does not show any changes because submodules had not been registered at all (and they are actually there cloned by west and checked out to wrong revision).

However:

1. Note, that that's how west revision control works. It does not show git submodules change because its not git.
   The fact that we do not remove git submodules is temporary because we can't do it easily.
2. I don't think that cloning "submodules" using west update to let's say new rimage_west directory is beneficial. Yes you would avoid confusion described in bad scenario above but the cost of redoing scripts, headers and adding tons of logic everywhere "it not rimage dir use rimage_west dir". Seems not worth it to me.

[marc-hb]

Thanks for the testing and analysis. I agree the issue you described is acceptable. If someone uses west, they should use west all the way and then they should ask west to manage everything. No back and forth between west and submodules, that's simply not supported.

[aborisovich]

You don't need to delete find_west_workspace() yet. Keep it for now and make this disruptive PR smaller and easier to review.

Yes you are right, especially that I still use "content" of this function in "west_init" function (execute_command(["west", "topdir"]).
I'll restore it with small modifications.

Of course you need to remove the --clone feature but I don't see why you need to remove the --west-path option, it should not matter here? Again this should make this disruptive PR smaller hence easier to review, test and troubleshoot (and less disruptive for CI)

--west-path option should be removed because:

1. sof/west.yml file defines hardcoded paths on where the zephyr project is located
   (see repo-path: zephyr).
2. There are plans to remove SOF from Zephyr manifest completely and it will no longer be downloaded to /modules/audio/sof as soon as SOF will be ready for it. SOF application is aready being moved to this repo.
3. This one is my personal opinion but I am really happy that now will be only "1 way" to build a project. This is not a restaurant where you order meals you wish. There should be only one proper way of building to reduce number of issues people face with different combinations and reduce complexity of scripts.

Removed "zephyr revision" or "-z" argument as it is no longer
needed and did not have actual implementation.

That's not correct, who would have added this option without testing it? I think we use this in CI now. I'm not saying this should be kept; I'm only saying the commit message is wrong.

I might need your help with this one. I need to figure out where it is used and provide appropriate resolution.
However it seems to me that is should not be used anywhere - I think it was added just for end-users convenience.
Now they may simply modify revision in west.yml in their PRs.

Removed rimage submodule from git, added it to .gitignore.

Again you probably don't need to do this yet, a transition period is likely possible. There are still git submodule commands in CMakeLists.txt. It's bad and they should have been removed earlier anyway but for the moment they are still here so it's probably easier to have a transition period. The simpler is probably for west to download rimage in a different directory and update CMake to use either.

Removing the rimage submodule is also breaking non-Zephyr builds which are still being tested in CI.

Lets get rid of "transition periods" and remove this all in single PR.
We do have experience with this solution - it turns out that people doing commits every time forget either to update git submodule while the update manifest SHA or they update git submodule SHA and forget to update manifest content.
Let's not do this. I'll try to will find all Cmake usages of submodules and provide alternatives to it.
I will also add west handling to non-zephyr platforms that checks out only SOF dependencies (actual content of "init_mode" in xtensa-build-zephyr.py script).

[aborisovich]

@lgirdwood I had checked content of scripts/docker_build/sof_builder/Dockerfile that clones alsa-lib and alsa-utils.
I would restrain from adding those steps in this PR because those libraries are:

• not only cloned by also build using make
• cloned to directory is outside of sof & Zephyr workspace - it would mess with users local installations

Handling of alsa dependency should be done in separate PR and it involves some considerate amount of work.
We should probably place those libs somewhere in sof workspace tree and add logic building them to CMake.

[marc-hb]

This one is my personal opinion but I am really happy that now will be only "1 way" to build a project. This is not a restaurant where you order meals you wish. There should be only one proper way of building to reduce number of issues people face with different combinations and reduce complexity of scripts.

Except we already support multiple ways. The current script has already and successfully been supporting multiple directory structures. Was this additional complexity and confusion that should have been best avoided? Maybe yes but we are already there, so adding a new option does not make things worse.

The magic that make different directory structures possible in the same git version is west. west is designed to support many different organizations and automatically adapt to them and it has already been adapting to that in this project. west can build upstream, downstream and any other random directory structure, it's designed and being used every day for that.

So it would be really sad not to try to leverage west's power and flexibility for a transition period. Don't forget we have not just one with 3 different CIs (with very different kind of tests): quickbuild, sof-jenkins and Github Actions. Making big changes to all 3 CIs at the same time will cause much more disruption, synchronization and downtime. The fewer CI changes are required and the less it is broken and the faster this PR can get merged.

[aborisovich]

Push update:

changes to xtensa-build-zephyr.py

• restored find_west_workspace() function as @marc-hb suggested
• removed "init_mode" as it is not needed in a script that does building with Zephyr
• removed needless changes to whitespaces
• added new "-p" flag (sorry for collision with previous -p that was deleted, just want to keep flags alighed with west)
  that performs pristine build (removes cache)
• restore -i flag for IPC

GIT:
Restored rimage submodule as it is too much work to refactor completely scripts/cmake/git-submodules.cmake and implement west for non-zephyr platforms.
Reviewers and contributors should now be careful to have always matching version of git submodule to west manifest!

Other changes:
Removed ./scripts/xtensa-build-zephyr.sh script as it will no longer work and is obsolete now.

[lgirdwood]

@wszypelt can you check CI, looks like it may have stalled. Thanks !

[wszypelt]

@lgirdwood
The problem occurs in all PR, I try to solve it, unfortunately a normal restart will not work, it takes some time, I will let you know as soon as I fix the problem

[aborisovich]

How can I add additional directory in the container that CI uses?
Rignt now there is some SOF_TOP in the container and I actually create .west and clone Zephyr with this PR to one directory above SOF_TOP/.. . I think I should add some extra directory in CIs but can't figure out where...

[aborisovich]

Checkpatch reports "Co-authored-by: " as "WARNING: Non-standard signature: Co-authored-by:".
Can we implement it to checkpatch? Looks like a standard feature and Zephyr has it.

[wszypelt]

@lgirdwood
It seems to me that everything should be working properly now, I have made retests for this and other PR

[marc-hb]

https://github.com/thesofproject/sof/blob/main/scripts/checkpatch.pl has Co-developed-by:, can you use that instead?

We're trying to minimize changes with the Linux version of checkpatch.pl to make it easier to catch-up: https://github.com/thesofproject/sof/commits/main/scripts/checkpatch.pl

This being said, if adding Co-authored-by is important for some reason (which one?) then adding a single line in sof/scripts/checkpatch.pl is not going to cause any frequent or difficult git conflict. You can submit that in a separate PR and it will automatically apply here because CI does not test PRs (pull/6005/head); CI tests merged PRs (pull/6005/merge)

[marc-hb]

How can I add additional directory in the container that CI uses?
Rignt now there is some SOF_TOP in the container and I actually create .west and clone Zephyr with this PR to one directory above SOF_TOP/.. . I think I should add some extra directory in CIs but can't figure out where...

I'm not 100% sure I understand the question but would this help?
zephyrproject-rtos/zephyr#34713 (comment)

BTW this discussion is linked from the issue this PR is fixing.

[aborisovich]

Push update:

1. added ./scripts/xtensa-build-zephyr.py --version flag that returns script version. It may me used now when incompatible changes are introduced to script by increasing Major part. @marc-hb is it ok like this?
2. added manifest version to west.yml schema to indicate supported west tool oldest version (that matches zephyr west version, reference: West Manifest - Version

[marc-hb]

I just performed a pretty extensive review, hope I did NOT miss anything. Overall logic looks good but there are still some local but important problems to address: exit code -1, strtobool() removed in Python 3.12,... see below. No logical change requested, should hopefully be easy and not controversial.

I'm also worried about the rimage key / symlink failure experienced by both @aiChaoSONG and @keqiaozhang... Can we use a direct link to the rimage key just for now instead of the indirect one through the (sometimes missing) symbolic link? Then we can discuss the symlink situation in a later, smaller and more focused PR. It's only a nice-to-have now and people can create it manually if it's missing for any reason.

PS @keqiaozhang: the function create_zephyr_sof_symlink() is still here, it has not changed and is still being called... in theory.

[marc-hb]

The "more pythonic" way is to drop check=False and replace with

except subprocess.CalledProcessError:
  return None, None

[aborisovich]

Well, generally yes but not in this specific case.
Look at the "check" flag docs fragment

If check is true, and the process exits with a non-zero exit code, a CalledProcessError exception will be raised. Attributes of that exception hold the arguments, the exit code, and stdout and stderr if they were captured.

So it's not like we catch an exception from subprocess directly but python command subprocess.run generates that exception and raises it if you set check=True. So in this case asking subprocess.run to raise exception just to catch it seems irrational.

[marc-hb]

The operating system only provides error codes, so all subprocess exceptions are "faked" anyway. subprocess synthesizes exceptions because it's more "pythonic".

My suggestion to catch CalledProcessError was also a suggestion to let other, unexpected exceptions/error codes "crash" the script with a stack trace to help debug it.

Nevermind, no real issue here.

[aborisovich]

Push update:

• fixed issues with code pointed out by @marc-hb (except one)
• added new commit e35894d8409495282c9560bfb923525f4f04ce6f that removes any commands invocation over symlink (except symlink creation) so rimage building and rimage keys are accessed directly now by sof directory. Script users should no longer have any symlink problems.

[lgirdwood]

@keqiaozhang fyi - current plan is to merge this tomorrow. Pls sync with @marc-hb and @mengdonglin

[marc-hb]

Looks good to me except for just ONE last problem which I'm afraid is a serious one, please prove me wrong: it seems to me that the previous west project is re-initialized with rm -rf even when the user answers "no". If correct then it is obviously a big problem!

[marc-hb]

I added this if/then/else so this docker script can be tested locally, outside CI without having to delete and re-clone everything.

Now things are different and it's not such a problem anymore but I think an if/then/else would still be nice to drop the -u when everything is already cloned, it would save a bit of time. No big deal, can be re-added later.

[marc-hb]

I'm afraid this perform re-initialization even when the user answered 'n'?

Suggested change

	if not reinitialize_answer:
	if reinitialize_answer != 'y':

[aborisovich]

Thanks, was fixing in hurry and missed this one!

[marc-hb]

The operating system only provides error codes, so all subprocess exceptions are "faked" anyway. subprocess synthesizes exceptions because it's more "pythonic".

My suggestion to catch CalledProcessError was also a suggestion to let other, unexpected exceptions/error codes "crash" the script with a stack trace to help debug it.

Nevermind, no real issue here.

[marc-hb]

I would print these in any case...

[aborisovich]

Well, west init kinda prints it already and I did not want to repeat after it:

[aborisovich]

The operating system only provides error codes, so all subprocess exceptions are "faked" anyway. subprocess synthesizes exceptions because it's more "pythonic".

Well, it depends on the operating system. There are systems like linux, written mostly in C where exceptions are "too C++" and can not be ever allowed according to linux kernel project religion
reference: Blog cpp in linux and another one...
There are systems that base on C++ for a long time now (like Windows), and use exceptions at system level ;-)
https://en.wikipedia.org/wiki/Microsoft-specific_exception_handling_mechanisms

[marc-hb]

Thanks @aborisovich for the huge effort.
@keqiaozhang you're the conductor, the timing is all yours.

[lgirdwood]

@wszypelt this should not be tested by CI yet, but the TGL DUT copy is failing again. Could be a bad DUT ?

[lgirdwood]

@keqiaozhang pls let us know when CI is ready and I'll merge this. Thanks !
@wszypelt fyi - need to integrate this PR with Internal CI too at some point.