Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(1031)

Issue 2876853004: Gate SK_FREETYPE_MINIMUM_RUNTIME_VERSION by use_system_freetype flag (Closed)

Created:
3 years, 7 months ago by drott
Modified:
3 years, 6 months ago
CC:
chromium-reviews
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

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/+/f677dc5c2d440d6e074a1d624e8a0b7a68371e08

Patch Set 1 #

Patch Set 2 : Rebased #

Unified diffs Side-by-side diffs Delta from patch set Stats (+2 lines, -1 line) Patch
M skia/BUILD.gn View 1 2 chunks +2 lines, -1 line 0 comments Download

Messages

Total messages: 33 (16 generated)
drott
3 years, 7 months ago (2017-05-12 16:45:08 UTC) #4
drott
3 years, 7 months ago (2017-05-12 16:45:30 UTC) #5
drott
Mike or Florin, maybe you could take a look as well? Thank you.
3 years, 7 months ago (2017-05-16 07:51:21 UTC) #9
f(malita)
Based on the linked bug comments, rs lgtm.
3 years, 7 months ago (2017-05-16 14:19:28 UTC) #10
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2876853004/1
3 years, 7 months ago (2017-05-17 06:31:11 UTC) #12
commit-bot: I haz the power
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_ng/builds/446885)
3 years, 7 months ago (2017-05-17 08:56:47 UTC) #14
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2876853004/1
3 years, 7 months ago (2017-05-17 08:57:58 UTC) #16
commit-bot: I haz the power
Failed to apply patch for build/config/freetype/freetype.gni: While running git apply --index -3 -p1; Falling back ...
3 years, 7 months ago (2017-05-17 11:22:22 UTC) #18
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2876853004/20001
3 years, 7 months ago (2017-05-18 16:36:37 UTC) #21
commit-bot: I haz the power
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_ng/builds/448908)
3 years, 7 months ago (2017-05-18 22:29:34 UTC) #23
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2876853004/20001
3 years, 7 months ago (2017-05-19 05:54:35 UTC) #25
commit-bot: I haz the power
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/f677dc5c2d440d6e074a1d624e8a0b7a68371e08
3 years, 7 months ago (2017-05-19 07:09:45 UTC) #28
Jamie Madill
A revert of this CL (patchset #2 id:20001) has been created in https://codereview.chromium.org/2892163003/ by jmadill@chromium.org. ...
3 years, 7 months ago (2017-05-19 14:35:08 UTC) #29
bungeman-chromium
On 2017/05/19 14:35:08, Jamie Madill wrote: > A revert of this CL (patchset #2 id:20001) ...
3 years, 6 months ago (2017-06-07 15:00:13 UTC) #30
bungeman-chromium
On 2017/06/07 15:00:13, bungeman-chromium wrote: > On 2017/05/19 14:35:08, Jamie Madill wrote: > > A ...
3 years, 6 months ago (2017-06-07 17:15:50 UTC) #31
bungeman-chromium
On 2017/06/07 17:15:50, bungeman-chromium wrote: > On 2017/06/07 15:00:13, bungeman-chromium wrote: > > On 2017/05/19 ...
3 years, 6 months ago (2017-06-08 19:18:56 UTC) #32
bungeman-chromium
3 years, 6 months ago (2017-06-08 20:44:16 UTC) #33
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.

Powered by Google App Engine
This is Rietveld 408576698