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

pulseaudio: switch to Meson, add new versions, rename CMake targets #20732

Closed
wants to merge 9 commits into from

Conversation

valgur
Copy link
Contributor

@valgur valgur commented Oct 22, 2023

Follow-up for #20275 to have an actual PR to merge.

pulseaudio have had Meson build system support for a while and the latest versions have dropped Autotools altogether. This PR updates the recipe accordingly.

I also noticed that the recipe does not explicitly set the CMake file and target names and I did not see a reference to an existing convention either. So I renamed the targets based on the CMake modules used by KDE at https://api.kde.org/ecm/find-module/FindPulseAudio.html, which seems authoritative enough to me.
@ericLemanissier, I saw you left a comment in the recipe regarding generated CMake variables. Does the above change look reasonable to you?

TODO:

  • I have not check all of the options and dependencies used by the project's meson.build. Only made minimal tweaks to get the existing functionality in the recipe to work.
  • The Meson-based build system should support Windows as well. Needs testing.

@ghost
Copy link

ghost commented Oct 22, 2023

I detected other pull requests that are modifying pulseaudio/all recipe:

This message is automatically generated by https://github.com/ericLemanissier/conan-center-conflicting-prs so don't hesitate to report issues/improvements there.

@SpaceIm
Copy link
Contributor

SpaceIm commented Oct 22, 2023

I also noticed that the recipe does not explicitly set the CMake file and target names and I did not see a reference to an existing convention either. So I renamed the targets based on the CMake modules used by KDE at https://api.kde.org/ecm/find-module/FindPulseAudio.html

which seems authoritative enough to me.

It's not.

There are 2 authoritative sources for a library:

Everything else is confidential and shouldn't be used as a reference.

@conan-center-bot

This comment has been minimized.

@valgur
Copy link
Contributor Author

valgur commented Oct 23, 2023

It's not.

There are 2 authoritative sources for a library:

Everything else is confidential and shouldn't be used as a reference.

I kind of agree, but IMHO the goal of the packages provided by CCI should be to work as frictionlessly as possibly with existing code bases and environments. Since PulseAudio is most likely used via a system package manager, I think matching the naming conventions used by Ubuntu and Debian would be quite reasonable. Both of them include a /usr/lib/x86_64-linux-gnu/cmake/PulseAudio/PulseAudioConfig.cmake CMake config file (https://packages.ubuntu.com/mantic/amd64/libpulse-dev/filelist) with the following content:

set(PULSEAUDIO_FOUND TRUE)

set(PULSEAUDIO_VERSION_MAJOR 16)
set(PULSEAUDIO_VERSION_MINOR 1)
set(PULSEAUDIO_VERSION 16.1)
set(PULSEAUDIO_VERSION_STRING "16.1")

find_path(PULSEAUDIO_INCLUDE_DIR pulse/pulseaudio.h HINTS "/usr/include")
find_library(PULSEAUDIO_LIBRARY NAMES pulse libpulse HINTS "/usr/lib/x86_64-linux-gnu")
find_library(PULSEAUDIO_MAINLOOP_LIBRARY NAMES pulse-mainloop-glib libpulse-mainloop-glib HINTS "/usr/lib/x86_64-linux-gnu")

With that in mind I still lean in favor of renaming it to PulseAudio and also additionally exporting the uppercase CMake variables used in the above config file.

@SpaceIm
Copy link
Contributor

SpaceIm commented Oct 23, 2023

It turns out that there is a CMake config file upstream: https://github.com/pulseaudio/pulseaudio/blob/v16.99.1/PulseAudioConfig.cmake.in#L1-L12

So I suggest to not define cmake_find_mode, because there is no Find file, and to not define cmake_target_name, because there is no cmake target, so it could be any arbitrary name (why not pulseaudio::pulseaudio instead of PulseAudio::PulseAudio, so stick to default CMakeDeps name pulseaudio::pulseaudio since no one is authoritative):

self.cpp_info.set_property("cmake_file_name", "PulseAudio")
self.cpp_info.filenames["cmake_find_package"] = "PulseAudio"
self.cpp_info.filenames["cmake_find_package_multi"] = "PulseAudio"

Moreover you'll have to create these custom CMake vars, because they won't be defined out of the box by CMakeDeps, and existing libraries likely rely on them if you want frictionless integrations:

  • PULSEAUDIO_INCLUDE_DIR
  • PULSEAUDIO_LIBRARY
  • PULSEAUDIO_VERSION_MAJOR
  • PULSEAUDIO_VERSION_MINOR
  • PULSEAUDIO_VERSION_STRING

And finally, you'll have to submit a PR to aws-sdk-cpp recipe, because a patch relies on previous default cmake_file_name of pulseaudio recipe.

@conan-center-bot

This comment has been minimized.

@valgur valgur marked this pull request as ready for review November 4, 2023 11:58
@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot
Copy link
Collaborator

Conan v1 pipeline ❌

Failure in build 6 (dfc851d04fc87fd869b3156efde49030074cfa82):

  • pulseaudio/16.1:
    Didn't run or was cancelled before finishing

  • pulseaudio/15.0:
    CI failed to create some packages (All logs)

    Logs for packageID 18cb7a9e234e37e13d2a1e34b539bcd9c3819e0c:
    [settings]
    arch=x86_64
    build_type=Release
    compiler=gcc
    compiler.libcxx=libstdc++11
    compiler.version=10
    os=Linux
    [options]
    pulseaudio:shared=True
    
    [...]
    -- Found: /home/conan/w/prod-v1/bsr/44887/acbcb/.conan/data/openssl/3.1.4/_/_/package/0020bc4112335790e43a736facc055c3337d4107/lib/libcrypto.a
    -- Library ssl found /home/conan/w/prod-v1/bsr/44887/acbcb/.conan/data/openssl/3.1.4/_/_/package/0020bc4112335790e43a736facc055c3337d4107/lib/libssl.a
    -- Found: /home/conan/w/prod-v1/bsr/44887/acbcb/.conan/data/openssl/3.1.4/_/_/package/0020bc4112335790e43a736facc055c3337d4107/lib/libssl.a
    -- Library z found /home/conan/w/prod-v1/bsr/44887/acbcb/.conan/data/zlib/1.3/_/_/package/19729b9559f3ae196cad45cb2b97468ccb75dcd1/lib/libz.a
    -- Found: /home/conan/w/prod-v1/bsr/44887/acbcb/.conan/data/zlib/1.3/_/_/package/19729b9559f3ae196cad45cb2b97468ccb75dcd1/lib/libz.a
    -- Configuring done
    -- Generating done
    -- Build files have been written to: /home/conan/w/prod-v1/bsr/cci-e0b03637/recipes/pulseaudio/all/test_v1_package/build/667cd6c612ff5fd372a071668f1cc1f532697bdb
    
    ----Running------
    > cmake --build '/home/conan/w/prod-v1/bsr/cci-e0b03637/recipes/pulseaudio/all/test_v1_package/build/667cd6c612ff5fd372a071668f1cc1f532697bdb' '--' '-j3'
    -----------------
    Scanning dependencies of target test_package
    [ 50%] Building C object test_package/CMakeFiles/test_package.dir/test_package.c.o
    CMake Warning:
      Manually-specified variables were not used by the project:
    
        CMAKE_EXPORT_NO_PACKAGE_REGISTRY
        CMAKE_INSTALL_BINDIR
        CMAKE_INSTALL_DATAROOTDIR
        CMAKE_INSTALL_INCLUDEDIR
        CMAKE_INSTALL_LIBDIR
        CMAKE_INSTALL_LIBEXECDIR
        CMAKE_INSTALL_OLDINCLUDEDIR
        CMAKE_INSTALL_SBINDIR
    
    
    /home/conan/w/prod-v1/bsr/cci-e0b03637/recipes/pulseaudio/all/test_package/test_package.c:2:10: fatal error: pulse/pulseaudio.h: No such file or directory
        2 | #include <pulse/pulseaudio.h>
          |          ^~~~~~~~~~~~~~~~~~~~
    compilation terminated.
    make[2]: *** [test_package/CMakeFiles/test_package.dir/build.make:82: test_package/CMakeFiles/test_package.dir/test_package.c.o] Error 1
    make[1]: *** [CMakeFiles/Makefile2:113: test_package/CMakeFiles/test_package.dir/all] Error 2
    make: *** [Makefile:103: all] Error 2
    pulseaudio/15.0 (test package): WARN: 
         ************************************************
         The 'cmake_find_package_multi' generator is deprecated.
         Please update your code and remove it.
         *************************************************
    
    pulseaudio/15.0 (test package): WARN: 
         ************************************************
         The 'cmake' generator is deprecated.
         Please update your code and remove it.
         *************************************************
    
    pulseaudio/15.0 (test package): WARN: **** The 'from conans import CMake' helper is deprecated. Please update your code and remove it. ****
    ERROR: pulseaudio/15.0 (test package): Error in build() method, line 12
    	cmake.build()
    	ConanException: Error 2 while executing cmake --build '/home/conan/w/prod-v1/bsr/cci-e0b03637/recipes/pulseaudio/all/test_v1_package/build/667cd6c612ff5fd372a071668f1cc1f532697bdb' '--' '-j3'
    
  • pulseaudio/14.2:
    Didn't run or was cancelled before finishing

  • pulseaudio/13.0:
    Didn't run or was cancelled before finishing

  • pulseaudio/14.0:
    Didn't run or was cancelled before finishing


Note: To save resources, CI tries to finish as soon as an error is found. For this reason you might find that not all the references have been launched or not all the configurations for a given reference. Also, take into account that we cannot guarantee the order of execution as it depends on CI workload and workers availability.

@ghost ghost mentioned this pull request Nov 16, 2023
3 tasks
@franramirez688 franramirez688 self-assigned this Nov 16, 2023
@ghost ghost mentioned this pull request Jan 19, 2024
3 tasks
@valgur
Copy link
Contributor Author

valgur commented Jan 22, 2024

Closing in favor of #22441.

@valgur valgur closed this Jan 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants