[Chromecast] Depend on Chromium's fontconfig. (issue 1585523002 by bcf@chromium.org)

[b...@chromium.org]

Reviewers: slan, wzhong, mbjorge
CL: https://codereview.chromium.org/1585523002/

Description:
[Chromecast] Depend on Chromium's fontconfig.

BUG= internal b/26249831
TEST= builds and runs with gyp and gn

Base URL: https://chromium.googlesource.com/chromium/src@master

Affected files (+26, -12 lines):
M build/common.gypi
M build/linux/BUILD.gn
M build/linux/system.gyp
M skia/BUILD.gn
M third_party/fontconfig/BUILD.gn
M third_party/harfbuzz-ng/BUILD.gn


Index: build/common.gypi
diff --git a/build/common.gypi b/build/common.gypi
index
e73e563dbc19ce32561c38cd616ec72ce725e500..eb6774170a3a24e05eb7577b313034b6df4102f6
100644
--- a/build/common.gypi
+++ b/build/common.gypi
@@ -1862,7 +1862,7 @@
'use_system_fontconfig%': 1,
}],
['chromecast==1', {
- 'use_custom_freetype%': 0,
+ 'use_system_freetype%': 0,
'use_playready%': 0,
'conditions': [
['target_arch=="arm"', {
Index: build/linux/BUILD.gn
diff --git a/build/linux/BUILD.gn b/build/linux/BUILD.gn
index
6a1939dd15b933f40123dd6992facfddc3ccb976..8665294324d5b363ecd170f1fe41a7c05bccde0f
100644
--- a/build/linux/BUILD.gn
+++ b/build/linux/BUILD.gn
@@ -12,6 +12,7 @@ gypi_values = exec_script("//build/gypi_to_gn.py",
[ "system.gyp" ])

use_system_fontconfig = !is_chromecast
+use_system_freetype = !is_chromecast

# If brlapi isn't needed, don't require it to be installed.
if (use_brlapi) {
@@ -115,3 +116,13 @@ group("fontconfig") {
]
}
}
+
+group("freetype2") {
+ if (use_system_freetype) {
+ public_configs = [ "//build/config/linux:freetype2" ]
+ } else {
+ public_deps = [
+ "//third_party/freetype2",
+ ]
+ }
+}
Index: build/linux/system.gyp
diff --git a/build/linux/system.gyp b/build/linux/system.gyp
index
fe6bf8c318027d91d7ea1967f0569581ba02b474..8380b0baa9bfc1292c1d89e9eec808c0f764e9c9
100644
--- a/build/linux/system.gyp
+++ b/build/linux/system.gyp
@@ -12,6 +12,7 @@
'linux_link_libpci%': 0,
'linux_link_libspeechd%': 0,
'linux_link_libbrlapi%': 0,
+ 'use_system_freetype%': 1,

# Used below for the various libraries. In this scope for sharing with
GN.
'libbrlapi_functions': [
@@ -780,7 +781,14 @@
'target_name': 'freetype2',
'type': 'none',
'conditions': [
- ['_toolset=="target"', {
+ ['use_system_freetype==0', {
+ 'dependencies': [
+ '../../third_party/freetype2/freetype2.gyp:freetype2',
+ ],
+ 'export_dependent_settings' : [
+ '../../third_party/freetype2/freetype2.gyp:freetype2',
+ ],
+ }, '_toolset=="target"', {
'direct_dependent_settings': {
'cflags': [
'<!@(<(pkg-config) --cflags freetype2)',
Index: skia/BUILD.gn
diff --git a/skia/BUILD.gn b/skia/BUILD.gn
index
1b38d807d8e625893e33311836abef58858af88b..747fc2de43dfeb7c474fb0f394a9050ebc2fd0df
100644
--- a/skia/BUILD.gn
+++ b/skia/BUILD.gn
@@ -458,13 +458,12 @@ component("skia") {
]

if (is_linux) {
- configs += [ "//build/config/linux:freetype2" ]
-
if (use_pango) {
configs += [ "//build/config/linux:pangocairo" ]
}
deps += [
"//build/linux:fontconfig",
+ "//build/linux:freetype2",
"//third_party/icu:icuuc",
]
}
Index: third_party/fontconfig/BUILD.gn
diff --git a/third_party/fontconfig/BUILD.gn
b/third_party/fontconfig/BUILD.gn
index
d77d84da297862549299f9bf51262d23a646ade8..cf6cfadbac4d9dae0da3b34333007fb7d5c6159c
100644
--- a/third_party/fontconfig/BUILD.gn
+++ b/third_party/fontconfig/BUILD.gn
@@ -52,15 +52,13 @@ component("fontconfig") {
]

deps = [
+ "//build/linux:freetype2",
"//third_party/libxml",
"//third_party/zlib",
]

configs -= [ "//build/config/compiler:chromium_code" ]
- configs += [
- "//build/config/compiler:no_chromium_code",
- "//build/config/linux:freetype2",
- ]
+ configs += [ "//build/config/compiler:no_chromium_code" ]

public_configs = [ ":fontconfig_config" ]

Index: third_party/harfbuzz-ng/BUILD.gn
diff --git a/third_party/harfbuzz-ng/BUILD.gn
b/third_party/harfbuzz-ng/BUILD.gn
index
87f51acc62e225439c68c4cb7aa5ca2809a8ef77..4824a4f864bfcb16e24cc7571d7bbef7be739726
100644
--- a/third_party/harfbuzz-ng/BUILD.gn
+++ b/third_party/harfbuzz-ng/BUILD.gn
@@ -201,11 +201,9 @@ if (use_system_harfbuzz) {
# See crbug.com/462689.
if (is_linux && use_pango && !is_chromeos && !is_official_build &&
current_cpu != "arm" && current_cpu != "mipsel") {
+ deps += [ "//build/linux:freetype2" ]
configs -= [ "//build/config/gcc:symbol_visibility_hidden" ]
- configs += [
- "//build/config/linux:freetype2",
- "//build/config/linux:glib",
- ]
+ configs += [ "//build/config/linux:glib" ]
sources += [
"src/hb-ft.cc",
"src/hb-ft.h",

[b...@chromium.org]

This implementation is based on how fontconfig is handled.

https://codereview.chromium.org/1585523002/

[sl...@chromium.org]

A well executed change, IMO. A couple of questions.


https://codereview.chromium.org/1585523002/diff/1/build/common.gypi
File build/common.gypi (left):

https://codereview.chromium.org/1585523002/diff/1/build/common.gypi#oldcode1865
build/common.gypi:1865: 'use_custom_freetype%': 0,
I think we should purge this flag from the codebase (see
content_shell.gypi), now that we're not using it any more. It should be
fine to unconditionally bring in the freetype dependency for Linux
builds there.

https://codereview.chromium.org/1585523002/diff/1/build/linux/BUILD.gn
File build/linux/BUILD.gn (right):

https://codereview.chromium.org/1585523002/diff/1/build/linux/BUILD.gn#newcode122
build/linux/BUILD.gn:122: public_configs = [
"//build/config/linux:freetype2" ]
The visibility for "//build/config/linux:freetype2" should be restricted
to this target, ie:

pkg_config("freetype2") {
visibility = [ "//build/linux:freetype2" ]
...
}

This will force the use of this target for all builds.

https://codereview.chromium.org/1585523002/diff/1/build/linux/BUILD.gn#newcode125
build/linux/BUILD.gn:125: "//third_party/freetype2",
Ideally the same thing would apply to this target. It may get some
pushback, but we should try it.

https://codereview.chromium.org/1585523002/diff/1/build/linux/system.gyp
File build/linux/system.gyp (right):

https://codereview.chromium.org/1585523002/diff/1/build/linux/system.gyp#newcode784
build/linux/system.gyp:784: ['use_system_freetype==0', {
Is this supposed to be instead of the logic below? Currently the
pkg_config build settings will also apply.

https://codereview.chromium.org/1585523002/

[b...@chromium.org]

https://codereview.chromium.org/1585523002/diff/1/build/common.gypi
File build/common.gypi (left):

https://codereview.chromium.org/1585523002/diff/1/build/common.gypi#oldcode1865
build/common.gypi:1865: 'use_custom_freetype%': 0,

On 2016/01/13 18:43:58, slan wrote:
> I think we should purge this flag from the codebase (see
content_shell.gypi),
> now that we're not using it any more. It should be fine to
unconditionally bring
> in the freetype dependency for Linux builds there.

Done.

https://codereview.chromium.org/1585523002/diff/1/build/linux/BUILD.gn
File build/linux/BUILD.gn (right):

https://codereview.chromium.org/1585523002/diff/1/build/linux/BUILD.gn#newcode122
build/linux/BUILD.gn:122: public_configs = [
"//build/config/linux:freetype2" ]

On 2016/01/13 18:43:58, slan wrote:
> The visibility for "//build/config/linux:freetype2" should be
restricted to this
> target, ie:

> pkg_config("freetype2") {
> visibility = [ "//build/linux:freetype2" ]
> ...
> }

> This will force the use of this target for all builds.

Done.

https://codereview.chromium.org/1585523002/diff/1/build/linux/BUILD.gn#newcode125
build/linux/BUILD.gn:125: "//third_party/freetype2",

On 2016/01/13 18:43:58, slan wrote:
> Ideally the same thing would apply to this target. It may get some
pushback, but
> we should try it.

It seems like //third_party/freetype2 is used by
//content/shell:content_shell_lib

https://codereview.chromium.org/1585523002/diff/1/build/linux/system.gyp
File build/linux/system.gyp (right):

https://codereview.chromium.org/1585523002/diff/1/build/linux/system.gyp#newcode784
build/linux/system.gyp:784: ['use_system_freetype==0', {

On 2016/01/13 18:43:58, slan wrote:
> Is this supposed to be instead of the logic below? Currently the
pkg_config
> build settings will also apply.

Yes this is supposed to be instead of the logic below.

The intended behavior is

if (use_system_freetype==0) {
... use third party freetype
} else {
if (_toolset=="target") {
...
}
}

so I just changed this to be
if (use_system_freetype==0) {
} else if (_toolset=="target") {
}

(I believe this change is the correct syntax for elseif
https://codereview.chromium.org/601353002 )

If it would be better to nest another conditional inside the else, I
could do that too.

https://codereview.chromium.org/1585523002/

[sl...@chromium.org]

nits. Feel free to add OWNERS for next patchset. Maybe send for dry run
first.

https://codereview.chromium.org/1585523002/diff/1/build/linux/BUILD.gn
File build/linux/BUILD.gn (right):

https://codereview.chromium.org/1585523002/diff/1/build/linux/BUILD.gn#newcode125
build/linux/BUILD.gn:125: "//third_party/freetype2",

On 2016/01/14 01:37:52, bcf wrote:
> On 2016/01/13 18:43:58, slan wrote:
> > Ideally the same thing would apply to this target. It may get some
pushback,
> but
> > we should try it.

> It seems like //third_party/freetype2 is used by
> //content/shell:content_shell_lib

Wouldn't content_shell_lib want to depend on the same freetype as other
targets? We should CC an owner of that file and find out.

https://codereview.chromium.org/1585523002/diff/1/build/linux/system.gyp
File build/linux/system.gyp (right):

https://codereview.chromium.org/1585523002/diff/1/build/linux/system.gyp#newcode784
build/linux/system.gyp:784: ['use_system_freetype==0', {

On 2016/01/14 01:37:52, bcf wrote:
> On 2016/01/13 18:43:58, slan wrote:
> > Is this supposed to be instead of the logic below? Currently the
pkg_config
> > build settings will also apply.

> Yes this is supposed to be instead of the logic below.

> The intended behavior is

> if (use_system_freetype==0) {
> ... use third party freetype
> } else {
> if (_toolset=="target") {
> ...
> }
> }

> so I just changed this to be
> if (use_system_freetype==0) {
> } else if (_toolset=="target") {
> }

> (I believe this change is the correct syntax for elseif
> https://codereview.chromium.org/601353002 )

> If it would be better to nest another conditional inside the else, I
could do
> that too.

TIL. Never seen this before. This syntax lgtm.

https://codereview.chromium.org/1585523002/diff/20001/content/content_shell.gypi
File content/content_shell.gypi (right):

https://codereview.chromium.org/1585523002/diff/20001/content/content_shell.gypi#newcode331
content/content_shell.gypi:331:
'../third_party/freetype2/freetype2.gyp:freetype2',
This should go with fontconfig above. No need to have 2 linux blocks

https://codereview.chromium.org/1585523002/

[b...@chromium.org]

https://codereview.chromium.org/1585523002/diff/1/build/linux/BUILD.gn
File build/linux/BUILD.gn (right):

https://codereview.chromium.org/1585523002/diff/1/build/linux/BUILD.gn#newcode125
build/linux/BUILD.gn:125: "//third_party/freetype2",

On 2016/01/14 18:45:37, slan wrote:
> On 2016/01/14 01:37:52, bcf wrote:
> > On 2016/01/13 18:43:58, slan wrote:
> > > Ideally the same thing would apply to this target. It may get some
pushback,
> > but
> > > we should try it.
> >
> > It seems like //third_party/freetype2 is used by
> > //content/shell:content_shell_lib

> Wouldn't content_shell_lib want to depend on the same freetype as
other targets?
> We should CC an owner of that file and find out.

I assumed not because the equivalent gyp target
(chromium/src/content/content_shell.gypi:content_shell_lib) uses
third_party/freetype2 directly, but you're right we should probably find
out the reason for this.

https://codereview.chromium.org/1585523002/diff/20001/content/content_shell.gypi
File content/content_shell.gypi (right):

https://codereview.chromium.org/1585523002/diff/20001/content/content_shell.gypi#newcode331
content/content_shell.gypi:331:
'../third_party/freetype2/freetype2.gyp:freetype2',

On 2016/01/14 18:45:37, slan wrote:
> This should go with fontconfig above. No need to have 2 linux blocks

Done. I missed that, thanks.

https://codereview.chromium.org/1585523002/

[b...@chromium.org]

+ OWNERS

https://codereview.chromium.org/1585523002/

[the...@chromium.org]

Are you sure you want to do this? The bottom of
third_party/freetype2/README.chromium says:

This code is not considered security critical since it is only to be linked
into test binaries! This should never be linked into chrome or any
production
code.

https://codereview.chromium.org/1585523002/

[wzh...@chromium.org]

+dpr...@chromium.org

Good catch.

Any plan to upgrade the library to ver 2.6+?

https://codereview.chromium.org/1585523002/

[b...@chromium.org]

On 2016/01/14 23:16:46, Lei Zhang wrote:
> Are you sure you want to do this? The bottom of
> third_party/freetype2/README.chromium says:

> This code is not considered security critical since it is only to be
> linked
> into test binaries! This should never be linked into chrome or any
> production
> code.

thestig@/dpranke@

Do you know why this is the case? Is it because this version is too old?

https://codereview.chromium.org/1585523002/

[the...@chromium.org]

Please read third_party/freetype2/README.chromium carefully. It's old on
purpose
and there's no intentions to upgrade. You shouldn't use it.

https://codereview.chromium.org/1585523002/

[Nico Weber]

Please don't delete issues that don't land. Just close them. Else it's much harder to link to them.

[b...@google.com]

Sorry! I'll remember that for the future.