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

[Linux/BSD] Include headers for dynamically loaded libraries to simplify build dependencies. #71263

Merged
merged 1 commit into from
Jan 23, 2023

Conversation

bruvzg
Copy link
Member

@bruvzg bruvzg commented Jan 12, 2023

Includes headers and licensing information for the libraries wrapped by https://github.com/hpvb/dynload-wrapper and dynamically loaded.

  • Simplify build dependencies, since libs are loaded dynamically, there's no reason to have the full library (and all of its dependencies) on the system used to build.
  • Ensures that wrappers and headers have the same version, Ubuntu 18.04 LTS (bionic) versions are used.
  • Adds missing licensing and version information for dynamically loaded libraries.

Fixes #67863 (and similar issues with build environment setup).

@hpvb
Copy link
Member

hpvb commented Jan 12, 2023

This is a good idea! I like it.

@bruvzg bruvzg marked this pull request as ready for review January 12, 2023 13:42
@bruvzg bruvzg requested review from a team as code owners January 12, 2023 13:42
@Riteo
Copy link
Contributor

Riteo commented Jan 18, 2023

I wonder if at this point dynload-wrapper could make (at least almost) all-in-one headers for that, GLAD style.

@Riteo
Copy link
Contributor

Riteo commented Jan 19, 2023

Why isn't X11 in there? It's dynamically loaded now.

Edit: note that if you try to regenerate the wrappers you'll stumble on a lot of issues, so I'd recommend not touching them for now.

@akien-mga
Copy link
Member

I think this is great!

The only thing I'm unsure of is the whole license/copyright documentation situation. If we're vendoring the headers it definitely makes sense to include a license file next to the headers to make it clear what terms this specific code is under.

But I don't see how using library headers from a vendored copy, or the ones provided on the build environment, would differ in practice - the same header code gets included in the Godot binary. So if we weren't documenting the license terms of all dlopen'ed libraries or system headers used by Godot code in COPYRIGHT.txt before, I don't think we ought to document them now.

It does make me doubt whether we should actually have been documenting these licenses all along, and that's something I'll try to get clarified with open source licensing experts. If we have to, I've basically never seen a project do this ever, whether open source or proprietary, so this would be an interesting situation.

So for this PR I'd suggest:

  • Move all Linux library headers in the same folder to avoid adding too many folders in thirdparty. Maybe thirdparty/linuxbsd_headers/.
  • Document the versions etc. of those libraries in thirdparty/README.md all together under the ## linuxbsd_headers section. Can be kept simple like for the ## misc section since the update instructions are trivial and don't need to be mentioned.
  • Don't list them in COPYRIGHT.txt.

As a side note, would be worth checking if we really needed to include the full set of headers or if we can get away with only a subset (i.e. the ones we include directly + their dependencies, if that's not the whole set).

@bruvzg bruvzg force-pushed the linux_sys_headers branch from 6941278 to 8184192 Compare January 23, 2023 14:13
@bruvzg
Copy link
Member Author

bruvzg commented Jan 23, 2023

Added X11 headers, moved headers to thirdparty/linuxbsd_headers/ alongside with own README file to list versions, and removed from COPYRIGHT.txt.

@bruvzg bruvzg force-pushed the linux_sys_headers branch from 8184192 to 5c4fe63 Compare January 23, 2023 14:37
@akien-mga akien-mga merged commit e6bd9c1 into godotengine:master Jan 23, 2023
@akien-mga
Copy link
Member

Thanks!

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.

DisplayServer.tts_get_voices() returns empty on Linux with official binaries
4 participants