| Message ID | 43974b799a22fd2b469494040b2ff02335f92315.1711259689.git.abhi@quic.us |
|---|---|
| State | New |
| Headers | show |
| Series | [bug#69971,v2] gnu: webkitgtk: Add locale and dri access to gtk sandbox in order to silence gtk locale warnings and enable hardware accelerated video, respectively. | expand |
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?
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? >
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
>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?
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
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
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
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.
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.