[bug#70446] [PATCH gnome-team] gnu: webkitgtk: Add system locale, dri access, and user profile access to gtk sandbox in order to silence gtk locale warnings and enable hardware accelerated video, respectively.

[bug#70446] [PATCH gnome-team] gnu: webkitgtk: Add system locale, dri access, and user profile 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
Add ~/.guix-profile 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.
---
 .../webkitgtk-adjust-bubblewrap-paths.patch   | 28 +++++++++++++++++--
 gnu/packages/webkit.scm                       | 11 +++++++-
 2 files changed, 35 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..2b6f54c912 100644
--- a/gnu/packages/patches/webkitgtk-adjust-bubblewrap-paths.patch
+++ b/gnu/packages/patches/webkitgtk-adjust-bubblewrap-paths.patch
@@ -1,11 +1,21 @@
 Share /gnu/store in the BubbleWrap container and remove FHS mounts.
+Also share user profile directory.
 
 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,1 +765,1 @@ 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* profileDir = g_strconcat(homeDir, "/.guix-profile", NULL);
+     Vector<CString> sandboxArgs = {
+         "--die-with-parent",
+         "--unshare-uts",
+@@ -786,28 +788,24 @@ GRefPtr<GSubprocess> bubblewrapSpawn(GSubprocessLauncher* launcher, const Proces
          "--ro-bind", "/sys/dev", "/sys/dev",
          "--ro-bind", "/sys/devices", "/sys/devices",
  
@@ -33,6 +43,18 @@ index f0a5e4b05dff..88b11f806968 100644
 +
 +        // Bind mount the store inside the WebKitGTK sandbox.
 +        "--ro-bind", "@storedir@", "@storedir@",
++
++        // Bind mount the guix profile directory
++        "--ro-bind", profileDir, profileDir,
++
++        // 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(profileDir);
  
-     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
-- 
2.41.0

[bug#70446] Explanation

[Abhishek Cherath]

This is to resubmit to gnome-team, along with adding the profile dir, as
recommended in https://issues.guix.gnu.org/69971

[bug#70446] [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. Adjust bubblewrap wrapper to add user profile.

[Abhishek Cherath]

* gnu/packages/patches/webkitgtk-adjust-bubblewrap-paths.patch:
Add @dridir@ and @localedir@ to bubblewrap gtk sandbox
Add ~/.guix-profile 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
---
apparently the space on the second line of the patch is significant,
doesn't apply otherwise

 .../webkitgtk-adjust-bubblewrap-paths.patch   | 28 +++++++++++++++++--
 gnu/packages/webkit.scm                       | 11 +++++++-
 2 files changed, 35 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..c81916279e 100644
--- a/gnu/packages/patches/webkitgtk-adjust-bubblewrap-paths.patch
+++ b/gnu/packages/patches/webkitgtk-adjust-bubblewrap-paths.patch
@@ -1,11 +1,21 @@
 Share /gnu/store in the BubbleWrap container and remove FHS mounts.
+Also share user profile directory.
 
 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,8 @@ 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* profileDir = g_strconcat(homeDir, "/.guix-profile", NULL);
+     Vector<CString> sandboxArgs = {
+         "--die-with-parent",
+         "--unshare-uts",
+@@ -786,28 +788,24 @@ GRefPtr<GSubprocess> bubblewrapSpawn(GSubprocessLauncher* launcher, const Proces
          "--ro-bind", "/sys/dev", "/sys/dev",
          "--ro-bind", "/sys/devices", "/sys/devices",
  
@@ -33,6 +43,18 @@ index f0a5e4b05dff..88b11f806968 100644
 +
 +        // Bind mount the store inside the WebKitGTK sandbox.
 +        "--ro-bind", "@storedir@", "@storedir@",
++
++        // Bind mount the guix profile directory
++        "--ro-bind", profileDir, profileDir,
++
++        // 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(profileDir);
  
-     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
-- 
2.41.0

[bug#70446] [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. Adjust bubblewrap wrapper to add user profile.

[Liliana Marie Prikler]

Am Donnerstag, dem 18.04.2024 um 00:06 -0400 schrieb Abhishek Cherath:
> * gnu/packages/patches/webkitgtk-adjust-bubblewrap-paths.patch:
> Add @dridir@ and @localedir@ to bubblewrap gtk sandbox
> Add ~/.guix-profile 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
> ---
> apparently the space on the second line of the patch is significant,
> doesn't apply otherwise
Wrapping the entire user profile looks evil.  Why?

[bug#70446] [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. Adjust bubblewrap wrapper to add user profile.

[Abhishek Cherath]

Could just add the locale and dri dir, but afaik the user profile is just stuff in the store, right? And the thing has access to the whole store anyhow, so no change, right?

[bug#70446] [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. Adjust bubblewrap wrapper to add user profile.

[Abhishek Cherath]

Will say, I thought it was kinda odd to begin with that it has access to the whole store, though. 

On 19 April 2024 4:24:56 pm GMT-04:00, Abhishek Cherath <abhi@quic.us> wrote:
>Could just add the locale and dri dir, but afaik the user profile is just stuff in the store, right? And the thing has access to the whole store anyhow, so no change, right?

[bug#70446] [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. Adjust bubblewrap wrapper to add user profile.

[Liliana Marie Prikler]

Am Freitag, dem 19.04.2024 um 16:24 -0400 schrieb Abhishek Cherath:
> Could just add the locale and dri dir, but afaik the user profile is
> just stuff in the store, right? And the thing has access to the whole
> store anyhow, so no change, right?
The user dir *is* just stuff in the store, but it is particularly stuff
in the store that's linked to the currently logged-in user.  That is,
you're giving the sandbox extra information by exposing it, and I don't
think it'd be solely (or even largely) useful for beneficial purposes.

Cheers

[bug#70446] [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. Adjust bubblewrap wrapper to add user profile.

[Abhishek Cherath]

That makes sense. I've modified the patch and sent a v3.

I only used the profile path instead of the specific paths because it's the first thing I got working, and I figured there wasn't really anything sensitive in the profile anyway.

[bug#70446] [PATCH gnome-team] gnu: webkitgtk: Add system locale, dri access, and user profile access to gtk sandbox in order to silence gtk locale warnings and enable hardware accelerated video, respectively.

[John Kehayias via Guix-patches via]

On Wed, Apr 17, 2024 at 10:52 PM, Abhishek Cherath wrote:

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

Perhaps combine with update for security issues as in
https://issues.guix.gnu.org/70404 ?

[bug#70446] [PATCH gnome-team] gnu: webkitgtk: Add system locale, dri access, and user profile access to gtk sandbox in order to silence gtk locale warnings and enable hardware accelerated video, respectively.

[Abhishek Cherath]

> Perhaps combine with update for security issues as in
> https://issues.guix.gnu.org/70404 ?

In this patch?

[bug#70446] [PATCH gnome-team] gnu: webkitgtk: Add system locale, dri access, and user profile access to gtk sandbox in order to silence gtk locale warnings and enable hardware accelerated video, respectively.

[Maxim Cournoyer]

Hi,

Abhishek Cherath <abhi@quic.us> writes:

>> Perhaps combine with update for security issues as in
>> https://issues.guix.gnu.org/70404 ?
>
> In this patch?

No, patches should remain separated, but I think John meant combining as
in merging at the same time, to avoid large rebuilds twice.

-- 
Thanks,
Maxim

[bug#70446] [PATCH 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.

[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,
++
++        // 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
-- 
2.41.0

[bug#70446] [PATCH 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.

[Liliana Marie Prikler]

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

[bug#70446] [PATCH 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.

[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)

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.

[bug#70446] [PATCH 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.

[Liliana Marie Prikler]

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 

[bug#70446] [PATCH 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.

[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);
}
```

>> > 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.

[bug#70446] [PATCH 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.

[Liliana Marie Prikler]

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

[bug#70446] [PATCH 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.

[Maxim Cournoyer]

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.

-- 
Thanks,
Maxim

[bug#70446] [PATCH v4] gnu: webkitgtk: Add access to system locale path and to paths from GUIX_LOCPATH, LOCPATH, and LIBVA_DRIVERS_PATH to gtk sandbox in order to silence gtk locale warnings and enable hardware accelerated video.

[Abhishek Cherath]

* gnu/packages/patches/webkitgtk-adjust-bubblewrap-paths.patch:
Add @dridir@ and @localedir@ to bubblewrap gtk sandbox
Add paths from GUIX_LOCPATH, LOCPATH, and LIBVA_DRIVERS_PATH
to bubblewrap gtk sandbox.

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

Change-Id: I6be0c473ebaa6c04ebb00a2b4afcae2c89396e4f
---
Thanks @LillianaPrikler@gmail.com for all the help :D, I thought about
this a bit more and looked at all the utility stuff in
BubblewrapLauncher.cpp. I realized that the correct thing to do here
is to simply mount the LIBVA_DRIVERS_PATH paths. I'm wondering if this
shouldn't be part of the gstreamer default mounts even upstream? along
with the LOCPATH mount.

 .../patches/webkitgtk-adjust-bubblewrap-paths.patch | 13 ++++++++++++-
 gnu/packages/webkit.scm                             |  8 +++++++-
 2 files changed, 19 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..4195aca388 100644
--- a/gnu/packages/patches/webkitgtk-adjust-bubblewrap-paths.patch
+++ b/gnu/packages/patches/webkitgtk-adjust-bubblewrap-paths.patch
@@ -1,11 +1,13 @@
 Share /gnu/store in the BubbleWrap container and remove FHS mounts.
+Also share system locale directory and paths in LOCPATH, GUIX_LOCPATH,
+and LIBVA_DRIVERS_PATH
 
 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
 --- 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,21 @@ GRefPtr<GSubprocess> bubblewrapSpawn(GSubprocessLauncher* launcher, const Proces
          "--ro-bind", "/sys/dev", "/sys/dev",
          "--ro-bind", "/sys/devices", "/sys/devices",
  
@@ -33,6 +35,15 @@ index f0a5e4b05dff..88b11f806968 100644
 +
 +        // Bind mount the store inside the WebKitGTK sandbox.
 +        "--ro-bind", "@storedir@", "@storedir@",
++
++        // This is needed for system locales
++        "--ro-bind-try", "@localedir@", "@localedir@",
      };
++    // User specified locale directory
++    bindPathVar(sandboxArgs, "LOCPATH");
++    // Locales in case of foreign system.    
++    bindPathVar(sandboxArgs, "GUIX_LOCPATH");
++    // Drivers for video hardware acceleration (va-api)
++    bindPathVar(sandboxArgs, "LIBVA_DRIVERS_PATH");
  
      if (launchOptions.processType == ProcessLauncher::ProcessType::DBusProxy) {
diff --git a/gnu/packages/webkit.scm b/gnu/packages/webkit.scm
index bf24a65e83..d057bb3aa2 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,12 @@ (define-public webkitgtk
               (let ((store-directory (%store-directory)))
                 (substitute*
                     "Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp"
-                  (("@storedir@") store-directory)))))
+                  (("@storedir@") store-directory)
+                  ;; 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
-- 
2.41.0

[bug#70446] [PATCH v4] gnu: webkitgtk: Add access to system locale path and to paths from GUIX_LOCPATH, LOCPATH, and LIBVA_DRIVERS_PATH to gtk sandbox in order to silence gtk locale warnings and enable hardware accelerated video.

[Liliana Marie Prikler]

Am Samstag, dem 20.04.2024 um 09:44 -0400 schrieb Abhishek Cherath:
> * gnu/packages/patches/webkitgtk-adjust-bubblewrap-paths.patch:
> Add @dridir@ and @localedir@ to bubblewrap gtk sandbox
> Add paths from GUIX_LOCPATH, LOCPATH, and LIBVA_DRIVERS_PATH
> to bubblewrap gtk sandbox.
> 
> * gnu/packages/webkit.scm (webkitgtk)[arguments]: In the
> 'configure-bubblewrap-store-directory' phase, also supply system
> locale to webkitgtk-adjust-bubblewrap-paths.patch template.
> 
> Change-Id: I6be0c473ebaa6c04ebb00a2b4afcae2c89396e4f
> ---
> Thanks [liliana.prikler@gmail.com] for all the help :D, I thought
> about this a bit more and looked at all the utility stuff in
> BubblewrapLauncher.cpp. I realized that the correct thing to do here
> is to simply mount the LIBVA_DRIVERS_PATH paths. I'm wondering if
> this shouldn't be part of the gstreamer default mounts even upstream?
> along with the LOCPATH mount.
This patch LGTM.  I think submitting it upstream sans GUIX_LOCPATH
would be a great idea – that way, we'd have fewer things to patch.

Is @localedir@ still needed with the bindPathVar in place?  Otherwise,
as already said, LGTM, and I'll look into forwarding it to/cherry-
picking it from gnome-team once I got new Webkit over there (still need
to wait for CI on that).

Cheers

[bug#70446] [PATCH v4] gnu: webkitgtk: Add access to system locale path and to paths from GUIX_LOCPATH, LOCPATH, and LIBVA_DRIVERS_PATH to gtk sandbox in order to silence gtk locale warnings and enable hardware accelerated video.

[Abhishek Cherath]

Hello,

>> Thanks [liliana.prikler@gmail.com] for all the help :D, I thought
>> about this a bit more and looked at all the utility stuff in
>> BubblewrapLauncher.cpp. I realized that the correct thing to do here
>> is to simply mount the LIBVA_DRIVERS_PATH paths. I'm wondering if
>> this shouldn't be part of the gstreamer default mounts even upstream?
>> along with the LOCPATH mount.
> This patch LGTM.  I think submitting it upstream sans GUIX_LOCPATH
> would be a great idea – that way, we'd have fewer things to patch.

Sweet! I'll get on that sometime next month.

> Is @localedir@ still needed with the bindPathVar in place?  Otherwise,
> as already said, LGTM, and I'll look into forwarding it to/cherry-
> picking it from gnome-team once I got new Webkit over there (still need
> to wait for CI on that).

Yes, it's still needed since Guix system doesn't generally set LOCPATH
(or GUIX_LOCPATH.)

Thanks again for the review and suggestions!

Yours sincerely,
Abhishek Cherath.

[bug#70446] [PATCH v4] gnu: webkitgtk: Add access to system locale path and to paths from GUIX_LOCPATH, LOCPATH, and LIBVA_DRIVERS_PATH to gtk sandbox in order to silence gtk locale warnings and enable hardware accelerated video.

[Maxim Cournoyer]

Hi,

Abhishek Cherath <abhi@quic.us> writes:

> Hello,
>
>>> Thanks [liliana.prikler@gmail.com] for all the help :D, I thought
>>> about this a bit more and looked at all the utility stuff in
>>> BubblewrapLauncher.cpp. I realized that the correct thing to do here
>>> is to simply mount the LIBVA_DRIVERS_PATH paths. I'm wondering if
>>> this shouldn't be part of the gstreamer default mounts even upstream?
>>> along with the LOCPATH mount.
>> This patch LGTM.  I think submitting it upstream sans GUIX_LOCPATH
>> would be a great idea – that way, we'd have fewer things to patch.
>
> Sweet! I'll get on that sometime next month.
>
>> Is @localedir@ still needed with the bindPathVar in place?  Otherwise,
>> as already said, LGTM, and I'll look into forwarding it to/cherry-
>> picking it from gnome-team once I got new Webkit over there (still need
>> to wait for CI on that).
>
> Yes, it's still needed since Guix system doesn't generally set LOCPATH
> (or GUIX_LOCPATH.)
>
> Thanks again for the review and suggestions!

I just finished catching up with the thread.  Great to see the review
back and forth converging to an increasingly fancier solution :-).

-- 
Thanks,
Maxim