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

Add libsndio recipe #23087

Merged

Conversation

spiderkeys
Copy link
Contributor

@spiderkeys spiderkeys commented Mar 13, 2024

libsndio/1.9.0

The current libsndfile recipe has a line:

tc.variables["CMAKE_DISABLE_FIND_PACKAGE_Sndio"] = True  # FIXME: missing sndio cci recipe (check whether it is really required)

This was ultimately causing issues for me within my machine's environment, for partially unknown reasons. I described my findings here (meant to file the issue here, though):
conan-io/conan#15240

By adding this sndio recipe, the complete chain of deps when building ffmpeg in a configuration that pulls in pulseaudio can be resolved (ffmpeg->pulseaudio->libsndfile->libsndio), and I was able to get the test to pass using the libsndio package.

I know that PRs should only change one recipe at a time, but I've currently put in the commit that adjusts libsndfile as well, for visibility and in case I'm missing something. I will remove this if this PR moves forward/gets buy-in.

I also haven't written a test-package specifically for this recipe yet, but will also do that once any issues are addressed. I have tested it indirectly by doing a conan create --version 6.1 for ffmpeg which had previously been exhibiting my linked issues.

libsndio is a little weird compared to other projects I've written recipes for in that it has a handwritten configure file and no configure.ac, so the standard approach for using the AutoToolsToolchain didn't really work. I have very little experience writing autotools recipes, so please let me know if there are ways to improve this.


^- will do this at some point

@CLAassistant
Copy link

CLAassistant commented Mar 13, 2024

CLA assistant check
All committers have signed the CLA.

@conan-center-bot

This comment has been minimized.

@AbrilRBS
Copy link
Member

AbrilRBS commented Mar 14, 2024

Thanks! The bad thing about modifying recipes/libsndfile/all/conanfile.py the current CI will complain and it won't try to compile the new recipe - I'd recommend removing that change from this PR and focusing on getting this merged knowing that we need to additionally check ffmpeg :)

Also don't forget to request access in #4 so that the PR gets built by the CI

@AbrilRBS AbrilRBS self-assigned this Mar 14, 2024
@spiderkeys
Copy link
Contributor Author

spiderkeys commented Mar 14, 2024

Thanks @RubenRBS. Just requested access and removed the commit against libsndfile.

The changes to libsndfile can still be seen here:
https://github.com/conan-io/conan-center-index/compare/3f9f55c087fea66e9f44ac45d8fc9db726bddffc..bfe2c4397145d979432441bf73ec267846611cc4

@conan-center-bot

This comment has been minimized.

1 similar comment
@conan-center-bot

This comment has been minimized.

recipes/libsndio/all/conanfile.py Outdated Show resolved Hide resolved
@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@spiderkeys
Copy link
Contributor Author

Added a test package for conan v2. It passes, but it seems there is a missing dependency still:

======== Testing the package: Executing test ========
libsndio/1.9.0@spiderkeys/test (test package): Running test()
libsndio/1.9.0@spiderkeys/test (test package): RUN: ./test_package
ALSA lib /home/conan/w/prod-v2/bsr/9663/cafde/p/b/libal9a348007750da/b/src/src/conf.c:4028:(snd_config_hooks_call) Cannot open shared library libasound_module_conf_pulse.so (//lib/alsa-lib/libasound_module_conf_pulse.so: cannot open shared object file: No such file or directory)
ALSA lib /home/conan/w/prod-v2/bsr/9663/cafde/p/b/libal9a348007750da/b/src/src/pcm/pcm.c:2675:(snd_pcm_open_noupdate) Unknown PCM hw:0
couldn't open play stream: No such file or directory
sio_open() failed

This libasound_module_conf_pulse.so appears to be an alsa plugin which is provided by:
https://github.com/alsa-project/alsa-plugins

@conan-center-bot

This comment has been minimized.

@spiderkeys
Copy link
Contributor Author

No idea why the v1 pipeline is failing. I've run conan create for the package in conan docker containers with both conan v1.60 and conan2 (in conanio/gcc11-ubuntu16.04:latest) and the build+test succeeds...

Co-authored-by: ericLemanissier <ericLemanissier@users.noreply.github.com>
@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@spiderkeys spiderkeys changed the title [WIP] Add libsndio recipe Add libsndio recipe Mar 15, 2024
@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@AbrilRBS
Copy link
Member

AbrilRBS commented Apr 1, 2024

Feel free to ping once you've addressed @valgur review :)

Copy link
Member

@memsharded memsharded left a comment

Choose a reason for hiding this comment

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

It looks good.

sources:
"1.9.0":
"source":
url: "https://github.com/ratchov/sndio/archive/refs/tags/v1.9.0.tar.gz"
Copy link
Member

Choose a reason for hiding this comment

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

Why the sources from https://sndio.org/install.html (https://sndio.org/sndio-1.9.0.tar.gz) aren't used? It seems they say their sources work in Linux, it is necessary to use this Github fork instead of the original project sources?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had originally used those sources, but found they didn't include the license file. I thought the GitHub release would because I saw the license file on the main branch, so I switched to GitHub sources. It turned out that the license file had been added in a commit later than the latest release, though, so it ended up not mattering.

I can switch this back to the sndio.org sources, if it is preferable. They should be the same.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's worth noting that Alexandre Ratchov is one of the authors of sndio though, which is why I thought it would be acceptable. Their fork/repo is the only place where I could find a complete license file that includes all authors.

Copy link
Member

Choose a reason for hiding this comment

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

I have approved it, sorry for the delays, it is very difficult to keep up with everything.
I think in the future it would make sense to use the official URLs, but that is not critical, I preferred to approve to not further delay it, and it can be changed in the future.

Many thanks again for this contribution!

spiderkeys and others added 2 commits April 2, 2024 11:49
@spiderkeys
Copy link
Contributor Author

I've added the suggested changes from @valgur 's review. @memsharded let me know if you want the source URL changed, based on the above conversation, otherwise I think this should be ready to land.

@conan-center-bot
Copy link
Collaborator

Conan v1 pipeline ✔️

All green in build 15 (61bab76d4eb58672c9433533029dd407ca9c7055):

  • libsndio/1.9.0:
    All packages built successfully! (All logs)

Conan v2 pipeline ✔️

Note: Conan v2 builds are now mandatory. Please read our discussion about it.

All green in build 15 (61bab76d4eb58672c9433533029dd407ca9c7055):

  • libsndio/1.9.0:
    All packages built successfully! (All logs)

@conan-center-bot conan-center-bot merged commit ae128e4 into conan-io:master Jul 12, 2024
14 checks passed
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.

8 participants