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 options to prefer system libs over some bundled ones #147

Closed
wants to merge 2 commits into from

Conversation

AMDmi3
Copy link
Contributor

@AMDmi3 AMDmi3 commented Feb 19, 2021

Dependency bundling is evil - it adds extra work for packages, introduces include/library conflicts, increases build times, prevents bug and security fixes in dependencies from affecting the product. Most package repositories (for instance, debian, fedora) have policies against dependency bundling.

This patch adds options to prefer system libraries over bundled ones for some dependencies (relatively widely packaged ones == these I have in my repository). Scintilla currently does not work as Bonzomatic seem to use internal headers. Merging at least 3 working ones would simplify my life as downstream package maintainer and hopefully help Bonzomatic make its way into more repositories.

Threads dependency was added because of this error which appeared after unbundling one of dependencies:

ld: error: undefined symbol: pthread_create
>>> referenced by FFT.cpp
>>>               CMakeFiles/bonzomatic.dir/src/platform_common/FFT.cpp.o:(ma_context_init)

With this patch, the application successfully builds and runs on FreeBSD.

@PoroCYon
Copy link
Contributor

Duplicate of #81 , issue is well-known. That one didn't get merged due to, I don't know why exactly anymore, sadly.

@Gargaj Gargaj closed this Feb 20, 2021
@AMDmi3
Copy link
Contributor Author

AMDmi3 commented Feb 20, 2021

@Gargaj doesn't this deserve an explanation?

@Gargaj
Copy link
Owner

Gargaj commented Feb 20, 2021

As Poro noted, there's a 18-comment thread over at #81 for this exact same discussion.

@AMDmi3
Copy link
Contributor Author

AMDmi3 commented Feb 20, 2021

As Poro noted, there's a 18-comment thread over at #81 for this exact same discussion.

Still not resolved, and this change contains extra code for stb. But OK, I'll move there.

@PoroCYon
Copy link
Contributor

A number of distros don't even package stb, and the Scintilla included here uses a bunch of modifications that renders using the system version not very useful.

alkama added a commit to alkama/Bonzomatic that referenced this pull request Feb 21, 2021
Incorporates changes of @PoroCYon on PR Gargaj#125 (remove useless lexers)
Incorporates changes of @linkmauve on PR Gargaj#81 (use system libs)

Generalize option for system libs to all external deps and whenever opting for system libs make finding it a requirement with no fallback to vendored like @AMDmi3 suggested on PR Gargaj#147 but use consistent and readable `pkg_search_module` for all libs and `find_path` for header-only deps.
Gargaj pushed a commit that referenced this pull request Mar 4, 2021
Incorporates changes of @PoroCYon on PR #125 (remove useless lexers)
Incorporates changes of @linkmauve on PR #81 (use system libs)

Generalize option for system libs to all external deps and whenever opting for system libs make finding it a requirement with no fallback to vendored like @AMDmi3 suggested on PR #147 but use consistent and readable `pkg_search_module` for all libs and `find_path` for header-only deps.
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.

3 participants