The script generate-bundle allows to generate a bundle of "jsc" or "MiniBrowser". The script currently has support for two types of bundles 1) --syslibs=generate-install-script will create a install script to install all the libraries the binary depends on, but this only works on Debian based distros. Problem: It requires that the user runs the bundle on the same distro the bundle was created 2) --syslibs=bundle-all It will bundle all the libraries inside the bundle and will use those libraries instead of the system ones. In theory this would allow to create a self-contained bundle that runs anywhere. Problem: This is currently only working ok for jsc bundles. The MiniBrowser bundles are not working as expected because there are many libraries (many opened via dlopen()) that the script doesn't capture correctly. There are also issues with system resources (icons, fonts, etc) So this bug is for allowing generate-bundle to create a bundle for the MiniBrowser with --syslibs=bundle-all that can work on any distro without issues and bundles everything needed (including icons or fonts) The goal is: 1) To replace the bundles at https://webkitgtk.org/built-products with one "generic" bundle that can work on any distro (Fedora, Ubuntu, Alpine, etc) Currently we only support Ubuntu because we are generating the bundles on the "packaging ubuntu" bots at https://build.webkit.org/#/builders 2) To have the bundles based on the official builds (flatpak one). That allows external CIs (https://wpt.fyi for example) to test the same build that we test (the official one) Currently on the bundles uploaded at https://webkitgtk.org/built-products we don't enable all features we enable on the official build because Ubuntu doesn't have some requires dependencies (for example libsoup3) 3) Reduce maintenance That way we can remove the "packaging ubuntu" bots and generate the bundles on the release bot directly
Created attachment 453027 [details] Patch
Created attachment 453064 [details] Patch
> 1) To replace the bundles at https://webkitgtk.org/built-products with one "generic" bundle that can work on any distro (Fedora, Ubuntu, Alpine, etc) Currently we only support Ubuntu because we are generating the bundles on the "packaging ubuntu" bots at https://build.webkit.org/#/builders Will the new bundles remain usable within the Flatpak SDK runtime as they are currently? I ask because the GNOME Web Canary relies on this assumption.
(In reply to Philippe Normand from comment #3) > > 1) To replace the bundles at https://webkitgtk.org/built-products with one "generic" bundle that can work on any distro (Fedora, Ubuntu, Alpine, etc) > Currently we only support Ubuntu because we are generating the bundles on > the "packaging ubuntu" bots at https://build.webkit.org/#/builders > > Will the new bundles remain usable within the Flatpak SDK runtime as they > are currently? > > I ask because the GNOME Web Canary relies on this assumption. Yes, I think there will no be change regarding that. AFAIK the built products used by GNOME Web Canary are the ones from the release bot which are uploaded to https://webkitgtk-release.igalia.com/built-products/ and those are different than the ones at https://webkitgtk.org/built-products The ones at https://webkitgtk-release.igalia.com/built-products/ are produced in the release bot and only work if you run them inside flatpak. I don't intend to do any change to them, those are useful for sharing the built product between bots (for example for passing the built-product from the build-release to the test-release) What this new patch adds is a bundle based on those built-products that also included all the system libraries from the flatpak inside the .zip .. so you can run it outside flatpak without needing flatpak. This bundle with those extra libraries is intended to be used in any system that doesn't want (or can't) depend on flatpak. Running flatpak inside docker is a problem (requires a privileged docker container), and many third-party CIs like WPT run inside docker containers, so this bundle allows to work around that problem: you can download this .zip and execute it and it will work without issues. It is also lighter than the whole flatpak. The zip file with webkit and all the libraries and resources is around 250MB
(In reply to Carlos Alberto Lopez Perez from comment #4) > (In reply to Philippe Normand from comment #3) > > > 1) To replace the bundles at https://webkitgtk.org/built-products with one "generic" bundle that can work on any distro (Fedora, Ubuntu, Alpine, etc) > > Currently we only support Ubuntu because we are generating the bundles on > > the "packaging ubuntu" bots at https://build.webkit.org/#/builders > > > > Will the new bundles remain usable within the Flatpak SDK runtime as they > > are currently? > > > > I ask because the GNOME Web Canary relies on this assumption. > > Yes, I think there will no be change regarding that. > > AFAIK the built products used by GNOME Web Canary are the ones from the > release bot which are uploaded to > https://webkitgtk-release.igalia.com/built-products/ and those are different > than the ones at https://webkitgtk.org/built-products > > The ones at https://webkitgtk-release.igalia.com/built-products/ are > produced in the release bot and only work if you run them inside flatpak. I > don't intend to do any change to them, those are useful for sharing the > built product between bots (for example for passing the built-product from > the build-release to the test-release) > OK, thanks for clearing this out, I was confused. > What this new patch adds is a bundle based on those built-products that also > included all the system libraries from the flatpak inside the .zip .. so you > can run it outside flatpak without needing flatpak. This bundle with those > extra libraries is intended to be used in any system that doesn't want (or > can't) depend on flatpak. > Running flatpak inside docker is a problem (requires a privileged docker > container), and many third-party CIs like WPT run inside docker containers, > so this bundle allows to work around that problem OK, I understand the intent now. I won't go against this approach, but did you know the SDK can be exported to Docker/OCI images too? I don't know if it's as light in terms of size though, compared to what you came up with.
> OK, I understand the intent now. I won't go against this approach, but did you know the SDK can be exported to Docker/OCI images too? I don't know if it's as light in terms of size though, compared to what you came up with. Thanks for the info, but AFAIK exporting the SDK to docker images won't be useful for the use case that this approach tries to serve. This external CIs want to run all the testing inside their own docker images. For example, the WPT CI runs inside their own docker image based on Ubuntu-20.04 and it won't be feasible for them to use a different docker image just for the webkit-linux tests, as they use the same docker image for all the Linux-based browser testing. And executing a docker image inside another docker (nested docker) requires a privileged docker container (AFAIK), so it will have the same issues than trying to run flatpak inside docker. I have uploaded here: https://people.igalia.com/clopez/wkbug/237107/ a test build of what this script produces. In theory you should be able to uncompress this and run it in any distro without needing to install anything or requiring any extra privilege (like sudo).
I'm still learning the docker way of things, but maybe a multi-stage setup could work for them? https://docs.docker.com/develop/develop-images/multistage-build/#use-multi-stage-builds
(In reply to Philippe Normand from comment #7) > I'm still learning the docker way of things, but maybe a multi-stage setup > could work for them? > https://docs.docker.com/develop/develop-images/multistage-build/#use-multi- > stage-builds I don't see how that will be useful in this case. The browser is not part of the image. The browser gets downloaded at test time, uncompressed and executed. Running apt-get to install dependencies at test time is ok, but it is not ok to execute another container (calling syscall pivot_root) because docker doesn't allow that unless it is run as privileged. Chrome and Firefox provide binaries that can work on almost any distro and that don't require setting up a container to run, that is what this bundle intends to do as well.
Note also that this patch will allow to remove the jhbuild insfrastructure from WebKit, since we are currently only keeping the jhbuild insfrastructure for the "ubuntu packaging" bots that will be deprecated by this.
Comment on attachment 453064 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=453064&action=review LGTM at first glance, I didn't know about these scripts before, so perhaps you want a review from someone more familiar with that code base. I worry a bit about hardcoding so many paths in the script, I'm afraid this could easily bitrot. Do you think some kind of unit test could be added, or is this un-realistic? > Source/WebCore/platform/network/soup/SoupNetworkSession.cpp:143 > + GUniqueOutPtr<GError> error; Move this to the if ;)
(In reply to Philippe Normand from comment #10) > Comment on attachment 453064 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=453064&action=review > > LGTM at first glance, I didn't know about these scripts before, so perhaps > you want a review from someone more familiar with that code base. I worry a > bit about hardcoding so many paths in the script, I'm afraid this could > easily bitrot. Do you think some kind of unit test could be added, or is > this un-realistic? > I have reworked a few functions to avoid hardcoding as much as possible the extra library names and also added extra checks to ensure that it either finds the libraries or exits with error. The script tries to check every path and if something is not found raises an error. However, to be on the safe side, I guess it can be worth to add an extra step on the bots to QA the generated bundle (something like loading a few test case with webdriver to ensure the browser loads and works as expected) before uploading the bundle. But I think that will be better done in a different patch after this lands because i will need to implement something like a custom webdriver runner script for this.
Created attachment 454153 [details] Patch diff to previous patch: http://sprunge.us/1K7RqW?diff
ping reviewers?
Comment on attachment 454153 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=454153&action=review Seems like a major effort to avoid using flatpak, but whatever.... > Tools/Scripts/generate-bundle:824 > + # See: https://github.com/p11-glue/p11-kit/issues/68 > + # Starting a p11-kit server is problematic for the use case of the bundle, and also will only work if the host is configured to use p11-kit > + # So we take a different approach here. > + # 1. We generate a bundle-ca-certificates.pem file with all the certs of the system were the bundle was generated and we ship this file > + # 2. At runtime we look for the system TLS ca-bundle.pem file and we use it. If we can't find it then we default to using the one shipped inside the bundle. > + # We pass the path to the TLS ca-bundle.pem file using the env var WEBKIT_TLS_CAFILE_PEM > + # The host p11-kit setup may still work if it is using the same paths for loading the p11-kit modules, but otherwise it will fail to load the modules. > + # That is ok, since we don't need anymore a working p11-kit setup. If it works it will be used, but if not we have a fallback. > + # To debug this is useful to export P11_KIT_DEBUG=all > + # This approach also allows this bundle script go generate a working bundle when gnutls is not configured to use p11-kit. There is a downside: this means GTlsConnection has to rely on GTlsDatabase to perform certificate verification, so you lose the benefit of "TLS session context" in performing the certificate verification. GTlsDatabase is just not as smart as GTlsConnection can be. For example, certificate revocation checks (specifically stapled OCSP) will not be performed if you use your own GTlsDatabase (including via setting libsoup's ssl-ca-file). Certificate basic constraints won't be honored either: believe it or not, GTlsDatabase will happily accept X.509 certificates that are only valid for S/MIME and not for TLS, whereas GTlsConnection can be smarter. These are extremely surprising and non-intuitive consequences, but I can't think of any way to fix it, so I've resorted to just documenting it and hoping people will notice and think twice. I'm not saying "don't do it like this" because there's probably no other way to accomplish what you're trying to do. But maybe add a warning comment. Instead of just "Optionally load a custom path with the TLS CAFile. This is used on the GTK/WPE bundles. See script generate-bundle" you could also add "Don't ever enable this outside DEVELOPER_MODE because using a non-default TLS database has surprising undesired effects on how certificate verification is performed. See https://gitlab.gnome.org/GNOME/glib/-/commit/82999879bcc53c0cdc16c01b62c0224081521dcf for details."
Comment on attachment 454153 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=454153&action=review > Source/WebCore/platform/network/soup/SoupNetworkSession.cpp:151 > +#if ENABLE(DEVELOPER_MODE) > + // Optionally load a custom path with the TLS CAFile. This is used on the GTK/WPE bundles. See script generate-bundle > + const char* customTLSCAFile = g_getenv("WEBKIT_TLS_CAFILE_PEM"); > +#if USE(SOUP2) > + if (customTLSCAFile) > + g_object_set(m_soupSession.get(), "ssl-ca-file", customTLSCAFile, NULL); > +#else > + if (customTLSCAFile) { > + GUniqueOutPtr<GError> error; > + GRefPtr<GTlsDatabase> customTLSDB = adoptGRef(g_tls_file_database_new(customTLSCAFile, &error.outPtr())); > + if (error) > + WTFLogAlways("Failed to load TLS database \"%s\": %s", customTLSCAFile, error->message); > + soup_session_set_tls_database(m_soupSession.get(), customTLSDB.get()); > + } > +#endif // USE(SOUP2) > +#endif // ENABLE(DEVELOPER_MODE) I don't like this either, but if there's no other option... Anyway, we can reduce the ifdef code here. Note that soup2 also has the tls-databse property, we can reduce the ifdef to set_tls_database or g_object_set. Or even better we can add soup_session_set_tls_database for soup2 in SoupVersioning.h and then we don't need any ifdef.
Took a look at the parts of the code I've worked on before. They look reasonable, but not sure about the intent behind some of these changes. What's the idea behind the separation between lib and sys/lib in the bundle? I could only find > # We only want to separate the libraries in lib and sys/lib for MiniBrowser and 'all' bundles but I'm not immediately clear as to the 'why'. A lot of effort goes into implementing patchelf_setinterpreter_relativepath; the only motivation I could find is > # If we have patched the binaries to use a relative relpath then load the binary directly without prefixing it with the interpreter (it allows the process to use the correct progname) Presumably this isn't just a cosmetic change? If so, would be good to know what depends on this. Conversely, if patchelf_setinterpreter_relativepath is necessary, would it make sense to streamline the logic and always do that?
Comment on attachment 454153 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=454153&action=review >> Source/WebCore/platform/network/soup/SoupNetworkSession.cpp:151 >> +#endif // ENABLE(DEVELOPER_MODE) > > I don't like this either, but if there's no other option... Anyway, we can reduce the ifdef code here. Note that soup2 also has the tls-databse property, we can reduce the ifdef to set_tls_database or g_object_set. Or even better we can add soup_session_set_tls_database for soup2 in SoupVersioning.h and then we don't need any ifdef. Also, as a reminder: to make completely relocatable binaries which do not depend on an external script pre-setting certain environment variables (which is finicky), in Linux a binary can readlink("/proc/self/exe", ...) to determine its own location, and derive paths from the result.
(In reply to Angelos Oikonomopoulos from comment #16) > Took a look at the parts of the code I've worked on before. They look > reasonable, but not sure about the intent behind some of these changes. > > What's the idea behind the separation between lib and sys/lib in the bundle? > I could only find > > > # We only want to separate the libraries in lib and sys/lib for MiniBrowser and 'all' bundles > > but I'm not immediately clear as to the 'why'. > I did this separation with the idea (in the future) of splitting the zip in two files: a flatpak_core_$uniquehash.zip and a minibrowser_gtk_rxx.zip bundle where the minibrowser_gtk_rxx.zip one includes a script to download its respective flatpak_core_$uniquehash.zip before starting the browser. If you compare the files inside the bundle on revision X with the files on a bundle for revision X+1, you will see that all the libraries that go into sys/ are identical, so this can be seized to optimize disk space on the server that hosts the bundles The part of the bundle that goes into flatpak_core_rxxx.zip should only change when the flatpak SDK is updated, which is not too often (maybe once per month) So instead of storing a bundle of 200MB per revision, you can end storing a bundle of 50MB per revision and a flatpak_core_$uniquehash.zip of 150MB once per month. > A lot of effort goes into implementing patchelf_setinterpreter_relativepath; > the only motivation I could find is > > > # If we have patched the binaries to use a relative relpath then load the binary directly without prefixing it with the interpreter (it allows the process to use the correct progname) > > Presumably this isn't just a cosmetic change? If so, would be good to know > what depends on this. > There are two motivations behind this change: 1. The main one: It allows to use the real process name (like MiniBrowser or WebKitWebProcess) which then makes things less confusing.. seeing a process named ld-linux-x86-64.so.2 is a bit confusing. 2. A secondary one: The gstreamer libraries at startup call a process named 'gst-plugin-scanner' that inspects all gst plugins and builds a db/cache with them.. so if i didn't changed the interpreter on that 'gst-plugin-scanner' program then i would have to wrap it on another shell script. > Conversely, if patchelf_setinterpreter_relativepath is necessary, would it > make sense to streamline the logic and always do that? When I uploaded the initial patch here, the EWS for jsc-mips tests broke because I initially applied this change also to the script bundle-binary that it uses. I suspect the breakage was caused because patchelf doesn't work well in MIPS, but I'm not sure. For example, this fixes https://github.com/NixOS/patchelf/pull/180 are still not present in the version of patchelf we ship on the flatpak (0.10) So I'm keeping this as an option for now.
(In reply to Michael Catanzaro from comment #14) > [...] > I'm not saying "don't do it like this" because there's probably no other way > to accomplish what you're trying to do. But maybe add a warning comment. > Instead of just "Optionally load a custom path with the TLS CAFile. This is > used on the GTK/WPE bundles. See script generate-bundle" you could also add > "Don't ever enable this outside DEVELOPER_MODE because using a non-default > TLS database has surprising undesired effects on how certificate > verification is performed. See > https://gitlab.gnome.org/GNOME/glib/-/commit/ > 82999879bcc53c0cdc16c01b62c0224081521dcf for details." Really interesting, thanks for the info. I will add a comment on the code. (In reply to Carlos Garcia Campos from comment #15) > [...] > I don't like this either, but if there's no other option... Anyway, we can > reduce the ifdef code here. Note that soup2 also has the tls-databse > property, we can reduce the ifdef to set_tls_database or g_object_set. Or > even better we can add soup_session_set_tls_database for soup2 in > SoupVersioning.h and then we don't need any ifdef. I'm uploading a new version of the patch with this change. Thanks for the tip!
Created attachment 455975 [details] Patch
Created attachment 456010 [details] Patch
Let's try this! rs=me
Committed r292109 (249028@trunk): <https://commits.webkit.org/249028@trunk>
Committed r292584 (249417@trunk): <https://commits.webkit.org/249417@trunk>
Committed r292769 (249553@trunk): <https://commits.webkit.org/249553@trunk>