Add helper comment for icecc users.

Add helper comment for icecc users.

Set linux_use_bundled_binutils to 1 when using icecc to avoid
the incompatible build flag; -B.
https://github.com/icecc/icecream/commit/b2ce5b9cc4bd1900f55c3684214e409fa81e7a92

BUG=352046

[dshwang, 2014-04-23 14:49:50]

Hi, imo, now handling icee becomes too complicated to manage by individual
themselves. What do you think?

[Inactive, 2014-04-23 14:52:21]

Right now, we use "linux_use_bundled_binutils=0" to get icecc working. This is
not very complicated IMHO.

[dshwang, 2014-04-23 15:03:39]

On 2014/04/23 14:52:21, Chris Dumez wrote:
> Right now, we use "linux_use_bundled_binutils=0" to get icecc working. This is
> not very complicated IMHO.

It's because your binutils version is less than 2.23. If you upgrade your gcc,
you will encounter
https://code.google.com/p/chromium/issues/detail?id=352046#c13
Currently I need following options; binutils_version=222
linux_use_bundled_gold=0 linux_use_bundled_binutils=0
IMO it's complicated. even worse it was continuously growing.

[dshwang, 2014-04-23 15:05:20]

https://codereview.chromium.org/249443002/diff/20001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/249443002/diff/20001/build/common.gypi#newcod...
build/common.gypi:170: 'use_goma%': 0,
we already have use_goma, so imo it's reasonable to introduce use_icecc because
most non-googler use icecc.

[Mostyn Bramley-Moore, 2014-04-23 15:06:51]

On 2014/04/23 14:52:21, Chris Dumez wrote:
> Right now, we use "linux_use_bundled_binutils=0" to get icecc working. This is
> not very complicated IMHO.

This currently works for me:
linux_use_bundled_binutils=0
binutils_version=0
linux_use_gold_flags=1

But If the combinations become more complicated in the future, I would prefer a
linux_use_debug_fission variable (and only add -B to ldflags) rather than
use_icecc.

[dshwang, 2014-04-23 15:33:09]

On 2014/04/23 15:06:51, Mostyn Bramley-Moore wrote:
> On 2014/04/23 14:52:21, Chris Dumez wrote:
> > Right now, we use "linux_use_bundled_binutils=0" to get icecc working. This
is
> > not very complicated IMHO.
> 
> This currently works for me:
> linux_use_bundled_binutils=0
> binutils_version=0
> linux_use_gold_flags=1

After reading your comment, linux_use_gold_flags is not very necessary to be
set.
However, I think using system gold is better than using thirdpary/bindutils
because icecc use system gcc. I'm afraid that there is conflict.
mithro@, wdyh?

> But If the combinations become more complicated in the future, I would prefer
a
> linux_use_debug_fission variable (and only add -B to ldflags) rather than
> use_icecc.

Before introducing -B, I agree on you. However, now icecc must concern about
both -B and debug fission. so linux_use_debug_fission is not enough.
In addition, googler also have similar purpose option; use_goma. googler uses
their own distributed build system, aka goma.

ch.dumez@, you can check your binutils version using following commend; > gcc
-Xassembler --version -x assembler -c /dev/null

[Lei Zhang, 2014-04-23 18:17:36]

I'd prefer use_icecc to set the other build flags as a convenience, rather than
checking use_icecc directly in conditionals.

[dshwang, 2014-04-23 19:28:45]

On 2014/04/23 18:17:36, Lei Zhang wrote:
> I'd prefer use_icecc to set the other build flags as a convenience, rather
than
> checking use_icecc directly in conditionals.

Thank you for review. I don't fully understand your suggestion. could you
explain again?

[Lei Zhang, 2014-04-23 20:26:17]

On 2014/04/23 19:28:45, dshwang wrote:
> On 2014/04/23 18:17:36, Lei Zhang wrote:
> > I'd prefer use_icecc to set the other build flags as a convenience, rather
> than
> > checking use_icecc directly in conditionals.
> 
> Thank you for review. I don't fully understand your suggestion. could you
> explain again?

Instead of:

use_icc = 1;

if (foo or bar or use_icc) {
  ...
}

Do:

use_icc = 1
if (use_icc) {
  foo = 1;
  qux = 0;
}

if (foo or bar) {
  ...
}

[dshwang, 2014-04-24 04:18:57]

thank you for kind explanation, but imo current code is not bad. could you
review my below comment?

https://codereview.chromium.org/249443002/diff/20001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/249443002/diff/20001/build/common.gypi#newcod...
build/common.gypi:734: ['OS=="linux" and (target_arch=="x64" or
target_arch=="arm") and use_icecc==0', {
use_icecc already set other build flags here.

If we want to change it to following pattern,
['use_icecc==1', {
  'linux_use_bundled_gold%': 0,
  'linux_use_bundled_binutils%': 0,
}],
these definition should stay more upper level, and linux_use_bundled_gold
variable will be in trouble, because "'linux_use_bundled_gold%': 1" means that
define linux_use_bundled_gold as 1 if linux_use_bundled_gold is not defined.
In this case, linux_use_bundled_gold may or may not be defined according to
use_icecc.

So imho current code is more succinct.

https://codereview.chromium.org/249443002/diff/20001/build/common.gypi#newcod...
build/common.gypi:3212: ['linux_use_gold_flags==1 and (clang==1 or
gcc_version>=48) and binutils_version>=223 and use_icecc==0', {
There is not foo variable for "if (use_icc) {  foo = 1; }"
icecc can use system gold.

[Mostyn Bramley-Moore, 2014-04-24 07:36:52]

https://codereview.chromium.org/249443002/diff/20001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/249443002/diff/20001/build/common.gypi#newcod...
build/common.gypi:267: 'use_icecc%': '<(use_icecc)',
nitpick: move this down a line, to keep the goma settings together.

https://codereview.chromium.org/249443002/diff/20001/build/common.gypi#newcod...
build/common.gypi:734: ['OS=="linux" and (target_arch=="x64" or
target_arch=="arm") and use_icecc==0', {
On 2014/04/24 04:18:57, dshwang wrote:
> use_icecc already set other build flags here.
> 
> If we want to change it to following pattern,
> ['use_icecc==1', {
>   'linux_use_bundled_gold%': 0,
>   'linux_use_bundled_binutils%': 0,
> }],
> these definition should stay more upper level, and linux_use_bundled_gold
> variable will be in trouble, because "'linux_use_bundled_gold%': 1" means that
> define linux_use_bundled_gold as 1 if linux_use_bundled_gold is not defined.
> In this case, linux_use_bundled_gold may or may not be defined according to
> use_icecc.
> 
> So imho current code is more succinct.

Is this really needed?  linux_use_bundled_gold is only used to modify ldflags,
which is only used in link commands, which icecc doesn't distribute anyway.

https://codereview.chromium.org/249443002/diff/20001/build/common.gypi#newcod...
build/common.gypi:1050: 'use_icecc%': '<(use_icecc)',
nitpick: move this down a line, to keep the goma settings together.

https://codereview.chromium.org/249443002/diff/20001/build/common.gypi#newcod...
build/common.gypi:3212: ['linux_use_gold_flags==1 and (clang==1 or
gcc_version>=48) and binutils_version>=223 and use_icecc==0', {
On 2014/04/24 04:18:57, dshwang wrote:
> There is not foo variable for "if (use_icc) {  foo = 1; }"
> icecc can use system gold.

IMO (but I think Nico disagreed previously), this is where we should use a new
linux_use_split_dwarf variable (the "foo" in Lei Zhang's suggestion?).

[dshwang, 2014-04-24 07:51:37]

Thank you for review.

https://codereview.chromium.org/249443002/diff/20001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/249443002/diff/20001/build/common.gypi#newcod...
build/common.gypi:267: 'use_icecc%': '<(use_icecc)',
On 2014/04/24 07:36:52, Mostyn Bramley-Moore wrote:
> nitpick: move this down a line, to keep the goma settings together.

yes, I'll fix it.

https://codereview.chromium.org/249443002/diff/20001/build/common.gypi#newcod...
build/common.gypi:734: ['OS=="linux" and (target_arch=="x64" or
target_arch=="arm") and use_icecc==0', {
That's good point. My machine works well without linux_use_bundled_gold.

However, linux_use_bundled_binutils=0 and linux_use_bundled_gold=1 looks
inconsistent to my eyes, because linux_use_bundled_binutils=0 makes gcc use
system "as" or "objcopy" but linux_use_bundled_gold=1 makes ninja use bundled
"ld".
For example, if system binutils version is 2.22, doesn't it break anything?
because we use 2.22 objcopy with 2.24 ld.

https://codereview.chromium.org/249443002/diff/20001/build/common.gypi#newcod...
build/common.gypi:3212: ['linux_use_gold_flags==1 and (clang==1 or
gcc_version>=48) and binutils_version>=223 and use_icecc==0', {
if we introduce linux_use_split_dwarf, that code looks like
['linux_use_gold_flags==1 and (clang==1 or gcc_version>=48) and
binutils_version>=223 and use_icecc==0', {
'linux_use_split_dwarf': 1
}],
...
['linux_use_split_dwarf==1', {
'cflags': ['-gsplit-dwarf'],
'ldflags': ['-Wl,--gdb-index'],
}],

IMO, looks not very advanced.

When Nico disagreed, icecc needs only binutils_version=0 hack, but now icecc
needs this whole CL. So Nico@, could I ask your opinion?

[Lei Zhang, 2014-04-24 19:06:41]

It seems you can just set these two flags to get what you want:
linux_use_bundled_binutils=0 linux_use_gold_flags==0

So if you go with my suggestion, just add an additional conditional in the block
between lines ~1346-2106:

['use_icecc==1', {
  'linux_use_bundled_binutils%: 0,
  'linux_use_gold_flags%': 0,
}],

and leave the other conditionals alone.

linux_use_gold_flags==1 is used to enable threading with gold, but that feature
causes gold to crash ~0.1% of the time, which was enough to annoy people to turn
it off. Even when it worked, it's only a slight improvement in build time.

https://codereview.chromium.org/249443002/diff/20001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/249443002/diff/20001/build/common.gypi#newcod...
build/common.gypi:734: ['OS=="linux" and (target_arch=="x64" or
target_arch=="arm") and use_icecc==0', {
On 2014/04/24 07:51:37, dshwang wrote:
> That's good point. My machine works well without linux_use_bundled_gold.
> 
> However, linux_use_bundled_binutils=0 and linux_use_bundled_gold=1 looks
> inconsistent to my eyes, because linux_use_bundled_binutils=0 makes gcc use
> system "as" or "objcopy" but linux_use_bundled_gold=1 makes ninja use bundled
> "ld".
> For example, if system binutils version is 2.22, doesn't it break anything?
> because we use 2.22 objcopy with 2.24 ld.

I don't think it'll break anything. Before third_party/binutils, we only had
third_party/gold, so we were using the system toolchain for everything but the
linker, which got version bumps independent of everything else.

[dshwang, 2014-04-24 20:13:04]

On 2014/04/24 19:06:41, Lei Zhang wrote:
> It seems you can just set these two flags to get what you want:
> linux_use_bundled_binutils=0 linux_use_gold_flags==0
> 
> So if you go with my suggestion, just add an additional conditional in the
block
> between lines ~1346-2106:
> 
> ['use_icecc==1', {
>   'linux_use_bundled_binutils%: 0,
>   'linux_use_gold_flags%': 0,
> }],
> 
> and leave the other conditionals alone.
> 
> linux_use_gold_flags==1 is used to enable threading with gold, but that
feature
> causes gold to crash ~0.1% of the time, which was enough to annoy people to
turn
> it off. Even when it worked, it's only a slight improvement in build time.

Thank you for kind explanation, but
linux_use_bundled_binutils and linux_use_gold_flags are already defined between
lines ~734-749
        ['OS=="linux" and (target_arch=="x64" or target_arch=="arm")', {
          'linux_use_bundled_gold%': 1,
        }, {
          'linux_use_bundled_gold%': 0,
        }],

        ['OS=="linux" and (target_arch=="x64")', {
          'linux_use_bundled_binutils%': 1,
        }, {
          'linux_use_bundled_binutils%': 0,
        }],

so definition between lines ~1346-2106 is skipped.
> ['use_icecc==1', {
>   'linux_use_bundled_binutils%: 0,
>   'linux_use_gold_flags%': 0,
> }],

Otherwise, if you put if(use_icecc) statement prior to the origin definition,
the origin definition is in trouble because if(use_icecc) statement defines
those variables.

> https://codereview.chromium.org/249443002/diff/20001/build/common.gypi
> File build/common.gypi (right):
> 
>
https://codereview.chromium.org/249443002/diff/20001/build/common.gypi#newcod...
> build/common.gypi:734: ['OS=="linux" and (target_arch=="x64" or
> target_arch=="arm") and use_icecc==0', {
> On 2014/04/24 07:51:37, dshwang wrote:
> > That's good point. My machine works well without linux_use_bundled_gold.
> > 
> > However, linux_use_bundled_binutils=0 and linux_use_bundled_gold=1 looks
> > inconsistent to my eyes, because linux_use_bundled_binutils=0 makes gcc use
> > system "as" or "objcopy" but linux_use_bundled_gold=1 makes ninja use
bundled
> > "ld".
> > For example, if system binutils version is 2.22, doesn't it break anything?
> > because we use 2.22 objcopy with 2.24 ld.
> 
> I don't think it'll break anything. Before third_party/binutils, we only had
> third_party/gold, so we were using the system toolchain for everything but the
> linker, which got version bumps independent of everything else.

Thank you for explaining history. linux_use_gold_flags is not related to this
CL, so I'll remove linux_use_gold_flags change in next patch set.

[Lei Zhang, 2014-04-24 20:48:09]

On 2014/04/24 20:13:04, dshwang wrote:
> Thank you for kind explanation, but
> linux_use_bundled_binutils and linux_use_gold_flags are already defined
between
> lines ~734-749
>         ['OS=="linux" and (target_arch=="x64" or target_arch=="arm")', {
>           'linux_use_bundled_gold%': 1,
>         }, {
>           'linux_use_bundled_gold%': 0,
>         }],
> 
>         ['OS=="linux" and (target_arch=="x64")', {
>           'linux_use_bundled_binutils%': 1,
>         }, {
>           'linux_use_bundled_binutils%': 0,
>         }],
> 
> so definition between lines ~1346-2106 is skipped.

The definitions between lines ~1346-2106 are one level outside of the variables
block for linux_use_foo, so they can overwrite the values in the inner variables
block. Look at the "use_aura" as an example that's doing exactly what I am
suggesting. In the inner most variables block, use_aura is set to 0, but the
next level outside of it has conditionals to set use_aura to 1.

[dshwang, 2014-04-25 07:07:01]

On 2014/04/24 20:48:09, Lei Zhang wrote:
> On 2014/04/24 20:13:04, dshwang wrote:
> > Thank you for kind explanation, but
> > linux_use_bundled_binutils and linux_use_gold_flags are already defined
> between
> > lines ~734-749
> >         ['OS=="linux" and (target_arch=="x64" or target_arch=="arm")', {
> >           'linux_use_bundled_gold%': 1,
> >         }, {
> >           'linux_use_bundled_gold%': 0,
> >         }],
> > 
> >         ['OS=="linux" and (target_arch=="x64")', {
> >           'linux_use_bundled_binutils%': 1,
> >         }, {
> >           'linux_use_bundled_binutils%': 0,
> >         }],
> > 
> > so definition between lines ~1346-2106 is skipped.
> 
> The definitions between lines ~1346-2106 are one level outside of the
variables
> block for linux_use_foo, so they can overwrite the values in the inner
variables
> block. Look at the "use_aura" as an example that's doing exactly what I am
> suggesting. In the inner most variables block, use_aura is set to 0, but the
> next level outside of it has conditionals to set use_aura to 1.

Thank you for your patience to explain me. I understand gyp more now.
I put following code near original definition for convenience.
        ['use_icecc==1', {
          'linux_use_bundled_binutils%': 0,
        }],
gyp seems to evaluate only the last condition statement when there are multiple
not-mutually-exclusive condition statements.
wdyt?

[Lei Zhang, 2014-04-25 07:34:21]

So the alternative is to have a linux_use_debug_fission variable as Mostyn
suggested. If we were to have that in place of use_icecc, then icecc users would
have to set linux_use_debug_fission=0 linux_use_bundled_binutils=0.
https://codereview.chromium.org/203153005/ already does this BTW, but Nico
already don't want to add more flags.

How do icecc users feel about this? It is slightly more of a hassle for you, but
it gives everyone an easy knob to turn off debug fission if needed. If I had to
pick one variable to add, it would be linux_use_debug_fission.

https://codereview.chromium.org/249443002/diff/70001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/249443002/diff/70001/build/common.gypi#newcod...
build/common.gypi:173: # When using icecc, we cannot use some build flags. e.g.
debug fission, -B cflags.
How about:

Set to 1 when using icecc to avoid incompatible build flags.
e.g. ...

[Mostyn Bramley-Moore, 2014-04-25 12:51:11]

On 2014/04/25 07:34:21, Lei Zhang wrote:
> So the alternative is to have a linux_use_debug_fission variable as Mostyn
> suggested. If we were to have that in place of use_icecc, then icecc users
would
> have to set linux_use_debug_fission=0 linux_use_bundled_binutils=0.
> https://codereview.chromium.org/203153005/ already does this BTW, but Nico
> already don't want to add more flags.

Actually I believe Nico's objection was more along the lines of (paraphrasing)
"don't introduce such a variable while this simple workaround works (and remains
simple)".  I'm not sure if he would consider the current setup to have crossed
the line over to a non-simple workaround.  I would argue that it has.  

@Nico: what do you think?

> How do icecc users feel about this? It is slightly more of a hassle for you,
but
> it gives everyone an easy knob to turn off debug fission if needed. If I had
to
> pick one variable to add, it would be linux_use_debug_fission.

I slightly prefer linux_use_debug_fission, since it is more understandable for
other build setups that are broken by -gdebug-fission, such as ccache (I see
patches, but no official solution yet).  People using ccache but not icecc might
find it confusing to have to set use_icecc=1.  

I will rebase my CL on master so we can compare the two options, but might not
find time to do this for the next few hours.

[dshwang, 2014-04-25 14:30:52]

On 2014/04/25 12:51:11, Mostyn Bramley-Moore wrote:
> On 2014/04/25 07:34:21, Lei Zhang wrote:
> > So the alternative is to have a linux_use_debug_fission variable as Mostyn
> > suggested. If we were to have that in place of use_icecc, then icecc users
> would
> > have to set linux_use_debug_fission=0 linux_use_bundled_binutils=0.
> > https://codereview.chromium.org/203153005/ already does this BTW, but Nico
> > already don't want to add more flags.
> 
> Actually I believe Nico's objection was more along the lines of (paraphrasing)
> "don't introduce such a variable while this simple workaround works (and
remains
> simple)".  I'm not sure if he would consider the current setup to have crossed
> the line over to a non-simple workaround.  I would argue that it has.  
> 
> @Nico: what do you think?
> 
> > How do icecc users feel about this? It is slightly more of a hassle for you,
> but
> > it gives everyone an easy knob to turn off debug fission if needed. If I had
> to
> > pick one variable to add, it would be linux_use_debug_fission.
> 
> I slightly prefer linux_use_debug_fission, since it is more understandable for
> other build setups that are broken by -gdebug-fission, such as ccache (I see
> patches, but no official solution yet).  People using ccache but not icecc
might
> find it confusing to have to set use_icecc=1.  
> 
> I will rebase my CL on master so we can compare the two options, but might not
> find time to do this for the next few hours.

After reading your comment, linux_use_debug_fission is also good. And Lei and
Mostyn prefer linux_use_debug_fission.
So I close this cl as WontFix. I hope https://codereview.chromium.org/226613005/
is landed asap for icecc user.

[dshwang, 2014-04-26 15:36:43]

Hey, I reopen this CL. Now this CL is just warning for icecc user via comment.
It would be helpful for new comers.

[Lei Zhang, 2014-04-28 20:16:28]

On 2014/04/26 15:36:43, dshwang wrote:
> Hey, I reopen this CL. Now this CL is just warning for icecc user via comment.
> It would be helpful for new comers.

I think you are better off adding an icecc section to
https://code.google.com/p/chromium/wiki/LinuxFasterBuilds rather than putting
comments in the gypi file. Ideally, that would be the first thing that shows up
when someone searches for "chromium icecc". Currently, this CL is the first hit
on Google Search.

[mithro-old, 2014-05-21 00:03:29]

On 2014/04/28 20:16:28, Lei Zhang wrote:
> On 2014/04/26 15:36:43, dshwang wrote:
> > Hey, I reopen this CL. Now this CL is just warning for icecc user via
comment.
> > It would be helpful for new comers.
> 
> I think you are better off adding an icecc section to
> https://code.google.com/p/chromium/wiki/LinuxFasterBuilds rather than putting
> comments in the gypi file. Ideally, that would be the first thing that shows
up
> when someone searches for "chromium icecc". Currently, this CL is the first
hit
> on Google Search.

Hi,

Can we close this issue now?

Tim

[dshwang, 2014-05-21 09:32:18]

On 2014/05/21 00:03:29, mithro wrote:
> On 2014/04/28 20:16:28, Lei Zhang wrote:
> > On 2014/04/26 15:36:43, dshwang wrote:
> > > Hey, I reopen this CL. Now this CL is just warning for icecc user via
> comment.
> > > It would be helpful for new comers.
> > 
> > I think you are better off adding an icecc section to
> > https://code.google.com/p/chromium/wiki/LinuxFasterBuilds rather than
putting
> > comments in the gypi file. Ideally, that would be the first thing that shows
> up
> > when someone searches for "chromium icecc". Currently, this CL is the first
> hit
> > on Google Search.
> 
> Hi,
> 
> Can we close this issue now?
> 
> Tim

Sorry for delay. I add an icecc section,
https://code.google.com/p/chromium/wiki/LinuxFasterBuilds?ts=1400664654&updat...
I close this CL as WontFix