[aborisovich]

Four commits:

1. Fixed warning in sof-logger issued from compilers newer than GCC 9.1 "char-subscripts" by adding (int) cast to isspace functions.
2. To enable Windows build, added additional include path to sof logger so required system standard POSIX headers could be found in MSYS2 environment. Unfortunately there are macros redefinitions between standard GCC headers and src\include\sof\compiler_attributes.h. Could not find the way around it, also GCC does not provide any policy to silence this particular warning, so had add #ifndef guards in the compiler_attributes.h header.
3. Added #ifndef guards to macros to avoid redefinitions with GCC standard headers on MSYS2.
4. Added xtensa-build-zephyr.py script that allows to build SOF with Zephyr on both Windows and Linux.

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

[lgirdwood]

@aborisovich I'm guessing the shell script is not working on Windows ?
These scripts are developer aids only (e.g. should not really be used as part of build infra) so the only issue here is keeping this aligned with shell version or deprecate the shell version ? @marc-hb any comments.

[aborisovich]

@lgirdwood yes, unfortunately xtensa-build-zephyr.sh does not work on Windows thus I made this counterpart.
Moreover it has some new features comparing to the old one:

• New flag -u that allows to clone any fork by providing URL in --clone mode
• Replaced "make" build commands with cmake --build what allows to use new CMake generators eg. Ninja

In my opinion this script should be thoroughly tested by end-users and when proved working replace existing xtensa-build-zephyr.sh. I did not remove it in this commit (xtensa-build-zephyr.sh) as I did not know that it is not used in processes or VMs.

[aborisovich]

Everyone are welcome to test this script against bugs since I surely did not find all flaws.
Especially on linux OS where it had run only several times (since I wrote it on Windows).

[lgirdwood]

• New flag -u that allows to clone any fork by providing URL in --clone mode
• Replaced "make" build commands with cmake --build what allows to use new CMake generators eg. Ninja

Best to put this in the commit message so it's obvious.
Fwiw, I think @marc-hb is using Ninja so should be able to comment.

[kv2019i]

Thanks @aborisovich ! This looks very nice. I think we might want to keep both scripts in parallel for a while to get some mileage in various Linux systems. I do have a few minor questions on the earlier two patches, please see inline.

[marc-hb]

Very incomplete review sorry, not enough time right now. Will get back to it soon.

Thanks for this rewrite, impressive effort! Yes all Zephyr users should be cherry-picking this (git fetch origin pull/5299/head && git cherry-pick FETCH_HEAD) and start using it ASAP - even before it gets merged.

An important, future and post-merge test will be to switch CI to it but many people in our CI team are out this week for Chinese New Year. Later after some transition period we can delete the shell script.

One important feature missing is logging. This is very important not just to help resolve bugs in this script but even more importantly to quickly assess what went wrong in its environment (there is always something going wrong somewhere). The log level should always be increased in CI.

With the shell script you could quickly insert a set -x and get some logging anywhere. The script already has some set -x in some critical places.

A hopefully quick way to get a lot of useful logs with minimal code changes is to add a simple wrapper for all calls to subprocess.run(). That wrapper uses logging.info() to print all its arguments and then passes them all through to subprocess.run(). This is one of the things west -v does BTW.

Tested example:

def run_command(*args, **kwargs):
    logging.warning("Running " + " ".join(args[0]))
    return subprocess.run(*args, **kwargs)   

[lgirdwood]

@aborisovich I will rerun CI this afternoon, looks like a server error for the container and some DUTs were not available. QB issue looks like a false positive.

[lgirdwood]

SOFCI TEST

[lgirdwood]

@marc-hb @greg-intel this PR has failed Zephyr/Fuzzer build twice on what looks like container network issues. Should be completely unrelated to this PR. Other PRs have been building the container OK today. Can you take a quick check and rerun CI if you think this is just bad luck here. Thanks

[aborisovich]

New fixes:

• removed previous policy to turn off -Werror on windows hosts. Fixed warnings by adding ifndef guards to macros in compiler_definitions.h
• @marc-hb added new flag "-vv" (more verbose) that implicitly implements -v (verbose) and additionally prints all commands invoked by subprocess.run(). Added a wrapper function as you suggested.
  When -vv flag set it prints:
  Running command: command in directory

[marc-hb]

@marc-hb @greg-intel this PR has failed Zephyr/Fuzzer build twice on what looks like container network issues.

Always share URLs to test failures as Github does not support force pushes (it only tolerates them)

[fredoh9]

I am able to see the errors. Looks temporary network glitch.
https://github.com/thesofproject/sof/runs/5035230313?check_suite_focus=true

Err:1 http://archive.ubuntu.com/ubuntu focal/main amd64 libsigsegv2 amd64 2.12-2
  Could not connect to archive.ubuntu.com:80 (91.189.88.152), connection timed out Could not connect to archive.ubuntu.com:80 (91.189.88.142), connection timed out

Coincidentally we had sof docker image update in yesterday, I don't think this is direct reason for network failure, but I will take a look for any suspicious warning or error.

[aborisovich]

@marc-hb Resolved issues:

• ./scripts/xtensa-build-zephyr.py -c tried to build platforms when no platforms provided. Fixed - when no platforms provided it just clones and updates submodules as it should.
• ./scripts/xtensa-build-zephyr.py -v -v apl did nothing when no -p flag was specified. Fixed - now when neither -p nor -c flag specified, -p with default directory SOF_TOP/zephyrproject is used.
• fixed issue with XTENSA_TOOLS_ROOT variable not defined in the environment. Fixed - now if platform selected for build has XTENSA_TOOLS_VERSION in its dictionary, XTENSA_TOOLS_ROOT is read from environment or exception with informative message thrown.
• help description of --all flag had been reworded so it is clear that it builds all currently supported platforms.

[marc-hb]

Thanks, I will test again in the next 24h

[aborisovich]

"flags", "modes", etc. I don't really care how you call them; afraid you lost me in the various definitions and nuances. My point was entirely the first sentence: "Cloning and linking are not mutually exclusive", I think that's why nothing built the first time I tried to use this script. I thought the "modes" terminology would help explain the problem but forget it, let's leave terminology aside.
I don't understand why you have to "fight argparser", some argument validation and processing can done after invoking argparse, it does not have to be entirely implemented in argparse.
Last but not least, do not "resolve" and hide discussions after asking "can you accept my point of view?" + leaving the "mutually exclusive" comment in place. Discussions are "resolved" only after agreement and it's fine to leave some discussions never resolved / never hidden - and easier to see even after merge.

Ok sorry I will not resolve them. I just did not get your point fully and I am also getting confused in all this. It is a complex script no doubt. I also got carried away for a while with that argparser subparser idea. For some reason I imagined for a while that we need both -p argument signaling that we actually want to rebuild and another -P argument giving a path to zephyr project.
Fortunately I realized it is not needed at all and it should be used just as you indicated - if no -p or -c flag specified just do -p with default path.

[lgirdwood]

@aborisovich @marc-hb Lets merge and fix as we go since we have an existing shell script that can be used on Linux/CI. This script is working on Windows so we can log bugs against it for Linux and deprecate the shell version when this is working well for all OSes.

[marc-hb]

I don't mind merging even with some bugs or room for improvement. I mind merging with a different user interface / "API" (at least not without some strong reason) because bringing everyone on the same page again will disrupt at least half the users.

I will give this another quick try today as promised.

[marc-hb]

Thanks for the cloning fixes.

Tried it again, now clones but fails to build with:

    raise RuntimeError(f"Platform {platform} uses Xtensa toolchain."
RuntimeError: Platform apl uses Xtensa toolchain.
Please set your XTENSA_TOOLS_ROOT environmental variable

There is no platform that requires XCC.

Again this would have not happened if you had simply copied the original logic.

We're making progress but using me as CI is really not efficient. Try this tiny test hack instead and github will test the most basic features (clone and build) for you:

diff --git a/zephyr/docker-build.sh b/zephyr/docker-build.sh
index 0700cbaa780b..d04ea21be028 100755
--- a/zephyr/docker-build.sh
+++ b/zephyr/docker-build.sh
@@ -36,5 +36,5 @@ else
     stat .
     sudo chgrp -R 1000 .
     sudo chmod -R g+rwX .
-    ./scripts/xtensa-build-zephyr.sh -a -c "$@"
+    ./scripts/xtensa-build-zephyr.py -a -c "$@"
 fi

[marc-hb]

This makes it impossible to build with the Zephyr SDK.

So far we've been able to build every platform with either the Zephyr SDK or XCC, both work fine. The Zephyr SDK requires zero license and zero configuration, install it in the HOME directory and that's it, job done. Nothing to do in this script to support it (besides removing this RunTimeError)

[aborisovich]

@marc-hb fixed exception issue with XTENSA_TOOLS_ROOT - now when XTENSA_TOOLS_ROOT environmental variable is set it is a signal to use user installed custom Xtensa installation. When not assume user has Zephyr SDK installed.
Performed manual tests on linux:

1. Command ./scripts/xtensa-build-zephyr.py -c -a works now exactly the same as ./scripts/xtensa-build-zephyr.sh -c -a. All supported platforms are built successfully using Zephyr SDK.
2. Command ./scripts/xtensa-build-zephyr.py -c tgl tgl-h with environmental variable: XTENSA_TOOLS_ROOT set works exactly the same as ./scripts/xtensa-build-zephyr.sh -c tgl tgl-h. User custom xtensa installation is used and platforms are successfully built. Unfortunately could not test -a flag as I could not find Xtensa cores to download for APL. I think they are unavailable now.
3. Command ./scripts/xtensa-build-zephyr.py -c tgl imx8m with environmental variable: XTENSA_TOOLS_ROOT set works exactly the same as ./scripts/xtensa-build-zephyr.sh -c tgl imx8m - does not work because scripts are unable to 'deinitialize' xtensa toolchain when building imx8m that was previously initialized to build tgl. But well maybe we will improve it someday.

Added new features:

• Added pretty printing to similar to linux tree command that xtensa-build-zephyr.sh uses to present output directory contents.

Created test PR replacing invocation of the xtensa-build-zephyr.sh with new script. Build Zephyr workflow is passing. Currently Windows build encountered regression as I am failing to build sof sources due to zephyr libc clashes when using standard gcc. I would submit this script for now without replacing existing xtensa-build-zephyr.sh. Passing CI build does not mean this script is fully mature and tested taking into consideration its complexity...

[aborisovich]

Minor update - installed files are not presented when platforms were not built (no need to do that when somebody uses only -c flag).

[marc-hb]

All basic features work for me, thanks @aborisovich very impressive effort.

I will help start the transition away from the .sh and we can refine if needed.

[lgirdwood]

Good work @aborisovich !