Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

audio/CMakeLists.txt: move testbench/plugin to new, separate file #8892

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

audio/CMakeLists.txt: move testbench/plugin to new, separate file #8892

wants to merge 2 commits into from

Conversation

marc-hb
Copy link
Collaborator

@marc-hb marc-hb commented Feb 29, 2024

Pure cleanup to prepare de-duplication #8260, zero functional change.

2 commits, the main one:

Commit 7680a7b ("cmake: add testbench host build") added a
return() in the middle of src/audio/CMakeLists.txt to exclude some
C files from testbench compilation.

It wasn't easy to read already at the time. Now that the file has grown
bigger it's become even harder to spot and even more confusing.

Soon, #8260 will also add Zephyr to this file which will make things
much worse.

Solve all this by moving testbench and plugin compilation to a new,
separate, native.cmake file.

Reported in #8606

@marc-hb
src/ipc/CMakeLists.txt: remove now useless return()
5c6ac87 
Commit 7680a7b ("cmake: add testbench host build") added
`if (CONFIG_LIBRARY) return()` in `src/ipc/CMakeLists.txt` to exclude
some .c files from testbench compilation.

Then commit 8b680d5 ("host: enable ipc handler module") moved the
`return()` later in the files to exclude fewer files and add more
files to fuzzing.

Then commit 0a7df45 ("library: add trace and shared memory
region") moved it down again to add more files.

Then commit d62e926 ("ipc: split IPC major code into IPC specific
directories") removed the last bit of code that was still after the
return()

Now there is no code left after the `return()`!? Remove it.

Signed-off-by: Marc Herbert <[email protected]>
@marc-hb
audio/CMakeLists.txt: move testbench/plugin to new, separate file
174e10a 
Commit 7680a7b ("cmake: add testbench host build") added a
`return()` in the middle of `src/audio/CMakeLists.txt` to exclude some
C files from testbench compilation.

It wasn't easy to read already at the time. Now that the file has grown
bigger it's become even harder to spot and even more confusing.

Soon, thesofproject#8260 will also add Zephyr to this file which will make things
much worse.

Solve all this by moving testbench and plugin compilation to a new,
separate, `native.cmake` file.

Reported in thesofproject#8606

Signed-off-by: Marc Herbert <[email protected]>
@marc-hb

This comment was marked as outdated.

Sign in to view
@marc-hb marc-hb marked this pull request as ready for review February 29, 2024 22:41
@wszypelt
Copy link

wszypelt commented Mar 1, 2024

@marc-hb Slowly, QB is getting back on track. After checking the build once again, FW building and tests in the Internal Intel CI system turn green

@marc-hb
Copy link
Collaborator Author

marc-hb commented Mar 1, 2024

Newer https://sof-ci.01.org/sof-pr-viewer/#/build/PR8892/build13655958 is green - thanks!

lgirdwood
lgirdwood reviewed Mar 1, 2024
View reviewed changes
src/audio/native.cmake
@@ -0,0 +1,108 @@
# SPDX-License-Identifier: BSD-3-Clause
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"native" could be a bit ambiguous, lets spell out plugin.cmake and have a comment here that says this is to build plugin, testbench etc.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We had the same discussion when coining CONFIG_COMP_MODULE_SHARED_LIBRARY_BUILD . Maybe stick to that name and have "audio/module_shared_library.cmake". I can live with "plugin.cmake" as well.

src/audio/native.cmake
subdirs(pipeline)

if(CONFIG_COMP_MODULE_ADAPTER)
add_subdirectory(module_adapter)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like an extra tab here

@marc-hb
Copy link
Collaborator Author

marc-hb commented Mar 1, 2024 •
edited
Loading

"native" could be a bit ambiguous, lets spell out plugin.cmake and have a comment here that says this is to build plugin, testbench etc.

I naively trusted the Kconfig help added for COMP_MODULE_SHARED_LIBRARY_BUILD in commit b720f1a (ALSA plugin PR #8132), it says:

Select if you want to build modules as shared objects that can be used to run pipelines on the host with the testbench or the ALSA plugin. (emphasis mine).

But after testing a bit more I just realized that this recent COMP_MODULE_SHARED_LIBRARY_BUILD is not used by the testbench at all...? Can someone explain? @singalsu maybe?

I also remembered that the testbench supports both "native" and non-native code. Neither uses COMP_MODULE_SHARED_LIBRARY_BUILD

scripts/rebuild-testbench.sh -h
usage: scripts/rebuild-testbench.sh [-f] [-p <platform>]
       -p Build testbench binary for xt-run for selected platform, e.g. -p tgl
          When omitted, perform a BUILD_TYPE=native, compile-only check.

The mystery deepens...

More context than anyone wants:

"native" is the non-negative term used by Zephyr to say "not compiled for the product":
https://docs.zephyrproject.org/latest/boards/posix/native_sim/doc/index.html
https://docs.zephyrproject.org/latest/boards/posix/native_posix/doc/index.html
https://docs.zephyrproject.org/latest/samples/drivers/uart/native_tty/README.html

"module", "shared" and "library" are all ambiguous now that we have shared "modules" / "libraries" running on DSP hardware too:

"host" can be quite ambiguous too:

@lgirdwood
Copy link
Member

"native" could be a bit ambiguous, lets spell out plugin.cmake and have a comment here that says this is to build plugin, testbench etc.

I naively trusted the Kconfig help added for COMP_MODULE_SHARED_LIBRARY_BUILD in commit b720f1a (ALSA plugin PR #8132), it says:

Lets just say any make target that does not run on the DSP can have a Cmake.plugin Cmake.testbench etc.. Need to be quick if this is for v2.9

kv2019i
kv2019i reviewed Mar 5, 2024
View reviewed changes
Copy link
Collaborator

@kv2019i kv2019i left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the detailed history in git commit messages, that was very helpful when reviewing the patches! No obstacles, I think only open is how native.cmake is named, see inline comments from Liam and me.

src/audio/native.cmake
@@ -0,0 +1,108 @@
# SPDX-License-Identifier: BSD-3-Clause
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We had the same discussion when coining CONFIG_COMP_MODULE_SHARED_LIBRARY_BUILD . Maybe stick to that name and have "audio/module_shared_library.cmake". I can live with "plugin.cmake" as well.

singalsu
singalsu reviewed Mar 5, 2024
View reviewed changes
Copy link
Collaborator

@singalsu singalsu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@marc-hb I think the SOF alsa plugin is the sole user of shared library version. Testbench and Cmocka unit tests are static builds.

src/audio/native.cmake
set(mixer_src mixer/mixer.c mixer/mixer_generic.c mixer/mixer_hifi3.c)
elseif(CONFIG_IPC_MAJOR_4)
set(mixer_src mixin_mixout/mixin_mixout.c mixin_mixout/mixin_mixout_generic.c mixin_mixout/mixin_mixout_hifi3.c)
endif()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is mixer here and not with other components later?

@lgirdwood
Copy link
Member

@marc-hb Any update here ?

@marc-hb marc-hb mentioned this pull request Sep 6, 2024
Open
@lgirdwood lgirdwood mentioned this pull request Sep 17, 2024
Closed
@lgirdwood
Copy link
Member

Needed for #9025

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kv2019i kv2019i kv2019i left review comments

@singalsu singalsu singalsu left review comments

@lgirdwood lgirdwood lgirdwood left review comments

@bardliao bardliao Awaiting requested review from bardliao bardliao is a code owner

@marcinszkudlinski marcinszkudlinski Awaiting requested review from marcinszkudlinski marcinszkudlinski is a code owner

@pblaszko pblaszko Awaiting requested review from pblaszko pblaszko is a code owner

@plbossart plbossart Awaiting requested review from plbossart plbossart is a code owner

@mmaka1 mmaka1 Awaiting requested review from mmaka1 mmaka1 is a code owner

@lbetlej lbetlej Awaiting requested review from lbetlej lbetlej is a code owner

@dbaluta dbaluta Awaiting requested review from dbaluta dbaluta is a code owner

@ranj063 ranj063 Awaiting requested review from ranj063

@andrula-song andrula-song Awaiting requested review from andrula-song

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@marc-hb @wszypelt @lgirdwood @singalsu @kv2019i