WebPlatformForEmbedded / libwpe

General-purpose library specifically developed for the WPE-flavored port of WebKit.
BSD 2-Clause "Simplified" License
46 stars 37 forks source link

Install loadable backend modules to a separate directory #59

Open mgorse opened 4 years ago

mgorse commented 4 years ago

Filing after some discussion on IRC. We rely on libWPEBackend-fdo.so-1.0.so being installed in the standard library directory, but distros typically don't install unversioned .so files in the standard library directory except in -devel packages. We should consider moving the plugin to its own directory.

mgorse commented 4 years ago

Not sure if I should have filed this against the FDO backend, although a solution could potentially affect libwpe as well. Pasting some IRC discussion:

mcatanzaro: I have a question about webkitgtk. We're trying to build webkitgtk on openSUSE and enable WPE. Webkitgtk calls wpe_loader_init("libWPEBackend-fdo-1.0.so"). We always have these unversioned .so files in the -devel packages, so wpe_init isn't finding the .so in our case, since it would need libWPEBackend-fdo-1.0.so.1. Should I file a webkit bug for this?

  • mcatanzaro wonders how this could possibly work on any distro mgorse: you should really install the inversioned .so with the normal wpebackend-fdo package, not the -devel one it's more of a loadable plug-in the same way you would package the libmod_foo.so files that implement Apache modules
  • mcatanzaro discovers we have WPE renderer disabled in Fedora, oops aperezdc: I kinda think they should be installed to a private location then, not the global libdir probably No distro is going to want unversioned .so in the global libdir, except in devel packages but then we need wpe_loader_init() to know about such a directory as well... I guess not a big deal we'll write the module directory in the libwpe .pc file then when a module is built, we pick the path from there to decide where to install also, each backend (e.g. wpebackend-fdo) needs its .pc file to include a -L in the linker flags It could work But remember it's not only intended to be dlopened as a plugin, we also have apps linking directly, so it's a weird mix... I'm not sure if I'm aware of any other software used this way OK so what is our answer for mgorse going to be... besides "oops" yes, that's why we need to have the -L as well Moving everything around will be disruptive, to say the least little bit, yeah I can just make a patch to modify the call, but might not be exactly what you want upstream
aperezdc commented 4 years ago

This will need changes in libwpe as well, and that's why I suggested to file the bug here. I think that the solution outlined in our chat earlier today would work. Fleshing it out a bit more in detail, it would be something like this:

This only needs internal changes in libwpe and minor changes the build system for the backends. Applications do not need any code changes, only a rebuild.

@zdobersek WDYT?

mcatanzaro commented 4 years ago

We accidentally pushed out a bad update to Fedora stable users due to this bug. :/

mcatanzaro commented 4 years ago

We accidentally pushed out a bad update to Fedora stable users due to this bug. :/

Ah sorry, it actually was still in testing... we caught it in time. :)

mcatanzaro commented 4 years ago

Any plans to work on this?

In retrospect, had we caught this during original package review, it would have been a failed review. Too late now....

aperezdc commented 4 years ago

Any plans to work on this?

I'll try to get it done for 1.6.x :man_factory_worker:

aperezdc commented 4 years ago

As discussed in a chat with @zdobersek: we are a bit late in the development cycle for 1.6 and while this seems safe to do we are wary that it might break backends and/or applications in subtle ways, so let's have 1.8 as target and merge the changes to master after branching for the 1.6 release.

mcatanzaro commented 4 years ago

I'm looking at SUSE's patch for this bug:

diff --git a/Source/WebKit/UIProcess/glib/WebProcessPoolGLib.cpp b/Source/WebKit/UIProcess/glib/WebProcessPoolGLib.cpp
index a380a25fa4b..d96d23bce2e 100644
--- a/Source/WebKit/UIProcess/glib/WebProcessPoolGLib.cpp
+++ b/Source/WebKit/UIProcess/glib/WebProcessPoolGLib.cpp
@@ -114,7 +114,7 @@ void WebProcessPool::platformInitializeWebProcess(const WebProcessProxy& process
 #if PLATFORM(WAYLAND)
     if (WebCore::PlatformDisplay::sharedDisplay().type() == WebCore::PlatformDisplay::Type::Wayland) {
 #if USE(WPE_RENDERER)
-        wpe_loader_init("libWPEBackend-fdo-1.0.so");
+        wpe_loader_init("libWPEBackend-fdo-1.0.so.1");
         if (wpe_fdo_initialize_for_egl_display(WebCore::PlatformDisplay::sharedDisplay().eglDisplay())) {
             parameters.hostClientFileDescriptor = wpe_renderer_host_create_client();
             parameters.implementationLibraryName = FileSystem::fileSystemRepresentation(wpe_loader_get_loaded_implementation_library_name());

which is... honestly, more elegant than installing an unversioned .so. I think I prefer that solution. But it begs the question: why not just link WebKitGTK directly to wpebackend-fdo? Then the problem disappears, and there is no need to install any unversioned .so.

Obviously WPE port can't do this, but WPE port is not attempting to dlopen a hardcoded .so anyway, so doesn't matter.

aperezdc commented 4 years ago

Changed target to 0.10.x, we are late in the development cycle to introduce a change such as this.

mcatanzaro commented 4 years ago

which is... honestly, more elegant than installing an unversioned .so. I think I prefer that solution. But it begs the question: why not just link WebKitGTK directly to wpebackend-fdo? Then the problem disappears, and there is no need to install any unversioned .so.

How about that?

mcatanzaro commented 4 years ago

I'm looking at SUSE's patch for this bug:

I'm moving libWPEBackend-fdo.so-1.0.so back to the -devel package in Fedora, and adding Mike's patch to our WebKitGTK package:

diff --git a/Source/WebKit/UIProcess/glib/WebProcessPoolGLib.cpp b/Source/WebKit/UIProcess/glib/WebProcessPoolGLib.cpp
index a380a25fa4b..d96d23bce2e 100644
--- a/Source/WebKit/UIProcess/glib/WebProcessPoolGLib.cpp
+++ b/Source/WebKit/UIProcess/glib/WebProcessPoolGLib.cpp
@@ -114,7 +114,7 @@ void WebProcessPool::platformInitializeWebProcess(const WebProcessProxy& process
 #if PLATFORM(WAYLAND)
     if (WebCore::PlatformDisplay::sharedDisplay().type() == WebCore::PlatformDisplay::Type::Wayland) {
 #if USE(WPE_RENDERER)
-        wpe_loader_init("libWPEBackend-fdo-1.0.so");
+        wpe_loader_init("libWPEBackend-fdo-1.0.so.1");
         if (wpe_fdo_initialize_for_egl_display(WebCore::PlatformDisplay::sharedDisplay().eglDisplay())) {
             parameters.hostClientFileDescriptor = wpe_renderer_host_create_client();
             parameters.implementationLibraryName = FileSystem::fileSystemRepresentation(wpe_loader_get_loaded_implementation_library_name());

That should suffice until we have an upstream solution. Still seems like a good upstream solution would be to link directly to WPEBackend-fdo as dlopening doesn't make a ton of sense when WebKitGTK only supports a single choice of backend....

aperezdc commented 3 years ago

[...]

That should suffice until we have an upstream solution. Still seems like a good upstream solution would be to link directly to WPEBackend-fdo as dlopening doesn't make a ton of sense when WebKitGTK only supports a single choice of backend...

WebKitGTK is already being linked directly to WPEBackend-fdo because it uses its exportable interface API. See:

% readelf -d /usr/lib/libwebkit2gtk-4.0.so|grep fdo     
 0x0000000000000001 (NEEDED)             Shared library: [libWPEBackend-fdo-1.0.so.1]
%

It turns out that it's not that easy and we still need to initialize libwpe with the name of the backend ELF binary, so dlopen() can be used on it to obtain the backend's _wpe_loader_interface which is used as factory to create e.g. view backend and renderer objects.

mcatanzaro commented 3 years ago

OK.

As discussed in a chat with @zdobersek: we are a bit late in the development cycle for 1.6 and while this seems safe to do we are wary that it might break backends and/or applications in subtle ways, so let's have 1.8 as target and merge the changes to master after branching for the 1.6 release.

Anyway, we missed the train on 1.8, which is branched now. Ready for master?

mcatanzaro commented 2 years ago

BTW we wound up doing this, which solved the immediate problem here. Moving things to a separate directory would still be good to do, of course, but the status quo is not broken at least.