Fix with a partial revert. "topology: cmake: use local ALSA utilities to build topology" #10073
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR reverts the previous commit that forced building ALSA utilities locally, so sof-ctl and the topology compiler now use the system-installed alsa-lib and alsatplg.
- Remove custom
ALSATPLG_CMDand invokealsatplgdirectly - Drop local ALSA library/include linking for
sof-ctl - Update warning/debug messages to reference the system tool
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tools/topology/CMakeLists.txt | Remove local ALSA tools dir, replace ${ALSATPLG_CMD} with alsatplg |
| tools/ctl/CMakeLists.txt | Remove local ALSA include/link dirs, link against system -lasound |
Comments suppressed due to low confidence (3)
tools/topology/CMakeLists.txt:5
- Consider using CMake’s find_program to locate the
alsatplgexecutable and store its full path in a variable. This ensures the correct binary is used even if multiple versions exist in PATH or in different install locations.
execute_process(COMMAND alsatplg --version
tools/ctl/CMakeLists.txt:5
- It would be more robust to use
find_package(ALSA REQUIRED)and then link with the imported target (e.g.,ALSA::ALSA) rather than hard-coding-lasound. This ensures CMake can locate both headers and libraries portably.
)
tools/topology/CMakeLists.txt:70
- Similar to the version check, consider referencing the variable set by
find_program(alsatplg ...)here instead of a barealsatplgcall to avoid surprises when multiple installations are present.
COMMAND alsatplg \$\${VERBOSE:+-v 1} -c ${ARGV0} -o ${ARGV1}
kv2019i
left a comment
kv2019i
left a comment
There was a problem hiding this comment.
Not sure do we want to revert the tplg part without an alternative solution in place?
tools/topology/CMakeLists.txt
Outdated
|
|
||
| function(alsatplg_version OUT_STATUS OUT_VERSION) | ||
| execute_process(COMMAND ${ALSATPLG_CMD} --version | ||
| execute_process(COMMAND alsatplg --version |
There was a problem hiding this comment.
Commit message refers to alsactl but this reverts the whole thing. This will cause invalid tplgs to be created by our CI system... (as the system alsa-lib is too old).
There was a problem hiding this comment.
ah right - the original commit does 2 things - will fix it to just do ctl only.
lgirdwood
force-pushed
the
lrg/topic/sof-ctl-fix
branch
from
183d580 to
4da9a87
Compare
kv2019i
left a comment
kv2019i
left a comment
There was a problem hiding this comment.
Looks good now. Only concern is that this require build host to have system asound-dev installed. I know @cgturner1 had at least one build environment where asound-dev was on purpose not installed to ensure the the locallyl built alsa is used used. maybe @cgturner1 to ack this?
|
@lyakh ping for review. |
lyakh
left a comment
lyakh
left a comment
There was a problem hiding this comment.
would be good to have a note explaining the reason for the revert somewhere
See above bug - and its no longer a full patch revert now. Updated in PR title. |
lgirdwood
changed the title
|
SOFCI TEST |
1 similar comment
|
SOFCI TEST |
The SOF ctl tool is conservative with API and does not need a bleeding edge ALSA library so it can be built with system ALSA. Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com>
lgirdwood
force-pushed
the
lrg/topic/sof-ctl-fix
branch
from
4da9a87 to
389fab9
Compare
5 participants
This reverts commit 47dd8d3.
sof-ctl does not need git version of alsa-lib like the topology compiler but can use system version of alsa-lib to build.