New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 605315 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Jul 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 3
Type: Feature

Blocked on:
issue 616819

Blocking:
issue v8:4101
issue 359429
issue 614381



Sign in to add a comment

gn: support build shared_library and native binary with a set of target_cpu

Project Member Reported by michaelbai@chromium.org, Apr 20 2016

Issue description

64-bit Monochrome and WebView needs 32-bit shared library to support 32-bit WebView apps running in 64-bit Android platform.

e.g. the 64-bit Monochrome.apk has two set of share libraries, v8 snapshot files

lib/armeabi-v7a/libmonochrome.so
lib/arm64-v8a/libmonochrome.so
assets/snapshot_blob_32.bin
assets/snapshot_blob_64.bin
assets/natives_blob_32.bin
assets/natives_blob_64.bin

currently we uses script (src/android_webview/tools/apk_merger.py) to generate this APK by combining the APKs from 32 and 64 release bots. it is not convenience for day to day work, might also difficult for builder to implement. We want to support below use cases,

a. Targets to build both 32 and 64 bit libraries.
# This target generates 2 libraries $output_dir/lib32/libfoo.so and $output_dir/lib64/libfoo.so
shared_library("foo_both_32_and_64") {
    target_abi = ["32", "64"]
}
We also need this for v8 snapshot binaries.


In Monochrome, libmonochrome.so has different dependencies for 32-bit and 64-bit libraries, GN also need to support use case like below

b. Targets to build both 32 and 64 bit libraries, but with different deps.
#  This target generates 2 libraries with different deps, but same library name.
shared_library("foo_with_different_deps") {
    target_abi = ["32", "64"]
     deps_32 = [...]
     deps_64= [...]
}


 
Blocking: 359429
Cc: torne@chromium.org kerz@chromium.org
Labels: Proj-GN-Migration
Status: Available (was: Untriaged)
+kerz, torne ...

This should be straightforward to implement in GN (though we wouldn't use the syntax you describe, I don't think, we'd use something more explicit with toolchains).

However, when I've talked about this requirement w/ kerz@ before, he's indicated that at least for official builds he was more inclined to keep things the way they are now and do builds in parallel on two machines and merge them afterwards, so that the build times would be faster.

So, I'm not sure what we really want to do here just yet. I have no particular problem with adding this as an option for devs even if we don't use it on the official builds, since trying to remember to build both by hand and merge them seems annoying.
Cc: jbudorick@chromium.org
+ jbudorick@, who gonna to replicate official release bot in builder
Labels: -Proj-GN-Migration
Dropping label:GN-Migration since this doesn't block us moving to GN.
@agrieve - do we already have something handling the fat-binary packaging, or do we need to track that somewhere? (I'm guessing we must, since we've shipped something already?)
ping, As the people is starting to work on Monochrome, using script to build monochrome seems weird and need a lot of explanation...

dpranke@ It seemed the implementation is easy, could you provide some instructions, so I can work on it.
Sorry - was having bug emails go to spam for a few weeks it seems :(.

I think to build both libs it'd be something like:

if (current_toolchain == "//build/toolchain/android:arm") {
  group("build_all_libs") {
    public_deps = [
      ":libmonochrome(//build/toolchain/android:arm)
      ":libmonochrome(//build/toolchain/android:arm64)
    ]
  }
} else if (current_toolchain == "//build/toolchain/android:clang_arm") {
  group("build_all_libs") {
    public_deps = [
      ":libmonochrome(//build/toolchain/android:clang_arm)
      ":libmonochrome(//build/toolchain/android:clang_arm64)
    ]
  }
}



We'd then need to update our packaging scripts to look for the libs in:
out/Default/libmonochrome.so
out/Default/$TOOLCHAIN_OUT/libmonochrome.so

Could perhaps make this easier with:
group("build_all_libs") {
  public_deps = [
    ":libmonochrome(//build/toolchain/android:clang_arm)
    ":libmonochrome(//build/toolchain/android:clang_arm64)
  ]
   write_runtime_deps = "$root_build_dir/monochrome_libs.runtime_deps"
}

monochrome_libs.runtime_deps will probably include other files we don't want, but will also include the path to the built libmonochrome.so files.

The below is new to me, what does it do?

":libmonochrome(//build/toolchain/android:arm)


"gn help toolchain" gives an overview. 

Basically, every compiler+arch combination has a toolchain that can build for it, and you can depend on targets in any toolchain. So what this does is say "build the library for the arm toolchain, then build it for arm64".

There are some helper globals: current_toolchain, default_toolchain, host_toolchain.

The other tricky bit is that the default toolchain outputs to //out/Default, but all non-default toolchains output to subdirectories within there. So, if the caller has target_cpu="arm", then arm32 will be in the root out dir, but if they have "target_cpu="arm64", then arm64 will be in the root instead.
Owner: michaelbai@chromium.org
Status: Started (was: Available)
sorry for the delay in responding; I also apparently had bug updates going to my spam folder :(.

agrieve@'s comments are on track. Let me know if you have other questions.
Blocking: 614381
Blockedon: 616819
Cc: sdefresne@chromium.org
Okay, I think what I wrote in comment #1 -- "This should be straightforward to implement" -- is not correct :).

While the concept of "target_cpu" and "target_os" is not baked into GN, it is kinda baked into the chromium and v8 build files. And, what I mean by that is that I suspect a fair number of places have written code assuming that target_cpu and target_os are constant (one single readonly value across the whole build).

agrieve@ fixed a great many of the target_cpu uses in  bug 616819 , but there are a few places left where I argued that using target_cpu is "right". However, it's now not clear to me that *any* use of target_cpu or target_os is right in a monochrome-like world where we actually want to build code for multiple CPU types in a single build invocation.

So, while I think we can make this work, my concern is that we need to be careful about it (hence the "not straightforward") part. In particular, it's clear that life is substantially complicated by the "v8_target_cpu" and "v8_snapshot" logic which already triggers sub-toolchains dependendent on the target_cpu. If NaCl was enabled, I think that would also complicate things.

It's possible that we can make this work for the very narrowly-scoped problem that monochrome needs, and hence it's possible that michaelbai@'s CL in https://codereview.chromium.org/2075203002/ might work (in particular, I need to stare at it some more and figure out if it'll still work once I fix  issue #3  of  bug 621581 ).

**However** trying to worry about this at the same time I'm trying to get GN builds out the door everywhere else in M-53 is making my life harder than I'd really like for it to be.

So, questions:

1) Do we really need this to work, vs. just compiling the DLLs twice in two different build directories and merging them together down the road (since, per comment #1 my understand is that that's what we want to do in official builds regardless, and that's what we're going to do on iOS, where we have a similar requirement), and

2) If we do need this to work, do we need this soon? Or can we wait until after some of the dust settles from getting M-53 out the door?
Cc: jochen@chromium.org machenb...@chromium.org thakis@chromium.org
+jochen, machenbach, since this affects v8 more than most things. Also +thakis who often cares about things like this.

See also some of the related 'v8_target_cpu' discussions in  bug 542853  and some of the hoop-jumping CL attempts in

https://codereview.chromium.org/2075203002/diff/20001/build/config/v8_target_cpu.gni
https://codereview.chromium.org/2073883002/

I think at the very least that if we want to support building multiple architectures in a single ninja invocation we should probably support not that *while simultaneously* also support setting v8_target_cpu to be different from target_cpu (i.e., we should have an assert that we're not trying to do a monochrome build w/ that arg set). Fortunately, I suspect we don't really need that functionality.

If we did want to support multiple v8_hybrids, we'd probably have to rework multiple things, e.g., make v8_target_cpu a value tied to the toolchain and support some way of naming toolchains that specified both the target_cpu and the v8_target_cpu, canadian-cross-style (https://en.wikipedia.org/wiki/Cross_compiler#Canadian_Cross).
Whoops, that should have been  bug 621581  in comment #15, not  bug 542853 .

Comment 17 by torne@chromium.org, Jun 23 2016

I think we should do this even if the official builders aren't going to, because it'll be extremely useful for developers (configuring things correctly to build both versions and merge them is a huge pain), and potentially relevant for non-official bots (trybots?) unless those are all going to be configured to have one bot depend on another in the same way as official? Right now we don't have many bots that use this configuration, but that's likely to change in future as we do more webview testing on bots and 64-bit devices become dominant (they are already a significant minority).

However, we can definitely wait until M53 is out of the door. We'd rather not put this off indefinitely, but it's not critical that it happen in the next month or two.
Re 15: See also v8 downstream http://crbug.com/v8/4101

Our developers used to use a single invocation of our toplevel Makefile that simultaneously builds combinations like:

target=x86, v8_target=x86
target=x86, v8_target=arm
target=x64, v8_target=x64
target=x64, v8_target=arm64

X release/debug

I'd be a great convenience if that continued working with ninja, but my hopes are small. See the file:
https://chromium.googlesource.com/v8/v8/+/master/Makefile
Blocking: v8:4101
There's no intrinsic reason we can't build all of that from a single invocation, but we'd need to make v8_target_arch a toolchain_arg, which is probably the right thing to do anyway.
Project Member

Comment 21 by bugdroid1@chromium.org, Jul 21 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e56fa3c0528dc62a3d423ed2d0b83b5e039cb602

commit e56fa3c0528dc62a3d423ed2d0b83b5e039cb602
Author: michaelbai <michaelbai@chromium.org>
Date: Thu Jul 21 22:17:22 2016

Build libmonochrome.so with secondary toolchain

This patch
- added Android secondary abi toolchain.
- built libmonochrome with secondary abi toolchain, the 64-bit
  build starts to building 64-bit webview only libmonochrome.so
  and 32-bit full libmonochrome.so.

BUG= 605315 

Review-Url: https://codereview.chromium.org/2161183003
Cr-Commit-Position: refs/heads/master@{#406965}

[modify] https://crrev.com/e56fa3c0528dc62a3d423ed2d0b83b5e039cb602/android_webview/BUILD.gn
[modify] https://crrev.com/e56fa3c0528dc62a3d423ed2d0b83b5e039cb602/build/config/android/config.gni
[modify] https://crrev.com/e56fa3c0528dc62a3d423ed2d0b83b5e039cb602/chrome/android/BUILD.gn

Project Member

Comment 22 by bugdroid1@chromium.org, Jul 22 2016

The following revision refers to this bug:
  https://chrome-internal.googlesource.com/clank/internal/apps/+/e012fa7c3044edd9bfc1f26e28b0d2359656af96

commit e012fa7c3044edd9bfc1f26e28b0d2359656af96
Author: Tao Bai <michaelbai@google.com>
Date: Fri Jul 22 01:36:15 2016

Status: Fixed (was: Started)
Project Member

Comment 24 by bugdroid1@chromium.org, Oct 31 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/042f129b96ea6e989df1efe0e4a30dcac09f05a7

commit 042f129b96ea6e989df1efe0e4a30dcac09f05a7
Author: thakis <thakis@chromium.org>
Date: Mon Oct 31 20:22:40 2016

Android: Prefix target toolchain names with "android_".

In x64 builds with clang, we also build a 32-bit binary to ship webview
in both 32-bit and 64-bit.  The 32-bit part is built twice, once for
the linux host to be able to run v8's mksnapshot, and once for the
android target for the actual binary.

Before this change, both the host toolchain and the target toolchain
were called "clang_x86", and they clobbered each other.  (In gcc builds,
the target toolchain was called just "x86" while the host still used
clang, so it happened to work there, mostly by accident.)

BUG= 660857 , 605315 

Review-Url: https://codereview.chromium.org/2463143002
Cr-Commit-Position: refs/heads/master@{#428783}

[modify] https://crrev.com/042f129b96ea6e989df1efe0e4a30dcac09f05a7/build/config/BUILDCONFIG.gn
[modify] https://crrev.com/042f129b96ea6e989df1efe0e4a30dcac09f05a7/build/config/android/config.gni
[modify] https://crrev.com/042f129b96ea6e989df1efe0e4a30dcac09f05a7/build/toolchain/android/BUILD.gn

Sign in to add a comment