PortAudio / portaudio

PortAudio is a cross-platform, open-source C language library for real-time audio input and output.
Other
1.47k stars 304 forks source link

Build portaudiocpp with cmake failed #939

Open donarturo11 opened 3 months ago

donarturo11 commented 3 months ago

Describe the bug Trying to create cmake build directory for portaudiocpp gets message:

CMake Error at cmake/modules/FindPortAudio.cmake:20 (message):
  PortAudio_FOUND but not target PortAudio::portaudio

To Reproduce Steps to reproduce the behavior. Include code if applicable.

  1. Install portaudio with cmake in any directory
    cmake -S portaudio -B portaudio-build -DCMAKE_INSTALL_PREFIX=${PORTAUDIO_PATH}
    cmake --build portaudio-build --config Release
    cmake --install portaudio-build --config Release
  2. Configure building with cmake portaudiocpp
    cmake -S portaudio/bindings/cpp -B portaudiocpp-build -DCMAKE_PREFIX_PATH=${PORTAUDIO_PATH} 

Expected behavior Configure build directory with no errors.

Actual behavior CMake throws error

CMake Error at cmake/modules/FindPortAudio.cmake:20 (message):
  PortAudio_FOUND but not target PortAudio::portaudio

Desktop (please complete the following information):

Proposed solution Unify target names cases.

Note: PortAudio is a community supported project. If you have a solution, please create a Pull Request for us to consider.

RossBencina commented 3 months ago

Our CI doesn't fail because our CI doesn't use portaudiocpp. But this does suggest a regression since presumably portaudiocpp used to build.

@donarturo11 would you please check PortAudio v19.7 release and confirm that it does not have this problem?

RossBencina commented 3 months ago

It's not failing CI because portaudiocpp is not tested by CI.

My concern now is that there is a regression in the PortAudio library CMakeLists.txt, and that maybe we should undo the library library name case change in PortAudio -> portaudio in the main PortAudio CMakeLists.txt.

@dechamps any thoughts?

donarturo11 commented 3 months ago

I think too that undoing change case in main CMakeList will be better solution than mine.

donarturo11 commented 3 months ago

@donarturo11 would you please check PortAudio v19.7 release and confirm that it does not have this problem?

Yes sure. So, preparing and building portaudiocpp using source from main branch and stable version of portaudio are successful. portaudiocpp-msvc

RossBencina commented 2 months ago

preparing and building portaudiocpp using source from main branch and stable version of portaudio are successful.

@donarturo11 thanks. So as it stands, am I correct to say that there is a regression in our main PortAudio CMakeLists.txt ?

RossBencina commented 2 months ago

Seems that this was part of @Be-ing's big rewrite (which I'm now strongly thinking we should just roll-back since there is no maintainer and it has been shown to have multiple issues):

https://github.com/PortAudio/portaudio/commit/242a0241f69bd359b692004f3974ce39ec5137fd#diff-1e7de1ae2d059d21e1dd75d5812d5a34b0222cef273b7c3a2af62eb747f9d20aR12

old add_library(portaudio:

https://github.com/PortAudio/portaudio/commit/242a0241f69bd359b692004f3974ce39ec5137fd#diff-1e7de1ae2d059d21e1dd75d5812d5a34b0222cef273b7c3a2af62eb747f9d20aL374

old add_library(portaudio_static:

https://github.com/PortAudio/portaudio/commit/242a0241f69bd359b692004f3974ce39ec5137fd#diff-1e7de1ae2d059d21e1dd75d5812d5a34b0222cef273b7c3a2af62eb747f9d20aL383

new: add_library(PortAudio:

https://github.com/PortAudio/portaudio/commit/242a0241f69bd359b692004f3974ce39ec5137fd#diff-1e7de1ae2d059d21e1dd75d5812d5a34b0222cef273b7c3a2af62eb747f9d20aR12

donarturo11 commented 2 months ago

@RossBencina Yes. In main CMakeLists.txt is a regression. Just undo cases in target names and portaudiocpp will built with no problem.

RossBencina commented 2 months ago

@donarturo11 Could you please close the old PR and create a new PR with our agreed fix?

donarturo11 commented 2 weeks ago

@RossBencina I'm sorry for huge delay, but I had a lot of work at home. I closed old PR and I'll create new soon as possible.

donarturo11 commented 2 weeks ago

So @RossBencina , could You now review a new PR, please? I hope that one will be more satysfying.