(CANCEL) Define 'USING_SYSTEM_ICU' when 'use_system_icu=1' in blink_platform

Define 'USING_SYSTEM_ICU' when 'use_system_icu=1' in blink_platform

This patch defines 'USING_SYSTEM_ICU' when 'use_system_icu=1' in
blink_platform. A previous CL[1] defines this in config.gyp, but this define
is not available in blink_platform.

[1] https://codereview.chromium.org/1167523003

BUG=584920

[kojii, 2016-03-01 06:37:30]

Description was changed from

==========
WIP: icu

BUG=
==========

to

==========
Define 'USING_SYSTEM_ICU' when 'use_system_icu=1' in blink_platform

This patch defines 'USING_SYSTEM_ICU' when 'use_system_icu=1' in
blink_platform. A previous CL[1] defines this in config.gyp, but this define
is not available in blink_platform.

[1] https://codereview.chromium.org/1167523003

BUG=584920
==========

[kojii, 2016-03-01 06:39:14]

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

[kojii, 2016-03-01 06:39:15]

Dirk, could you mind to advice me why this definition in config.gyp is available
in wtf but not in platform? Copying "defines" from config.gyp seems to fix this,
but I see wtf.gyp can use it without copying.

[Dirk Pranke, 2016-03-01 23:03:47]

On 2016/03/01 06:39:15, kojii wrote:
> Dirk, could you mind to advice me why this definition in config.gyp is
available
> in wtf but not in platform? Copying "defines" from config.gyp seems to fix
this,
> but I see wtf.gyp can use it without copying.

Hmm, I don't know. It looks like both targets depend on '../config.gyp:config',
which sets it in
its direct_dependent_settings, so I don't know why you would need this.

[Dirk Pranke, 2016-03-01 23:04:31]

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

[Dirk Pranke, 2016-03-01 23:04:32]

+thakis .. thakis, do you know why this CL would be needed and we wouldn't be
getting the setting propagated from ../config.gyp:config ?

[Nico, 2016-03-01 23:24:39]

Huh, no. I would expect this to work. Have you tried debugging this a bit? This
CL seems like a workaround a real bug somewhere.

[kojii, 2016-03-02 01:03:33]

On 2016/03/01 at 23:24:39, thakis wrote:
> Huh, no. I would expect this to work. Have you tried debugging this a bit?
This CL seems like a workaround a real bug somewhere.

I don't know how to debug gyp...

I agree this is a workaround, spent several hours to figure out the difference
between blink_platform.gyp and wtf.gyp (USING_SYSTEM_ICU works for wtf.gyp) and
tried a few possibilities without luck.

If this was not easy for you, assuming we're moving towards to gn, is it ok to
go with this CL? A few Linux distributers are in trouble with this issue to
bundle Chromium to their distributions.

[Nico, 2016-03-02 01:05:51]

i looked at this for 20 seconds, i don't know how difficult it is

if i debug gyp, i usually add print statements in tools/gyp/pylib/gyp/input.py
and __init__.py

[Dirk Pranke, 2016-03-02 01:36:41]

The change lgtm, but I'd also like Nico to weigh in.

I sympathize w/ the desire to figure out the underlying bug, but I also think
it's a bit much to ask someone
not familiar w/ the GYP code to try and debug things.

(And I'm not planning to debug it myself at the moment).

kojii, maybe file a bug and cc thakis@ and I and that'll be good enough?

[kojii, 2016-03-02 02:34:54]

Thank you dpranke@ for your help, filed crbug.com/591268.

I'll see if Nico would like to comment for a while before landing.

[kojii, 2016-03-04 02:46:53]

thakis@, please chime in if you have any, I'm going to land this tomorrow.

[kojii, 2016-03-05 14:37:37]

The CQ bit was checked by kojii@chromium.org

[commit-bot: I haz the power, 2016-03-05 14:37:43]

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

[Nico, 2016-03-05 15:00:32]

The CQ bit was unchecked by thakis@chromium.org

[Nico, 2016-03-05 15:01:48]

On 2016/03/05 14:37:43, commit-bot: I haz the power wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/1754603002/1
> View timeline at
>  https://chromium-cq-status.appspot.com/patch-timeline/1754603002/1

I think we should understand what's going on here. Since you don't want to
check, I added it to my list. I've been traveling this week and didn't have a
chance to look yet.

[kojii, 2016-03-05 15:17:40]

On 2016/03/05 at 15:01:48, thakis wrote:
> On 2016/03/05 14:37:43, commit-bot: I haz the power wrote:
> > CQ is trying da patch. Follow status at
> >  https://chromium-cq-status.appspot.com/patch-status/1754603002/1
> > View timeline at
> >  https://chromium-cq-status.appspot.com/patch-timeline/1754603002/1
> 
> I think we should understand what's going on here. Since you don't want to
check, I added it to my list. I've been traveling this week and didn't have a
chance to look yet.

Ok, thanks, that's helpful.

In crbug.com/584920#c23 the reporter says this still doesn't work. I'm
simulating his environment to make the fix, so the way I simulated was wrong, or
maybe the way it patched was incorrect. Landing can confirm that, but the latter
is unlikely, I think. Seeking for other options in crbug.com/584920#c27 but your
thoughts would be greatly appreciated.

[Nico, 2016-03-06 15:32:56]

I tried to investigate, but I can't reproduce the problem.

I ran

GYP_DEFINES='fastbuild=1 use_system_icu=1 component=shared_library
disable_nacl=1 dcheck_always_on=1' build/gyp_chromium

and then looked at
out/Release/obj/third_party/WebKit/Source/platform/blink_platform.ninja. It
contains -DUSING_SYSTEM_ICU without this patch already.

(I had to apply this change to third_party/icu/icu.gyp to make things gyp with
use_system_icu=1 set:

diff --git a/icu.gyp b/icu.gyp
index ee206fe..ef7361c 100644
--- a/icu.gyp
+++ b/icu.gyp
@@ -75,7 +75,7 @@
     'msvs_disabled_warnings': [4005, 4068, 4355, 4996, 4267],
   },
   'conditions': [
-    ['use_system_icu==0 or want_separate_host_toolset==1', {
+    ['use_system_icu==0', {
       'targets': [
         {
           'target_name': 'copy_icudtl_dat',
@@ -383,10 +383,10 @@
             ['OS!="qnx"', {
               'link_settings': {
                 'ldflags': [
-                  '<!@(icu-config --ldflags)',
+                  #'<!@(icu-config --ldflags)',
                 ],
                 'libraries': [
-                  '<!@(icu-config --ldflags-libsonly)',
+                  #'<!@(icu-config --ldflags-libsonly)',
                 ],
               },
             }],

The 2nd and 3rd change 'cause I'm on a mac and can't run icu-config, but the
first looks like it might be a real problem with icu.gyp)

In general phajdan maintains the use_system_foo flags, so I'm not sure you
should have to spend time to support this.

In any case, this patch has no effect as far as I can tell (which is good, since
that's what I had expected).

[kojii, 2016-03-07 02:19:33]

thank you for looking into this, and for commenting on the bug. I'll close this
CL and look forward to the better solution in crbug.com/584920