| Message ID | 20180922235333.85642-3-aperez@igalia.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | webkitgtk: update to the latest stable and add a number of fixes | expand |
Hello, On Sun, 23 Sep 2018 02:53:30 +0300, Adrian Perez de Castro wrote: > The woff2 dependency is used to support Web fonts in WOFF2 format. > This is a Web-facing feature that Web sites expect WebKit to support, > and it is recommended to be unconditionally enabled. While it is > possible to disable the feature at build time, upstream only recommends > doing so if the target system cannot provide a woff2 package. > > Signed-off-by: Adrian Perez de Castro <aperez@igalia.com> > --- > package/webkitgtk/Config.in | 1 + > package/webkitgtk/webkitgtk.mk | 3 ++- > 2 files changed, 3 insertions(+), 1 deletion(-) In general, we are not really happy with optional dependencies turned into mandatory dependencies. For example, the EFL package makes a lot of dependencies optional, even if upstream strongly recommends to have such dependencies enabled (to the point where efl.mk needs to pass --enable-i-really-know-what-i-am-doing-and-that-this-will-probably-break-things-and-i-will-fix-them-myself-and-send-patches-abb). However, since woff2 has very few dependencies, and woff2+brotli+host-pkgconf build in 19 seconds, it's nothing compared to webkitgtk, so I applied your patch. Thanks, Thomas
On Tue, 25 Sep 2018 22:52:55 +0200, Thomas Petazzoni <thomas.petazzoni@bootlin.com> wrote: > Hello, > > On Sun, 23 Sep 2018 02:53:30 +0300, Adrian Perez de Castro wrote: > > The woff2 dependency is used to support Web fonts in WOFF2 format. > > This is a Web-facing feature that Web sites expect WebKit to support, > > and it is recommended to be unconditionally enabled. While it is > > possible to disable the feature at build time, upstream only recommends > > doing so if the target system cannot provide a woff2 package. > > > > Signed-off-by: Adrian Perez de Castro <aperez@igalia.com> > > --- > > package/webkitgtk/Config.in | 1 + > > package/webkitgtk/webkitgtk.mk | 3 ++- > > 2 files changed, 3 insertions(+), 1 deletion(-) > > In general, we are not really happy with optional dependencies turned > into mandatory dependencies. For example, the EFL package makes a lot > of dependencies optional, even if upstream strongly recommends to have > such dependencies enabled (to the point where efl.mk needs to pass > --enable-i-really-know-what-i-am-doing-and-that-this-will-probably-break-things-and-i-will-fix-them-myself-and-send-patches-abb). Wow, that's a loooong command line option :) > However, since woff2 has very few dependencies, and > woff2+brotli+host-pkgconf build in 19 seconds, it's nothing compared to > webkitgtk, so I applied your patch. I will make a follow-up patch to make WOFF2 optional. I think it should be fine to enable it by default if BR2_PACKAGE_WOFF2 is enabled, and add a line in the help description telling that it is recommended to enable it. Cheers, -Adrián
>>>>> "Adrian" == Adrian Perez de Castro <aperez@igalia.com> writes: Hi, >> However, since woff2 has very few dependencies, and >> woff2+brotli+host-pkgconf build in 19 seconds, it's nothing compared to >> webkitgtk, so I applied your patch. > I will make a follow-up patch to make WOFF2 optional. I think it should be > fine to enable it by default if BR2_PACKAGE_WOFF2 is enabled, and add a line > in the help description telling that it is recommended to enable it. Correct. Now that we have updated kconfig we could even consider using the imply keyword to enable woff2 by default if it is really an option that most people should enable: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=237e3ad0f195d8fd34f1299e45f04793832a16fc
Hello Peter, On Thu, 27 Sep 2018 13:37:48 +0200, Peter Korsgaard <peter@korsgaard.com> wrote: > >>>>> "Adrian" == Adrian Perez de Castro <aperez@igalia.com> writes: > > Hi, > > >> However, since woff2 has very few dependencies, and > >> woff2+brotli+host-pkgconf build in 19 seconds, it's nothing compared to > >> webkitgtk, so I applied your patch. > > > I will make a follow-up patch to make WOFF2 optional. I think it should be > > fine to enable it by default if BR2_PACKAGE_WOFF2 is enabled, and add a line > > in the help description telling that it is recommended to enable it. > > Correct. Now that we have updated kconfig we could even consider using > the imply keyword to enable woff2 by default if it is really an option > that most people should enable: > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=237e3ad0f195d8fd34f1299e45f04793832a16fc This is just perfect. Thanks for pointing this out :-) Cheers, -Adrián
>>>>> "Adrian" == Adrian Perez de Castro <aperez@igalia.com> writes: > The woff2 dependency is used to support Web fonts in WOFF2 format. > This is a Web-facing feature that Web sites expect WebKit to support, > and it is recommended to be unconditionally enabled. While it is > possible to disable the feature at build time, upstream only recommends > doing so if the target system cannot provide a woff2 package. > Signed-off-by: Adrian Perez de Castro <aperez@igalia.com> Committed to 2018.02.x, 2018.05.x and 2018.08.x, thanks.
Hello Peter (and all the mailing list members), On Thu, 27 Sep 2018 13:37:48 +0200, Peter Korsgaard <peter@korsgaard.com> wrote: > >>>>> "Adrian" == Adrian Perez de Castro <aperez@igalia.com> writes: > > Hi, > > >> However, since woff2 has very few dependencies, and > >> woff2+brotli+host-pkgconf build in 19 seconds, it's nothing compared to > >> webkitgtk, so I applied your patch. > > > I will make a follow-up patch to make WOFF2 optional. I think it should be > > fine to enable it by default if BR2_PACKAGE_WOFF2 is enabled, and add a line > > in the help description telling that it is recommended to enable it. > > Correct. Now that we have updated kconfig we could even consider using > the imply keyword to enable woff2 by default if it is really an option > that most people should enable: > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=237e3ad0f195d8fd34f1299e45f04793832a16fc There's a small doubt I have here... How exactly would we prefer to have this? I can think of two options. One would be the following, and would allow to disable WOFF2 support in WebKitGTK+ even if the woff2 package is built: config BR2_PACKAGE_WEBKITGTK # ... imply BR2_PACKAGE_WEBKITGTK_WOFF2 config BR2_PACKAGE_WEBKITGTK_WOFF2 bool "woff2 support" depends on BR2_PACKAGE_WOFF2 imply BR2_PACKAGE_WOFF2 The other, simpler option, always enables WOFF2 support in WebKitGTK+ if the woff2 package can be built: config BR2_PACKAGE_WEBKITGTK # ... imply BR2_PACKAGE_WOFF2 With this second option, the only way of disabling WOFF2 support in WebKitGTK+ is *also* disabling the woff2 package completely. While this is not a problem in this case right now (I think WebKitGTK+ is the only user of the woff2 package, after all), I would like to apply a similar treatment to some of the build options. Any preference/suggestion/comments? Thanks in advance, -Adrián
>>>>> "Adrian" == Adrian Perez de Castro <aperez@igalia.com> writes: Hi, >> Correct. Now that we have updated kconfig we could even consider using >> the imply keyword to enable woff2 by default if it is really an option >> that most people should enable: >> >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=237e3ad0f195d8fd34f1299e45f04793832a16fc > There's a small doubt I have here... How exactly would we prefer to have this? > I can think of two options. One would be the following, and would allow to > disable WOFF2 support in WebKitGTK+ even if the woff2 package is built: > config BR2_PACKAGE_WEBKITGTK > # ... > imply BR2_PACKAGE_WEBKITGTK_WOFF2 > config BR2_PACKAGE_WEBKITGTK_WOFF2 > bool "woff2 support" > depends on BR2_PACKAGE_WOFF2 > imply BR2_PACKAGE_WOFF2 > The other, simpler option, always enables WOFF2 support in WebKitGTK+ if the > woff2 package can be built: > config BR2_PACKAGE_WEBKITGTK > # ... > imply BR2_PACKAGE_WOFF2 I would go for the 2nd option. I cannot think of any sensible use cases where we would want woff2 enabled but not use it in webkitgtk, and this matches what we do elsewhere for E.G. openssl.
On Thu, 11 Oct 2018 20:19:28 +0200, Peter Korsgaard <peter@korsgaard.com> wrote: > >>>>> "Adrian" == Adrian Perez de Castro <aperez@igalia.com> writes: > > Hi, > > >> Correct. Now that we have updated kconfig we could even consider using > >> the imply keyword to enable woff2 by default if it is really an option > >> that most people should enable: > >> > >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=237e3ad0f195d8fd34f1299e45f04793832a16fc > > > There's a small doubt I have here... How exactly would we prefer to have this? > > I can think of two options. One would be the following, and would allow to > > disable WOFF2 support in WebKitGTK+ even if the woff2 package is built: > > > config BR2_PACKAGE_WEBKITGTK > > # ... > > imply BR2_PACKAGE_WEBKITGTK_WOFF2 > > > config BR2_PACKAGE_WEBKITGTK_WOFF2 > > bool "woff2 support" > > depends on BR2_PACKAGE_WOFF2 > > imply BR2_PACKAGE_WOFF2 > > > The other, simpler option, always enables WOFF2 support in WebKitGTK+ if the > > woff2 package can be built: > > > config BR2_PACKAGE_WEBKITGTK > > # ... > > imply BR2_PACKAGE_WOFF2 > > I would go for the 2nd option. I cannot think of any sensible use cases > where we would want woff2 enabled but not use it in webkitgtk, and this > matches what we do elsewhere for E.G. openssl. For now I won't be sending a patch to do this because it's not clear whether everybody is okay using “imply” in Buildroot. Cheers, -Adrián
diff --git a/package/webkitgtk/Config.in b/package/webkitgtk/Config.in index df2aeef3d9..96a7ab0c94 100644 --- a/package/webkitgtk/Config.in +++ b/package/webkitgtk/Config.in @@ -42,6 +42,7 @@ config BR2_PACKAGE_WEBKITGTK select BR2_PACKAGE_SQLITE select BR2_PACKAGE_WEBP select BR2_PACKAGE_WEBP_DEMUX + select BR2_PACKAGE_WOFF2 select BR2_PACKAGE_XLIB_LIBXCOMPOSITE if BR2_PACKAGE_LIBGTK3_X11 select BR2_PACKAGE_XLIB_LIBXDAMAGE if BR2_PACKAGE_LIBGTK3_X11 select BR2_PACKAGE_XLIB_LIBXRENDER if BR2_PACKAGE_LIBGTK3_X11 diff --git a/package/webkitgtk/webkitgtk.mk b/package/webkitgtk/webkitgtk.mk index eccd9bbae5..f28417ac73 100644 --- a/package/webkitgtk/webkitgtk.mk +++ b/package/webkitgtk/webkitgtk.mk @@ -14,7 +14,7 @@ WEBKITGTK_LICENSE_FILES = \ Source/WebCore/LICENSE-LGPL-2.1 WEBKITGTK_DEPENDENCIES = host-ruby host-flex host-bison host-gperf \ enchant harfbuzz icu jpeg libgcrypt libgtk3 libsecret libsoup \ - libtasn1 libxml2 libxslt sqlite webp + libtasn1 libxml2 libxslt sqlite webp woff2 WEBKITGTK_CONF_OPTS = \ -DENABLE_API_TESTS=OFF \ -DENABLE_GEOLOCATION=OFF \ @@ -22,6 +22,7 @@ WEBKITGTK_CONF_OPTS = \ -DENABLE_INTROSPECTION=OFF \ -DENABLE_MINIBROWSER=ON \ -DENABLE_SPELLCHECK=ON \ + -DENABLE_WOFF2=ON \ -DPORT=GTK \ -DUSE_LIBNOTIFY=OFF \ -DUSE_LIBHYPHEN=OFF
The woff2 dependency is used to support Web fonts in WOFF2 format. This is a Web-facing feature that Web sites expect WebKit to support, and it is recommended to be unconditionally enabled. While it is possible to disable the feature at build time, upstream only recommends doing so if the target system cannot provide a woff2 package. Signed-off-by: Adrian Perez de Castro <aperez@igalia.com> --- package/webkitgtk/Config.in | 1 + package/webkitgtk/webkitgtk.mk | 3 ++- 2 files changed, 3 insertions(+), 1 deletion(-)