[marc-hb]

2 commits.

Should help with:

• [FEATURE][Zephyr] Remove duplication of CMake rules between Zephyr and XTOS #8260

[lgirdwood]

Nice start !

[andyross]

Very much needed. The existing Zephyr kconfig CMakeLists.txt is indeed a giant merge collision factory as it stands. Some drive-by notes.

[andyross]

Might be worth creating a little bit of new API here. Now, I personally know perfectly well what this means ("CONFIG_SOF" is defined in $ZEPHYR_BASE/modules/Kconfig.sof and declares that the "SOF" module is active, therefore we must be a Zephyr build).

But anyone looking at this for the first time is going to balk: "Wait, what? SOF means ZEPHYR? I thought it was all SOF?"

And if they were a little more sophisticated and knew that we had Kconfigs used in both modes, they'd probably assume that "CONFIG_SOF" means not Zephyr.

Maybe set a variable somewhere so you can write if(__ZEPHYR__) instead? (By analogy to the same symbol present in the C preprocessor).

[marc-hb]

Agreed, I didn't like that too. I added a comment but...

Maybe set a variable somewhere so you can write if(ZEPHYR) instead?

The problem is "somewhere" and as usual racing to be first before users. Enough bootstrap issues in the real code already, the fewer in CMake the better :-) CONFIG_XXX is great because CMake code uses it almost everywhere so I'm not the only one "racing it".

I can do two things:

1. Switch to the much longer CONFIG_ZEPHYR_SOF_MODULE that I just discovered. It seems generated by the build system. Not sure how but it's already being used elsewhere for the very same purpose.

2. Add an indirection layer on top of CONFIG_ZEPHYR_SOF_MODULE but with a well-named is_zephyr() CMake function instead. So the build fails if it's not defined somewhere unlike an ambiguous and dangerous ifdef. Also, the indirection layer will provide room for verbose comments if needed and a single place to patch if/when things change. Now the question is still: where do I define this function?

[andyross]

If you do it in the upstream zephyr module, you'll be before any of the cmake code in SOF proper for sure. But yeah, if there's a clearer kconfig that's probably even better (I'm not much of an expert on the module layer, but if it's exposed in kconfig it's surely valid to rely on).

[andyross]

This is a spot where Zephyr's promiscuous cmake layer hurts pretty badly, because you can't add files to the project in the same way in both modes. Again, maybe work around this by adding yet another layer of indirection and have a "sof_sources()".

Also, FWIW: the "_library_sources" variant really doesn't do anything useful beyond make the build slower by taking the time to create a static library. There is (AFAIK) exactly one spot in Zephyr where we have to do this for obscure linkage reasons[1], but for general C code it's almost always faster and cleaner to link all the object files at once.

I've tried to make that point upstream and basically failed: "zephyr_library_sources()" is a pervasive paradigm. But really it's needless except in specific situations.

(For reference: the reason to use it is that a static library only presents internal files to the link if one of their symbols is accessed, so you can build it and include static stuff like e.g. SYS_INIT() or interrupt declarations, and avoid having those end up in the final output unless something is using the file. But DTS and kconfig are the right mechanisms for that kind of dependency tracking, doing it with the linker is dangerous and surprising.)

[marc-hb]

Again, maybe work around this by adding yet another layer of indirection and have a "sof_sources()".

I thought about this and decided against it. There are already layers of indirection-obfuscation in both XTOS and Zephyr, don't want to stack yet another one on top. A dead simple set(common_files ...) works great and avoids enough duplication.

[marc-hb]

I've tried to make that point upstream and basically failed: "zephyr_library_sources()" is a pervasive paradigm. But really it's needless except in specific situations.

Would you care to change this in sof/zephyr/CMakeLists.txt right now? Don't mind conflicting with this, I'll happily rebase[*] It's a good time now because:

• we just started the SOF release cycle (and the Zephyr one for that matter)
• They are all still in one file for now! Quick :-)
• There is no rush to "de-centralize" yet.

[*] As much as one can be happy resolving git conflicts :-D

[andyross]

Heh, I've spent my bikeshed quota for the month already.

The way this goes is that someone says "libraries make linking faster!" and the response is "only after the ranlib stage builds the table, and in zephyr most symbols are actually needed by the final link", then someone else demands benchmarks, etc...

Really I was mostly just whining about a personal peeve. I've made my peace with it: per-directory/per-app fine grained static libraries are the way zephyr links itself.

[btian1]

1. can you add details for how decentralize zephyr/CMakeList.txt in PR comments?
2. anyway, two patches share exact same comments is not good.
3. is this all for decentralize? or just a beginning?

[marc-hb]

can you add details for how decentralize zephyr/CMakeList.txt in PR comments?

I don't understand the question, can you be more specific? This PR is just moving CMake code from one giant file to multiple CMake files in multiple subdirectories. What about this move don't you understand?

anyway, two patches share exact same comments is not good.

The two commits do the same thing in two different directories. So the only difference between the two commit messages is the directory name.

is this all for decentralize? or just a beginning?

Did you look at the title of the PR?

[lgirdwood]

@andyross good for you as a starting point to merge, we can factor in more review comments incrementally ?

[andyross]

Sorry, realized I never had a +1 on this

[kv2019i]

Thanks @marc-hb . This is a great start.

[kv2019i]

This looks strange, but it matches the existing logic, so great to have this become more visible in the cmake code. The long per-product sections in one-giant-zephyr-cmakefile easily hide these things (when we have per-platform compilation of stuff that really shouldn't be per-platform).

[marc-hb]

I fixed the unrelated zmain failures in https://github.com/thesofproject/sof/actions/runs/6725126747/job/18278872844?pr=8422 with unrelated:

• power_domain_intel_adsp.c: revert recent INIT_PRIORITY change zephyrproject-rtos/zephyr#64712

Not force-pushing not to lose existing approvals :-)

The multiple-pause resume failure in https://sof-ci.01.org/sofpr/PR8422/build14478/devicetest/index.html is also unrelated, due to

• multiple-pause-resume.sh: Check for aplay/arecord pid when waiting sof-test#1126