Name V8 snapshot according to target architecture (32/64 bit).

Name V8 snapshot according to target architecture (32/64 bit).

Renaming webview copy of V8 snapshot to be able to differentiate
between snapshots for 32- and 64 bit architectures.
This is done so that snapshots for different architectures can be
repacked into the same APK.

BUG=455699
Committed: https://crrev.com/0ae506b5a6eae5cf0e440752a4fb99b35e2b0f05
Cr-Commit-Position: refs/heads/master@{#315314}

[gsennton, 2015-02-05 17:39:01]

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

[Torne, 2015-02-06 12:17:23]

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

[Torne, 2015-02-06 12:17:24]

+rmcilroy

Ross, do you think this is too gross as a short term solution? Would it be
better if we used the architecture name instead of just 32/64? (though i'm not
sure if that's already #defined somewhere).

one general comment: you have a bunch of tab indents, we use spaces for
indentation.

https://codereview.chromium.org/903793002/diff/1/android_webview/apk/system_w...
File android_webview/apk/system_webview_apk_common.gypi (right):

https://codereview.chromium.org/903793002/diff/1/android_webview/apk/system_w...
android_webview/apk/system_webview_apk_common.gypi:36:
['v8_use_external_startup_data==1 and (target_arch=="arm" or target_arch=="x86"
or target_arch=="mips")', {
We should probably put this section in a .gypi file since it's repeated three
times.

[rmcilroy, 2015-02-06 15:31:50]

This is a bit gross, but it is at least limited to webview and I can't think of
a better short-term solution.

I agree with Torne's other comments.

[gsennton, 2015-02-06 18:23:10]

Removed tabs and added gypi file to avoid duplication.

[Torne, 2015-02-09 14:37:26]

Latest version LGTM.

[gsennton, 2015-02-09 14:58:14]

The CQ bit was checked by gsennton@chromium.org

[commit-bot: I haz the power, 2015-02-09 14:59:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/903793002/60001

[rmcilroy, 2015-02-09 14:59:59]

Do we need to do this for GN as well?

https://codereview.chromium.org/903793002/diff/60001/android_webview/lib/main...
File android_webview/lib/main/aw_main_delegate.cc (right):

https://codereview.chromium.org/903793002/diff/60001/android_webview/lib/main...
android_webview/lib/main/aw_main_delegate.cc:100: const char kSnapshotFileName[]
= "snapshot_blob_32.bin";
nit - could you add a comment that we should reuse
gin::IsolateHolder::kNativesFileName/kSnapshotFileName once they are fixed to be
arch specific names.

https://codereview.chromium.org/903793002/diff/60001/android_webview/snapshot...
File android_webview/snapshot_copying.gypi (right):

https://codereview.chromium.org/903793002/diff/60001/android_webview/snapshot...
android_webview/snapshot_copying.gypi:6: ['v8_use_external_startup_data==1 and
(target_arch=="arm" or target_arch=="x86" or target_arch=="mips")', {
nit - could you use the arch suffix trick here to reduce repeated conditions -
i.e.:

'conditions': [
  ['(target_arch=="arm" or target_arch=="x86" or target_arch=="mips")', {
    'arch_suffix':'32'
  }],
  ['(target_arch=="arm64" or target_arch=="x86_64" or target_arch=="mips64")', {
    'arch_suffix':'64'
  }],
],
'snapshot_additional_input_paths': [
  '<(asset_location)/natives_blob_<(arch_suffix).bin',
  '<(asset_location)/snapshot_blob_<(arch_suffix).bin',
],
'snapshot_copy_files': [
  '<(PRODUCT_DIR)/natives_blob_<(arch_suffix).bin',
  '<(PRODUCT_DIR)/snapshot_blob_<(arch_suffix).bin',
],

[Torne, 2015-02-09 15:00:52]

The CQ bit was unchecked by torne@chromium.org

[Torne, 2015-02-09 15:08:51]

On 2015/02/09 14:59:59, rmcilroy wrote:
> Do we need to do this for GN as well?

There aren't any GN files for android_webview yet.

[gsennton, 2015-02-09 15:47:06]

New patchsets have been uploaded after l-g-t-m from torne@chromium.org

[gsennton, 2015-02-09 15:47:46]

https://codereview.chromium.org/903793002/diff/60001/android_webview/lib/main...
File android_webview/lib/main/aw_main_delegate.cc (right):

https://codereview.chromium.org/903793002/diff/60001/android_webview/lib/main...
android_webview/lib/main/aw_main_delegate.cc:100: const char kSnapshotFileName[]
= "snapshot_blob_32.bin";
On 2015/02/09 14:59:59, rmcilroy wrote:
> nit - could you add a comment that we should reuse
> gin::IsolateHolder::kNativesFileName/kSnapshotFileName once they are fixed to
be
> arch specific names.

Done.

https://codereview.chromium.org/903793002/diff/60001/android_webview/snapshot...
File android_webview/snapshot_copying.gypi (right):

https://codereview.chromium.org/903793002/diff/60001/android_webview/snapshot...
android_webview/snapshot_copying.gypi:6: ['v8_use_external_startup_data==1 and
(target_arch=="arm" or target_arch=="x86" or target_arch=="mips")', {
On 2015/02/09 14:59:59, rmcilroy wrote:
> nit - could you use the arch suffix trick here to reduce repeated conditions -
> i.e.:
> 
> 'conditions': [
>   ['(target_arch=="arm" or target_arch=="x86" or target_arch=="mips")', {
>     'arch_suffix':'32'
>   }],
>   ['(target_arch=="arm64" or target_arch=="x86_64" or target_arch=="mips64")',
{
>     'arch_suffix':'64'
>   }],
> ],
> 'snapshot_additional_input_paths': [
>   '<(asset_location)/natives_blob_<(arch_suffix).bin',
>   '<(asset_location)/snapshot_blob_<(arch_suffix).bin',
> ],
> 'snapshot_copy_files': [
>   '<(PRODUCT_DIR)/natives_blob_<(arch_suffix).bin',
>   '<(PRODUCT_DIR)/snapshot_blob_<(arch_suffix).bin',
> ],

This doesn't work because of gyp-scoping...

[gsennton, 2015-02-09 15:47:54]

The CQ bit was checked by gsennton@chromium.org

[commit-bot: I haz the power, 2015-02-09 15:49:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/903793002/80001

[rmcilroy, 2015-02-09 15:50:30]

On 2015/02/09 15:47:46, gsennton wrote:
>
https://codereview.chromium.org/903793002/diff/60001/android_webview/lib/main...
> File android_webview/lib/main/aw_main_delegate.cc (right):
> 
>
https://codereview.chromium.org/903793002/diff/60001/android_webview/lib/main...
> android_webview/lib/main/aw_main_delegate.cc:100: const char
kSnapshotFileName[]
> = "snapshot_blob_32.bin";
> On 2015/02/09 14:59:59, rmcilroy wrote:
> > nit - could you add a comment that we should reuse
> > gin::IsolateHolder::kNativesFileName/kSnapshotFileName once they are fixed
to
> be
> > arch specific names.
> 
> Done.
> 
>
https://codereview.chromium.org/903793002/diff/60001/android_webview/snapshot...
> File android_webview/snapshot_copying.gypi (right):
> 
>
https://codereview.chromium.org/903793002/diff/60001/android_webview/snapshot...
> android_webview/snapshot_copying.gypi:6: ['v8_use_external_startup_data==1 and
> (target_arch=="arm" or target_arch=="x86" or target_arch=="mips")', {
> On 2015/02/09 14:59:59, rmcilroy wrote:
> > nit - could you use the arch suffix trick here to reduce repeated conditions
-
> > i.e.:
> > 
> > 'conditions': [
> >   ['(target_arch=="arm" or target_arch=="x86" or target_arch=="mips")', {
> >     'arch_suffix':'32'
> >   }],
> >   ['(target_arch=="arm64" or target_arch=="x86_64" or
target_arch=="mips64")',
> {
> >     'arch_suffix':'64'
> >   }],
> > ],
> > 'snapshot_additional_input_paths': [
> >   '<(asset_location)/natives_blob_<(arch_suffix).bin',
> >   '<(asset_location)/snapshot_blob_<(arch_suffix).bin',
> > ],
> > 'snapshot_copy_files': [
> >   '<(PRODUCT_DIR)/natives_blob_<(arch_suffix).bin',
> >   '<(PRODUCT_DIR)/snapshot_blob_<(arch_suffix).bin',
> > ],
> 
> This doesn't work because of gyp-scoping...

I figured this might be the case, ohh well - hopefully GN will solve all this
:D.  Thanks for checking.

[commit-bot: I haz the power, 2015-02-09 16:11:48]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-02-09 16:12:33]

Patchset 5 (id:??) landed as
https://crrev.com/0ae506b5a6eae5cf0e440752a4fb99b35e2b0f05
Cr-Commit-Position: refs/heads/master@{#315314}

[mef, 2015-02-10 00:54:26]

On 2015/02/09 16:12:33, I haz the power (commit-bot) wrote:
> Patchset 5 (id:??) landed as
> https://crrev.com/0ae506b5a6eae5cf0e440752a4fb99b35e2b0f05
> Cr-Commit-Position: refs/heads/master@{#315314}

It seems that this CL is breaking builds on Cronet x86 and MIPS builders:

http://build.chromium.org/p/chromium.fyi/builders/Android%20Cronet%20x86%20Bu...

with this error on runhooks step:

Updating projects from gyp files...
gyp: Undefined variable arch_suffix in
/b/build/slave/Android_Cronet_x86_Builder/build/src/android_webview/android_webview.gyp
Error: Command /usr/bin/python src/build/gyp_chromium returned non-zero exit
status 1 in /b/build/slave/Android_Cronet_x86_Builder/build
Hook '/usr/bin/python src/build/gyp_chromium' took 13.14 secs

Any ideas?

[Kibeom Kim (inactive), 2015-02-10 01:09:05]

On 2015/02/10 00:54:26, mef wrote:
> On 2015/02/09 16:12:33, I haz the power (commit-bot) wrote:
> > Patchset 5 (id:??) landed as
> > https://crrev.com/0ae506b5a6eae5cf0e440752a4fb99b35e2b0f05
> > Cr-Commit-Position: refs/heads/master@{#315314}
> 
> It seems that this CL is breaking builds on Cronet x86 and MIPS builders:
> 
>
http://build.chromium.org/p/chromium.fyi/builders/Android%20Cronet%20x86%20Bu...
> 
> with this error on runhooks step:
> 
> Updating projects from gyp files...
> gyp: Undefined variable arch_suffix in
>
/b/build/slave/Android_Cronet_x86_Builder/build/src/android_webview/android_webview.gyp
> Error: Command /usr/bin/python src/build/gyp_chromium returned non-zero exit
> status 1 in /b/build/slave/Android_Cronet_x86_Builder/build
> Hook '/usr/bin/python src/build/gyp_chromium' took 13.14 secs
> 
> Any ideas?

Also breaking downstream Android Chrome buildbots.

https://uberchromegw.corp.google.com/i/clank/builders/x86-builder/builds/8985...
https://uberchromegw.corp.google.com/i/clank/builders/mips-builder/builds/551...
https://uberchromegw.corp.google.com/i/clank/builders/x64-builder/builds/4765...

[Kibeom Kim (inactive), 2015-02-10 01:26:48]

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

[Kibeom Kim (inactive), 2015-02-10 01:26:48]

On those bots, target_arch variables are x64, mipsel, and ia32.

[boliu, 2015-02-10 02:17:44]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/868363003/ by boliu@chromium.org.

The reason for reverting is: Some target_arch constants are wrong/missing,
causing build breaks on those architectures..

[boliu, 2015-02-10 02:23:50]

build/common.gypi is a good source of truth for what target_arch could be:

arm
arm64
mipsel
mips64el
x64
ia32

For when this relands :)

[Torne, 2015-02-10 10:07:24]

On 2015/02/10 02:23:50, boliu wrote:
> build/common.gypi is a good source of truth for what target_arch could be:
> 
> arm
> arm64
> mipsel
> mips64el
> x64
> ia32
> 
> For when this relands :)

Whoops, sorry, I told Gustav the wrong architecture names (the ones in this
patch are the names Android uses, which are different to Chrome's, woohoo).