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

Use system libs when available #81

Closed
wants to merge 4 commits into from

Conversation

linkmauve
Copy link
Contributor

These two commits make cmake check for system GLFW and GLEW, and build against these if they are found. This removes quite a lot of direct dependencies, on X11 especially when the system GLFW is built against Wayland, and makes updates easier.

@alkama
Copy link
Contributor

alkama commented Oct 6, 2018

Hello linkmauve,

I'm not so sure about that PR. The fact it checks for system GLFW makes it troublesome for mac where the current release of those libs might not be optimal to support latest system updates. Ex: only master branch of glfw upcoming 3.3 is solving some fullscreen problems.
One might also want to not have a single version of the library.
We currently onboard the source and include its cmake so I believe options to fine tune it are available. I might be wrong but i think your problem with Wayland is just a matter for you to set the correct fields during CMake setup so that the resulting glfw library is adapted to your tastes / os flavour.

All in all. If one really want to change linux configs, please try to restrict those changes to that platform so that windows and mac stay functional.

@linkmauve
Copy link
Contributor Author

I know I could change the options, but (at least on Linux) it’s always easier to install just one version of a library rather than to configure it in every single software which may use it. If you require a specific version, I changed it to require glfw3>=3.3 so it will fallback to your vendored version if the user doesn’t have this version installed. This fallback should work on all platforms, so if e.g. OS X users have 3.2.1 installed this PR will fallback to the version in external/.

I could add an option to not even try pkg-config, and instead always use the version in external/, this could be useful for developers and CI who want to test both versions, but I doubt it would be useful for users.

Note: I’m the maintainer of GLFW’s Wayland backend.

@alkama
Copy link
Contributor

alkama commented Oct 6, 2018

I'm sorry I probably wasn't clear.

TLDR: I have systemwide glfw3 and I DONT want to use it for Bonzomatic (but need it for other apps because of decisions like those).

Unless you own a mac / windows and are ready to maintain those flavours, please dont make assumptions. Your testing of unreleased glfw3.3 doesn't help in any way for the problem I was talking about (because I can bet that it'll be the same with 3.4). It's great to fix a problem you face, but lets just try to not break others :)

If you want to go with that PR (because maybe it's optimal for linux), please make your check for library in the linux #if branch. If it breaks it for linux users and they come to complain we can forward the debates to you and you can argue about the pro and cons of your solution.

For now, your solution is impacting the flavour I use, in a negative way, and bans me from easy building head glfw binaries that work with 10.13 and 10.14.
And no, i dont want to swap my systemwide glfw library, or have to alter my own cmakefile because someone decided to break it for me :D

note: I'm not the maintainer, I'm just a PRer like you that just wants his flavor to be simple to build and use.

@Gargaj
Copy link
Owner

Gargaj commented Oct 6, 2018

I'd rather build with the GLFW pinned in the repo; that way it's less likely someone builds a broken version for themselves because their current system version has a change that we don't know about. From the perspective of project integrity it's a much better solution.

@linkmauve
Copy link
Contributor Author

I replaced the new option with an option to force the system-wide version, made it disabled by default, and added a note that it is unsupported by upstream. This should be reserved to people and distributions who want to use their normal system GLFW and GLEW.

Is this acceptable, even if it isn’t specific to Linux? I indeed don’t have a way to check on other systems (except for Wine), but if I ever have to use one of those I’d also much rather use their packages than an undetermined vendor version, so I know exactly what I am building against.

This also isn’t only a problem I face, it is a problem I’ve heard most distribution maintainers complain about for various software, they generally add this kind of patch downstream, which is a waste of resource to do in every single distribution.

@alkama
Copy link
Contributor

alkama commented Oct 6, 2018

I voiced my opinion on that PR.
I'm against systemwide decisions when solving local problems unless people understand what it means for every platform. And this obviously means more than surface things (for the mac platform at least) that are imho overlooked here. And no, adding a default option for mac doesn't help, it just underlines the overlook.

@linkmauve
Copy link
Contributor Author

Why should mac be a special case here? I understand your system’s GLFW copy is broken in some way, but wouldn’t it make sense to get your vendor to fix that instead of making every software build their own version, each which could be similarly broken and with no way to centrally fix it for all of them?

Also with the latest version of this PR, the only change there is (for mac and every other system) is a new discouraged option which, if enabled, will try to find a system-wide GLFW (resp. GLEW), in every other case the vendor version is still used.

@alkama
Copy link
Contributor

alkama commented Oct 6, 2018

You're right, I will call Apple immediately and keep you updated.

@alkama
Copy link
Contributor

alkama commented Oct 6, 2018

So when this option is checked, i guess mac builds will have to handle glfw versions 3.0, 3.0.1, 3.0.2, 3.0.2, 3.0.3, 3.0.4, 3.1, 3.1.1, 3.1.2, 3.2, 3.2.1, upcoming 3.3 and all its siblings.
I cant wait to do ifdef for glfw versions inside the platform code.

If code ever succeeds in building with any an all of those and produce a binary, any side effect seen by user of said binary and coming to us would have to be identified and handled, right?
Or we will have to code a visible notification on all systems to tell the user he's using a build that is not supported?

On version 3.2.x and less, for example, the fact that systemwide libs might be built with retina support and use frame buffers twice larger than the user requested and result in strange performance losses (because shaders are usually demanding), would need to be assessed?
Or a system install of glfw with chdir burnt-in, resulting in the whole app looking for its config, textures and all assets from an obfuscated location (inside the app bundle). That would make for an indistinguishable binary that would build and run, but make for poorly usable binary, and a user coming back here to submit an issue.

I'm trying to get you to understand that it's excellent that you fix things for your platform, you master it. But it's a bad idea to try to enforce your choices unless you're sure they're safe and without side effects.
Disabled or not, with or without warning for the builder, I see that option HAS side effects for the mac (and maybe windows, i dont know) platforms.

All I (a mere user) asked was that you limit your changes to the linux flavour, the one you master, and let others chose what's better for them.

@alkama
Copy link
Contributor

alkama commented Oct 6, 2018

And as context. We come from SDL, we've had that pain asking users to have libs installed, locating the right lib version and have it working everywhere across systems. Maybe linux is better at that but we're working cross-platform here, and other systems are not that fun. Embedding lock-stepped libs and building them is a conscious decision. It saves everyone's time and comes with guarantees.

@linkmauve
Copy link
Contributor Author

Hi, I still disagree that Windows or OS X users will never ever want to use their system libraries, but for the sake of making this go forward I made the two new options only available on UNIX AND (NOT APPLE), this can be relaxed later.

@linkmauve
Copy link
Contributor Author

Hi, any news on this?

@Gargaj
Copy link
Owner

Gargaj commented Jan 14, 2019

I'm officially undecided on this because my focus is pretty much only on the Windows build and since there's no system lib for GLFW in Windows, it affects me fairly little.

@PoroCYon
Copy link
Contributor

I guess I'll comment because I somehow got to be the "de-facto Linux maintainer".

I don't think it's entirely a bad idea (because I have to do similar things to make it work using Wayland directly, and it might also fix parts of #82 ), but there are a few issues Alkama highlighted (but maybe exaggerated a little imo):

  • The default 'code' run by CMake should stay the same on all platforms (to avoid things breaking on eg. Windows when pkg-config isn't installed, which might happen because MS are sniffing glue as usual), so the find_package(PkgConfig) etc. should be put inside the if.
  • Of course, supporting multiple versions of GLFW is too much effort, so if the system version isn't compatible with the version Bonzomatic depends on, though luck. However, there might still be people who'd think it's our fault if it doesn't work with a different version, so that needs to be clear when turning on that flag.
  • Then there was this silly discussion Alkama and I had on IRC about 'supporting' system libs of the other dependencies, too. I guess that's possible in principle (if it isn't enabled by default, does nothing on other platforms, etc...), but having to add all these flags right away is a bit stupid, too. (And some libs like stb, mini_al, kiss_fft aren't often available to begin with.) So about this, ¯_(ツ)_/¯

@alkama
Copy link
Contributor

alkama commented Jan 17, 2019

My comment: do what you want. Eventually, if the CMakefile becomes too encumbered because of platform deviations, I'll split it into "per platform" pieces and just include the right one from the main file so that each maintainer takes care of his file without having to bother.

@sacredbanana
Copy link
Contributor

Is there such thing as a Gradle, Maven etc for C++ projects? It makes dependency management of specific versions of libraries so much easier and you don’t need the library files itself in the repo

@linkmauve
Copy link
Contributor Author

Is there such thing as a Gradle, Maven etc for C++ projects? It makes dependency management of specific versions of libraries so much easier and you don’t need the library files itself in the repo

It wouldn’t be of any use for the purpose of this PR anyway, it exists in order to simplify packaging in distributions, which will want to use the system version in any case. It would be possible to apply this patch downstream in every distribution, but it’s a lot better to have it upstream to avoid duplicating efforts.

@alkama
Copy link
Contributor

alkama commented Jun 10, 2019

Still doesn't cover the will (or the refusal) to support different versions of the same lib in the main code, because some systems will receive different versions from their package managers.
The preprocessor danse is already sad, would have been kinda fun with 3.2 vs 3.3.
And it could apply to everything else (good thing minial is single header... because oh, wait, it's miniaudio now and all calls have changed).

@AMDmi3
Copy link
Contributor

AMDmi3 commented Feb 20, 2021

Still doesn't cover the will (or the refusal) to support different versions of the same lib in the main code

I should note that not accepting this (or similar #147) change will not prevent package maintainers from unbundling dependencies, because it's the right thing to do and it's required by downstream policies (for instance, debian, fedora).

You shouldn't be too worried though, as possible issues caused by using different versions of dependencies would be handled by package maintainers too.

So right as @linkmauve says,

It would be possible to apply this patch downstream in every distribution, but it’s a lot better to have it upstream to avoid duplicating efforts.

Also note that #147 may have some improvements over this patch, namely

  • added similar handling for system stb
  • stricter reproducible behavior (if a flag to prefer system lib is set, that lib is REQUIRED to exist and always used without fallback to bundled one, which may cause inconsistent build results)
  • more modern glew handling via cmake bundled module

@alkama
Copy link
Contributor

alkama commented Feb 20, 2021

I can see how one could overlook the implications (After all you yourself missed the reason behind scintilla's inclusion by thinking it's just about our using of internal headers :D).
And btw, it'd be awesome to see a PR about migration to latest Scintilla!

Similarly and interestingly nobody answered the funnier "what happens when your package system requests glfw or glfw3 and get a "3.0" (or 3.1 or 3.2) because that's what user might have installed. Should we start to maintain code that adapts to all possible variants of those libs APIs?
Bundling libs is our way to ensure we can support the build of the project from source for everyone and guarantees the libs are in versions our code expects. Build is static so one shouldn't care much about whatever conflicting version is installed on system.
Having downstream patches in every distributions is convenient because when something breaks it'll be this distro's problem, not "ours". If the binary they end up having is misbehaving for unknown reasons, we can always recommend users to build from source from here instead. Inversely, if we adopt this, supporting it and ensuring it works everywhere will be "our" problem.

More specifically it will be @PoroCYon 's problem so in a way I'm only expressing my opinion here.
Plus if one can demonstrate me how finding package for glfw3 can guarantee to give 3.3 (or whatever version we might see fit), or one stb package finding to give the exact version of stb_image we use, I'd be happy to change my mind and side with you on this and your PR.

@PoroCYon
Copy link
Contributor

PoroCYon commented Feb 20, 2021

@alkama you don't seem to be able to think about the context of this patch: packaging distro versions, not random people compiling the source cloned from github by themselves

distros may package certain libraries differently, depending on their needs -- which they know better than us, so we can't push our will down their throats -- so they'd want to avoid using the vendored versions used in the repo. (Arch having both an X11 and a Wayland version of GLFW is one such example)

package maintainers DO know what they're doing, and are able to set specific versions of libraries as dependencies, and the system will take care of it. it won't be random breakage that will happen for an arbitrary number of end users, it'll be the same distro-wide.

package management software can set additional constraints to dependencies (using their database and package declaration files), so that way they can also ensure that "glfw3" will resolve to a good one. and when they do update to, say, 3.4, they can always add patches themselves -- we just give them a switch to do so. adding that switch isn't much of a maintenance burden in itself.

alkama, it'd be great if you would learn about how package manager software works (for example, here are the docs on how Void Linux packages are made), and how package maintainers do their job, before posting your kneejerk-sounding (in tone), ridiculing and slightly paternalistic/knowing-better-than-you reactions every time this issue is brought up

@alkama
Copy link
Contributor

alkama commented Feb 20, 2021

I stated my opinion and already said what I would do if it caused trouble for my branches 2 years ago.
So feel free to merge that PR.

(Note: when you introduce visible features, it's for everybody. Expect people to use what's available. You have no control upon whether this feature is used solely by package maintainers or not. At least in this PR, end users are warned by the option about the consequences. From experience, this wont stop seeing users in the future with issues we don't understand because they use mixed (possibly unsupported) versions of the vendor deps).

alkama added a commit to alkama/Bonzomatic that referenced this pull request Feb 20, 2021
Incorporates changes of @PoroCYon on PR Gargaj#125 (remove useless lexers)
Incorporates changes of @linkmauve on PR Gargaj#81 (use system libs)
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.
@alkama
Copy link
Contributor

alkama commented Mar 4, 2021

So version in #148 tries its best to at least hide those options from the end users and systematizes system libs across all dependencies and is updated to build with current master.
As was suggested, when a given system lib is selected, it's without fallback.

@linkmauve and @AMDmi3 feel free to adapt your PR to #148 if case you want to own the change. And/Or to review that it would fit your needs.

I'm convinced we'll see people checking those option boxes (even thought they're warned it's unsupported and options are marked "advanced"), but windows and macos wont see them so eventual problems will be limited to other OSes. I found no alternative enabling those options to exist for package maintainers but not for the rest of the users yet. But as I understand from the thread it's not a concern anyway, so I'm fine with it.

Also note that trying to use system-provided scintilla will obviously fail since our included one has some opengl specific wiring. But the option is there for when someone succeeds in porting to latest version. It should be harmless to try and just break at compilation with little chance to make a viable executable.

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.
@Gargaj Gargaj closed this Mar 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants