Skip to content

Conversation

@hasezoey
Copy link
Contributor

This PR adds a way to properly dynamic link without requiring a custom environment variable, let the linker find the correct library.

Also added a explicit feature for bundling libopus, which in turn made it possible to have cmake be optional.
This new bundled feature is enabled by default, to preserve previous behavior.
Though due to this new feature, i made it overwrite dynamic linking, as i dont know of a way to actually know if the library exists and then decide to build it or not. If wanted, i can re-add some kind of way to at least check OPUS_LIB_DIR with the default feature.

Also fixes some slight formatting and fixes clippy warnings.

@DoumanAsh
Copy link
Owner

Also fixes some slight formatting and fixes clippy warnings.

?????
rustfmt should be disabled though https://github.com/DoumanAsh/opusic-sys/blob/master/.rustfmt.toml

In any case, this is normally not appropriate as by default most users wouldn't have library available (especially on windows)
Most of the time library look up is implemented via third party tools (pkg-config on Linux systems and vpkg on Windows)
That is why I never bother to implement it as I prefer static builds

If there is necessity I will take a look at these options myself later on I guess

@DoumanAsh
Copy link
Owner

Ok, looking at example of openssl-sys it seems people are fine with pkg-config/vcpkg being optional thingies, so I guess it should be fine

@hasezoey
Copy link
Contributor Author

hasezoey commented Oct 10, 2025

?????
rustfmt should be disabled though https://github.com/DoumanAsh/opusic-sys/blob/master/.rustfmt.toml

I did manual formatting, in this PR specifically, changed comments to be a consistent style (a space between // and the first word)

That is why I never bother to implement it as I prefer static builds

This PR does not remove the functionality, and also keeps it as the default; this PR is aimed at not requiring to set the custom environment variable, if it is already in PATH, which the linker can resolve.
I can understand it for windows, but at least for linux, bsd and macos, i think it is actually more common to dynamic link for bigger things (like libopus).
This is especially good for the use-case of a music player (which is our purpose we use this package for), also supports mpv (via libmpv; dynamic linked) as a backend, which also dynamic links against libopus, so we shouldnt need 2 libopus and we shouldnt need to specify a extra environment variable to use default linking paths.

In any case, this is normally not appropriate as by default most users wouldn't have library available (especially on windows)
Most of the time library look up is implemented via third party tools (pkg-config on Linux systems and vpkg on Windows)

I dont know much about rust ffi binding creation, but from what i can tell, i implemented what i though was common functionality (bundled vs dynamic), doing it similarly as the only ffi i had looked at which was simple (soundtouch-ffi). I had looked at libsqlite3-sys, but it looked way more complex than i wanted to implement.

@DoumanAsh DoumanAsh merged commit bca8258 into DoumanAsh:master Oct 11, 2025
3 checks passed
@DoumanAsh
Copy link
Owner

Yeah, it is fine, thank you
I will release new version soon, do you need update for opusic-c too?

@DoumanAsh
Copy link
Owner

I released 0.5.8 with additional change to customize link mode to static via env variable

@DoumanAsh
Copy link
Owner

In case you use my wrapper, I released new version 1.5.4 to include bundled feature too
https://github.com/DoumanAsh/opusic-c/blob/master/Cargo.toml#L18

@hasezoey hasezoey deleted the properDynamicLink branch October 11, 2025 08:01
@hasezoey
Copy link
Contributor Author

Thanks for merging.

I will release new version soon, do you need update for opusic-c too?

I think a update there is also good, though we use opusic-sys directly through symphonia-adapter-libopus.

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.

2 participants