Kconfig: don't fall back on CONFIG_TIGERLAKE #7763
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
Makes sense. FWIW in the long term it's probably best to to some work to unify the SOF "platform" choice with the Zephyr-side "board"/"soc"/"arch" tuple.
As regards the fuzzer: there were definitely some leaky kconfigs in there. But for that application specifically, it seemed worthwhile to make sure as much of the firmware source base gets included as it would for the real device, even if the configuration is a little non-orthogonal.
| @@ -18,6 +18,8 @@ elseif(CONFIG_MT8188) | |||
| set(platform_folder mt8188) | |||
| elseif(CONFIG_MT8195) | |||
| set(platform_folder mt8195) | |||
| else() | |||
| message(FATAL_ERROR "Platform not defined, check your Kconfiguration?") | |||
There was a problem hiding this comment.
FWIW: you could move this to a build-time #error in a C file somewhere to avoid having to do this in two different cmake schemes.
There was a problem hiding this comment.
I respectfully but strongly disagree. I think it's better to check and fail as early as possible. Also, C compilation is parallelized and typically unreadable before adding -j1 whereas CMake configuration is single-threaded. I think it's also better to always have a "default" error clause in a case/switch, and/or to check a variable is not empty before using it (the -I platform/${EMPTY}/include wasted a large amount of my time!)
I avoid CMake as much as possible but the duplication here is only one line. It's only because Zephyr versus non-Zephyr so it won't grow larger.
Finally, in which C file?
All this being said, if there is some C file where this check could be useful TOO then why not; "defence in depth".
Use get_filename_component() to get the basename and remove the $platform_folder special case for CONFIG_RENOIR and CONFIG_REMBRANDT. Zero change, binaries are identical. Signed-off-by: Marc Herbert <marc.herbert@intel.com>
This saves a lot of backslashes and is easier to read. Zero change. Signed-off-by: Marc Herbert <marc.herbert@intel.com>
Don't rely on some semi-random default value. The final `zephyr/.config` and binaries are strictly identical. Signed-off-by: Marc Herbert <marc.herbert@intel.com>
There's really no reason to fall back on a default "platform" and pick up a bunch of other, totally random settings. Now the following command fails immediately with a useful `platform not defined` error message instead of stopping with a cryptic `platform/trace/trace.h: No such file or directory` after successfully compiling half the .c files in a Frankenstein CONFIG_uration. ``` west build -p --board mimx8mm_evk_a53 modules/audio/sof/app ``` More context in thesofproject#7192. Signed-off-by: Marc Herbert <marc.herbert@intel.com>
marc-hb
force-pushed
the
no-default-platform
branch
from
a5b64ff to
13ae274
Compare
|
Totally unrelated test failures in https://sof-ci.01.org/sofpr/PR7763/build9189/devicetest https://sof-ci.01.org/sofpr/PR7763/build9190/devicetest. This PR does not affect these platforms. |
marc-hb
requested review from
aborisovich,
lgirdwood,
plbossart,
mmaka1 and
lbetlej
as code owners
|
All LNL tests failed in https://sof-ci.01.org/sof-pr-viewer/#/build/PR7763/build12144879, just like they did in totally different #7772, so this is clearly unrelated. |
|
Thanks to whoever (@wszypelt ?) restored the LNL device. Now the newer https://sof-ci.01.org/sof-pr-viewer/#/build/PR7763/build12147803 is mostly passing except for this In any case it's totally unrelated to this PR. |
|
@marc-hb Thanks for the info, I'll debug it today |
|
@marc-hb It worked, it was a tricky problem with path lengths, depending on the number of reruns, some PR passed, some not. Anyway, everything should work now :) |
Glad I could report a actionable bug! Thanks for fixing the issue, https://sof-ci.01.org/sof-pr-viewer/#/build/PR7763/build12159888 is all green now. |
There's really no reason to fall back on a default "platform" and pick up a bunch of other, totally random settings.
Now the following command fails immediately with a useful
platform not definederror message instead of stopping with a crypticplatform/trace/trace.h: No such file or directoryafter successfully compiling half the .c files in a Frankenstein CONFIG_uration.More context in #7192.