Introduce arm_version to allow building for armv5, v6 or v7.

Introduce arm_version to allow building for armv5, v6 or v7.

This patch deprecates armv7 and adds arm_version that takes an integer
value representing the ARM architecture level.

In addition arm_arch, arm_tune, arm_fpu, arm_float_abi and arm_thumb can
be set to fine tune CPU related compiler flags, defaults are provided for
ARM versions 5 to 7.

BUG=234135
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=196702

[Fredrik Öhrn, 2013-04-10 15:09:44]

We use this patch to better tune both our embedded Linux and Android builds to
the various ARM devices out there.

If this change is approved, I have a follow up patch that eliminates all
remaining armv7 use from the tree.

I have no idea who to add as reviewer for the changes in /build, suggestions
appreciated.

CCed torne becasue this replaces his android_webview_build changes.
CCed piman becasue it looks like he made similar changes before.

[commit-bot: I haz the power, 2013-04-10 15:16:45]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a lowly provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[Stephen White, 2013-04-10 15:24:12]

The skia.gyp changes seem reasonable to me, but I defer to piman for the
common.gypi changes.

[Torne, 2013-04-10 15:29:49]

cjhopman, yfriedman: does this look reasonable for android?

[piman, 2013-04-10 17:45:48]

I understand the idea, and support the cleanup, but you're significantly
changing the defaults, and I suspect you'll have to change the Chrome OS build
scripts for all the arm bots.

[Fredrik Öhrn, 2013-04-10 17:55:56]

On 2013/04/10 17:45:48, piman wrote:
> I understand the idea, and support the cleanup, but you're significantly
> changing the defaults, and I suspect you'll have to change the Chrome OS build
> scripts for all the arm bots.

I was wondering about that, for Android all I had to do was fix the the envsetup
script. Can someone please point me in the right direction how to update
ChromeOS and other affected builds?

[Yaron, 2013-04-10 18:01:44]

https://codereview.chromium.org/14065005/diff/6001/build/android/envsetup_fun...
File build/android/envsetup_functions.sh (right):

https://codereview.chromium.org/14065005/diff/6001/build/android/envsetup_fun...
build/android/envsetup_functions.sh:109: DEFINES+=" arm_neon=0 arm_version=7
arm_thumb=1 arm_fpu=vfpv3-d16"
Rather than just updating this field, I'd prefer that you didn't set
"arm_version" here and move the default value into gyp itself. The less we
depend on envsetup, the better.

[piman, 2013-04-10 18:03:50]

On 2013/04/10 17:55:56, Fredrik Öhrn wrote:
> On 2013/04/10 17:45:48, piman wrote:
> > I understand the idea, and support the cleanup, but you're significantly
> > changing the defaults, and I suspect you'll have to change the Chrome OS
build
> > scripts for all the arm bots.
> 
> I was wondering about that, for Android all I had to do was fix the the
envsetup
> script. Can someone please point me in the right direction how to update
> ChromeOS and other affected builds?

On Chrome OS, the build script for Chrome is in what's called the "ebuild":
http://git.chromium.org/gitweb/?p=chromiumos/overlays/chromiumos-overlay.git;...

Some of the configuration that varies per platform is in the per-board overlay,
e.g.
http://git.chromium.org/gitweb/?p=chromiumos/overlays/board-overlays.git;a=bl...

See also http://dev.chromium.org/chromium-os/developer-guide

I'm adding davidjames in cc, a Chrome OS build expert who may be able to help
you navigate this.

[Fredrik Öhrn, 2013-04-10 19:16:10]

https://codereview.chromium.org/14065005/diff/6001/build/android/envsetup_fun...
File build/android/envsetup_functions.sh (right):

https://codereview.chromium.org/14065005/diff/6001/build/android/envsetup_fun...
build/android/envsetup_functions.sh:109: DEFINES+=" arm_neon=0 arm_version=7
arm_thumb=1 arm_fpu=vfpv3-d16"
On 2013/04/10 18:01:44, Yaron wrote:
> Rather than just updating this field, I'd prefer that you didn't set
> "arm_version" here and move the default value into gyp itself. The less we
> depend on envsetup, the better.

OK, I did it this way because the existing defaults in the gyp file where set to
a armv5 build.

I'll change the defaults to match this line and remove it.

[Torne, 2013-04-11 08:38:06]

On 2013/04/10 19:16:10, Fredrik Öhrn wrote:
>
https://codereview.chromium.org/14065005/diff/6001/build/android/envsetup_fun...
> File build/android/envsetup_functions.sh (right):
> 
>
https://codereview.chromium.org/14065005/diff/6001/build/android/envsetup_fun...
> build/android/envsetup_functions.sh:109: DEFINES+=" arm_neon=0 arm_version=7
> arm_thumb=1 arm_fpu=vfpv3-d16"
> On 2013/04/10 18:01:44, Yaron wrote:
> > Rather than just updating this field, I'd prefer that you didn't set
> > "arm_version" here and move the default value into gyp itself. The less we
> > depend on envsetup, the better.
> 
> OK, I did it this way because the existing defaults in the gyp file where set
to
> a armv5 build.
> 
> I'll change the defaults to match this line and remove it.

Similarly, we also want to eliminate envsetup.sh for the webview build, so it'd
be preferable to handle this in the gyp file as well using the
"android_webview_build" gyp variable which is the master switch for this build
type.

[Fredrik Öhrn, 2013-04-11 12:40:32]

Updated gyp defaults and trimmed envsetup.

[Torne, 2013-04-11 13:04:22]

https://codereview.chromium.org/14065005/diff/16001/build/common.gypi
File build/common.gypi (left):

https://codereview.chromium.org/14065005/diff/16001/build/common.gypi#oldcode...
build/common.gypi:2844: # Flags suitable for Android emulator
This block doesn't seem to have a replacement..

https://codereview.chromium.org/14065005/diff/16001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/14065005/diff/16001/build/common.gypi#newcode215
build/common.gypi:215: 'arm_version%': 7,
This changes the default; is this intentional?

https://codereview.chromium.org/14065005/diff/16001/build/common.gypi#newcode218
build/common.gypi:218: 'arm_neon%': 0,
Same here.

https://codereview.chromium.org/14065005/diff/16001/build/common.gypi#newcode613
build/common.gypi:613: 'arm_version%': 7,
This is setting it to the same value as above; if the change of default is
intentional then this is unnecessary.

https://codereview.chromium.org/14065005/diff/16001/build/common.gypi#newcode...
build/common.gypi:1030: 'armv7': 0,
This shouldn't be necessary; this value will always be overwritten by the one
set below.

https://codereview.chromium.org/14065005/diff/16001/build/common.gypi#newcode...
build/common.gypi:1598: 'conditions': [
I generally prefer not to use nested conditions sections like this because deep
gyp nesting is hard to read; it may be neater to just repeat "and
android_webview_build==0" and have all of these blocks at the same level.

https://codereview.chromium.org/14065005/diff/16001/skia/skia.gyp
File skia/skia.gyp (right):

https://codereview.chromium.org/14065005/diff/16001/skia/skia.gyp#newcode345
skia/skia.gyp:345: '__ARM_ARCH__=7',
We could just define __ARM_ARCH__=<(arm_version) ?

[Fredrik Öhrn, 2013-04-11 13:42:18]

https://codereview.chromium.org/14065005/diff/16001/build/common.gypi
File build/common.gypi (left):

https://codereview.chromium.org/14065005/diff/16001/build/common.gypi#oldcode...
build/common.gypi:2844: # Flags suitable for Android emulator
On 2013/04/11 13:04:22, Torne wrote:
> This block doesn't seem to have a replacement..

It's replaced by the arm_version==5 block on line 1600.
The defines are redundant, they are compiler builtins.

https://codereview.chromium.org/14065005/diff/16001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/14065005/diff/16001/build/common.gypi#newcode215
build/common.gypi:215: 'arm_version%': 7,
On 2013/04/11 13:04:22, Torne wrote:
> This changes the default; is this intentional?

Maybe I misunderstood your comments on the previous version of the diff. I
thought you guys wanted the gyp defaults to be changed to match what was
previously configured in envsetup.

I can restore the armv5 defaults and set arm_version to 7 in an OS==android
condition if you prefer.

https://codereview.chromium.org/14065005/diff/16001/build/common.gypi#newcode...
build/common.gypi:1030: 'armv7': 0,
On 2013/04/11 13:04:22, Torne wrote:
> This shouldn't be necessary; this value will always be overwritten by the one
> set below.

I had to put it in or gyp would throw and undefined error for no good reason
when reading a gyp file in third_party.

This was when I initially wrote the patch back on top of 25.0.1364. Seems the
problem is gone now, I'll remove it.

https://codereview.chromium.org/14065005/diff/16001/skia/skia.gyp
File skia/skia.gyp (right):

https://codereview.chromium.org/14065005/diff/16001/skia/skia.gyp#newcode345
skia/skia.gyp:345: '__ARM_ARCH__=7',
On 2013/04/11 13:04:22, Torne wrote:
> We could just define __ARM_ARCH__=<(arm_version) ?

Sure, but it looks like it's not used in skia. Shall I remove it instead?

Also, __ARM_ARCH__ is set by machine/cpu-features.h in the NDK, so there should
be no need to set it in the build system.

[Torne, 2013-04-11 13:55:00]

https://codereview.chromium.org/14065005/diff/16001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/14065005/diff/16001/build/common.gypi#newcode215
build/common.gypi:215: 'arm_version%': 7,
On 2013/04/11 13:42:19, Fredrik Öhrn wrote:
> On 2013/04/11 13:04:22, Torne wrote:
> > This changes the default; is this intentional?
> 
> Maybe I misunderstood your comments on the previous version of the diff. I
> thought you guys wanted the gyp defaults to be changed to match what was
> previously configured in envsetup.
> 
> I can restore the armv5 defaults and set arm_version to 7 in an OS==android
> condition if you prefer.

That envsetup.sh script is specifically for Android builds; what you've changed
is the global default for Chromium. I don't know what other users of Chromium on
ARM may want the defaults to be, so leaving it alone seems preferable.
I'm not sure that the default is intended to be armv5, either; the Android
config uses that when armv7 isn't set, but that's not true of linux/chromeos as
far as I can see..

https://codereview.chromium.org/14065005/diff/16001/skia/skia.gyp
File skia/skia.gyp (right):

https://codereview.chromium.org/14065005/diff/16001/skia/skia.gyp#newcode345
skia/skia.gyp:345: '__ARM_ARCH__=7',
On 2013/04/11 13:42:19, Fredrik Öhrn wrote:
> On 2013/04/11 13:04:22, Torne wrote:
> > We could just define __ARM_ARCH__=<(arm_version) ?
> 
> Sure, but it looks like it's not used in skia. Shall I remove it instead?
> 
> Also, __ARM_ARCH__ is set by machine/cpu-features.h in the NDK, so there
should
> be no need to set it in the build system.

Probably this can be removed then, yes.

[Fredrik Öhrn, 2013-04-11 15:18:46]

https://codereview.chromium.org/14065005/diff/16001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/14065005/diff/16001/build/common.gypi#newcode215
build/common.gypi:215: 'arm_version%': 7,
On 2013/04/11 13:55:00, Torne wrote:
> On 2013/04/11 13:42:19, Fredrik Öhrn wrote:
> > On 2013/04/11 13:04:22, Torne wrote:
> > > This changes the default; is this intentional?
> > 
> > Maybe I misunderstood your comments on the previous version of the diff. I
> > thought you guys wanted the gyp defaults to be changed to match what was
> > previously configured in envsetup.
> > 
> > I can restore the armv5 defaults and set arm_version to 7 in an OS==android
> > condition if you prefer.
> 
> That envsetup.sh script is specifically for Android builds; what you've
changed
> is the global default for Chromium. I don't know what other users of Chromium
on
> ARM may want the defaults to be, so leaving it alone seems preferable.
> I'm not sure that the default is intended to be armv5, either; the Android
> config uses that when armv7 isn't set, but that's not true of linux/chromeos
as
> far as I can see..

Well, I could perhaps add an arm_version 0 default that sets no compiler flags,
that would leave the defaults more or less unchanged for other products.

https://codereview.chromium.org/14065005/diff/16001/build/common.gypi#newcode218
build/common.gypi:218: 'arm_neon%': 0,
On 2013/04/11 13:04:22, Torne wrote:
> Same here.

I have to keep arm_neon disabled when changing the arm_version default back to
<= 5 or it won't build.

This is because third_party/libvpx/libvpx.gyp uses arm_neon without testing that
it's also a v7 build.

[Fredrik Öhrn, 2013-04-11 15:37:13]

https://codereview.chromium.org/14065005/diff/16001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/14065005/diff/16001/build/common.gypi#newcode218
build/common.gypi:218: 'arm_neon%': 0,
On 2013/04/11 15:18:46, Fredrik Öhrn wrote:
> On 2013/04/11 13:04:22, Torne wrote:
> > Same here.
> 
> I have to keep arm_neon disabled when changing the arm_version default back to
> <= 5 or it won't build.
> 
> This is because third_party/libvpx/libvpx.gyp uses arm_neon without testing
that
> it's also a v7 build.

Turns out libvpx throws some handcrafted armv6 assembler into the mix as well,
so building armv5 isn't currently possible.

I'm guessing none of your projects are building anything less than armv7 or
these two problems would have been noticed.

Perhaps bumping the defaults to armv7 isn't such a bad idea?

[piman, 2013-04-11 17:05:55]

On Thu, Apr 11, 2013 at 8:37 AM, <ohrn@opera.com> wrote:

>
>
https://codereview.chromium.**org/14065005/diff/16001/build/**common.gypi<htt...
> File build/common.gypi (right):
>
> https://codereview.chromium.**org/14065005/diff/16001/build/**
>
common.gypi#newcode218<https://codereview.chromium.org/14065005/diff/16001/build/common.gypi#newcode218>
> build/common.gypi:218: 'arm_neon%': 0,
> On 2013/04/11 15:18:46, Fredrik Öhrn wrote:
>
>> On 2013/04/11 13:04:22, Torne wrote:
>> > Same here.
>>
>
>  I have to keep arm_neon disabled when changing the arm_version default
>>
> back to
>
>> <= 5 or it won't build.
>>
>
>  This is because third_party/libvpx/libvpx.gyp uses arm_neon without
>>
> testing that
>
>> it's also a v7 build.
>>
>
> Turns out libvpx throws some handcrafted armv6 assembler into the mix as
> well, so building armv5 isn't currently possible.
>
> I'm guessing none of your projects are building anything less than armv7
> or these two problems would have been noticed.
>
> Perhaps bumping the defaults to armv7 isn't such a bad idea?
>

Chrome OS definitely doesn't have pre-armv7 board.


>
>
https://codereview.chromium.**org/14065005/<https://codereview.chromium.org/1...
>

[Fredrik Öhrn, 2013-04-12 09:43:52]

On 2013/04/11 17:05:55, piman wrote:
> On Thu, Apr 11, 2013 at 8:37 AM, <mailto:ohrn@opera.com> wrote:
> >
> > Perhaps bumping the defaults to armv7 isn't such a bad idea?
> >
> 
> Chrome OS definitely doesn't have pre-armv7 board.
> 

So who can make the decision?

[Torne, 2013-04-12 10:00:25]

On 2013/04/12 09:43:52, Fredrik Öhrn wrote:
> On 2013/04/11 17:05:55, piman wrote:
> > On Thu, Apr 11, 2013 at 8:37 AM, <mailto:ohrn@opera.com> wrote:
> > >
> > > Perhaps bumping the defaults to armv7 isn't such a bad idea?
> > >
> > 
> > Chrome OS definitely doesn't have pre-armv7 board.
> > 
> 
> So who can make the decision?

Nobody in particular, so I will. :)

Let's change the default to armv7, since android and chromeos both pretty much
expect that to be the default.

So, can you just address the other nits here? :)

[Fredrik Öhrn, 2013-04-12 12:01:02]

On 2013/04/12 10:00:25, Torne wrote:
> 
> So, can you just address the other nits here? :)

Done.

[Torne, 2013-04-12 12:20:33]

Looks mostly fine, but a couple of nits still, sorry :/

Also, just to be clear, let's get approvals from yfriedman (for Android
non-webview) and someone from ChromeOS as well to make sure we are covering all
the bases..

https://codereview.chromium.org/14065005/diff/26001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/14065005/diff/26001/build/common.gypi#newcode218
build/common.gypi:218: 'arm_neon%': 0,
Setting this to 0 changes the default config for arm/linux still (since
previously it jsut set armv7 to 1 below) - maybe we should also default to neon
enabled?

https://codereview.chromium.org/14065005/diff/26001/build/common.gypi#newcode...
build/common.gypi:1616: ['android_webview_build==1', {
I'm not sure you can rely on these being executed in order. What I meant was for
*all* the sections to test android_webview_build..

[Fredrik Öhrn, 2013-04-12 12:58:05]

Last two issues addressed in patch 5.

https://codereview.chromium.org/14065005/diff/26001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/14065005/diff/26001/build/common.gypi#newcode218
build/common.gypi:218: 'arm_neon%': 0,
On 2013/04/12 12:20:33, Torne wrote:
> Setting this to 0 changes the default config for arm/linux still (since
> previously it jsut set armv7 to 1 below) - maybe we should also default to
neon
> enabled?

To make it compatible with more devices by default, using

arm_neon: 0
arm_neon_optional: 1 

seems better.

https://codereview.chromium.org/14065005/diff/26001/build/common.gypi#newcode...
build/common.gypi:1616: ['android_webview_build==1', {
On 2013/04/12 12:20:33, Torne wrote:
> I'm not sure you can rely on these being executed in order. What I meant was
for
> *all* the sections to test android_webview_build..

Not being able to depend on the execution order feels rather primitive. It
worked as expected in this case.

[Torne, 2013-04-12 14:26:41]

On 2013/04/12 12:58:05, Fredrik Öhrn wrote:
> Last two issues addressed in patch 5.
> 
> https://codereview.chromium.org/14065005/diff/26001/build/common.gypi
> File build/common.gypi (right):
> 
>
https://codereview.chromium.org/14065005/diff/26001/build/common.gypi#newcode218
> build/common.gypi:218: 'arm_neon%': 0,
> On 2013/04/12 12:20:33, Torne wrote:
> > Setting this to 0 changes the default config for arm/linux still (since
> > previously it jsut set armv7 to 1 below) - maybe we should also default to
> neon
> > enabled?
> 
> To make it compatible with more devices by default, using
> 
> arm_neon: 0
> arm_neon_optional: 1 
> 
> seems better.
> 
>
https://codereview.chromium.org/14065005/diff/26001/build/common.gypi#newcode...
> build/common.gypi:1616: ['android_webview_build==1', {
> On 2013/04/12 12:20:33, Torne wrote:
> > I'm not sure you can rely on these being executed in order. What I meant was
> for
> > *all* the sections to test android_webview_build..
> 
> Not being able to depend on the execution order feels rather primitive. It
> worked as expected in this case.

It may well, but I doubt gyp guarantees this, and it's not a method we generally
use in gyp files. The order in *general* is not linear (e.g. things merged in
using conditions overwrite existing values no matter what the relative order of
them in the file) and many parts of a gyp file are unordered dictionaries, so
depending on textual order just seems gross.

So, this LGTM now, but it's worth waiting for approval from yfriedman and piman
to cover all the platforms (I am primarily the webview build guru, not general)
:)

[piman, 2013-04-12 16:21:53]

LGTM from the Chrome OS side. This looks reasonable and shouldn't change the
defaults there. Thanks for the cleanup!
After landing, please keep an eye on the Daisy bots on the Chrome OS builders:
http://build.chromium.org/p/chromium.chromiumos/console and
http://build.chromium.org/p/chromiumos/console

[Yaron, 2013-04-12 16:35:33]

Seems good but probably best to let digit have a once over for chrome android
app

https://codereview.chromium.org/14065005/diff/30001/build/android/envsetup_fu...
File build/android/envsetup_functions.sh (right):

https://codereview.chromium.org/14065005/diff/30001/build/android/envsetup_fu...
build/android/envsetup_functions.sh:109: DEFINES+=" arm_neon_optional=1"  #
Enable dynamic NEON support.
One more to go.. can you push this one into gyp too since you're already
addressing the others?

[Fredrik Öhrn, 2013-04-12 16:57:34]

On 2013/04/12 16:21:53, piman wrote:
> LGTM from the Chrome OS side. This looks reasonable and shouldn't change the
> defaults there. Thanks for the cleanup!

You're welcome, but note that the ebuild configuration your mentioned earlier
will need an update to build anything but the default CPU now.

Doing a bit of on the fly diff writing for:
http://git.chromium.org/gitweb/?p=chromiumos/overlays/chromiumos-overlay.git;...

line 227 should be changed as follows:
- armv7=$([[ ${CHOST} == armv7* ]]; echo10)
+ arm_version=${CHOST:4:1}

Since I'm not working on Chrome OS I'm ill equipped to do this update myself,
I'd very much appreciate if you, davidjames or someone else with the knowledge
could take care of it.

[Fredrik Öhrn, 2013-04-12 17:05:25]

https://codereview.chromium.org/14065005/diff/30001/build/android/envsetup_fu...
File build/android/envsetup_functions.sh (right):

https://codereview.chromium.org/14065005/diff/30001/build/android/envsetup_fu...
build/android/envsetup_functions.sh:109: DEFINES+=" arm_neon_optional=1"  #
Enable dynamic NEON support.
On 2013/04/12 16:35:33, Yaron wrote:
> One more to go.. can you push this one into gyp too since you're already
> addressing the others?

Of course, I'll do it first thing Monday.

[Fredrik Öhrn, 2013-04-15 09:21:48]

On 2013/04/12 17:05:25, Fredrik Öhrn wrote:
> On 2013/04/12 16:35:33, Yaron wrote:
> > One more to go.. can you push this one into gyp too since you're already
> > addressing the others?
> 
> Of course, I'll do it first thing Monday.

Done.

[digit1, 2013-04-15 09:35:41]

lgtm - sorry I'm late :)

[Yaron, 2013-04-16 21:10:32]

lgtm too for the record

[commit-bot: I haz the power, 2013-04-18 10:48:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ohrn@opera.com/14065005/36001

[commit-bot: I haz the power, 2013-04-18 10:48:29]

Presubmit check for 14065005-36001 failed and returned exit status 1.

INFO:root:Found 3 file(s).

INFO:PRESUBMIT:Skipping pylint: no matching changes.
Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/build/android/PRESUBMIT.py
Running
/b/commit-queue/workdir/chromium/build/android/buildbot/tests/bb_run_bot_test.py

** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    skia/skia.gyp

Presubmit checks took 4.4s to calculate.

[Fredrik Öhrn, 2013-04-18 11:02:18]

On 2013/04/18 10:48:29, I haz the power (commit-bot) wrote:
> 
> ** Presubmit ERRORS **
> Missing LGTM from an OWNER for these files:
>     skia/skia.gyp
> 

Stephen, can you please give the final word.

[Stephen White, 2013-04-18 15:49:12]

On 2013/04/18 11:02:18, Fredrik Öhrn wrote:
> On 2013/04/18 10:48:29, I haz the power (commit-bot) wrote:
> > 
> > ** Presubmit ERRORS **
> > Missing LGTM from an OWNER for these files:
> >     skia/skia.gyp
> > 
> 
> Stephen, can you please give the final word.

LGTM

[commit-bot: I haz the power, 2013-04-19 08:25:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ohrn@opera.com/14065005/36001

[commit-bot: I haz the power, 2013-04-19 08:32:43]

Retried try job too often on mac for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number...

[Fredrik Öhrn, 2013-04-19 10:38:08]

Latest patch should fix the build errors from patch 6, the trybots are currently
wonky but it works on my machine.

[Yaron, 2013-04-19 17:43:38]

lgtm

[commit-bot: I haz the power, 2013-04-22 07:08:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ohrn@opera.com/14065005/56002

[commit-bot: I haz the power, 2013-04-22 07:16:15]

Sorry for I got bad news for ya.
Compile failed with a clobber build on mac.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[Fredrik Öhrn, 2013-04-22 09:26:48]

Several places in the gyp files incorrectly assumed an arm build if armv7 was
set, I've added target_arch checks in patch 8.

In addition third_party/ffmpeg/ffmpeg.gyp needs to be fixed, but I can't seem to
figure out how to get git cl upload to understand that I've updated the
third_party/ffmpeg submodule. Can anyone please point me to some documentation
on how to handle this?

[Torne, 2013-04-22 09:35:16]

On 2013/04/22 09:26:48, Fredrik Öhrn wrote:
> Several places in the gyp files incorrectly assumed an arm build if armv7 was
> set, I've added target_arch checks in patch 8.
> 
> In addition third_party/ffmpeg/ffmpeg.gyp needs to be fixed, but I can't seem
to
> figure out how to get git cl upload to understand that I've updated the
> third_party/ffmpeg submodule. Can anyone please point me to some documentation
> on how to handle this?

CLs can only make changes in one repository. To modify ffmpeg you need to make a
branch in that repo, make the changes there, upload it separately, commit it,
and then get DEPS in the chromium repo rolled to pick up the new change. This
means you can't do this atomically across multiple projects.

[Fredrik Öhrn, 2013-04-22 11:24:09]

On 2013/04/22 09:35:16, Torne wrote:
> CLs can only make changes in one repository. To modify ffmpeg you need to make
a
> branch in that repo, make the changes there, upload it separately, commit it,
> and then get DEPS in the chromium repo rolled to pick up the new change. This
> means you can't do this atomically across multiple projects.

Thanks for the info, I've now created a CL for ffmpeg,
https://codereview.chromium.org/14402006/

[commit-bot: I haz the power, 2013-04-25 09:02:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ohrn@opera.com/14065005/70003

[commit-bot: I haz the power, 2013-04-25 09:02:28]

Presubmit check for 14065005-70003 failed and returned exit status 1.

INFO:root:Found 4 file(s).

INFO:PRESUBMIT:Skipping pylint: no matching changes.
Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/build/android/PRESUBMIT.py
Running
/b/commit-queue/workdir/chromium/build/android/buildbot/tests/bb_run_bot_test.py
Running /b/commit-queue/workdir/chromium/third_party/PRESUBMIT.py

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    third_party/libwebp/libwebp.gyp

Presubmit checks took 7.9s to calculate.

[Fredrik Öhrn, 2013-04-25 09:11:57]

On 2013/04/25 09:02:28, I haz the power (commit-bot) wrote:
> 
> Missing LGTM from an OWNER for these files:
>     third_party/libwebp/libwebp.gyp
> 

Darin, can you review this file please.

[darin (slow to review), 2013-04-25 18:36:26]

LGTM for third_party/

[commit-bot: I haz the power, 2013-04-26 08:04:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ohrn@opera.com/14065005/70003

[commit-bot: I haz the power, 2013-04-26 08:04:35]

Failed to apply patch for third_party/libwebp/libwebp.gyp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file third_party/libwebp/libwebp.gyp
  Hunk #1 FAILED at 66.
  Hunk #2 FAILED at 77.
  2 out of 2 hunks FAILED -- saving rejects to file
third_party/libwebp/libwebp.gyp.rej

Patch:       third_party/libwebp/libwebp.gyp
Index: third_party/libwebp/libwebp.gyp
diff --git a/third_party/libwebp/libwebp.gyp b/third_party/libwebp/libwebp.gyp
index
1eb2217aa2e94bb256165cc6bbcc9be83f38512a..38900c0d42f71326fcd6af16a62736c58cf64e2f
100644
--- a/third_party/libwebp/libwebp.gyp
+++ b/third_party/libwebp/libwebp.gyp
@@ -66,7 +66,7 @@
         {
           'target_name': 'libwebp_dsp_neon',
           'conditions': [
-            ['armv7 == 1', {
+            ['target_arch == "arm" and arm_version >= 7', {
               'type': 'static_library',
               'include_dirs': ['.'],
               'sources': [
@@ -77,7 +77,7 @@
               # behavior similar to *.c.neon in an Android.mk
               'cflags!': [ '-mfpu=vfpv3-d16' ],
               'cflags': [ '-mfpu=neon' ],
-            },{  # "armv7 != 1"
+            },{  # "target_arch != "arm" or arm_version < 7"
               'type': 'none',
             }],
             ['order_profiling != 0', {

[commit-bot: I haz the power, 2013-04-26 08:21:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ohrn@opera.com/14065005/78001

[commit-bot: I haz the power, 2013-04-26 12:06:55]

Change committed as 196702