Skip to content

Conversation

@mrajwa
Copy link
Contributor

@mrajwa mrajwa commented Jul 12, 2020

This patch adds missing header file asoc.h which is required
to build Topology parser (tplg_parser.c)

Signed-off-by: Marcin Rajwa marcin.rajwa@linux.intel.com

This patch adds missing header file asoc.h which is required
to build Topology parser (tplg_parser.c)

Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>
Copy link
Collaborator

@paulstelian97 paulstelian97 left a comment

Choose a reason for hiding this comment

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

Shouldn't this be in src/include/kernel?

@lgirdwood
Copy link
Member

The file is included with the kernel headers, I suspect we probably need to point to the kernel headers in the include paths.

@mrajwa
Copy link
Contributor Author

mrajwa commented Jul 14, 2020

@lgirdwood, it is in kernel repo include I agree, but I can't see it in FW anywhere.

@paulstelian97 , that may be a good idea.

@jajanusz
Copy link
Contributor

It shouldn't be in our repo at all, it's part of kernel 'core' codebase (not even sof), we don't copy alsa headers to sof, we also shouldn't copy non-SOFABI kernel headers. These files can be installed in system headers or added with CFLAGS (prepended to cmake command). What can be improved in regard to this problem - someone can make use of FIND cmake functions and notify if there are no needed headers in system path.

@marc-hb
Copy link
Collaborator

marc-hb commented Jul 15, 2020

someone can make use of FIND cmake functions and notify if there are no needed headers in system path.

What's the added value compared to a basic "asoc.h not found" message from the pre-processor?

@lgirdwood
Copy link
Member

lgirdwood commented Jul 16, 2020

someone can make use of FIND cmake functions and notify if there are no needed headers in system path.

What's the added value compared to a basic "asoc.h not found" message from the pre-processor?

This would help to avoid some confusion. It could appear a FW/SDK bug to anyone new. I would also add this in sof-docs as a dependency.

@marc-hb
Copy link
Collaborator

marc-hb commented Jul 16, 2020

This would help to avoid some confusion. It could appear a FW/SDK bug to anyone new.

I hope most C developers interpret any "missing X.h" error as "What dependency do I miss?". Sure you can always provide more information in a CMake error but last time I checked we had 1.5 CMake developers in the team (counting myself as 0.5) and unsurprisingly no one else anywhere close to volunteering. So less CMake code is more IMHO.

I would also add this in sof-docs as a dependency.

Absolutely - I went through the getting started guide not very long ago and the list of dependencies was there and easy to use. EDIT: it is there already, see next comment.

@marc-hb marc-hb requested review from aiChaoSONG and xiulipan July 16, 2020 15:19
@marc-hb
Copy link
Collaborator

marc-hb commented Jul 16, 2020

tl;dr

I tested on 20.04 the apt-get command documented for Ubuntu 18.10 at
https://thesofproject.github.io/latest/getting_started/build-guide/build-from-scratch.html#install-packaged-dependencies

It just works and installs /usr/include/sound/asoc.h. Can you please try it? Or the equivalent command for your Linux distribution? Please let us know.

Longer story

I searched and found on my Ubuntu 20.04 system 3 copies of the same asoc.h file.

linux-libc-dev:amd64: /usr/include/sound/asoc.h
libasound2-dev:amd64: /usr/include/alsa/sound/asoc.h
~/SOF/linux/include/uapi/sound/asoc.h # the original

I recommend against creating a 4th copy :-)

By default, ./scripts/build-tools.sh -f uses on my system the one from linux-libc-dev and only that one.

While linux-libc-dev is not explicitly listed in https://thesofproject.github.io/latest/getting_started/build-guide/build-from-scratch.html#install-packaged-dependencies, it is a dependency of libncurses5-dev which is explicitly listed. It's also a dependency of many other -dev packages so I'm surprised you don't have it:

apt remove linux-libc-dev

The following packages would be REMOVED:

  build-essential clang clang-10 debhelper dh-autoreconf g++ g++-9 g++-9-multilib g++-multilib
  gcc-9-multilib gcc-multilib gfortran gfortran-9 lib32stdc++-9-dev libatk-bridge2.0-dev libatk1.0-dev
  libatspi2.0-dev libblkid-dev libc6-dev libc6-dev-i386 libc6-dev-x32 libcaca-dev libcairo2-dev
  libelf-dev libexpat1-dev libfontconfig1-dev libfreetype-dev libfreetype6-dev libgdk-pixbuf2.0-dev
  libglib2.0-dev libgtk-3-dev libharfbuzz-dev libibus-1.0-dev libicu-dev libmount-dev libncurses-dev
  libncurses5-dev libpango1.0-dev libpcre2-dev libpcre3-dev libpng-dev libpulse-dev libpython3-dev
  libpython3.8-dev libselinux1-dev libslang2-dev libstdc++-9-dev libtool libtool-bin libx32stdc++-9-dev
  libxft-dev linux-libc-dev llvm-10-dev llvm-dev python3-dev python3.8-dev uuid-dev zlib1g-dev

Do you want to continue? [Y/n]  NO

PS/shameless plug: about alsa-lib packages see also thesofproject/linux#2211, advice welcome.

Copy link
Collaborator

@marc-hb marc-hb 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
Copy link
Collaborator

marc-hb commented Jul 27, 2020

I tested on 20.04 the apt-get command documented for Ubuntu 18.10 It just works and installs /usr/include/sound/asoc.h. Can you please try it? Or the equivalent command for your Linux distribution.

@mrajwa ?

Just Works, close?

@mrajwa
Copy link
Contributor Author

mrajwa commented Jul 29, 2020

It's also a dependency of many other -dev packages so I'm surprised you don't have it

I know and I am also surprised why my Ubuntu 16.04 doesn't install it during that apt-get step

Content of my /usr/include/sound/

ls /usr/include/sound/
asequencer.h compress_offload.h firewire.h sb16_csp.h
asound_fm.h compress_params.h hdsp.h sfnt_info.h
asound.h emu10k1.h hdspm.h

as you can see there is no asoc.h

however it is here /usr/include/alsa/sound/asoc.h - a quick and simple copy of asoc.h to /usr/include/sound/ fixes the issue.

Yes this PR is out of question and should be closed. I just got confused by this missing header file. Can I close now or we still want to discuss this case?

@lgirdwood
Copy link
Member

We can close.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants