[crazoes]

More control over which tools should be built, instead of building
everything everytime.

Signed-off-by: Shreeya Patel shreeya.patel23498@gmail.com

Related issue: #3059

[crazoes]

I think this build fails because of version difference and there's no -S option in that version. What version of cmake are we trying to support that I should be using as reference for commands?

Edit: Removed the usage of -S option, it is now compatible with 3.12

[crazoes]

CC: @dbaluta @marc-hb

[marc-hb]

Thanks for doing this and sorry for the belated review. My main issue is with the (major!) change to an incremental build, see below.

Please do a quick shellcheck run if you haven't yet to make sure this doesn't add any new warning (do not fix the two already there in this PR, off-topic)

[marc-hb]

tl;dr: keep the rm -rf build_tools.

This script is rebuilding from scratch and is documented as such
https://thesofproject.github.io/latest/getting_started/build-guide/build-from-scratch.html#step-4-build-topology-and-tools

This change is converting it to incremental builds which is very big change.

In an ideal world, building from scratch is never required. In the real world, incremental builds are frequently broken.

There's also the case of people working on the build system itself. cmake tries hard to detect CMakeLists.txt changes and to reconfigure itself but there is only so much it can do.

Anyone working on the code itself (as opposed to the build system) and needing the fastest build time can and should invoke [c]make (or ninja) directly, they don't need and shouldn't use this script. Removing a layer of indirection also allows finer-grained control on the build, think cmake -DFOO=bar, make VERBOSE=1, etc.

I understand cmake and make offer a slightly less user-friendly interface but I don't think that's a huge price to pay if you want incremental builds. One possible way to make cmake and make easier could be this, up to you:

build_tool()
{
    tool=$1
    ( set -x
       make -C ....
    )
}

This cheap logging makes it possible to just copy/paste the command for re-use.
It doesn't hurt to see the exact command in CI either.

[crazoes]

Yes, docs seem to already cover and recommend the approach you stated. However, if we remove build_tools everytime, having individual switch to build individual tools might not make sense, because if I run ./scripts/build-tools -f, I get fuzzer, if I do ./scripts/build-tools -p I will get probes, but the fuzzer will be removed. At any given moment it will only contain tools that were last built by the script.

[marc-hb]

This was already the case with the fuzzer but fair enough, so how about something like

for tool in ctl fuzzer logger probes test topology; do
       if eval '$DO_BUILD_'$tool; then
        then	                        
                rm -rf "$BUILD_TOOLS_DIR/$tool"
                make_tool $tool
        fi
done

tests and empty-everything are special and handled with rm -rf "$BUILD_TOOLS_DIR

What I find a bit painful in every CMake project (not just one) is rebuilding from scratch. For now this script provides a dead simple and documented command for that and I'd be really sad to lose it, especially considering how quick it is to rebuild everything from scratch. Again anyone who needs a split second incremental build can and should invoke make or ninja as they do in every other CMake project.

[marc-hb]

I spent a bit more time looking at this. rm -rf "$BUILD_TOOLS_DIR/$tool" is not guaranteed to work because the top-level CMakeLists.txt file does not promise that every $tool will be built in a subdirectory name $tool.

because if I run ./scripts/build-tools -f, I get fuzzer, if I do ./scripts/build-tools -p I will get probes, but the fuzzer will be removed. At any given moment it will only contain tools that were last built by the script.

It depends, one tool/target could depend on another, so you could have more than one.

This made me realize something else: so you apparently want to rebuild one tool without touching the others yet cmake .. is re-run at the top-level for ALL the tools!

• cmake + make for the tools requested
• cmake but no make for the tools not requested

That's inconsistent and not every intuitive.

Trying this locally I also found that building everything takes less than 3 seconds on my system. I also wonder how you tested this because I found the make targets to be called sof-logger, sof-ctl and sof-probes. The targets logger and ctl exist but they don't build anything.

I'm wondering what problem(s) this really solves.

[crazoes]

This made me realize something else: so you apparently want to rebuild one tool without touching the others yet cmake .. is re-run at the top-level for ALL the tools!

• cmake + make for the tools requested
• cmake but no make for the tools not requested

Initially I tried to cmake only the tool asked to build. But when I tried

rm -rf tools/build_tools/
mkdir tools/build_tools/logger
cd tools/build_tools/logger
cmake ../../logger

the makefiles were written to tools/logger, weird. So I tried cmake -S tools/logger -B tools/build_tools/logger which worked and was used initially in my PR. However, that is not supported by the cmake in our CI and the README also suggests CMake > 3.10 hence, only way I found was to cmake the whole tools directory and make it work.

Trying this locally I also found that building everything takes less than 3 seconds on my system. I also wonder how you tested this because I found the make targets to be called sof-logger, sof-ctl and sof-probes. The targets logger and ctl exist but they don't build anything.

I didn't test make command inside in the tools/build_tools directory. I just tested the script, checked the executables in the build directories and even tested make commands in the individual build directories. Does the targets have to be named logger, ctl, etc?

I'm wondering what problem(s) this really solves.

Only specified tools are built, not other tools.

[dbaluta]

@marc-hb we are trying to make the script more flexible so only you compile the things you need. For example, for me it is always a problem when I try to compile the topologies because I always need to export the LD_LIBRARY_PATH to a newer version of alsa-lib so that topology can compile fine.

So, for example I only want to build the fuzzer and not to deal with building the topologies.

[marc-hb]

see below

[marc-hb]

For example, for me it is always a problem when I try to compile the topologies because I always need to export the LD_LIBRARY_PATH to a newer version of alsa-lib so that topology can compile fine.

Ha, this is not about build speed. I understand better now. I guess you're referring to alsa-lib bug #2543? BTW I have a quick, dirty and lazy hack for this situation: disabling ASRC topologies. I just shared it there.

I didn't test make command inside in the tools/build_tools directory. I just tested the script, checked the executables in the build directories and even tested make commands in the individual build directories. Does the targets have to be named logger, ctl, etc?

Oops sorry I missed that, I didn't pay enough attention and assumed this PR invoked the targets that can be seen in make help So this is another way the current version of this PR violates cmake's "encapsulation": cmake's subdirectories are internal cmake implementation details not supposed to be relied on. They're not part of the "build API". The build API if any is made of the targets printed by make help or ninja -t targets

if I run ./scripts/build-tools -f, I get fuzzer, if I do ./scripts/build-tools -p I will get probes, but the fuzzer will be removed. At any given moment it will only contain tools that were last built by the script.

Thinking about it again, this really doesn't seem like the end of the world. That's just the small price to pay for the lack of build modularity. UPDATE: it wouldn't be a bug but a feature, see next comment.

---

Initially I tried to cmake only the tool asked to build. [...]

The current structure of the CMakeLists.txt files is simply not designed for that much modularity; that's what I was trying to to highlight.

If the build of some tool(s) is truly independent from others then this should be addressed in the CMakeLists.txt files themselves, not poking at various [c]make internals like this from an additional layer of indirection.

While make has some drawbacks, it's most convenient feature is the ability to make [ -C build ] anything at the user interface level without writing any line of user interface code - unlike this PR. So here's an alternative to this PR you could consider: instead of adding code for each tool every time, how about a new ./scripts/host-build-tool.sh --only-cmake-you-name-it option that runs only cmake and does not run make but prints instead a one-line help/hint like cd build_tools; make sof-logger sof-fuzzer ... at the end? It would avoid build issues like #2543 without trying to incompletely duplicate [c]make user interface in an additional layer of indirection while relying on internal [c]make implementation details.

[marc-hb]

I've done some additional CMake research and found this nice entry:
https://stackoverflow.com/questions/33443164/cmake-share-library-with-multiple-executables
The question is very close and the answers very relevant. Other sources generally concur.

Reminder:

• cmake = build configuration
• make/ninja = actual build

tl;dr:

1. To build a collection of truly independent projects, each with their independent cmake configuration, use ExternalProject_add() at the top.
2. To build a collection of librairies/components with a SINGLE, global cmake configuration, use add_subdirectory() at the top layer. You can still build one component at a time but not configure one at a time.

Between 1. and 2. I'm not sure what is the best fit for sof/tools/. Maybe it should even be on a per tool basis? But until someone demonstrates ExternalProject is better at the top and volunteers to switch to it, we must stick to the approach currently in place. For now this is 2, so partial cmake configuration is not supported. It doesn't make sense either to run cmake and regenerate the global configuration while leaving some semi-random collection of former builds behind. Worst case it could even confuse the next build.

[aiChaoSONG]

Our CI use -t option to build test topology, ctl, etc. This PR will influence CI. @xiulipan

[marc-hb]

The sof-ci/jenkins/pr-fw-build — FW Build Failed:byt_xcc in https://sof-ci.01.org/sofpr/PR3064/build6424/build/ is "will not fit" issue #3063

[marc-hb]

I find the code in this version 99c08e299b92 very clean, thanks for your patience. Minor style comments.

I especially appreciate the new print_build_info() which I often struggled to figure out.

[marc-hb]

Could you change Build ctl to Re-build ctl etc.?

The [ ] brackets are not needed: they mean something is optional which makes sense only in the SYNOPSIS. Look at man tar and tar --help for a good example.

Couldn't decide on a better single letter option to represent "No build, only cmake". Open to suggestions.

-n is fine, maybe you'll prefer this:

usage: $0 [OPTION...]
...
  -C  No build, only CMake re-configuration. Ignored when rebuilding specific tool(s).

The "ignored" part I added is IMHO important.

[marc-hb]

It's very rare that adding a negation helps but in this particular case I find this easier to understand:

if ! "$CMAKE_ONLY"; then

[crazoes]

actually removing the conditional altogether. first, DO_BUILD is undefined until L89 so ./scripts/build-tools.sh fails. And when we do specify -n the number of options will never be 0, hence, this will never execute, so the additional conditional is unnecessary.

[marc-hb]

Thx I missed one exit. Funny how easy it is to get lost in not that much cyclomatic complexity. I think your latest version 1c3dfde303489f reduces it.

[marc-hb]

Can you move this zero arg test after flags have been removed below, so it's ready for adding a flag that would affects all builds in the future?

[crazoes]

if we move this zero arg test below getopts, we shift everything by then and it will always be 0. Everything gets parsed and no way to know if any arguments were provided(build specific things) or none were provided(build everything).

[marc-hb]

You're right, sorry. So I guess this would requires something like this instead:

BUILD_ALL=true
while getopt
                c) DO_BUILD_ctl=true; BUILD_ALL=false ;;
                f) DO_BUILD_fuzzer=true; BUILD_ALL=false ;;

Let's worry about that when the time comes.

[crazoes]

Yes, and I was trying to avoid not having to set BUILD_ALL with each option provided.

[marc-hb]

Thx I missed one exit. Funny how easy it is to get lost in not that much cyclomatic complexity. I think your latest version 1c3dfde303489f reduces it.

[marc-hb]

Approved except for the DO_BUILD that escaped the most recent search/replace.

Although it is not ignored when building specific tool.

Indeed it looks like I got that wrong. So it's the other way round: using -C ignores asking for specific tool(s), correct? The current help could be a little bit more explicit but it's OK.

[crazoes]

Indeed it looks like I got that wrong. So it's the other way round: using -C ignores asking for specific tool(s), correct? The current help could be a little bit more explicit but it's OK.

simply put: if -C is specified nothing gets built, ever.

So in a sense all other options are ignored if -C is provided.

[marc-hb]

You're right, sorry. So I guess this would requires something like this instead:

BUILD_ALL=true
while getopt
                c) DO_BUILD_ctl=true; BUILD_ALL=false ;;
                f) DO_BUILD_fuzzer=true; BUILD_ALL=false ;;

Let's worry about that when the time comes.

[crazoes]

I see travis test is failing with a timeout for cnl icl. Is getting a timeout usual? Can someone restart that stage?

[marc-hb]

I see travis test is failing with a timeout for cnl icl. Is getting a timeout usual?

Yes it is the usual way to stop qemu, please review thesofproject/qemu#44 to make that clearer.

Can someone restart that stage?

I suggest we merge #3101 restore qemu errors first (my bad) so we get some information in case it fails again.

[marc-hb]

I suggest we merge #3101 restore qemu errors first (my bad) so we get some information in case it fails again.

#3101 just got merged, @crazoes can you rebase?

BTW did you try to reproduce locally?

[crazoes]

@marc-hb everything passes now. And no, I hadn't tried reproducing this locally.

[aiChaoSONG]

SOFCI TEST
CI is updated, this PR is good to go.

[lgirdwood]

@zrombel any comment on CI failure ? This PR seems unrelated.

[zrombel]

@lgirdwood there was a hardware failure over night, PR is already waiting for rebuild.

[crazoes]

@lgirdwood just a reminder for this PR to be merged.

[marc-hb]

Fixing shellcheck warnings including some typos in variable names in #3365