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.

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

[BUG] alsatplg: invalid option -- 'I' in debian:unstable #5323

Closed
afq984 opened this issue Feb 8, 2022 · 17 comments · Fixed by #6153
Closed

[BUG] alsatplg: invalid option -- 'I' in debian:unstable #5323

afq984 opened this issue Feb 8, 2022 · 17 comments · Fixed by #6153
Assignees
Labels
alsa-utils Issues caused by alsa-utils bug Something isn't working as expected P2 Critical bugs or normal features
Milestone

Comments

@afq984
Copy link
Contributor

afq984 commented Feb 8, 2022

To Reproduce
Steps to reproduce the behavior: (e.g. list commands or actions used to reproduce the bug)

% ./scripts/build-tools.sh -T
...
gmake[2]: *** [topology/topology2/cavs/CMakeFiles/topology2_cavs-jsl-nocodec.dir/build.make:73: topology/topology2/cavs/cavs-jsl-nocodec.tplg] Error 1
gmake[2]: *** [topology/topology2/cavs/CMakeFiles/topology2_cavs-icl-nocodec.dir/build.make:73: topology/topology2/cavs/cavs-icl-nocodec.tplg] Error 1
gmake[1]: *** [CMakeFiles/Makefile2:7427: topology/topology2/cavs/CMakeFiles/topology2_cavs-jsl-nocodec.dir/all] Error 2
gmake[1]: *** [CMakeFiles/Makefile2:7453: topology/topology2/cavs/CMakeFiles/topology2_cavs-icl-nocodec.dir/all] Error 2
alsatplg: invalid option -- 'I'
Try `alsatplg --help' for more information.
alsatplg: invalid option -- 'I'
Try `alsatplg --help' for more information.
[  4%] Building C object ctl/CMakeFiles/sof-ctl.dir/ctl.c.o
[  4%] Building C object logger/CMakeFiles/sof-logger.dir/filter.c.o
gmake[2]: *** [topology/topology2/cavs/CMakeFiles/topology2_cavs-cnl-nocodec.dir/build.make:73: topology/topology2/cavs/cavs-cnl-nocodec.tplg] Error 1
gmake[2]: *** [topology/topology2/cavs/CMakeFiles/topology2_cavs-cml-nocodec.dir/build.make:73: topology/topology2/cavs/cavs-cml-nocodec.tplg] Error 1
...

Environment

% alsatplg --version
alsatplg version 1.2.6
libasound version 1.2.6.1
libatopology version 1.2.6.1

alsatplg is from the alsa-utils 1.2.6-1 package in the debian:unstable docker image.

there's indeed no -I flag in https://github.com/alsa-project/alsa-utils/blob/v1.2.6/topology/topology.c so I don't think it is a downstream packaging problem

@afq984 afq984 added the bug Something isn't working as expected label Feb 8, 2022
@lgirdwood
Copy link
Member

@ranj063 @juimonen do we need latest alsa-utils HEAD ?

@lgirdwood
Copy link
Member

@afq984 can you try with latest alsa-utils HEAD.
@greg-intel fyi - may need a container update.

@lgirdwood lgirdwood added the P2 Critical bugs or normal features label Feb 9, 2022
@afq984
Copy link
Contributor Author

afq984 commented Feb 9, 2022

Haven't tested alsa-utils HEAD, but probably just the version check needs to be updated once -I makes itself into a release:

if(${ALSATPLG_VERSION_NUMBER} VERSION_LESS "1.2.6")

@lgirdwood lgirdwood added this to the v2.1 milestone Feb 9, 2022
@marc-hb
Copy link
Collaborator

marc-hb commented Feb 9, 2022

The -I is missing on Fedora too, I already reported this in #5192 (comment)

I think this -I option is supposed to be present in 1.2.5, so this looks more like an ALSA configuration bug.

@ranj063
Copy link
Collaborator

ranj063 commented Feb 9, 2022

@marc-hb the -I option was added after the 1.2.6 release. So we need a new release version for alsatplg and update the check in our scripts.

@marc-hb
Copy link
Collaborator

marc-hb commented Feb 9, 2022

Thanks, now that I see that commit https://github.com/alsa-project/alsa-utils/commits/c8c348e28a258f17e387 is 3 commits after tag v1.2.6. I got confused by the test #if SND_LIB_VER(1, 2, 5) < SND_LIB_VERSION . Maybe the -I user interface was added after v1.2.6 but the corresponding feature was added in alsa-lib in 1.2.5?

@greg-intel fyi - may need a container update.

Funny enough I think we need a container "downgrade" right now, it would have caught this. More precisely, we need to stop using bleeding edge and unreleased versions in our container and stick to official ALSA releases so tests like if(${ALSATPLG_VERSION_NUMBER} VERSION_LESS "1.2.6") get a chance to do something useful which is not the case right now. Right now the container is uniquely successful = the opposite of what's expected from CI.

So we need a new release version for alsatplg and update the check in our scripts.

Should we bump the topology v2 test to the not yet released if(${ALSATPLG_VERSION_NUMBER} VERSION_LESS "1.2.7") in the meantime to fix everyone's build?

The current situation is was really awful:

So the only ALSA version that produces something usable right now is 1.2.5. It does not build v2 and is not shipped by any major distro right now (see #5192). Good thing there are few SOF developers :-((

@greg-intel
Copy link
Contributor

@marc-hb @lgirdwood give me a container, and I'll make it happen. Whatever is decided. If it's a roll-back, just tell me what recent container should be leveraged, and I'll have the reference updated in minutes. That's super easy.

@perexg
Copy link

perexg commented Feb 10, 2022

Why the test in the build system is not against the '-I' option in this case (avoid the version check)?

Also note that 1.2.6 should work, but the relative include path (against the parent file) is used which probably does not align with your tree (add symlinks?).

@marc-hb
Copy link
Collaborator

marc-hb commented Feb 10, 2022

Why the test in the build system is not against the '-I' option in this case (avoid the version check)?

Because the dependency is not just on -I and because a version number is so far the best known way to label, identify and track a number of features and bugs.

BTW isn't #if SND_LIB_VER(1, 2, 5) < SND_LIB_VERSION a version check too? Maybe autoconf could do this instead? I digress sorry.

Also note that 1.2.6 should work

The commit that introduced -I is 3 commits after v1.2.6. Looks like -I has simply never been released yet.

@perexg
Copy link

perexg commented Feb 10, 2022

The goal should be to make things working with the released and the development code, if you want to use or test some new functionality ahead of the release. So the version check is irrelevant for this and you need to find another way. Another goal is to be friendly to the users of the build system.

The SND_LIB_VER() checks were added to allow compilation of alsatlg with the older alsa-lib without a strict requirement for the newer alsa-lib in configure.ac . It's a nonstandard configuration and I keep up to the package maintainers or users, if they want to do this for a reason. But the key is that the build does not fail when someone wants to test / update another utility from this package with the older alsa-lib.

@mengdonglin mengdonglin added the alsa-utils Issues caused by alsa-utils label Feb 15, 2022
@lgirdwood
Copy link
Member

Luckily topology2 is not end user released yet, it's still WIP so it can be aligned with ALSA 1.2.7 when its ready. I think short term we need to remind developers that they need ALSA git for topology2 based development.

@lgirdwood lgirdwood modified the milestones: v2.1, v2.2 Feb 16, 2022
@marc-hb
Copy link
Collaborator

marc-hb commented Feb 16, 2022

I think short term we need to remind developers...

This works only as long as all SOF developers know each other.

they need ALSA git for topology2 based development.

v2 is really not the worst problem right now. v1 is silently corrupted by most ALSA versions right now, fixed by revert #5192

@juimonen
Copy link

@marc-hb is this still an issue? so topology2 needs the -I flag and if it is used in our build scripts for topology2, thats an issue, we need to either remove the topology2 compilation until available or message the developers to update to alsa-utils upstream head.

Unfortunately we are in transit with topology2 (some needed functionality not officially released), so what can we do? we can't even compare to release number as it doesn't exist...

@marc-hb
Copy link
Collaborator

marc-hb commented Feb 21, 2022

Until a released version of alsatplg can successfully build topoV2 I suggest commenting out the wrong alsatplg version check in topology/topology2/CMakeLists.txt and replace it with a test for the (empty) file topology/I_want_unstable_v2. CMake would build v2 if and only if this trigger file is found. Developers and CIs who have a recent enough alsatplg can very easily create this empty file, everyone else still builds topoV1.

@perexg
Copy link

perexg commented Feb 21, 2022

The detection can be automated:

# new alsatplg
$ touch a.config
$ alsatplg -I /tmp -c a.config -o a.bin
$ echo $?
0

# old alsatplg
$ alsatplg -I /tmp -c a.config -o a.bin
alsatplg: invalid option -- 'I'
Try `alsatplg --help' for more information.
$ echo $?
1

@marc-hb
Copy link
Collaborator

marc-hb commented Feb 21, 2022

The detection can be automated:

Until the next (and worse) problem is found (like some silent corruption?). The explicit topology/I_want_unstable_v2 method provides a manual switch not just for this -I problem but for any future issue too. It's a constant reminder in git status that the topologies are produced by an unreleased and unpredictable alsatplg version. It draws a very clear line between developers who need bleeding edge stuff and others who only want things to just work. It is also a reminder and incentive to replace it with a proper version check when possible later.

marc-hb added a commit to marc-hb/sof that referenced this issue Aug 17, 2022
Fixes: thesofproject#5323

The v1.2.6 requirement was a lie the whole time because alsatplg v1.2.6
does not have any `-I` option. The truth was: un-released alsa-utils
from git was required. The check for v1.2.6 was an approximation good
enough for many users but not for everyone, see for instance thesofproject#5323.

Now that alsa-utils v1.2.7 has been released we can stop lying:
 alsa-project/alsa-utils@7d934f3142549

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

marc-hb commented Aug 17, 2022

v1.2.7 is out, tentative fix in:

lgirdwood pushed a commit that referenced this issue Aug 18, 2022
Fixes: #5323

The v1.2.6 requirement was a lie the whole time because alsatplg v1.2.6
does not have any `-I` option. The truth was: un-released alsa-utils
from git was required. The check for v1.2.6 was an approximation good
enough for many users but not for everyone, see for instance #5323.

Now that alsa-utils v1.2.7 has been released we can stop lying:
 alsa-project/alsa-utils@7d934f3142549

Signed-off-by: Marc Herbert <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
alsa-utils Issues caused by alsa-utils bug Something isn't working as expected P2 Critical bugs or normal features
Projects
None yet
Development

Successfully merging a pull request may close this issue.