Fix Mac fontconfig build

Fix Mac fontconfig build

The Mac fontconfig just #defines the cache directory, which works fine if fontconfig never has to look up any fonts (the case until now).  If it has to actually find fonts from the disk, the cache directory and config directory need to be properly defined as well as running fc-cache to populate the cache directory.

Populating the cache directory can take some time, but should only happen on a clean build.  To remove this extra time, we have to not build poppler on Mac, which can now be accomplished with GYP_DEFINES="skia_mac_poppler=0"

Committed: http://code.google.com/p/skia/source/detail?r=12994

[Lei Zhang, 2013-12-18 09:06:23]

The defines section does not get path expansion. Try the approach in
skia_gyp.diff in my home dir instead. You'll need to #include the generated
header file in the mac config.h file.

[vandebo (ex-Chrome), 2014-01-06 21:39:04]

ping

[bungeman-skia, 2014-01-07 21:13:29]

A few comments. I think epoger had some thoughts about this. While this looks
pretty good from a technical sense, I think this change is a bit more
build/policy for the build team.

https://codereview.chromium.org/113203003/diff/60001/gyp/common_variables.gypi
File gyp/common_variables.gypi (right):

https://codereview.chromium.org/113203003/diff/60001/gyp/common_variables.gyp...
gyp/common_variables.gypi:99: 'skia_mac_poppler%': 1,
It looks like the actual meaning of this variable is whether the gm tool (or any
tool) should depend on poppler or not. Maybe something like skia_poppler_enabled
and the logic. Come to think of it, what is really wanted here is a
skia_init_fontconfig to control if fontconfig should be set up or not. (This
would directly control the new dependencies added in gyp/fontconfig.gyp.)

https://codereview.chromium.org/113203003/diff/60001/gyp/fontconfig.gyp
File gyp/fontconfig.gyp (right):

https://codereview.chromium.org/113203003/diff/60001/gyp/fontconfig.gyp#newco...
gyp/fontconfig.gyp:30: } ],
nit: there normally isn't a space between the curly and square when used like
this.

https://codereview.chromium.org/113203003/diff/60001/gyp/fontconfig.gyp#newco...
gyp/fontconfig.gyp:137: ],
nit: outdent 1 space

https://codereview.chromium.org/113203003/diff/60001/gyp/fontconfig.gyp#newco...
gyp/fontconfig.gyp:197: } ],
nit: extra space

https://codereview.chromium.org/113203003/diff/60001/third_party/fontconfig/p...
File third_party/fontconfig/process-template.py (right):

https://codereview.chromium.org/113203003/diff/60001/third_party/fontconfig/p...
third_party/fontconfig/process-template.py:11: "-s", "--substitution", 
action="append", nargs=2, metavar=('KEY', 'VALUE'),    help="Changes @KEY@ to
VALUE in the template.")
nit: this line is a bit long, looks like the next one (-p) is nicely formatted.

[epoger, 2014-01-08 19:40:22]

https://codereview.chromium.org/113203003/diff/60001/gyp/common_variables.gypi
File gyp/common_variables.gypi (right):

https://codereview.chromium.org/113203003/diff/60001/gyp/common_variables.gyp...
gyp/common_variables.gypi:3: # Use of this source code is governed by a
BSD-style license that can be
Toplevel questions:

1. You say "Populating the cache directory can take some time, but should only
happen on a clean build."  Why is that so?  I would think that a cache directory
is something that would be constructed at RUNTIME, not COMPILE TIME.  (My
ignorance of fontconfig is showing!)

2. You say "To remove this extra time, we have to not build poppler on Mac,
which can now be accomplished with GYP_DEFINES="skia_mac_poppler=0""  What's the
relationship between fontconfig cache directory and poppler?

https://codereview.chromium.org/113203003/diff/60001/gyp/fontconfig.gyp
File gyp/fontconfig.gyp (right):

https://codereview.chromium.org/113203003/diff/60001/gyp/fontconfig.gyp#newco...
gyp/fontconfig.gyp:17: 'dependencies': [
Why the additional level of indirection here?

Is it just to add a dependency on fontconfig_setup for Mac?  If so, couldn't you
just add that within the old fontconfig target's dependencies?

https://codereview.chromium.org/113203003/diff/60001/gyp/gm.gyp
File gyp/gm.gyp (right):

https://codereview.chromium.org/113203003/diff/60001/gyp/gm.gyp#newcode76
gyp/gm.gyp:76: ['skia_os in ["linux", "win"] or (skia_os == "mac" and
skia_mac_poppler)', {
I think it would be clearer to use a single flag (e.g. skia_enable_poppler) for
the conditional, and (elsewhere) set its default value based on skia_os.

Like how skia_arch_width is automatically set (in common_variables.gypi)
depending on skia_os, but can be overridden from the command line...

This may be what Ben is referring to in
https://codereview.chromium.org/113203003/diff/60001/gyp/common_variables.gyp...
, but I'm not sure.

https://codereview.chromium.org/113203003/diff/60001/third_party/fontconfig/R...
File third_party/fontconfig/README.chromium (right):

https://codereview.chromium.org/113203003/diff/60001/third_party/fontconfig/R...
third_party/fontconfig/README.chromium:38: additional flags.
remove this "additional flags." line?

https://codereview.chromium.org/113203003/diff/60001/third_party/fontconfig/p...
File third_party/fontconfig/process-template.py (right):

https://codereview.chromium.org/113203003/diff/60001/third_party/fontconfig/p...
third_party/fontconfig/process-template.py:1: #!/usr/bin/python
Add copyright and documentation header?

https://codereview.chromium.org/113203003/diff/60001/third_party/fontconfig/p...
third_party/fontconfig/process-template.py:8: parser.add_argument("-i",
"--input")
Please be consistent with either single- or double-quotes for Python string
constants, as per
http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=String...

I've seen much more use of single-quotes in our Python code (but double-quotes
for docstrings).

https://codereview.chromium.org/113203003/diff/60001/third_party/fontconfig/p...
third_party/fontconfig/process-template.py:10: parser.add_argument(
I would suggest naming this arg --keyword_substitution (-k), for symmetry with
--path_substitution (-p).

https://codereview.chromium.org/113203003/diff/60001/third_party/fontconfig/p...
third_party/fontconfig/process-template.py:23: except IOError:
Why the manual try/except handling?  If you just don't bother, won't the default
exception handling provide what you want, with more information yielded to the
developer?

https://codereview.chromium.org/113203003/diff/60001/third_party/fontconfig/p...
third_party/fontconfig/process-template.py:42: line = line.replace('@' + key +
'@', value)
Rather than mandating the @ delimiters, why not just let the caller pass in
whatever key she wants?  ("@FC_CACHEDIR@", "%%FC_CACHEDIR%%", or whatever is
desired)

[vandebo (ex-Chrome), 2014-01-08 21:24:15]

https://codereview.chromium.org/113203003/diff/60001/gyp/common_variables.gypi
File gyp/common_variables.gypi (right):

https://codereview.chromium.org/113203003/diff/60001/gyp/common_variables.gyp...
gyp/common_variables.gypi:3: # Use of this source code is governed by a
BSD-style license that can be
On 2014/01/08 19:40:22, epoger wrote:
> Toplevel questions:
> 
> 1. You say "Populating the cache directory can take some time, but should only
> happen on a clean build."  Why is that so?  I would think that a cache
directory
> is something that would be constructed at RUNTIME, not COMPILE TIME.  (My
> ignorance of fontconfig is showing!)

The fontconfig works when it's installed on a system is that install time, it
walks all the fonts and generates the cache directory.  When a new font is added
the cache is updated.  So usually the cache is generated at install time (not
compile time).

In this case, we're doing a skia local install, into the output directory. 
Since we're not touching fontconfig, the only time the cache will need to be
updated is when the output directory gets clobbered.

> 2. You say "To remove this extra time, we have to not build poppler on Mac,
> which can now be accomplished with GYP_DEFINES="skia_mac_poppler=0""  What's
the
> relationship between fontconfig cache directory and poppler?

poppler depends on fontconfig.  If fontconfig's cache directory is not present
then fontconfig and poppler will fail to find fonts that are not embedded in the
PDF.

https://codereview.chromium.org/113203003/diff/60001/gyp/common_variables.gyp...
gyp/common_variables.gypi:99: 'skia_mac_poppler%': 1,
On 2014/01/07 21:13:29, bungeman1 wrote:
> It looks like the actual meaning of this variable is whether the gm tool (or
any
> tool) should depend on poppler or not. Maybe something like
skia_poppler_enabled
> and the logic. Come to think of it, what is really wanted here is a
> skia_init_fontconfig to control if fontconfig should be set up or not. (This
> would directly control the new dependencies added in gyp/fontconfig.gyp.)

We don't want to have poppler without fontconfig installed/initialized - you'll
get spew from poppler and bad output.  But your other points are right, changed
to skia_poppler_enabled and made it not mac specific.

https://codereview.chromium.org/113203003/diff/60001/gyp/fontconfig.gyp
File gyp/fontconfig.gyp (right):

https://codereview.chromium.org/113203003/diff/60001/gyp/fontconfig.gyp#newco...
gyp/fontconfig.gyp:17: 'dependencies': [
On 2014/01/08 19:40:22, epoger wrote:
> Why the additional level of indirection here?
> 
> Is it just to add a dependency on fontconfig_setup for Mac?  If so, couldn't
you
> just add that within the old fontconfig target's dependencies?

You mean make the thing that depends on fontconfig also depend on 
fontconfig_setup on Mac?  I think this style keeps it more contained.

https://codereview.chromium.org/113203003/diff/60001/gyp/fontconfig.gyp#newco...
gyp/fontconfig.gyp:30: } ],
On 2014/01/07 21:13:29, bungeman1 wrote:
> nit: there normally isn't a space between the curly and square when used like
> this.

Done.

https://codereview.chromium.org/113203003/diff/60001/gyp/fontconfig.gyp#newco...
gyp/fontconfig.gyp:137: ],
On 2014/01/07 21:13:29, bungeman1 wrote:
> nit: outdent 1 space

Done.

https://codereview.chromium.org/113203003/diff/60001/gyp/gm.gyp
File gyp/gm.gyp (right):

https://codereview.chromium.org/113203003/diff/60001/gyp/gm.gyp#newcode76
gyp/gm.gyp:76: ['skia_os in ["linux", "win"] or (skia_os == "mac" and
skia_mac_poppler)', {
On 2014/01/08 19:40:22, epoger wrote:
> I think it would be clearer to use a single flag (e.g. skia_enable_poppler)
for
> the conditional, and (elsewhere) set its default value based on skia_os.
> 
> Like how skia_arch_width is automatically set (in common_variables.gypi)
> depending on skia_os, but can be overridden from the command line...
> 
> This may be what Ben is referring to in
>
https://codereview.chromium.org/113203003/diff/60001/gyp/common_variables.gyp...
> , but I'm not sure.

Changed to skia_enable_popper and made that work across all platforms.

https://codereview.chromium.org/113203003/diff/60001/third_party/fontconfig/R...
File third_party/fontconfig/README.chromium (right):

https://codereview.chromium.org/113203003/diff/60001/third_party/fontconfig/R...
third_party/fontconfig/README.chromium:38: additional flags.
On 2014/01/08 19:40:22, epoger wrote:
> remove this "additional flags." line?

Oops. Done.

https://codereview.chromium.org/113203003/diff/60001/third_party/fontconfig/p...
File third_party/fontconfig/process-template.py (right):

https://codereview.chromium.org/113203003/diff/60001/third_party/fontconfig/p...
third_party/fontconfig/process-template.py:8: parser.add_argument("-i",
"--input")
On 2014/01/08 19:40:22, epoger wrote:
> Please be consistent with either single- or double-quotes for Python string
> constants, as per
>
http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=String...
> 
> I've seen much more use of single-quotes in our Python code (but double-quotes
> for docstrings).

Done.

https://codereview.chromium.org/113203003/diff/60001/third_party/fontconfig/p...
third_party/fontconfig/process-template.py:10: parser.add_argument(
On 2014/01/08 19:40:22, epoger wrote:
> I would suggest naming this arg --keyword_substitution (-k), for symmetry with
> --path_substitution (-p).

Done.

https://codereview.chromium.org/113203003/diff/60001/third_party/fontconfig/p...
third_party/fontconfig/process-template.py:11: "-s", "--substitution", 
action="append", nargs=2, metavar=('KEY', 'VALUE'),    help="Changes @KEY@ to
VALUE in the template.")
On 2014/01/07 21:13:29, bungeman1 wrote:
> nit: this line is a bit long, looks like the next one (-p) is nicely
formatted.

Done.

https://codereview.chromium.org/113203003/diff/60001/third_party/fontconfig/p...
third_party/fontconfig/process-template.py:23: except IOError:
On 2014/01/08 19:40:22, epoger wrote:
> Why the manual try/except handling?  If you just don't bother, won't the
default
> exception handling provide what you want, with more information yielded to the
> developer?

Done.

https://codereview.chromium.org/113203003/diff/60001/third_party/fontconfig/p...
third_party/fontconfig/process-template.py:42: line = line.replace('@' + key +
'@', value)
On 2014/01/08 19:40:22, epoger wrote:
> Rather than mandating the @ delimiters, why not just let the caller pass in
> whatever key she wants?  ("@FC_CACHEDIR@", "%%FC_CACHEDIR%%", or whatever is
> desired)

Done.

[epoger, 2014-01-08 21:38:12]

Looks pretty good to me, a couple of lingering questions.

https://codereview.chromium.org/113203003/diff/60001/gyp/fontconfig.gyp
File gyp/fontconfig.gyp (right):

https://codereview.chromium.org/113203003/diff/60001/gyp/fontconfig.gyp#newco...
gyp/fontconfig.gyp:17: 'dependencies': [
On 2014/01/08 21:24:15, vandebo wrote:
> On 2014/01/08 19:40:22, epoger wrote:
> > Why the additional level of indirection here?
> > 
> > Is it just to add a dependency on fontconfig_setup for Mac?  If so, couldn't
> you
> > just add that within the old fontconfig target's dependencies?
> 
> You mean make the thing that depends on fontconfig also depend on 
> fontconfig_setup on Mac?  I think this style keeps it more contained.

No, I meant making fontconfig depend on fontconfig_setup.  But perhaps there is
some reason that is not OK to do...

https://codereview.chromium.org/113203003/diff/60001/gyp/gm.gyp
File gyp/gm.gyp (right):

https://codereview.chromium.org/113203003/diff/60001/gyp/gm.gyp#newcode76
gyp/gm.gyp:76: ['skia_os in ["linux", "win"] or (skia_os == "mac" and
skia_mac_poppler)', {
On 2014/01/08 21:24:15, vandebo wrote:
> On 2014/01/08 19:40:22, epoger wrote:
> > I think it would be clearer to use a single flag (e.g. skia_enable_poppler)
> for
> > the conditional, and (elsewhere) set its default value based on skia_os.
> > 
> > Like how skia_arch_width is automatically set (in common_variables.gypi)
> > depending on skia_os, but can be overridden from the command line...
> > 
> > This may be what Ben is referring to in
> >
>
https://codereview.chromium.org/113203003/diff/60001/gyp/common_variables.gyp...
> > , but I'm not sure.
> 
> Changed to skia_enable_popper and made that work across all platforms.

Why do we still need the list of operating systems here?  Can't you just check
for skia_poppler_enabled , and set the default for that variable based on the
OS?

[vandebo (ex-Chrome), 2014-01-08 22:08:18]

https://codereview.chromium.org/113203003/diff/60001/gyp/fontconfig.gyp
File gyp/fontconfig.gyp (right):

https://codereview.chromium.org/113203003/diff/60001/gyp/fontconfig.gyp#newco...
gyp/fontconfig.gyp:17: 'dependencies': [
On 2014/01/08 21:38:12, epoger wrote:
> On 2014/01/08 21:24:15, vandebo wrote:
> > On 2014/01/08 19:40:22, epoger wrote:
> > > Why the additional level of indirection here?
> > > 
> > > Is it just to add a dependency on fontconfig_setup for Mac?  If so,
couldn't
> > you
> > > just add that within the old fontconfig target's dependencies?
> > 
> > You mean make the thing that depends on fontconfig also depend on 
> > fontconfig_setup on Mac?  I think this style keeps it more contained.
> 
> No, I meant making fontconfig depend on fontconfig_setup.  But perhaps there
is
> some reason that is not OK to do...

I need fontconfig_setup to run after fontconfig is built, so I don't think I can
do that.

https://codereview.chromium.org/113203003/diff/60001/gyp/gm.gyp
File gyp/gm.gyp (right):

https://codereview.chromium.org/113203003/diff/60001/gyp/gm.gyp#newcode76
gyp/gm.gyp:76: ['skia_os in ["linux", "win"] or (skia_os == "mac" and
skia_mac_poppler)', {
On 2014/01/08 21:38:12, epoger wrote:
> On 2014/01/08 21:24:15, vandebo wrote:
> > On 2014/01/08 19:40:22, epoger wrote:
> > > I think it would be clearer to use a single flag (e.g.
skia_enable_poppler)
> > for
> > > the conditional, and (elsewhere) set its default value based on skia_os.
> > > 
> > > Like how skia_arch_width is automatically set (in common_variables.gypi)
> > > depending on skia_os, but can be overridden from the command line...
> > > 
> > > This may be what Ben is referring to in
> > >
> >
>
https://codereview.chromium.org/113203003/diff/60001/gyp/common_variables.gyp...
> > > , but I'm not sure.
> > 
> > Changed to skia_enable_popper and made that work across all platforms.
> 
> Why do we still need the list of operating systems here?  Can't you just check
> for skia_poppler_enabled , and set the default for that variable based on the
> OS?

Done.

[epoger, 2014-01-09 15:22:31]

lgtm

https://codereview.chromium.org/113203003/diff/60001/gyp/fontconfig.gyp
File gyp/fontconfig.gyp (right):

https://codereview.chromium.org/113203003/diff/60001/gyp/fontconfig.gyp#newco...
gyp/fontconfig.gyp:17: 'dependencies': [
On 2014/01/08 22:08:18, vandebo wrote:
> On 2014/01/08 21:38:12, epoger wrote:
> > On 2014/01/08 21:24:15, vandebo wrote:
> > > On 2014/01/08 19:40:22, epoger wrote:
> > > > Why the additional level of indirection here?
> > > > 
> > > > Is it just to add a dependency on fontconfig_setup for Mac?  If so,
> couldn't
> > > you
> > > > just add that within the old fontconfig target's dependencies?
> > > 
> > > You mean make the thing that depends on fontconfig also depend on 
> > > fontconfig_setup on Mac?  I think this style keeps it more contained.
> > 
> > No, I meant making fontconfig depend on fontconfig_setup.  But perhaps there
> is
> > some reason that is not OK to do...
> 
> I need fontconfig_setup to run after fontconfig is built, so I don't think I
can
> do that.

Ah, now I understand.  Might be worth adding a comment in the gyp file,
indicating that reasoning.

[commit-bot: I haz the power, 2014-01-09 17:06:35]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/vandebo@chromium.org/113203003/260001

[commit-bot: I haz the power, 2014-01-09 17:37:43]

Change committed as 12994

[vandebo (ex-Chrome), 2014-01-09 17:52:43]

A revert of this CL has been created in
https://codereview.chromium.org/132093003/ by vandebo@chromium.org.

The reason for reverting is: Broken under XCode..