Set rpath to let content_shell find our own libfreetype

Set rpath to let content_shell find our own libfreetype

We always build third_party/freetype2 as a dynamic library even for
static build to avoid symbol conflicts. We need to explicitly set
rpath for static build [1] to let content_shell find our own
libfreetype instead of the system one.

[1] We omit rpath for static gn builds. See
    https://codereview.chromium.org/1535863002.

Committed: https://crrev.com/1a34fa31d0bc9b1e77ca31019e5c327cbf866826
Cr-Commit-Position: refs/heads/master@{#380463}

[Xianzhu, 2016-03-10 00:26:00]

wangxianzhu@chromium.org changed reviewers:
+ dpranke@chromium.org, mkwst@chromium.org

[Xianzhu, 2016-03-10 00:27:05]

Description was changed from

==========
Set rpath to let content_shell find our own libfreetype

We always build third_party/freetype2 as a dynamic library even for
static build because of symbol conflicts. We need to explicit set
rpath for static build (which omit rpath by default) to let
content_shell find our own libfreetype instead of the system one.
==========

to

==========
Set rpath to let content_shell find our own libfreetype

We always build third_party/freetype2 as a dynamic library even for
static build because of symbol conflicts. We need to explicit set
rpath for static build [1] to let content_shell find our own
libfreetype instead of the system one.

[1] We omit rpath for static gn builds. See
    https://codereview.chromium.org/1535863002.
==========

[Xianzhu, 2016-03-10 00:28:50]

Description was changed from

==========
Set rpath to let content_shell find our own libfreetype

We always build third_party/freetype2 as a dynamic library even for
static build because of symbol conflicts. We need to explicit set
rpath for static build [1] to let content_shell find our own
libfreetype instead of the system one.

[1] We omit rpath for static gn builds. See
    https://codereview.chromium.org/1535863002.
==========

to

==========
Set rpath to let content_shell find our own libfreetype

We always build third_party/freetype2 as a dynamic library even for
static build to avoid symbol conflicts. We need to explicitly set
rpath for static build [1] to let content_shell find our own
libfreetype instead of the system one.

[1] We omit rpath for static gn builds. See
    https://codereview.chromium.org/1535863002.
==========

[Dirk Pranke, 2016-03-10 01:37:12]

https://codereview.chromium.org/1781903002/diff/1/content/shell/BUILD.gn
File content/shell/BUILD.gn (right):

https://codereview.chromium.org/1781903002/diff/1/content/shell/BUILD.gn#newc...
content/shell/BUILD.gn:469: }
This is basically the right fix, but I think it might be better to make this
config be a part of the public_configs for freetype2 (i.e., move this line to
//third_party/freetype2/BUILD.gn:12).

That way things can't get out of sync.

Does that work?

[Mike West, 2016-03-10 08:46:46]

Either way, LGTM.

https://codereview.chromium.org/1781903002/diff/1/content/shell/BUILD.gn
File content/shell/BUILD.gn (right):

https://codereview.chromium.org/1781903002/diff/1/content/shell/BUILD.gn#newc...
content/shell/BUILD.gn:469: }
On 2016/03/10 at 01:37:12, Dirk Pranke wrote:
> This is basically the right fix, but I think it might be better to make this
config be a part of the public_configs for freetype2 (i.e., move this line to
//third_party/freetype2/BUILD.gn:12).
> 
> That way things can't get out of sync.
> 
> Does that work?

I'd be fine with that.

[Xianzhu, 2016-03-10 17:31:27]

https://codereview.chromium.org/1781903002/diff/1/content/shell/BUILD.gn
File content/shell/BUILD.gn (right):

https://codereview.chromium.org/1781903002/diff/1/content/shell/BUILD.gn#newc...
content/shell/BUILD.gn:469: }
On 2016/03/10 01:37:12, Dirk Pranke wrote:
> This is basically the right fix, but I think it might be better to make this
> config be a part of the public_configs for freetype2 (i.e., move this line to
> //third_party/freetype2/BUILD.gn:12).
> 
> That way things can't get out of sync.
> 
> Does that work?

I think the current method is better because:
- It's consistent with the method used for other binaries in the similar
situation (https://codereview.chromium.org/1535863002);
- It can also handle other dynamic libraries (if any) linked to content_shell.

[Dirk Pranke, 2016-03-10 18:10:45]

On 2016/03/10 17:31:27, Xianzhu wrote:
> https://codereview.chromium.org/1781903002/diff/1/content/shell/BUILD.gn
> File content/shell/BUILD.gn (right):
> 
>
https://codereview.chromium.org/1781903002/diff/1/content/shell/BUILD.gn#newc...
> content/shell/BUILD.gn:469: }
> On 2016/03/10 01:37:12, Dirk Pranke wrote:
> > This is basically the right fix, but I think it might be better to make this
> > config be a part of the public_configs for freetype2 (i.e., move this line
to
> > //third_party/freetype2/BUILD.gn:12).
> > 
> > That way things can't get out of sync.
> > 
> > Does that work?
> 
> I think the current method is better because:
> - It's consistent with the method used for other binaries in the similar
> situation (https://codereview.chromium.org/1535863002);
> - It can also handle other dynamic libraries (if any) linked to content_shell.

Okay, I don't feel that strongly about my suggestion, so this was is fine as
well.

lgtm.

[Xianzhu, 2016-03-10 18:11:46]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2016-03-10 18:12:09]

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

[commit-bot: I haz the power, 2016-03-10 19:40:06]

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

[commit-bot: I haz the power, 2016-03-10 19:40:07]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Xianzhu, 2016-03-10 20:21:34]

Patchset #2 (id:20001) has been deleted

[Xianzhu, 2016-03-10 20:21:40]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2016-03-10 20:22:24]

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

[commit-bot: I haz the power, 2016-03-10 21:03:07]

Description was changed from

==========
Set rpath to let content_shell find our own libfreetype

We always build third_party/freetype2 as a dynamic library even for
static build to avoid symbol conflicts. We need to explicitly set
rpath for static build [1] to let content_shell find our own
libfreetype instead of the system one.

[1] We omit rpath for static gn builds. See
    https://codereview.chromium.org/1535863002.
==========

to

==========
Set rpath to let content_shell find our own libfreetype

We always build third_party/freetype2 as a dynamic library even for
static build to avoid symbol conflicts. We need to explicitly set
rpath for static build [1] to let content_shell find our own
libfreetype instead of the system one.

[1] We omit rpath for static gn builds. See
    https://codereview.chromium.org/1535863002.
==========

[commit-bot: I haz the power, 2016-03-10 21:03:09]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-03-10 21:04:10]

Description was changed from

==========
Set rpath to let content_shell find our own libfreetype

We always build third_party/freetype2 as a dynamic library even for
static build to avoid symbol conflicts. We need to explicitly set
rpath for static build [1] to let content_shell find our own
libfreetype instead of the system one.

[1] We omit rpath for static gn builds. See
    https://codereview.chromium.org/1535863002.
==========

to

==========
Set rpath to let content_shell find our own libfreetype

We always build third_party/freetype2 as a dynamic library even for
static build to avoid symbol conflicts. We need to explicitly set
rpath for static build [1] to let content_shell find our own
libfreetype instead of the system one.

[1] We omit rpath for static gn builds. See
    https://codereview.chromium.org/1535863002.

Committed: https://crrev.com/1a34fa31d0bc9b1e77ca31019e5c327cbf866826
Cr-Commit-Position: refs/heads/master@{#380463}
==========

[commit-bot: I haz the power, 2016-03-10 21:04:11]

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