Rework how use_system_harfbuzz works a bit.

Rework how use_system_harfbuzz works a bit.

We want 'use_system_harfbuzz' to always be false unless you are
building against a real ChromeOS sysroot targeted for an actual
device (i.e., just building with chromeos=1 isn't good enough).

This CL changes the defaults for the variables and fixes a few
unnecessary dependencies in the pangocairo pkg-config settings
in GYP that were causing us to get strange linker errors.

With this change, CrOS builds will need to explicitly set
use_system_harfbuzz=true, we will no longer try to guess
whether to use it or not.

R=drott@chromium.org, behdad@chromium.org
BUG=589342
Committed: https://crrev.com/518b35929ff33d0dc2397a186eecbc3b536acb40
Cr-Commit-Position: refs/heads/master@{#385487}

[Dirk Pranke, 2016-04-06 00:50:02]

I think this is good to go. Let's see what the bots think.

This is based on drott's CL:

https://codereview.chromium.org/1801813002/

And we will need to modify the CrOS ebuild to make sure to
set this flag, as per

https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/ma...

@drott, do you happen to have a cros checkout that you can use to change the
chrome ebuild settings? If not, let me
know and I can set one up for that patch.

[Dirk Pranke, 2016-04-06 00:50:07]

The CQ bit was checked by dpranke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-06 00:50:38]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1862893002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1862893002/1

[drott, 2016-04-06 01:06:57]

\o/ Thank you!

> This CL changes the defaults for the variables and fixes a few
> unnecessary dependencies in the pangocairo pkg-config settings
> in GYP that were causing us to get strange linker errors.

So this is why I didn't get the linker warnings in GN? I don't really know
enough to review whether this part of the CL is making pangocairo happy enough.
But if you tried it, I am more than happy if it works. How does this relate to
https://bugs.chromium.org/p/chromium/issues/detail?id=457307 ? 

Are you going to do a separate CL to update the use_system_harfbuzz to explicit
true in the CrOS build overlays? Or would we leave this to those handling the
CrOS "real" build?

LGTM

[Dirk Pranke, 2016-04-06 01:38:25]

On 2016/04/06 01:06:57, drott wrote:
> \o/ Thank you!
> 
> > This CL changes the defaults for the variables and fixes a few
> > unnecessary dependencies in the pangocairo pkg-config settings
> > in GYP that were causing us to get strange linker errors.
> 
> So this is why I didn't get the linker warnings in GN? 

Yes.

> I don't really know
> enough to review whether this part of the CL is making pangocairo happy
enough.
> But if you tried it, I am more than happy if it works. How does this relate to
> https://bugs.chromium.org/p/chromium/issues/detail?id=457307 ? 

I think that's mostly unrelated, apart from the harfbuzz-ng/pangoft comment, 
which is accurate.

> 
> Are you going to do a separate CL to update the use_system_harfbuzz to
explicit
> true in the CrOS build overlays?

That was what I was asking you in comment #1 :).

> Or would we leave this to those handling the CrOS "real" build?

I think one of us should at least post the CL and have them review it. I suspect
that
if we don't do this the real CrOS builds (like on the chromium.chromiumos
waterfall)
will either break or link in both harfbuzzes, with possibly strange things
resulting.

[Dirk Pranke, 2016-04-06 01:38:56]

dpranke@chromium.org changed reviewers:
+ thestig@chromium.org

[Dirk Pranke, 2016-04-06 01:38:57]

@thestig, maybe you should also sanity-check me re: pangoft2 as well ...

[drott, 2016-04-06 02:36:31]

> @drott, do you happen to have a cros checkout that you can use to change the
chrome ebuild settings? If not, let me
> know and I can set one up for that patch.

Sorry, perhaps I was too excited about the CL so that I missed this. I did get a
checkout earlier to look at this part. I'll try to update the checkout and get a
CL ready.

[drott, 2016-04-06 02:57:11]

> > Or would we leave this to those handling the CrOS "real" build?
> 
> I think one of us should at least post the CL and have them review it. I
suspect that
> if we don't do this the real CrOS builds (like on the chromium.chromiumos
waterfall)
> will either break or link in both harfbuzzes, with possibly strange things
resulting.

Submitted https://chromium-review.googlesource.com/#/c/337104 - perhaps you know
better whom to add as reviewer on this issue.

[behdad, 2016-04-06 02:57:25]

lgtm

I don't claim to fully understand how this works, but from reading the change,
it looks good to me.  Thanks!

[commit-bot: I haz the power, 2016-04-06 06:51:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-06 06:51:55]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win_chromium_rel_ng on tryserver.chromium.win (JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[drott, 2016-04-06 15:35:23]

The CQ bit was checked by drott@chromium.org

[commit-bot: I haz the power, 2016-04-06 15:35:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1862893002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1862893002/1

[commit-bot: I haz the power, 2016-04-06 17:05:23]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-04-06 17:06:43]

Description was changed from

==========
Rework how use_system_harfbuzz works a bit.

We want 'use_system_harfbuzz' to always be false unless you are
building against a real ChromeOS sysroot targeted for an actual
device (i.e., just building with chromeos=1 isn't good enough).

This CL changes the defaults for the variables and fixes a few
unnecessary dependencies in the pangocairo pkg-config settings
in GYP that were causing us to get strange linker errors.

With this change, CrOS builds will need to explicitly set
use_system_harfbuzz=true, we will no longer try to guess
whether to use it or not.

R=drott@chromium.org, behdad@chromium.org
BUG=589342
==========

to

==========
Rework how use_system_harfbuzz works a bit.

We want 'use_system_harfbuzz' to always be false unless you are
building against a real ChromeOS sysroot targeted for an actual
device (i.e., just building with chromeos=1 isn't good enough).

This CL changes the defaults for the variables and fixes a few
unnecessary dependencies in the pangocairo pkg-config settings
in GYP that were causing us to get strange linker errors.

With this change, CrOS builds will need to explicitly set
use_system_harfbuzz=true, we will no longer try to guess
whether to use it or not.

R=drott@chromium.org, behdad@chromium.org
BUG=589342

Committed: https://crrev.com/518b35929ff33d0dc2397a186eecbc3b536acb40
Cr-Commit-Position: refs/heads/master@{#385487}
==========

[commit-bot: I haz the power, 2016-04-06 17:06:44]

Patchset 1 (id:??) landed as
https://crrev.com/518b35929ff33d0dc2397a186eecbc3b536acb40
Cr-Commit-Position: refs/heads/master@{#385487}

[Lei Zhang, 2016-04-06 23:35:08]

Late to the party...

https://codereview.chromium.org/1862893002/diff/1/third_party/harfbuzz-ng/BUI...
File third_party/harfbuzz-ng/BUILD.gn (right):

https://codereview.chromium.org/1862893002/diff/1/third_party/harfbuzz-ng/BUI...
third_party/harfbuzz-ng/BUILD.gn:10: # Blink uses a cutting-edge version of
Harfbuzz; most Linux distros do not
On Linux, we will hopefully bump the sysroot to Debian 8 or Ubuntu 14.04
sometime this year. Both distros have libpangoft2-1.0-0 1.36.x. Is that new
enough?

Maybe we can deprecate the pangoft hack targets then and assume this is always
true for Linux/ChromeOS.

[behdad, 2016-04-07 00:48:02]

On 2016/04/06 23:35:08, Lei Zhang wrote:
> Both distros have libpangoft2-1.0-0 1.36.x. Is that new enough?

About once a quarter, I (HarfBuzz developer) and Emil or Dominik (Blink text
developers) sit down in a room for three days, trying to fix things in the
Chrome+HarfBuzz integration layer.  Many times, this requires new API or
bugfixes in HarfBuzz.  While there, I make the HarfBuzz changes, and we need to
be able to use it in Blink immediately.  So, no, even a six-month-old distro is
not going to be recent-enough.  It's not feasible to make the HarfBuzz change,
then wait a year for distros to catch up before using it in Blink.