[bug#70446,v3] gnu: webkitgtk: Add locale and dri access to gtk sandbox in order to silence gtk locale warnings and enable hardware accelerated video, respectively. Adjust bubblewrap wrapper to add user profile locale and dri directories.

* gnu/packages/patches/webkitgtk-adjust-bubblewrap-paths.patch:
Add @dridir@ and @localedir@ to bubblewrap gtk sandbox
Add ~/.guix-profile/lib/dri and ~/.guix-profile/share/locale
to bubblewrap gtk sandbox.

* gnu/packages/webkit.scm (webkitgtk)[arguments]: In the
'configure-bubblewrap-store-directory' phase, also supply locale
and dri directory paths to webkitgtk-adjust-bubblewrap-paths.patch
template.

Change-Id: I6be0c473ebaa6c04ebb00a2b4afcae2c89396e4f
---
Only shares user profile locale and dri folders.

 .../webkitgtk-adjust-bubblewrap-paths.patch   | 33 +++++++++++++++++--
 gnu/packages/webkit.scm                       | 11 ++++++-
 2 files changed, 40 insertions(+), 4 deletions(-)


base-commit: b05bb6608c7f25ddce6b563194ba5a3007009282

Am Freitag, dem 19.04.2024 um 17:55 -0400 schrieb Abhishek Cherath:
> * gnu/packages/patches/webkitgtk-adjust-bubblewrap-paths.patch:
> Add @dridir@ and @localedir@ to bubblewrap gtk sandbox
> Add ~/.guix-profile/lib/dri and ~/.guix-profile/share/locale
> to bubblewrap gtk sandbox.
> 
> * gnu/packages/webkit.scm (webkitgtk)[arguments]: In the
> 'configure-bubblewrap-store-directory' phase, also supply locale
> and dri directory paths to webkitgtk-adjust-bubblewrap-paths.patch
> template.
> 
> Change-Id: I6be0c473ebaa6c04ebb00a2b4afcae2c89396e4f
> ---
> Only shares user profile locale and dri folders.
> 
>  .../webkitgtk-adjust-bubblewrap-paths.patch   | 33
> +++++++++++++++++--
>  gnu/packages/webkit.scm                       | 11 ++++++-
>  2 files changed, 40 insertions(+), 4 deletions(-)
> 
> diff --git a/gnu/packages/patches/webkitgtk-adjust-bubblewrap-
> paths.patch b/gnu/packages/patches/webkitgtk-adjust-bubblewrap-
> paths.patch
> index 18ddb645ad..0cf1498b92 100644
> --- a/gnu/packages/patches/webkitgtk-adjust-bubblewrap-paths.patch
> +++ b/gnu/packages/patches/webkitgtk-adjust-bubblewrap-paths.patch
> @@ -1,11 +1,22 @@
>  Share /gnu/store in the BubbleWrap container and remove FHS mounts.
> +Also share locale and dri directories (user and system.)
>  
>  This is a Guix-specific patch not meant to be upstreamed.
>  diff --git
> a/Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp
> b/Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp
> -index f0a5e4b05dff..88b11f806968 100644
> +index 99395d6..3604730 100644
>  --- a/Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp
>  +++ b/Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp
> -@@ -854,27 +854,12 @@ GRefPtr<GSubprocess>
> bubblewrapSpawn(GSubprocessLauncher* launcher, const Proces
> +@@ -765,6 +765,9 @@ GRefPtr<GSubprocess>
> bubblewrapSpawn(GSubprocessLauncher* launcher, const Proces
> +         return adoptGRef(g_subprocess_launcher_spawnv(launcher,
> argv, error));
> + 
> +     const char* runDir = g_get_user_runtime_dir();
> ++    const char* homeDir = g_get_home_dir();
> ++    char* userDriDir = g_strconcat(homeDir, "/.guix-
> profile/lib/dri", NULL);
> ++    char* userLocaleDir = g_strconcat(homeDir, "/.guix-
> profile/share/locale", NULL);
> +     Vector<CString> sandboxArgs = {
> +         "--die-with-parent",
> +         "--unshare-uts",
> +@@ -786,28 +788,28 @@ GRefPtr<GSubprocess>
> bubblewrapSpawn(GSubprocessLauncher* launcher, const Proces
>           "--ro-bind", "/sys/dev", "/sys/dev",
>           "--ro-bind", "/sys/devices", "/sys/devices",
>   
> @@ -33,6 +44,22 @@ index f0a5e4b05dff..88b11f806968 100644
>  +
>  +        // Bind mount the store inside the WebKitGTK sandbox.
>  +        "--ro-bind", "@storedir@", "@storedir@",
> ++
> ++        // Bind mount the locales in profile
> ++        "--ro-bind-try", userLocaleDir, userLocaleDir,
> ++
> ++        // Bind mount the dri dir in profile
> ++        "--ro-bind-try", userDriDir, userDriDir,
For reference, why are these two needed here?  Can't we do this with
the locales and drivers referenced below?  Should we perhaps expand
GUIX_LOCPATH here?
> ++
> ++        // This is needed for locales if not in profile
> ++        "--ro-bind-try", "@localedir@", "@localedir@",
> ++
> ++        // This is needed for video hardware acceleration (va-api)
> ++        // via /lib/dri if not in profile
> ++        "--ro-bind-try", "@dridir@", "@dridir@",
>       };
> ++    free(userLocaleDir);
> ++    free(userDriDir);
>   
> -     if (launchOptions.processType ==
> ProcessLauncher::ProcessType::DBusProxy) {
> +     if (enableDebugPermissions()) {
> +         const char* dataDir = g_get_user_data_dir();
> diff --git a/gnu/packages/webkit.scm b/gnu/packages/webkit.scm
> index bf24a65e83..a0d04f31d3 100644
> --- a/gnu/packages/webkit.scm
> +++ b/gnu/packages/webkit.scm
> @@ -8,6 +8,7 @@
>  ;;; Copyright © 2019 Marius Bakke <mbakke@fastmail.com>
>  ;;; Copyright © 2021, 2022, 2023 Maxim Cournoyer
> <maxim.cournoyer@gmail.com>
>  ;;; Copyright © 2022, 2023 Efraim Flashner <efraim@flashner.co.il>
> +;;; Copyright © 2024 Abhishek Cherath <abhi@quic.us>
>  ;;;
>  ;;; This file is part of GNU Guix.
>  ;;;
> @@ -190,7 +191,15 @@ (define-public webkitgtk
>                (let ((store-directory (%store-directory)))
>                  (substitute*
>                     
> "Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp"
> -                  (("@storedir@") store-directory)))))
> +                  (("@storedir@") store-directory)
> +                  ;; this adds access to drivers for va-api
> +                  ;; for hardware accelerated video
> +                  (("@dridir@") "/run/current-
> system/profile/lib/dri")
> +                  ;; this silences gtk locale errors
> +                  ;; Unfortunately, simply bind mounting
> /run/current-system
> +                  ;; does not work since it leads to weird issues
> +                  ;; with symlinks that confuse bubblewrap.
> +                  (("@localedir@") "/run/current-system/locale")))))
>            (add-after 'unpack 'do-not-disable-new-dtags
>              ;; Ensure the linker uses new dynamic tags as this is
> what Guix
>              ;; uses and validates in the validate-runpath phase.
> 
> base-commit: b05bb6608c7f25ddce6b563194ba5a3007009282
Note that any item you add here which references the user home will
fail to be loaded correctly when using `guix shell' in a way that hides
it; or even just using `guix shell' normally with a user who doesn't
have the hardware-accelerated drivers in their home.  For system paths,
this is somewhat different, since we can more or less expect them to
exist and mirror the layout of other distros to some extent.

Cheers

Hello,

>> ++        "--ro-bind-try", userLocaleDir, userLocaleDir,
>> ++
>> ++        // Bind mount the dri dir in profile
>> ++        "--ro-bind-try", userDriDir, userDriDir,
> For reference, why are these two needed here?  Can't we do this with
> the locales and drivers referenced below?  Should we perhaps expand
> GUIX_LOCPATH here?

Initially, I only had the system paths below those. I added these
so that people could have hardware accel by only installing the required
drivers in their local profiles (as recommended in 69971, unless I
entirely misunderstood)

I'm afraid I don't really know what adding stuff to GUIX_LOCPATH would
do. That's for foreign distros, correct? To reiterate, The locale
problem here is that the bubblewrapped process doesn't have access to
the locales, without which it throws warnings.

> Note that any item you add here which references the user home will
> fail to be loaded correctly when using `guix shell' in a way that hides
> it; or even just using `guix shell' normally with a user who doesn't
> have the hardware-accelerated drivers in their home.  For system paths,
> this is somewhat different, since we can more or less expect them to
> exist and mirror the layout of other distros to some extent.

Hmm, since it's in an ro-bind-try, that'll cause the drivers not to
work, and fall back to trying the system drivers. Is there a better
solution you could recommend?

Yours sincerely,
Abhishek Cherath.

Am Freitag, dem 19.04.2024 um 20:22 -0400 schrieb Abhishek Cherath:
> Hello,
> 
> > > ++        "--ro-bind-try", userLocaleDir, userLocaleDir,
> > > ++
> > > ++        // Bind mount the dri dir in profile
> > > ++        "--ro-bind-try", userDriDir, userDriDir,
> > For reference, why are these two needed here?  Can't we do this
> > with the locales and drivers referenced below?  Should we perhaps
> > expand GUIX_LOCPATH here?
> 
> Initially, I only had the system paths below those. I added these
> so that people could have hardware accel by only installing the
> required drivers in their local profiles (as recommended in 69971,
> unless I entirely misunderstood)
Ah, yes, Maxim did mention this, but yeah, non-static paths are NG
(nogood) here.  There really is no reason that those paths ought to
exist or be useful in a container, for example.

> I'm afraid I don't really know what adding stuff to GUIX_LOCPATH
> would do. That's for foreign distros, correct? To reiterate, The
> locale problem here is that the bubblewrapped process doesn't have
> access to the locales, without which it throws warnings.
Adding stuff *from* GUIX_LOCPATH, the idea being that this is where we
already advocate locales be put.

> > Note that any item you add here which references the user home will
> > fail to be loaded correctly when using `guix shell' in a way that
> > hides it; or even just using `guix shell' normally with a user who
> > doesn't have the hardware-accelerated drivers in their home.  For
> > system paths, this is somewhat different, since we can more or less
> > expect them to exist and mirror the layout of other distros to some
> > extent.
> 
> Hmm, since it's in an ro-bind-try, that'll cause the drivers not to
> work, and fall back to trying the system drivers. Is there a better
> solution you could recommend?
Unless a hard dependency on Mesa is appropriate (which we'd have to
confirm), I think just rolling with the system ones is okay.

Cheers

Hello,

Liliana Marie Prikler <liliana.prikler@gmail.com> writes:

>> Initially, I only had the system paths below those. I added these
>> so that people could have hardware accel by only installing the
>> required drivers in their local profiles (as recommended in 69971,
>> unless I entirely misunderstood)
> Ah, yes, Maxim did mention this, but yeah, non-static paths are NG
> (nogood) here.  There really is no reason that those paths ought to
> exist or be useful in a container, for example.
>

Gotcha.

>> I'm afraid I don't really know what adding stuff to GUIX_LOCPATH
>> would do. That's for foreign distros, correct? To reiterate, The
>> locale problem here is that the bubblewrapped process doesn't have
>> access to the locales, without which it throws warnings.
> Adding stuff *from* GUIX_LOCPATH, the idea being that this is where we
> already advocate locales be put.

I see, so something along these lines?
```C
const char* guixLocPath = g_getenv("GUIX_LOCPATH");
char** locPaths = NULL;
if (guixLocPath != NULL) {
   locPaths = g_strsplit(guixLocPath,':', 4096);
   for (int i = 0; i < g_strv_length(locPaths); i++) {
       sandboxArgs.appendVector(Vector<CString>({
        "--ro-bind", *locPaths[i], *locPaths[i]
       }));
   }
   g_strfreev(locPaths);
}
```

>> > Note that any item you add here which references the user home will
>> > fail to be loaded correctly when using `guix shell' in a way that
>> > hides it; or even just using `guix shell' normally with a user who
>> > doesn't have the hardware-accelerated drivers in their home.  For
>> > system paths, this is somewhat different, since we can more or less
>> > expect them to exist and mirror the layout of other distros to some
>> > extent.
>> 
>> Hmm, since it's in an ro-bind-try, that'll cause the drivers not to
>> work, and fall back to trying the system drivers. Is there a better
>> solution you could recommend?
> Unless a hard dependency on Mesa is appropriate (which we'd have to
> confirm), I think just rolling with the system ones is okay.

Sounds good to me! Will send v4 with just that.

Am Freitag, dem 19.04.2024 um 21:52 -0400 schrieb Abhishek Cherath:
> 
> Hello,
> 
> Liliana Marie Prikler <liliana.prikler@gmail.com> writes:
> 
> > > Initially, I only had the system paths below those. I added these
> > > so that people could have hardware accel by only installing the
> > > required drivers in their local profiles (as recommended in
> > > 69971,
> > > unless I entirely misunderstood)
> > Ah, yes, Maxim did mention this, but yeah, non-static paths are NG
> > (nogood) here.  There really is no reason that those paths ought to
> > exist or be useful in a container, for example.
> > 
> 
> Gotcha.
> 
> > > I'm afraid I don't really know what adding stuff to GUIX_LOCPATH
> > > would do. That's for foreign distros, correct? To reiterate, The
> > > locale problem here is that the bubblewrapped process doesn't
> > > have
> > > access to the locales, without which it throws warnings.
> > Adding stuff *from* GUIX_LOCPATH, the idea being that this is where
> > we already advocate locales be put.
> 
> I see, so something along these lines?
> ```C
> const char* guixLocPath = g_getenv("GUIX_LOCPATH");
> char** locPaths = NULL;
> if (guixLocPath != NULL) {
>    locPaths = g_strsplit(guixLocPath,':', 4096);
>    for (int i = 0; i < g_strv_length(locPaths); i++) {
>        sandboxArgs.appendVector(Vector<CString>({
>         "--ro-bind", *locPaths[i], *locPaths[i]
>        }));
>    }
>    g_strfreev(locPaths);
> }
> ```
You can (and arguably should) use C++ semantics, and should not attempt
to hardcode any magic numbers here.  Historically, there used to be
more patches to deal with e.g. fonts, try to check if a procedure by
the name "bindIfExists" can still be found in the Webkit source.


Cheers

Hi Abhishek,

Abhishek Cherath <abhi@quic.us> writes:

> Hello,
>
> Liliana Marie Prikler <liliana.prikler@gmail.com> writes:
>
>>> Initially, I only had the system paths below those. I added these
>>> so that people could have hardware accel by only installing the
>>> required drivers in their local profiles (as recommended in 69971,
>>> unless I entirely misunderstood)
>> Ah, yes, Maxim did mention this, but yeah, non-static paths are NG
>> (nogood) here.  There really is no reason that those paths ought to
>> exist or be useful in a container, for example.
>>
>
> Gotcha.

Sorry for the confusion; I agree with Liliana that honoring GUIX_LOCPATH
is better than hard-coding any specific file name.