Assert that the chosen Android config is safe.

Assert that the chosen Android config is safe.

The default Android release build configuration fails to link on 32-bit
devices, because the combination of a 32-bit target CPU, a non-component
build, and symbol_level=2 produces a binary that's >4GiB. Assert that at
least one of these conditions doesn't apply at gn time, to prevent
people being surprised by the inevitable link failure.

BUG=648948
Committed: https://crrev.com/090bc3fbad2a085f1eaffa27921072e769729377
Cr-Commit-Position: refs/heads/master@{#421860}

[Torne, 2016-09-28 12:13:16]

torne@chromium.org changed reviewers:
+ agrieve@chromium.org

[Torne, 2016-09-28 12:13:16]

People are continuing to discover their build doesn't link. What do you think of
this? A couple of questions:

1) Do we want to change the default config? I'm not sure that quietly setting
symbol_level=1 is sensible, because it seems likely to surprise people who debug
with gdb. I think a noisy assert is "safer", but may be more annoying?

2) Is this a sensible place to put the assert? I tried to add it to
build/config/android/config.gni but can't check symbol_level there due to a
circular dependency with compiler.gni.

3) Is that the right way to wrap a long string in GN? gn format is happy with
it, but it looks a bit weird.

[agrieve, 2016-09-28 13:57:25]

On 2016/09/28 12:13:16, Torne wrote:
> People are continuing to discover their build doesn't link. What do you think
of
> this? A couple of questions:
> 
> 1) Do we want to change the default config? I'm not sure that quietly setting
> symbol_level=1 is sensible, because it seems likely to surprise people who
debug
> with gdb. I think a noisy assert is "safer", but may be more annoying?
> 
> 2) Is this a sensible place to put the assert? I tried to add it to
> build/config/android/config.gni but can't check symbol_level there due to a
> circular dependency with compiler.gni.
> 
> 3) Is that the right way to wrap a long string in GN? gn format is happy with
> it, but it looks a bit weird.

1) Fully agree assert is better than quietly changing the value.
2) Seems sensible
3) That's how I've been wrapping them anyways.

[agrieve, 2016-09-28 13:57:33]

On 2016/09/28 13:57:25, agrieve wrote:
> On 2016/09/28 12:13:16, Torne wrote:
> > People are continuing to discover their build doesn't link. What do you
think
> of
> > this? A couple of questions:
> > 
> > 1) Do we want to change the default config? I'm not sure that quietly
setting
> > symbol_level=1 is sensible, because it seems likely to surprise people who
> debug
> > with gdb. I think a noisy assert is "safer", but may be more annoying?
> > 
> > 2) Is this a sensible place to put the assert? I tried to add it to
> > build/config/android/config.gni but can't check symbol_level there due to a
> > circular dependency with compiler.gni.
> > 
> > 3) Is that the right way to wrap a long string in GN? gn format is happy
with
> > it, but it looks a bit weird.
> 
> 1) Fully agree assert is better than quietly changing the value.
> 2) Seems sensible
> 3) That's how I've been wrapping them anyways.

4) lgtm :)

[agrieve, 2016-09-28 14:23:04]

On 2016/09/28 13:57:33, agrieve wrote:
> On 2016/09/28 13:57:25, agrieve wrote:
> > On 2016/09/28 12:13:16, Torne wrote:
> > > People are continuing to discover their build doesn't link. What do you
> think
> > of
> > > this? A couple of questions:
> > > 
> > > 1) Do we want to change the default config? I'm not sure that quietly
> setting
> > > symbol_level=1 is sensible, because it seems likely to surprise people who
> > debug
> > > with gdb. I think a noisy assert is "safer", but may be more annoying?
> > > 
> > > 2) Is this a sensible place to put the assert? I tried to add it to
> > > build/config/android/config.gni but can't check symbol_level there due to
a
> > > circular dependency with compiler.gni.
> > > 
> > > 3) Is that the right way to wrap a long string in GN? gn format is happy
> with
> > > it, but it looks a bit weird.
> > 
> > 1) Fully agree assert is better than quietly changing the value.
> > 2) Seems sensible
> > 3) That's how I've been wrapping them anyways.
> 
> 4) lgtm :)

Actually - I think this might actually be one better if put here:
https://cs.chromium.org/chromium/src/build/config/compiler/compiler.gni

It probably also makes sense to update the default value logic.

[Dirk Pranke, 2016-09-28 16:45:40]

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

[Dirk Pranke, 2016-09-28 16:45:40]

I agree that the change should be in compiler.gni instead.

Yes, that is the correct way to wrap strings.

[Torne, 2016-09-29 16:12:49]

Moved to compiler.gni.

Do we want to change the default selection behaviour or not? You contradicted
yourself Andrew :)

I'm fine with either, but have a slight preference to leave it alone to avoid
surprising people who use gdb, since it seems (from recent internal list
threads) that people have a hard enough time getting gdb debugging to work on
android as it is ;)

[agrieve, 2016-09-29 16:36:41]

On 2016/09/29 16:12:49, Torne wrote:
> Moved to compiler.gni.
> 
> Do we want to change the default selection behaviour or not? You contradicted
> yourself Andrew :)
> 
> I'm fine with either, but have a slight preference to leave it alone to avoid
> surprising people who use gdb, since it seems (from recent internal list
> threads) that people have a hard enough time getting gdb debugging to work on
> android as it is ;)

It's a cointoss :P. lgtm

[Torne, 2016-09-29 16:37:44]

If it annoys a lot of people I'll change it then. Leaving it as-is for now.

[Torne, 2016-09-29 16:37:48]

The CQ bit was checked by torne@chromium.org

[commit-bot: I haz the power, 2016-09-29 16:38:15]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-29 17:29:35]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-09-29 17:31:45]

Description was changed from

==========
Assert that the chosen Android config is safe.

The default Android release build configuration fails to link on 32-bit
devices, because the combination of a 32-bit target CPU, a non-component
build, and symbol_level=2 produces a binary that's >4GiB. Assert that at
least one of these conditions doesn't apply at gn time, to prevent
people being surprised by the inevitable link failure.

BUG=648948
==========

to

==========
Assert that the chosen Android config is safe.

The default Android release build configuration fails to link on 32-bit
devices, because the combination of a 32-bit target CPU, a non-component
build, and symbol_level=2 produces a binary that's >4GiB. Assert that at
least one of these conditions doesn't apply at gn time, to prevent
people being surprised by the inevitable link failure.

BUG=648948

Committed: https://crrev.com/090bc3fbad2a085f1eaffa27921072e769729377
Cr-Commit-Position: refs/heads/master@{#421860}
==========

[commit-bot: I haz the power, 2016-09-29 17:31:46]

Patchset 2 (id:??) landed as
https://crrev.com/090bc3fbad2a085f1eaffa27921072e769729377
Cr-Commit-Position: refs/heads/master@{#421860}

[Dirk Pranke, 2016-09-29 17:54:31]

lgtm.