So that it's code does not bitrot.
Created attachment 439626 [details] Patch
Comment on attachment 439626 [details] Patch Ehh, I can't decide if this is a good change or not. It seems reasonable, so r=me if you decide to proceed. Here's what I'm thinking: building bubblewrap sandbox enabled on buildbots and EWS is definitely good to do. We surely want all tests to run with the sandbox enabled, because they are not meaningfully testing the code that users actually run otherwise (or, in cases where the sandbox is not yet enabled, the code that we want users to actually run). So we surely want to do that. The downside is that now we need to modify each downstream flatpak runtime and each flatpak app that bundles WebKitGTK to build with -DENABLE_BUBBLEWRAP_SANDBOX=OFF (because bubblewrap will not be available in these environments, since it is useless under flatpak). That is hardly a big deal. For the GNOME runtime, it would be a one-line change. It might confuse some app developers, but hopefully most app developers inherit WebKitGTK from a runtime instead of building it themselves. So... I think that's OK. The alternative way to accomplish this would be to enable -DENABLE_BUBBLEWRAP_SANDBOX at the buildbot config or build-webkit level rather than here in CMake. Modifying the config of every bot would be annoying, but perhaps this could be something that build-webkit just knows to pass, because build-webkit is the one special time that we want to build with bubblewrap sandbox enabled inside a flatpak environment? Whatever. Your call.
(In reply to Michael Catanzaro from comment #2) > We surely > want all tests to run with the sandbox enabled, because they are not > meaningfully testing the code that users actually run otherwise (or, in > cases where the sandbox is not yet enabled, the code that we want users to > actually run). So we surely want to do that. Also I know we want to avoid mistakes like: spend months developing feature using build-webkit, then discover it doesn't work with the sandbox enabled and is really hard to redesign. I understand you've been there, done that with a web process feature that was going to depend on host network access. That sounds horribly disheartening. Much better to notice the problems immediately instead.
In g-b-m you already build-depend on bwrap: https://gitlab.gnome.org/GNOME/gnome-build-meta/-/blob/master/elements/sdk/webkitgtk.bst#L12 Was that an oversight? IIUC the bwrap process launcher won't be used at runtime anyway if the UIProcess is running in a flatpak runtime.
(In reply to Philippe Normand from comment #4) > In g-b-m you already build-depend on bwrap: > > https://gitlab.gnome.org/GNOME/gnome-build-meta/-/blob/master/elements/sdk/ > webkitgtk.bst#L12 > > Was that an oversight? > > IIUC the bwrap process launcher won't be used at runtime anyway if the > UIProcess is running in a flatpak runtime. Or maybe I am misunderstanding this: https://github.com/WebKit/WebKit/blob/main/Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp#L174..L184
(In reply to Philippe Normand from comment #4) > In g-b-m you already build-depend on bwrap: > > https://gitlab.gnome.org/GNOME/gnome-build-meta/-/blob/master/elements/sdk/ > webkitgtk.bst#L12 > > Was that an oversight? An oversight in my previous comment, yes! But not in gnome-build-meta. That's there for GNOME OS. > IIUC the bwrap process launcher won't be used at runtime anyway if the > UIProcess is running in a flatpak runtime. Right, and gnome-build-meta proves that it works. I'm typing this comment in Tech Preview with bubblewrap sandbox enabled at build time but disabled at runtime. Just didn't realize it was enabled before you noticed. That said, it's still not available in the flatpak runtime (you'll notice we have it in sdk-deps to ensure it doesn't appear in the SDK) so every app bundling WebKit will have to pass -DENABLE_BUBBLEWRAP_SANDBOX=OFF. But apps should really probably not bundle WebKit. So I think it's fine. Still your call.
Committed r283436 (242423@main): <https://commits.webkit.org/242423@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 439626 [details].
(In reply to Philippe Normand from comment #4) > IIUC the bwrap process launcher won't be used at runtime anyway if the > UIProcess is running in a flatpak runtime. How this works? It may be interesting to do the same when running inside docker. Do you know which part of the code does this check?
https://github.com/WebKit/WebKit/blob/main/Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp#L174..L184 :)
As you can see, the check is there for docker already, but I'm not sure if it actually works for podman, it might be limited to moby only.