Skip to content

Commit

Permalink
Reland "Reland: Update build rules to support ARCore"
Browse files Browse the repository at this point in the history
This reverts commit 7bd37b6.

Reason for revert: this did not appear to be the culprit.

Original change's description:
> Revert "Reland: Update build rules to support ARCore"
> 
> This reverts commit 14c289b.
> 
> Reason for revert:
> Speculative revert for APK merger failure on official 64 bit builders:
> https://bugs.chromium.org/p/chromium/issues/detail?id=840342
> 
> Original change's description:
> > Reland: Update build rules to support ARCore
> > 
> > In this change, I've moved the code in chrome_public_apk_tmpl from the
> > monochrome to he chrome template and ensured that apk_merger will pass.
> > 
> > > Define enable_arcore and add some build rules to use it.
> > >
> > > Bug: 833511
> > > Change-Id: I716123c2a282d7d123883df81ebbc00a0a883be8
> > > Reviewed-on: https://chromium-review.googlesource.com/1033580
> > > Commit-Queue: Ian Vollick <vollick@chromium.org>
> > > Reviewed-by: John Budorick <jbudorick@chromium.org>
> > > Reviewed-by: Eric Stevenson <estevenson@chromium.org>
> > > Reviewed-by: Yaron Friedman <yfriedman@chromium.org>
> > > Reviewed-by: Michael Thiessen <mthiesse@chromium.org>
> > > Reviewed-by: David Dorwin <ddorwin@chromium.org>
> > > Cr-Commit-Position: refs/heads/master@{#555174}
> > 
> > Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_vr
> > Change-Id: I155d420f3b6606642cdfea43db48840f2c21a0cb
> > Reviewed-on: https://chromium-review.googlesource.com/1040905
> > Reviewed-by: Tao Bai <michaelbai@chromium.org>
> > Reviewed-by: Scott Violet <sky@chromium.org>
> > Reviewed-by: Will Harris <wfh@chromium.org>
> > Reviewed-by: Bill Orr <billorr@chromium.org>
> > Reviewed-by: Eric Stevenson <estevenson@chromium.org>
> > Reviewed-by: Richard Coles <torne@chromium.org>
> > Reviewed-by: Yaron Friedman <yfriedman@chromium.org>
> > Commit-Queue: Ian Vollick <vollick@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#556200}
> 
> TBR=vollick@chromium.org,sky@chromium.org,michaelbai@chromium.org,yfriedman@chromium.org,torne@chromium.org,wfh@chromium.org,estevenson@chromium.org,jbudorick@chromium.org,billorr@chromium.org
> 
> # Not skipping CQ checks because original CL landed > 1 day ago.
> 
> Change-Id: I99a778b62c27506b820d36dfc90aafac3c88f553
> Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_vr
> Reviewed-on: https://chromium-review.googlesource.com/1047945
> Reviewed-by: Ted Choc <tedchoc@chromium.org>
> Commit-Queue: Ted Choc <tedchoc@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#556508}

TBR=vollick@chromium.org,sky@chromium.org,michaelbai@chromium.org,yfriedman@chromium.org,torne@chromium.org,tedchoc@chromium.org,wfh@chromium.org,estevenson@chromium.org,jbudorick@chromium.org,billorr@chromium.org

Change-Id: I8186226aa5be33b11f01fa7b751c41b3f94bb32f
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_vr
Reviewed-on: https://chromium-review.googlesource.com/1049845
Reviewed-by: Ian Vollick <vollick@chromium.org>
Commit-Queue: Ian Vollick <vollick@chromium.org>
Cr-Commit-Position: refs/heads/master@{#556764}
  • Loading branch information
Ian Vollick authored and Commit Bot committed May 8, 2018
1 parent 83eea24 commit cdb8e99
Show file tree
Hide file tree
Showing 7 changed files with 71 additions and 9 deletions.
32 changes: 30 additions & 2 deletions android_webview/tools/apk_merger.py
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,11 @@ def CheckFilesExpected(actual_files, expected_files, component_build):
duplicate_file_set = set(
f for f, n in actual_file_names.iteritems() if n > 1)

# TODO(crbug.com/839191): Remove this once we're plumbing the lib correctly.
missing_file_set = set(
f for f in missing_file_set if not os.path.basename(f) ==
'libarcore_sdk_c_minimal.so')

errors = []
if unexpected_file_set:
errors.append(
Expand Down Expand Up @@ -172,6 +177,16 @@ def MergeApk(args, tmp_apk, tmp_dir_32, tmp_dir_64):
if args.has_unwind_cfi:
expected_files['unwind_cfi_32'] = False

# TODO(crbug.com/839191): we should pass this in via script arguments.
if not args.loadable_module_32:
args.loadable_module_32.append('libarcore_sdk_c_minimal.so')

for f in args.loadable_module_32:
expected_files[f] = not args.uncompress_shared_libraries

for f in args.loadable_module_64:
expected_files[f] = not args.uncompress_shared_libraries

# need to unpack APKs to compare their contents
UnpackApk(args.apk_64bit, tmp_dir_64)
UnpackApk(args.apk_32bit, tmp_dir_32)
Expand All @@ -195,8 +210,15 @@ def MergeApk(args, tmp_apk, tmp_dir_32, tmp_dir_64):
CheckFilesExpected(diff_files, expected_files, args.component_build)

with zipfile.ZipFile(tmp_apk, 'w') as out_zip:
build_utils.MergeZips(out_zip, [args.apk_64bit],
exclude_patterns=['META-INF/*'])
exclude_patterns = ['META-INF/*']

# If there are libraries for which we don't want the 32 bit versions, we
# should remove them here.
if args.loadable_module_32:
exclude_patterns.extend(['*' + f for f in args.loadable_module_32 if
f not in args.loadable_module_64])

build_utils.MergeZips(out_zip, [args.apk_64bit], exclude_patterns)
AddDiffFiles(diff_files, tmp_dir_32, out_zip, expected_files,
args.component_build, args.uncompress_shared_libraries)

Expand Down Expand Up @@ -224,6 +246,12 @@ def main():
parser.add_argument('--ignore-classes-dex', action='store_true')
parser.add_argument('--has-unwind-cfi', action='store_true',
help='Specifies if the 32-bit apk has unwind_cfi file')
parser.add_argument('--loadable_module_32', action='append', default=[],
help='Use for each 32-bit library added via '
'loadable_modules')
parser.add_argument('--loadable_module_64', action='append', default=[],
help='Use for each 64-bit library added via '
'loadable_modules')
args = parser.parse_args()

if (args.zipalign_path is not None and
Expand Down
7 changes: 5 additions & 2 deletions build/android/gyp/assert_static_initializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,11 @@ def main():
parser.add_argument('apk', help='APK file path.')
args = parser.parse_args()

#TODO(crbug.com/838414): add support for files included via loadable_modules.
ignored_libs = ['libarcore_sdk_c_minimal.so']

si_count = resource_sizes.AnalyzeStaticInitializers(
args.apk, args.tool_prefix, False, '.')
args.apk, args.tool_prefix, False, '.', ignored_libs)
if si_count != args.expected_count:
print 'Expected {} static initializers, but found {}.'.format(
args.expected_count, si_count)
Expand All @@ -40,7 +43,7 @@ def main():
print 'Dumping static initializers via dump-static-initializers.py:'
sys.stdout.flush()
resource_sizes.AnalyzeStaticInitializers(
args.apk, args.tool_prefix, True, '.')
args.apk, args.tool_prefix, True, '.', ignored_libs)
print
print 'If the above list is not useful, consider listing them with:'
print ' //tools/binary_size/diagnose_bloat.py'
Expand Down
13 changes: 10 additions & 3 deletions build/android/resource_sizes.py
Original file line number Diff line number Diff line change
Expand Up @@ -562,12 +562,14 @@ def _AnnotatePakResources(out_dir):


# This method also used by //build/android/gyp/assert_static_initializers.py
def AnalyzeStaticInitializers(apk_filename, tool_prefix, dump_sis, out_dir):
def AnalyzeStaticInitializers(apk_filename, tool_prefix, dump_sis, out_dir,
ignored_libs):
# Static initializer counting mostly copies logic in
# infra/scripts/legacy/scripts/slave/chromium/sizes.py.
with zipfile.ZipFile(apk_filename) as z:
so_files = [f for f in z.infolist()
if f.filename.endswith('.so') and f.file_size > 0]
if f.filename.endswith('.so') and f.file_size > 0
and os.path.basename(f.filename) not in ignored_libs]
# Skip checking static initializers for 32 bit .so files when 64 bit .so files
# are present since the 32 bit versions will be checked by bots that only
# build the 32 bit version. This avoids the complexity of finding 32 bit .so
Expand Down Expand Up @@ -703,6 +705,9 @@ def main():
dest='dump_sis',
help='Run dump-static-initializers.py to get the list'
'of static initializers (slow).')
argparser.add_argument('--loadable_module',
action='append',
help='Use for libraries added via loadable_modules')
argparser.add_argument('--estimate-patch-size',
action='store_true',
help='Include patch size estimates. Useful for perf '
Expand Down Expand Up @@ -730,8 +735,10 @@ def main():
PrintApkAnalysis(args.apk, tool_prefix, out_dir, chartjson=chartjson)
_PrintDexAnalysis(args.apk, chartjson=chartjson)

ignored_libs = args.loadable_module if args.loadable_module else []

si_count = AnalyzeStaticInitializers(
args.apk, tool_prefix, args.dump_sis, out_dir)
args.apk, tool_prefix, args.dump_sis, out_dir, ignored_libs)
perf_tests_results_helper.ReportPerfResult(
chartjson, 'StaticInitializersCount', 'count', si_count, 'count')

Expand Down
11 changes: 11 additions & 0 deletions chrome/android/chrome_public_apk_tmpl.gni
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import("//build/config/locales.gni")
import("//build/config/android/extract_unwind_tables.gni")
import("//build/config/compiler/compiler.gni")
import("//chrome/common/features.gni")
import("//device/vr/buildflags/buildflags.gni")
import("//third_party/leakcanary/config.gni")
import("channel.gni")

Expand Down Expand Up @@ -193,5 +194,15 @@ template("monochrome_public_apk_tmpl") {
proguard_configs += [ "//android_webview/apk/java/proguard.flags" ]
png_to_webp = true
}

if (enable_arcore) {
deps += [
"//third_party/arcore-android-sdk:libarcore_library",
"//third_party/arcore-android-sdk:libdynamite_client_java",
]

# We store this as a separate .so in the APK and only load as needed.
loadable_modules = [ "${root_out_dir}/libarcore_sdk_c_minimal.so" ]
}
}
}
8 changes: 8 additions & 0 deletions device/vr/buildflags/buildflags.gni
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import("//build/config/chrome_build.gni")
import("//build/config/chromecast_build.gni")
import("//build/config/gclient_args.gni")
import("//chrome/android/channel.gni")

declare_args() {
# TODO(733935): Enable for other Android architectures too. Currently we only
Expand All @@ -29,4 +30,11 @@ declare_args() {

# Whether to include VR extras like test APKs in non-VR-specific targets
include_vr_data = false

# TODO(crbug.com/837999): We currently only support arm and arm64 and we are
# limiting to canary and dev until binary size issues are resolved.
enable_arcore = is_android && !is_chromecast &&
(current_cpu == "arm" || current_cpu == "arm64") &&
(android_channel == "default" || android_channel == "dev" ||
android_channel == "canary")
}
4 changes: 2 additions & 2 deletions third_party/arcore-android-sdk/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,11 @@ source_set("libarcore_sdk") {
}

copy("libarcore_library") {
if (target_cpu == "arm") {
if (current_cpu == "arm") {
sources = [
"libraries/android_arm/libarcore_sdk_c_minimal.so",
]
} else if (target_cpu == "arm64") {
} else if (current_cpu == "arm64") {
sources = [
"libraries/android_arm64/libarcore_sdk_c_minimal.so",
]
Expand Down
5 changes: 5 additions & 0 deletions third_party/arcore-android-sdk/README.chromium
Original file line number Diff line number Diff line change
Expand Up @@ -16,5 +16,10 @@ To address binary size concerns, we are using a minimal shim produced for
Chromium that is not publicly distributed elsewhere. We have also stripped the
.so's to minimize their size.

Also, added BUILD.gn for compilation in chrome.

The LICENSE file is taken from
* https://github.com/google-ar/arcore-unity-sdk/blob/master/LICENSE

Changes:
2018-05-04 - Updated BUILD.gn to work properly for arm64.

0 comments on commit cdb8e99

Please sign in to comment.