|
|
DescriptionGate SK_FREETYPE_MINIMUM_RUNTIME_VERSION by use_system_freetype flag
BUG=719345
R=bungeman
Review-Url: https://codereview.chromium.org/2876853004
Cr-Commit-Position: refs/heads/master@{#473117}
Committed: https://chromium.googlesource.com/chromium/src/+/f677dc5c2d440d6e074a1d624e8a0b7a68371e08
Patch Set 1 #Patch Set 2 : Rebased #Messages
Total messages: 33 (16 generated)
The CQ bit was checked by drott@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
drott@chromium.org changed reviewers: + bungeman@chromium.org - bungeman@gmail.com
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
drott@chromium.org changed reviewers: + fmalita@chromium.org, reed@chromium.org
Mike or Florin, maybe you could take a look as well? Thank you.
Based on the linked bug comments, rs lgtm.
The CQ bit was checked by drott@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by drott@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for build/config/freetype/freetype.gni: While running git apply --index -3 -p1; Falling back to three-way merge... Applied patch to 'build/config/freetype/freetype.gni' with conflicts. U build/config/freetype/freetype.gni Patch: NR build/config/freetype/BUILD.gn->build/config/freetype/freetype.gni Index: build/config/freetype/freetype.gni diff --git a/build/config/freetype/BUILD.gn b/build/config/freetype/freetype.gni similarity index 75% copy from build/config/freetype/BUILD.gn copy to build/config/freetype/freetype.gni index 976661a7cb84a80ee949f141fc95efdf711a20d1..b4eced2d6508f32483fc64a1a5a7ccfad75e4208 100644 --- a/build/config/freetype/BUILD.gn +++ b/build/config/freetype/freetype.gni @@ -2,8 +2,6 @@ # Use of this source code is governed by a BSD-style license that can be # found in the LICENSE file. -import("//build/config/features.gni") - declare_args() { # Blink needs a recent and properly build-configured FreeType version to # support OpenType variations, color emoji and avoid security bugs. By default @@ -14,13 +12,3 @@ declare_args() { # RENDERING AND SECURITY REGRESSIONS. use_system_freetype = false } - -group("freetype") { - if (use_system_freetype) { - public_configs = [ "//build/linux:freetype_from_pkgconfig" ] - } else { - public_deps = [ - "//third_party/freetype", - ] - } -}
The CQ bit was checked by drott@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fmalita@chromium.org Link to the patchset: https://codereview.chromium.org/2876853004/#ps20001 (title: "Rebased")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by drott@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 20001, "attempt_start_ts": 1495173238882240, "parent_rev": "fabad59de9cee87ffff2848c9497ae324d844eff", "commit_rev": "f677dc5c2d440d6e074a1d624e8a0b7a68371e08"}
Message was sent while issue was closed.
Description was changed from ========== Gate SK_FREETYPE_MINIMUM_RUNTIME_VERSION by use_system_freetype flag BUG=719345 R=bungeman ========== to ========== Gate SK_FREETYPE_MINIMUM_RUNTIME_VERSION by use_system_freetype flag BUG=719345 R=bungeman Review-Url: https://codereview.chromium.org/2876853004 Cr-Commit-Position: refs/heads/master@{#473117} Committed: https://chromium.googlesource.com/chromium/src/+/f677dc5c2d440d6e074a1d624e8a... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/f677dc5c2d440d6e074a1d624e8a...
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:20001) has been created in https://codereview.chromium.org/2892163003/ by jmadill@chromium.org. The reason for reverting is: This is causing a build failure on the Linux GPU FYI bots, likely because of a BUILD.gn error. First failing build: https://build.chromium.org/p/chromium.gpu.fyi/builders/GPU%20Linux%20Builder%... Error text: FAILED: gles2_conform_test_windowless python "../../build/toolchain/gcc_link_wrapper.py" --output="./gles2_conform_test_windowless" -- <snip> -lfreetype ./libskia.so: error: undefined reference to 'FT_Get_Var_Design_Coordinates' ./libskia.so: error: undefined reference to 'FT_Set_Default_Properties'.
Message was sent while issue was closed.
On 2017/05/19 14:35:08, Jamie Madill wrote: > A revert of this CL (patchset #2 id:20001) has been created in > https://codereview.chromium.org/2892163003/ by mailto:jmadill@chromium.org. > > The reason for reverting is: This is causing a build failure on the Linux GPU > FYI bots, likely because of a BUILD.gn error. > > First failing build: > https://build.chromium.org/p/chromium.gpu.fyi/builders/GPU%20Linux%20Builder%... > > Error text: > FAILED: gles2_conform_test_windowless > python "../../build/toolchain/gcc_link_wrapper.py" > --output="./gles2_conform_test_windowless" -- <snip> -lfreetype > ./libskia.so: error: undefined reference to 'FT_Get_Var_Design_Coordinates' > ./libskia.so: error: undefined reference to 'FT_Set_Default_Properties'. It appears that to build the failing target internal_gles2_conform_tests = true needs to be added to the args.gn. Once doing so gn desc out/debug/ //gpu/gles2_conform_support:gles2_conform_test_windowless libs shows that 'freetype' is a dependent library. Unfortunately, --blame doesn't currently work with libs. But this is trying to link against the system FreeType.
Message was sent while issue was closed.
On 2017/06/07 15:00:13, bungeman-chromium wrote: > On 2017/05/19 14:35:08, Jamie Madill wrote: > > A revert of this CL (patchset #2 id:20001) has been created in > > https://codereview.chromium.org/2892163003/ by mailto:jmadill@chromium.org. > > > > The reason for reverting is: This is causing a build failure on the Linux GPU > > FYI bots, likely because of a BUILD.gn error. > > > > First failing build: > > > https://build.chromium.org/p/chromium.gpu.fyi/builders/GPU%20Linux%20Builder%... > > > > Error text: > > FAILED: gles2_conform_test_windowless > > python "../../build/toolchain/gcc_link_wrapper.py" > > --output="./gles2_conform_test_windowless" -- <snip> -lfreetype > > ./libskia.so: error: undefined reference to 'FT_Get_Var_Design_Coordinates' > > ./libskia.so: error: undefined reference to 'FT_Set_Default_Properties'. > > It appears that to build the failing target > > internal_gles2_conform_tests = true > > needs to be added to the args.gn. Once doing so > > gn desc out/debug/ //gpu/gles2_conform_support:gles2_conform_test_windowless > libs > > shows that 'freetype' is a dependent library. Unfortunately, --blame doesn't > currently work with libs. But this is trying to link against the system > FreeType. So it appears that //gpu/gles2_conform_support:gles2_conform_test_windowless depends on //build/config/linux/gtk2 and # gn desc out/debug/ //build/config/linux/gtk2:gtk2 libs shows that this depends on system freetype. This comes from gtk2_internal_config which uses pkg_config on gtk+-2.0 which in the sysroot produces # PKG_CONFIG_PATH=build/linux/debian_jessie_amd64-sysroot/usr/lib/pkgconfig pkg-config --libs gtk+-2.0 -lgtk-x11-2.0 -lgdk-x11-2.0 -lpangocairo-1.0 -latk-1.0 -lcairo -lgdk_pixbuf-2.0 -lgio-2.0 -lpangoft2-1.0 -lpango-1.0 -lgobject-2.0 -lglib-2.0 -lfontconfig -lfreetype So that explains why we get -lfreetype but does not explain why ./libskia.so shows up on the link line but ./libfreetype.so.6 does not.
Message was sent while issue was closed.
On 2017/06/07 17:15:50, bungeman-chromium wrote: > On 2017/06/07 15:00:13, bungeman-chromium wrote: > > On 2017/05/19 14:35:08, Jamie Madill wrote: > > > A revert of this CL (patchset #2 id:20001) has been created in > > > https://codereview.chromium.org/2892163003/ by mailto:jmadill@chromium.org. > > > > > > The reason for reverting is: This is causing a build failure on the Linux > GPU > > > FYI bots, likely because of a BUILD.gn error. > > > > > > First failing build: > > > > > > https://build.chromium.org/p/chromium.gpu.fyi/builders/GPU%20Linux%20Builder%... > > > > > > Error text: > > > FAILED: gles2_conform_test_windowless > > > python "../../build/toolchain/gcc_link_wrapper.py" > > > --output="./gles2_conform_test_windowless" -- <snip> -lfreetype > > > ./libskia.so: error: undefined reference to 'FT_Get_Var_Design_Coordinates' > > > ./libskia.so: error: undefined reference to 'FT_Set_Default_Properties'. > > > > It appears that to build the failing target > > > > internal_gles2_conform_tests = true > > > > needs to be added to the args.gn. Once doing so > > > > gn desc out/debug/ //gpu/gles2_conform_support:gles2_conform_test_windowless > > libs > > > > shows that 'freetype' is a dependent library. Unfortunately, --blame doesn't > > currently work with libs. But this is trying to link against the system > > FreeType. > > So it appears that //gpu/gles2_conform_support:gles2_conform_test_windowless > depends on //build/config/linux/gtk2 and > > # gn desc out/debug/ //build/config/linux/gtk2:gtk2 libs > > shows that this depends on system freetype. This comes from gtk2_internal_config > which uses pkg_config on gtk+-2.0 which in the sysroot produces > > # PKG_CONFIG_PATH=build/linux/debian_jessie_amd64-sysroot/usr/lib/pkgconfig > pkg-config --libs gtk+-2.0 > > -lgtk-x11-2.0 -lgdk-x11-2.0 -lpangocairo-1.0 -latk-1.0 -lcairo -lgdk_pixbuf-2.0 > -lgio-2.0 -lpangoft2-1.0 -lpango-1.0 -lgobject-2.0 -lglib-2.0 -lfontconfig > -lfreetype > > So that explains why we get -lfreetype but does not explain why ./libskia.so > shows up on the link line but ./libfreetype.so.6 does not. Inside libskia.so it says that it requires libfreetype.so.6, which would by default be the one in the same (build) directory, but because -lfreetype is on the link line that one gets used instead. It appears that other targets are getting ./libfreetype.so.6 semi-directly through other targets (particularly harfbuzz-ng and pdfium) putting the :freetype target in their public_deps instead of deps. Indeed, if I move :freetype from deps to public_deps in :skia then everything appears to work. However, this isn't really what we want as Skia doesn't actually push any FreeType types in its headers. I think what it comes down to is that in a component build we have two libfreetype.so.6, one we build and one in the sysroot. I'm not sure we can really satisfy both at the same time. FreeType should always be forward compatible (they are quite mindful of that) so we should just always take the newer one (which should always be the one we build in), at least in a component build. Is it still necessary to name our component build version of FreeType libfreetype.so.6? If not, we could just change the name.
Message was sent while issue was closed.
On 2017/06/08 19:18:56, bungeman-chromium wrote: > On 2017/06/07 17:15:50, bungeman-chromium wrote: > > On 2017/06/07 15:00:13, bungeman-chromium wrote: > > > On 2017/05/19 14:35:08, Jamie Madill wrote: > > > > A revert of this CL (patchset #2 id:20001) has been created in > > > > https://codereview.chromium.org/2892163003/ by > mailto:jmadill@chromium.org. > > > > > > > > The reason for reverting is: This is causing a build failure on the Linux > > GPU > > > > FYI bots, likely because of a BUILD.gn error. > > > > > > > > First failing build: > > > > > > > > > > https://build.chromium.org/p/chromium.gpu.fyi/builders/GPU%20Linux%20Builder%... > > > > > > > > Error text: > > > > FAILED: gles2_conform_test_windowless > > > > python "../../build/toolchain/gcc_link_wrapper.py" > > > > --output="./gles2_conform_test_windowless" -- <snip> -lfreetype > > > > ./libskia.so: error: undefined reference to > 'FT_Get_Var_Design_Coordinates' > > > > ./libskia.so: error: undefined reference to 'FT_Set_Default_Properties'. > > > > > > It appears that to build the failing target > > > > > > internal_gles2_conform_tests = true > > > > > > needs to be added to the args.gn. Once doing so > > > > > > gn desc out/debug/ //gpu/gles2_conform_support:gles2_conform_test_windowless > > > libs > > > > > > shows that 'freetype' is a dependent library. Unfortunately, --blame doesn't > > > currently work with libs. But this is trying to link against the system > > > FreeType. > > > > So it appears that //gpu/gles2_conform_support:gles2_conform_test_windowless > > depends on //build/config/linux/gtk2 and > > > > # gn desc out/debug/ //build/config/linux/gtk2:gtk2 libs > > > > shows that this depends on system freetype. This comes from > gtk2_internal_config > > which uses pkg_config on gtk+-2.0 which in the sysroot produces > > > > # PKG_CONFIG_PATH=build/linux/debian_jessie_amd64-sysroot/usr/lib/pkgconfig > > pkg-config --libs gtk+-2.0 > > > > -lgtk-x11-2.0 -lgdk-x11-2.0 -lpangocairo-1.0 -latk-1.0 -lcairo > -lgdk_pixbuf-2.0 > > -lgio-2.0 -lpangoft2-1.0 -lpango-1.0 -lgobject-2.0 -lglib-2.0 -lfontconfig > > -lfreetype > > > > So that explains why we get -lfreetype but does not explain why ./libskia.so > > shows up on the link line but ./libfreetype.so.6 does not. > > Inside libskia.so it says that it requires libfreetype.so.6, which would by > default be the one in the same (build) directory, but because -lfreetype is on > the link line that one gets used instead. It appears that other targets are > getting ./libfreetype.so.6 semi-directly through other targets (particularly > harfbuzz-ng and pdfium) putting the :freetype target in their public_deps > instead of deps. Indeed, if I move :freetype from deps to public_deps in :skia > then everything appears to work. However, this isn't really what we want as Skia > doesn't actually push any FreeType types in its headers. > > I think what it comes down to is that in a component build we have two > libfreetype.so.6, one we build and one in the sysroot. I'm not sure we can > really satisfy both at the same time. FreeType should always be forward > compatible (they are quite mindful of that) so we should just always take the > newer one (which should always be the one we build in), at least in a component > build. Is it still necessary to name our component build version of FreeType > libfreetype.so.6? If not, we could just change the name. I have been able to get everything checked out and have reproduced the issue locally. As expected, manually editing out/debug/obj/gpu/gles2_conform_support/gles2_conform_test_windowless.ninja and removing the -lfreetype from the link line allows for linking and and running. I commented out the bit about adding "//build/config/linux/gtk2" to deps and it still builds and runs fine, it doesn't seem like it's needed anymore. I may put up a CL to remove it. |