[bug#69971] [PATCH] gnu: webkitgtk: Add locale and dri access to gtk sandbox in order to silence gtk locale warnings and enable hardware accelerated video, respectively.

[bug#69971] [PATCH] gnu: webkitgtk: Add locale and dri access to gtk sandbox in order to silence gtk locale warnings and enable hardware accelerated video, respectively.

[Abhishek Cherath]

* gnu/packages/patches/webkitgtk-adjust-bubblewrap-paths.patch:
Add @dridir@ and @localedir@ 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: Id1ffe23e56a8da4ff3c81a2cde7d9622f024bdea
---
 .../patches/webkitgtk-adjust-bubblewrap-paths.patch   |  8 +++++++-
 gnu/packages/webkit.scm                               | 11 ++++++++++-
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/gnu/packages/patches/webkitgtk-adjust-bubblewrap-paths.patch b/gnu/packages/patches/webkitgtk-adjust-bubblewrap-paths.patch
index 18ddb645ad..793f6a414b 100644
--- a/gnu/packages/patches/webkitgtk-adjust-bubblewrap-paths.patch
+++ b/gnu/packages/patches/webkitgtk-adjust-bubblewrap-paths.patch
@@ -5,7 +5,7 @@ diff --git a/Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp b/Sour
 index f0a5e4b05dff..88b11f806968 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
+@@ -854,27 +854,18 @@ GRefPtr<GSubprocess> bubblewrapSpawn(GSubprocessLauncher* launcher, const Proces
          "--ro-bind", "/sys/dev", "/sys/dev",
          "--ro-bind", "/sys/devices", "/sys/devices",
  
@@ -33,6 +33,12 @@ index f0a5e4b05dff..88b11f806968 100644
 +
 +        // Bind mount the store inside the WebKitGTK sandbox.
 +        "--ro-bind", "@storedir@", "@storedir@",
++
++	// This is needed for locales in /run/current-system/locales
++	"--ro-bind-try", "@localedir@", "@localedir@",
++
++	// This is needed for video hardware acceleration (va-api) via /lib/dri
++	"--ro-bind-try", "@dridir@", "@dridir@",
      };
  
      if (launchOptions.processType == ProcessLauncher::ProcessType::DBusProxy) {
diff --git a/gnu/packages/webkit.scm b/gnu/packages/webkit.scm
index bf24a65e83..4777a9b96e 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
+                  (("@localedir@") "/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.
+                  (("@dridir@") "/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: d67e4f0f9b10c7ddac8fb0ca68cbf1d6ad0a6e5d
prerequisite-patch-id: 2feff8a49a2bca7cb55d49c21c04736f9828df0e
prerequisite-patch-id: c3460fa91fad7c4f67859f672420ca72e616d89b
-- 
2.41.0

[bug#69971] [PATCH v2] gnu: webkitgtk: Add locale and dri access to gtk sandbox in order to silence gtk locale warnings and enable hardware accelerated video, respectively.

[Abhishek Cherath]

* gnu/packages/patches/webkitgtk-adjust-bubblewrap-paths.patch:
Add @dridir@ and @localedir@ 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: Id1ffe23e56a8da4ff3c81a2cde7d9622f024bdea
---
Messed up dri-dir and locale dir
 .../patches/webkitgtk-adjust-bubblewrap-paths.patch   |  8 +++++++-
 gnu/packages/webkit.scm                               | 11 ++++++++++-
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/gnu/packages/patches/webkitgtk-adjust-bubblewrap-paths.patch b/gnu/packages/patches/webkitgtk-adjust-bubblewrap-paths.patch
index 18ddb645ad..793f6a414b 100644
--- a/gnu/packages/patches/webkitgtk-adjust-bubblewrap-paths.patch
+++ b/gnu/packages/patches/webkitgtk-adjust-bubblewrap-paths.patch
@@ -5,7 +5,7 @@ diff --git a/Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp b/Sour
 index f0a5e4b05dff..88b11f806968 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
+@@ -854,27 +854,18 @@ GRefPtr<GSubprocess> bubblewrapSpawn(GSubprocessLauncher* launcher, const Proces
          "--ro-bind", "/sys/dev", "/sys/dev",
          "--ro-bind", "/sys/devices", "/sys/devices",
  
@@ -33,6 +33,12 @@ index f0a5e4b05dff..88b11f806968 100644
 +
 +        // Bind mount the store inside the WebKitGTK sandbox.
 +        "--ro-bind", "@storedir@", "@storedir@",
++
++	// This is needed for locales in /run/current-system/locales
++	"--ro-bind-try", "@localedir@", "@localedir@",
++
++	// This is needed for video hardware acceleration (va-api) via /lib/dri
++	"--ro-bind-try", "@dridir@", "@dridir@",
      };
  
      if (launchOptions.processType == ProcessLauncher::ProcessType::DBusProxy) {
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: d67e4f0f9b10c7ddac8fb0ca68cbf1d6ad0a6e5d
prerequisite-patch-id: 2feff8a49a2bca7cb55d49c21c04736f9828df0e
prerequisite-patch-id: c3460fa91fad7c4f67859f672420ca72e616d89b
-- 
2.41.0

[bug#69971] [PATCH v2] gnu: webkitgtk: Add locale and dri access to gtk sandbox in order to silence gtk locale warnings and enable hardware accelerated video, respectively.

[Maxim Cournoyer]

Hello!

Abhishek Cherath <abhi@quic.us> writes:

> * gnu/packages/patches/webkitgtk-adjust-bubblewrap-paths.patch:
> Add @dridir@ and @localedir@ 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.

This looks reasonable to me, thanks for your contribution!  I suppose
for security reasons the file names must be static, e.g. cannot be
$HOME/.guix-profile/share/locale or similar?

LGTM; Liliana, I remember you would prefer not having webkitgtk changes
happen on master; do you have a suggestion of which branch this should
be committed to?  gnome-team?

-- 
Thanks,
Maxim

[bug#69971] [PATCH v2] gnu: webkitgtk: Add locale and dri access to gtk sandbox in order to silence gtk locale warnings and enable hardware accelerated video, respectively.

[Abhishek Cherath]

It was a conservative choice, but not made for security reasons, I'm just not sure where and how this wrapper runs, and I was mildly tired of recompiling webkitgtk.

I'm not opposed to having it be $HOME, if that works; I don't see what security issues there could be.



On 31 March 2024 9:33:41 pm GMT-04:00, Maxim Cournoyer <maxim.cournoyer@gmail.com> wrote:
>Hello!
>
>Abhishek Cherath <abhi@quic.us> writes:
>
>> * gnu/packages/patches/webkitgtk-adjust-bubblewrap-paths.patch:
>> Add @dridir@ and @localedir@ 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.
>
>This looks reasonable to me, thanks for your contribution!  I suppose
>for security reasons the file names must be static, e.g. cannot be
>$HOME/.guix-profile/share/locale or similar?
>
>LGTM; Liliana, I remember you would prefer not having webkitgtk changes
>happen on master; do you have a suggestion of which branch this should
>be committed to?  gnome-team?
>

[bug#69971] [PATCH v2] gnu: webkitgtk: Add locale and dri access to gtk sandbox in order to silence gtk locale warnings and enable hardware accelerated video, respectively.

[Liliana Marie Prikler]

Am Sonntag, dem 31.03.2024 um 22:17 -0400 schrieb Abhishek Cherath:
> It was a conservative choice, but not made for security reasons, I'm
> just not sure where and how this wrapper runs, and I was mildly tired
> of recompiling webkitgtk.
> 
> I'm not opposed to having it be $HOME, if that works; I don't see
> what security issues there could be.
I think dynamic choices should be possible – IIRC, std::strings are
used for arguments, but even if not, we're dealing with C++, so we can
allocate "on the stack".

Am Sonntag, dem 31.03.2024 um 21:33 -0400 schrieb Maxim Cournoyer:
> Liliana, I remember you would prefer not having webkitgtk changes
> happen on master; do you have a suggestion of which branch this
> should be committed to?  gnome-team?
We can do this on gnome-team, we still have some leftover world
rebuilds from 44.10.  I'd prefer if stuff that rebuilds webkitgtk on
master were grafted, as it causes more than the prescribed 300 rebuilds
and is a nasty build itself.

Cheers

[bug#69971] [PATCH v2] gnu: webkitgtk: Add locale and dri access to gtk sandbox in order to silence gtk locale warnings and enable hardware accelerated video, respectively.

[Abhishek Cherath]

>I think dynamic choices should be possible – IIRC, std::strings are
>used for arguments, but even if not, we're dealing with C++, so we can
>allocate "on the stack".

👍. I can make that change tomorrow. One QoL thing. How do you run a program built in /tmp/<<build_folder>> without it complaining about store paths and suchlike?

I ask because ideally, I'd debug this by interrupting the webkit build somewhere while I have --keep-failed, then `guix shell -D webkitgtk --pure && . environment-variables`, then running the minibrowser. But that doesn't work because it complains about stuff not being in the store.

Oh, but I suppose I could use LD_LIBRARY_PATH unless it compiles in some strings.  Will try.

>rebuilds from 44.10.  I'd prefer if stuff that rebuilds webkitgtk on
>master were grafted, as it causes more than the prescribed 300 rebuilds
>and is a nasty build itself.

👍, so call this webkitgtk-bubblewrap-fixed and have a replacement field in the other package?

[bug#69971] [PATCH v2] gnu: webkitgtk: Add locale and dri access to gtk sandbox in order to silence gtk locale warnings and enable hardware accelerated video, respectively.

[Liliana Marie Prikler]

Am Montag, dem 01.04.2024 um 06:49 -0400 schrieb Abhishek Cherath:
> > I think dynamic choices should be possible – IIRC, std::strings are
> > used for arguments, but even if not, we're dealing with C++, so we
> > can allocate "on the stack".
> 
> 👍. I can make that change tomorrow. One QoL thing. How do you run a
> program built in /tmp/<<build_folder>> without it complaining about
> store paths and suchlike?
> 
> I ask because ideally, I'd debug this by interrupting the webkit
> build somewhere while I have --keep-failed, then `guix shell -D
> webkitgtk --pure && . environment-variables`, then running the
> minibrowser. But that doesn't work because it complains about stuff
> not being in the store.
> 
> Oh, but I suppose I could use LD_LIBRARY_PATH unless it compiles in
> some strings.  Will try.
You should be able to run things from the build folder, but you could
also throw a post-install error if needed.  Just note that webkitgtk in
and of itself doesn't really come with a full browser, so you'd have to
compile one as well…

I think with webkit in particular the problem is that store paths are
getting hard-coded in places where file existence is required, so you
might want to replace those store paths with /tmp/guix-build/…

> > rebuilds from 44.10.  I'd prefer if stuff that rebuilds webkitgtk
> > on master were grafted, as it causes more than the prescribed 300
> > rebuilds and is a nasty build itself.
> 
> 👍, so call this webkitgtk-bubblewrap-fixed and have a replacement
> field in the other package?
Ahh, sorry, grafts are for security purposes – changes like these would
have to go through the usual channels (i.e. gnome-team).  We will be
jumping ahead to 46 at some point in the future, but for now the branch
is chill and we still need to catch up on stuff we missed for master.

Cheers

[bug#69971] [PATCH v2] gnu: webkitgtk: Add locale and dri access to gtk sandbox in order to silence gtk locale warnings and enable hardware accelerated video, respectively.

[Jack Hill]

[-- Attachment #1: Type: text/plain, Size: 557 bytes --]

On Mon, 1 Apr 2024, Liliana Marie Prikler wrote:

> Ahh, sorry, grafts are for security purposes – changes like these would
> have to go through the usual channels (i.e. gnome-team).  We will be
> jumping ahead to 46 at some point in the future, but for now the branch
> is chill and we still need to catch up on stuff we missed for master.

I don't mean to hold up this patch, however, this reminded me to check and 
It looks like we'll be do for a rebuild anyway for the new major version: 
https://webkitgtk.org/2024/03/27/webkigit-2.44.html

Best
Jack

[bug#69971] [PATCH v2] gnu: webkitgtk: Add locale and dri access to gtk sandbox in order to silence gtk locale warnings and enable hardware accelerated video, respectively.

[Liliana Marie Prikler]

Am Montag, dem 01.04.2024 um 14:11 -0400 schrieb Jack Hill:
> On Mon, 1 Apr 2024, Liliana Marie Prikler wrote:
> 
> > Ahh, sorry, grafts are for security purposes – changes like these
> > would have to go through the usual channels (i.e. gnome-team).  We
> > will be jumping ahead to 46 at some point in the future, but for
> > now the branch is chill and we still need to catch up on stuff we
> > missed for master.
> 
> I don't mean to hold up this patch, however, this reminded me to
> check and  It looks like we'll be do for a rebuild anyway for the new
> major version: 
> https://webkitgtk.org/2024/03/27/webkigit-2.44.html
Yeah, we're in great luck that we are chill on gnome-team atm.  We can
do these big juicy webkit builds there, then do a mini-merge before we
start real work™ again.

Cheers

[bug#69971] [PATCH v2] gnu: webkitgtk: Add locale and dri access to gtk sandbox in order to silence gtk locale warnings and enable hardware accelerated video, respectively.

[Abhishek Cherath]

So I've no problem with adding the home profile paths and resubmitting to gnome-team, but I'll likely only be able to do that this weekend.

If anyone else wants to make the changes or merge this one as is for now, I have no problem.

[bug#69971] Explanation

[Abhishek Cherath]

Hello,

So this patch fixes two things. First, it gives the webkit gtk process
access to va-api drivers, which allows hardware acceleration for video
and prevents the errors below:

0:00:00.489161195    21       0xfd4200 INFO               vadisplay gstvadisplay.c:268:_va_info:<vadisplaydrm2> VA info: Trying to open /home/abhishek/.guix-profile/lib/dri/i965_drv_video.so
0:00:00.489224548    21       0xfd4200 INFO               vadisplay gstvadisplay.c:268:_va_info:<vadisplaydrm2> VA info: Trying to open /run/current-system/profile/lib/dri/i965_drv_video.so
0:00:00.489278879    21       0xfd4200 INFO               vadisplay gstvadisplay.c:268:_va_info:<vadisplaydrm2> VA info: va_openDriver() returns -1
0:00:00.489287135    21       0xfd4200 WARN               vadisplay gstvadisplay.c:316:gst_va_display_initialize:<vadisplaydrm2> vaInitialize: unknown libva error
0:00:00.489302829    21       0xfd4200 ERROR            msdkcontext gstmsdkcontext.c:183:gst_msdk_context_use_vaapi: Couldn't create a VA DRM display

Second, it gives access to the locale dir, which silences some warnings
of the sort below:

(process:2): Gtk-WARNING **: 02:21:08.731: Locale not supported by C library.
	Using the fallback 'C' locale.

Yours sincerely,
Abhishek Cherath.

[bug#69971] Some more information

[Abhishek Cherath]

The reason the driver path stuff is particularly important is that I get
my env vars for LIBVA_DRIVERS_PATH from guix package --search paths as
follows

```bash
eval "$(guix package --search-paths \
-p $HOME/.config/guix/current \
-p $HOME/.guix-profile \
-p $HOME/.guix-extra-profiles/emacs/emacs \
-p $HOME/.guix-home/profile \
-p /run/current-system/profile)"
```

and this gives the following for LIBVA_DRIVERS_PATH:
```bash
export LIBVA_DRIVERS_PATH="/run/current-system/profile/lib/dri:/home/abhishek/.guix-profile/lib/dri"
```

This means that any sandboxed program with access to one of those won't
be able to use hardware acceleration. I only figured this out when I got
curious about why mpv could use hardware accel just fine but nyxt
couldn't. It's also a problem for firefox. Guess I should put in a bug
report there?

[bug#69971] Some more information

[Abhishek Cherath]

> This means that any sandboxed program with access to one of those won't
I mean *without* access.

[bug#69971] Submitted to gnome-team

[Abhishek Cherath]

close 69971

Hello,

I've made the changes and submitted as 70446 to gnome-team.

[bug#69971] close 69971

[Abhishek Cherath]

close 69971