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

Install loadable backend modules to a separate directory #59

Closed
mgorse opened this issue Sep 19, 2019 · 18 comments · May be fixed by #64
Closed

Install loadable backend modules to a separate directory #59

mgorse opened this issue Sep 19, 2019 · 18 comments · May be fixed by #64
Assignees
Milestone

Comments

@mgorse
Copy link

mgorse commented Sep 19, 2019

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
Copy link
Author

mgorse commented Sep 19, 2019

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
Copy link
Contributor

aperezdc commented Sep 19, 2019

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:

  • At libwpe build time:
    • The location where backend modules will be loaded from is baked in the executable; <libdir>/wpe-<apiversion>/ (resulting in /usr/lib/wpe-1.0/ for typical configurations).
    • The location is also written to the backendsdir variable in the wpe-<apiversion>.pc file.
    • The location is also added to the linker flags in the wpe-<apiversion>.pc file (i.e. -L<lidir>/wpe-<apiversion>/). This would allow the linker to find backend library modules which export public API for applications to use (for example WPEBackend-fdo does this), when an application is being built.
  • When building a backend module (e.g. WPEBackend-fdo):
    • The module build system uses pkg-config to obtain the value of the backendsdir variable from libwpe's wpe-<apiversion>.pc file, and uses it for installation.
  • At run time:
    • Programs call wpe_loader_init() in the same way they already do, with the name of the backend module library as parameter. No changes needed.
    • When libwpe needs to load a backend module, it searches in the modules directory configured at build time, instead of letting the dynamic linker search for them in the standard locations.
    • Optional: For development builds, allow overriding the backend modules path using an environment variable e.g. WPE_BACKEND_LIBRARY_PATH.

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?

@aperezdc aperezdc changed the title Consider installing to a separate directory Install loadable backend modules to a separate directory Sep 19, 2019
@aperezdc aperezdc self-assigned this Sep 19, 2019
@mcatanzaro
Copy link
Contributor

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

@mcatanzaro
Copy link
Contributor

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
Copy link
Contributor

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 aperezdc added this to the 1.6.0 milestone Feb 25, 2020
@aperezdc
Copy link
Contributor

Any plans to work on this?

I'll try to get it done for 1.6.x 👨‍🏭

aperezdc added a commit that referenced this issue Feb 25, 2020
Make the backend loader search for implementations in the
<libdir>/wpe-<apiversion> directory (typically /usr/lib/wpe-1.0)
when a relative path is passed to wpe_loader_init().

Fixes #59
@aperezdc aperezdc modified the milestones: 1.6.0, 1.8.0 Feb 25, 2020
@aperezdc
Copy link
Contributor

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
Copy link
Contributor

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 added a commit that referenced this issue Jul 28, 2020
Make the backend loader search for implementations in the
<libdir>/wpe-<apiversion> directory (typically /usr/lib/wpe-1.0)
when a relative path is passed to wpe_loader_init().

Fixes #59
@aperezdc aperezdc modified the milestones: 1.8.0, 1.10.0 Sep 1, 2020
@aperezdc
Copy link
Contributor

aperezdc commented Sep 1, 2020

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

@mcatanzaro
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

[...]

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
Copy link
Contributor

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?

@aperezdc aperezdc added this to the 1.14 milestone Sep 28, 2021
@mcatanzaro
Copy link
Contributor

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.

@septatrix
Copy link

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.

That patch only applies the behavior to GTK, this is still broken for WPE...

@mcatanzaro
Copy link
Contributor

The affected code was GTK-specific (guarded by WebCore::PlatformDisplay::Type::Wayland which is not used by WPE), and doesn't exist anymore (deleted when GTK port stopped supporting WPE renderer). There's no way you're hitting this problem with WPE. This issue can be closed.

I would create a new issue report for the problem you're encountering.

@septatrix
Copy link

I would create a new issue report for the problem you're encountering.

Should I create that under the cog repo, here, or under bugs.webkit.org?

@mgorse mgorse closed this as completed Oct 17, 2024
@mcatanzaro
Copy link
Contributor

Should I create that under the cog repo, here, or under bugs.webkit.org?

Sorry I missed this. I don't know, but the cog repo is a safe place to start.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

4 participants