Load X11 dynamically

[Riteo]

The loaders have been generated through @hpvb's dynload-wrapper, although they had to be heavily handpatched to workaround some already reported issues with it. I added a note to each generated file to account for that.

As GLAD uses X11 stuff directly, I had to define the GLAD_GLX_NO_X11 macro to not let do it that, and handle myself the display loading and screen handling part myself, which wasn't that hard but it's still something worth saying.

I plan to improve greatly the X11 backend (including this aspect) but, as the release isn't that far and I'm also working on the Wayland backend, this will do for now, I hope.

As the patches are pretty big, I made a zip with the modified .so wrappers and the patches used to make them: x11-dynwrappers.zip

[Riteo]

Bruh it builds on both my machine and on my Debian partition, let me check.

[Riteo]

Ok so it looks like that this is choking on a type implemented in libX11 1.7.0, while we use... 1.6.9.

This is a certified bruh moment, let's see what I can do.

[akien-mga]

It might fail on the older Ubuntu from CI if the wrappers were generated from a more recent X11.
For maximum compatibility we likely need to generate the wrappers against an older X11 so that they don't include any of the recent APIs (since we still need these symbols to be in the actual shared library on the user's system).

[Riteo]

@akien-mga the thing is that I heavily handpatched this stuff and redoing all that work again would be quite a bit of a chore. I'll try removing the affected thing and if it compiles we should be OK, right?

I also actually think that 99% of the symbols here are probably unused but I've still done this for completeness for the time being. Note that this will be improved considerably going forward, but this is a pretty huge blocker for everything else that I'd like to get rid of.

[akien-mga]

the thing is that I heavily handpatched this stuff and redoing all that work again would be quite a bit of a chore.

You should probably generate a .patch file with the differences between the output from the generator and what you had to do to make it work. So we can redo those changes whenever we want to update - and possibly to be able to reapply those changes on an older version of X11 if this is confirmed to be needed.

You can indeed also try to remove APIs but you'd have to check the diff between headers down to the oldest version we want to support to see what was added and should be removed.

[Riteo]

@akien-mga I already described most of the important changes done in the note and I've already reported most of those issues to @hpvb. Perhaps a patch would be useful for improving the generator, but I doubt that it would be that useful (as it's mostly removing duplicate symbols, there are actually very few parsing issues) and I don't think that it'd be worth to keep in the source tree.

Edit: also this is supposed to be temporary-ish. The X11 API won't probably change that much if at all in the near future, the generator will be fixed and I'll keep on improving considerably this part in general, perhaps moving to XCB along with some other stuff I'll be experimenting on in the near future.

[Riteo]

Oh my I also added some extra files... One second

[Riteo]

There, I also removed all new symbols I could find making diffs from the old and new headers. This should now be ready to be reviewed again.

[dsnopek]

You should probably generate a .patch file with the differences between the output from the generator and what you had to do to make it work.

I already described most of the important changes done in the note and I've already reported most of those issues to hpvb. Perhaps a patch would be useful for improving the generator, but I doubt that it would be that useful (as it's mostly removing duplicate symbols, there are actually very few parsing issues) and I don't think that it'd be worth to keep in the source tree.

A patch file in the source tree would be most useful when someone else (ie. not you) wants to update the code with the latest output from the generator. The notes are nice, but being able to see the actual diff is clearer than any description. (It also makes this PR potentially easier to review, because the reviewers can see exactly what changes you had to make.)

Also, if the issues were reported to hpvb in public issues, including links to those in a text file or comments in the source code would also be helpful. That way, if someone is updating the code they can check the issues as a way to see if the changes you made are still necessary.

[Riteo]

@dsnopek The bug reports aren't yet public, I literally sent them through RC to hpvb. Now that I hand-downgraded them to an older version this will require even more work, but at this point I might give it a try. Let me see what I can do.

Edit: no wait, hpvb said that they would have fixed the main issue in this week, so I don't want to step on their feet. I think I'll just regenerate everything on the same version, handpatch them and make some patches, along with some more detailed patchnotes.

Edit2: also note that this was supposed to be a temporary-ish hack, but at this point I might go the extra mile to make something way more useful.

[Riteo]

There, I regenerated everything from the right version and added more detailed notes. I'm not sure where I should put the aforementioned patches though.

[Riteo]

Ok I now made a x11/dynwrappers directory with all the dynamic wrappers along with the diffs of the original, broken, .so wrappers. This should now be ready to be reviewed again.

[Riteo]

Rebased on top of master because my fork choked early on godot-cpp. I haven't investigated at all but I presume that it was a change in godot-cpp that had to be brought over this PR too.

[akien-mga]

Hm OK the patches are huge, I didn't expect so many changes. This should really be fixed in the wrapper tool as this will be hard to maintain going forward.

But I guess we can remove the patches from this PR then, and maybe just upload them to the PR description as reference.

The separate folder for wrappers is still a nice change.

[Riteo]

Removed the patches as requested.

Edit: I also added a ZIP file in the PR description.

[akien-mga]

Thanks!

[omar-polo]

This seems to have broken the build on OpenBSD.

The various *-so_wrap.c files includes the major in and this can't work. In -CURRENT libX11 is, for example, libX11.so.18.0, but this is (of course) subject to change on the next X11 update (if they break the ABI somehow)

I guess that these needs to be regen'd without the major?

[akien-mga]

The major is needed, .so files are symlimks only present when development packages are installed.

For systems which provide it anyway, we can fallback to it if the major version is not found. But then there's no guarantee that it will work - that's the whole point of a SONAME to mark that the compat is broken.

It would be easier if downstream platforms like OpenBSD didn't unilaterally change the SONAME of libraries (current upstream SONAME is libX11.so.6...) but well, that's a different debate.

Eventually, for platforms like *BSD and Nix which do not or cannot handle portable binaries, we should re-add build options to skip the wrappers and link the system libraries directly (like we have with the builtin_* options for non-dlopen'ed libraries).

[omar-polo]

OpenBSD doesn’t provide symlinks for libraries in base (like X11), and we actively remove them from packages too. I’m not involved in the maintaining of X11 in base, but I guess that the soname rules that applies to the rest of the system apply there too. Start at 0.0, bump minor when new functions get added, bump major when function are removed / arguments change / size of exported stuff changes / etc...

IMHO hardcoding the major at "source-generation time" instead of build time is strange. I'd like Godot to build in the future after a X11 update without waiting for you guys to ship a release with the regenerated dynwrappers. (without any offence to the work you’re doing; you rock!)

If Godot properly linked to libX11, or used dlopen() without the major, then the shipped package will be automatically updated when libX11 changes, without having to patch the sources. From the POV of a package maintainer this would be painless and reduce friction.

[omar-polo]

Eventually, for platforms like BSD and Nix which do not or cannot handle portable binaries, we should re-add build options to skip the wrappers and link the system libraries directly (like we have with the builtin_ options for non-dlopen'ed libraries).

If this "portable binaries" means setting ABI in stone I fear this won’t work at all on OpenBSD. Source-code compatibility is what is generally guaranteed, not binary. (we had at least two major bump of libc and libX11 in the last two years, and many more minor bumps.)

Would love to have an option to link to libraries like Godot 3 did, but I fear I'm not skilled enough to work on such a patch, apologies :(

[akien-mga]

If Godot properly linked to libX11, or used dlopen() without the major, then the shipped package will be automatically updated when libX11 changes, without having to patch the sources. From the POV of a package maintainer this would be painless and reduce friction.

Note that if Godot linked to libX11, you'd still need to recompile the Godot package whenever the libX11 soname changes in OpenBSD. So if a recompile is needed either way, currently you could patch the file to hardcode OpenBSD's soname (and hope it works against the parts of the API Godot relies on). It's of course not an elegant solution, just noting it as something that could work for now.

[omar-polo]

That's true, but bulk builds run every day so it's not a problem. Libraries gets updated all the time and packages rebuit/linked to the new versions. if a hypothetical new X11 update introduces a breaking change i'll wake up one morning finding a (failed) build log in my inbox :)
On the other hand, this yolo-dlopen-at-runtime approach means I need to periodically rebuild and test Godot. FWIW i prefer breaking at build-time if possible.

Now, this is just my preference from the POV of a package maintainer, I'm sure that if you've introduced this mechanisms there were sound reasons for it and I'm not arguing that. (to be fair i've never written software that supports the wide range of OSes that Godot does!)

I've seen the PR to make optional the use of these wrappers and I'm about to test it. Thanks a lot for considering this use case too and working for making it possible! :)