Android bundle #8421

wchen342 · 2021-04-02T13:40:04Z

Resolves brave/brave-browser/issues/15087. Resolves brave/brave-browser/issues/15063.
Needs https://github.com/brave/devops/issues/4815

Submitter Checklist:

I confirm that no security/privacy review is needed, or that I have requested one
There is a ticket for my issue
Used Github auto-closing keywords in the PR description above
Wrote a good PR/commit description
Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
Checked the PR locally: npm run test -- brave_browser_tests, npm run test -- brave_unit_tests, npm run lint, npm run gn_check, npm run tslint
Ran git rebase master (if needed)

Reviewer Checklist:

A security review is not needed, or a link to one is included in the PR description
New files have MPL-2.0 license header
Adequate test coverage exists to prevent regressions
Major classes, functions and non-trivial code blocks are well-commented
Changes in component dependencies are properly reflected in gn
Code follows the style guide
Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

SergeyZhukovsky · 2021-04-05T21:54:01Z

build/android/BUILD.gn

-  outputs = [ "$target_sign_app_path-singed" ]
-  args = [
+  outputs = [ "$target_sign_app_path-signed" ]
+  args = []


we don't need that

SergeyZhukovsky · 2021-04-05T22:04:03Z

build/commands/lib/build.js

@@ -41,7 +41,8 @@ const touchOverriddenFiles = () => {

  const chromiumSrcDir = path.join(config.srcDir, 'brave', 'chromium_src')
  var sourceFiles = util.walkSync(chromiumSrcDir, applyFileFilter)
-  const additionalGen = getAdditionalGenLocation()
+//   const additionalGen = getAdditionalGenLocation()
+  const additionalGen = undefined


why do we need that?

There is a bug when building release+x64, where the output folder will be android_Release in first run but npm will use android_Release_x64 while resuming which makes the build starts over.

SergeyZhukovsky · 2021-04-08T13:19:40Z

android/java/org/chromium/chrome/browser/base/SplitCompatJobIntentService.java

@@ -0,0 +1,72 @@
+/* Copyright (c) 2020 The Brave Authors. All rights reserved.


SergeyZhukovsky · 2021-04-08T13:20:32Z

android/java/org/chromium/chrome/browser/upgrade/BraveUpgradeJobIntentServiceImpl.java

@@ -0,0 +1,70 @@
+/* Copyright (c) 2020 The Brave Authors. All rights reserved.


mkarolin · 2021-04-08T15:05:48Z

patches/chrome-android-BUILD.gn.patch

@@ -8,7 +8,7 @@ index b067a3f7af514bc48632365427ba8ecffa4885df..dadadab8dd95e47bc228053bd1335cbd
   ]
 +  deps += brave_chrome_app_java_resources_deps sources += brave_java_resources resource_overlay = true
 }
-


Please, don't remove the space on "empty lines" in patches. Some Windows versions of git will fail to apply the patch if the space is missing.

mkarolin

patches change LGTM

build/android/BUILD.gn

AlexeyBarabash

@wchen342 awesome work!

The PR looks good in general.

I have a minor notices I left here about the commented out lines.

Also it seems the PR drops out ability to build apk. This may make local development and posting apks to Github a bit harder. Is it possible to have ability to choose what to build, either aab or old akps by command line switch?

samartnik

lgtm

AlexeyBarabash

👍

android/brave_java_sources.gni

bridiver · 2021-04-12T21:33:30Z

build/android/sign_apk.py

-  argument_parser.add_argument('prvt_key_passwd')
-  argument_parser.add_argument('key_name')
-  args = argument_parser.parse_args()
+    argument_parser = argparse.ArgumentParser()


can we not use apkbuilder.py from upstream? Or at least use FinalizeApk from finalize_apk.py? I feel more comfortable with this stuff when we're doing the same thing as upsteam

The files you mentioned are only appicable to apk files. The only place upstream tried to sign a bundle is when creating a wrapper for running bundles, and they only sign the extracted apks in that case not the bundle itself (see //build/config/android/rules.gni#L4915).

ok, they looked very similar, but maybe there were some differences I didn't notice

bridiver · 2021-04-12T21:36:05Z

build/commands/lib/config.js

@@ -86,6 +86,7 @@ const Config = function () {
  this.targetOS = getNPMConfig(['target_os'])
  this.gypTargetArch = 'x64'
  this.targetApkBase ='classic'
+  this.targetOutput = 'apk'


shouldn't the default for release be aab? specifying Release should set everything correctly and the only "overrides" we should have in CI are for things we can't set here like api keys and urls

also we have two defaults here because of https://github.com/brave/brave-core/pull/8421/files#diff-c762921b1126e4475bffab885d122da3939e98234baa81d784b613ee352bc9cdR61 so this one should be removed

ex Config.prototype.isOfficialBuild = function () { return this.buildConfig === 'Release' }

The duplicate is removed.

bridiver · 2021-04-12T21:38:32Z

build/commands/scripts/commands.js

@@ -72,6 +73,7 @@ program
  .option('--target_os <target_os>', 'target OS')
  .option('--target_arch <target_arch>', 'target architecture')
  .option('--target_apk_base <target_apk_base>', 'target Android OS apk (classic, modern, mono)', 'classic')


should this param name change or at least update the description so we know it applies to apk and aab?

The parameter name and descriptions are updated.

bridiver · 2021-04-12T21:41:30Z

build/commands/scripts/commands.js

@@ -58,6 +58,7 @@ program
  .option('--target_os <target_os>', 'target OS')
  .option('--target_arch <target_arch>', 'target architecture')
  .option('--target_apk_base <target_apk_base>', 'target Android OS apk (classic, modern, mono)', 'classic')
+  .option('--target_output <target_output>', 'target Android output format (apk, aab)', 'apk')


target_apk_base and target_output aren't relevant to gn_check. Only target_os and target_arch are relevant here because those affect the output dir

I would probably leave the default out of this one since we want to do it by build config type

I am not sure which one is better, it will probably depend on how CI will be changed. @SergeyZhukovsky did you discuss with Mihai?

I will change the default to aab for now but that means local debugging needs an extra option.

Maybe @bridiver meant default to aab only for Release?

The change on gn_check is reverted.

Maybe @bridiver meant default to aab only for Release?

I checked the js files, the only place isOfficialBuild is checked is when setting channels. I am not sure whether it is a good place to change default though.

mihaiplesa · 2021-04-15T18:27:59Z

build/commands/scripts/commands.js

@@ -134,7 +135,8 @@ program
  .option('--notarize', 'notarize the macOS app with Apple')
  .option('--target_os <target_os>', 'target OS')
  .option('--target_arch <target_arch>', 'target architecture')
-  .option('--target_apk_base <target_apk_base>', 'target Android OS apk (classic, modern, mono)', 'classic')
+  .option('--target_android_base <target_android_base>', 'target Android SDk level for apk or aab (classic, modern, mono)', 'classic')
+  .option('--target_android_output_format <target_android_output_format>', 'target Android output format (apk, aab)', 'aab')


do we want this to be APK by default? for developers at least

@mihaiplesa we do it here e03657f

for create_dist we want to keep aab by default as it's a release distribution

mihaiplesa · 2021-04-15T18:28:11Z

build/commands/scripts/commands.js

@@ -134,7 +135,8 @@ program
  .option('--notarize', 'notarize the macOS app with Apple')
  .option('--target_os <target_os>', 'target OS')
  .option('--target_arch <target_arch>', 'target architecture')
-  .option('--target_apk_base <target_apk_base>', 'target Android OS apk (classic, modern, mono)', 'classic')
+  .option('--target_android_base <target_android_base>', 'target Android SDk level for apk or aab (classic, modern, mono)', 'classic')


On iOS 17 rotating the device with a full screen modal presented (e.g. Playlist, Tab Tray) to landscape then back to portrait does not trigger `traitCollectionDidChange`/`willTransition`/etc calls and so the toolbar remains in the wrong state.

wchen342 added CI/skip-linux CI/skip-macos-x64 Do not run CI builds for macOS x64 CI/skip-ios Do not run CI builds for iOS labels Apr 2, 2021

wchen342 requested a review from fmarier as a code owner April 2, 2021 13:40

github-actions bot assigned wchen342 Apr 2, 2021

wchen342 force-pushed the android-bundle branch from 126bc1d to 59012a6 Compare April 2, 2021 20:53

SergeyZhukovsky reviewed Apr 5, 2021

View reviewed changes

wchen342 force-pushed the android-bundle branch from 59012a6 to 1980318 Compare April 6, 2021 16:05

fmarier removed their request for review April 7, 2021 01:27

wchen342 requested a review from a team as a code owner April 7, 2021 13:39

wchen342 force-pushed the android-bundle branch from 78bf909 to ee10e08 Compare April 7, 2021 21:17

SergeyZhukovsky reviewed Apr 8, 2021

View reviewed changes

mkarolin reviewed Apr 8, 2021

View reviewed changes

mkarolin approved these changes Apr 8, 2021

View reviewed changes

AlexeyBarabash requested review from samartnik and AlexeyBarabash April 12, 2021 13:25

AlexeyBarabash reviewed Apr 12, 2021

View reviewed changes

build/android/BUILD.gn Outdated Show resolved Hide resolved

AlexeyBarabash reviewed Apr 12, 2021

View reviewed changes

build/android/BUILD.gn Outdated Show resolved Hide resolved

AlexeyBarabash reviewed Apr 12, 2021

View reviewed changes

build/android/BUILD.gn Outdated Show resolved Hide resolved

AlexeyBarabash reviewed Apr 12, 2021

View reviewed changes

samartnik approved these changes Apr 12, 2021

View reviewed changes

AlexeyBarabash approved these changes Apr 12, 2021

View reviewed changes

bridiver reviewed Apr 12, 2021

View reviewed changes

android/brave_java_sources.gni Show resolved Hide resolved

bridiver reviewed Apr 12, 2021

View reviewed changes

wchen342 and others added 12 commits April 15, 2021 13:30

Remove Trichrome; clean up

e1a76fd

Revert several modifications

3fad39b

Fix copy file extension

3146dbb

Move BraveUpgradeJobIntentService to base module

358b417

Format fix

1a58a6d

Revert build.js

545e63e

Fix space in patch

99fa970

Add flag for aab

ada17fe

Change flag default

dee7ab5

default to apk except in Release

e03657f

Change command line argument names

1cc3f05

pinned branch

ffafa4b

mihaiplesa force-pushed the android-bundle branch from 03e988f to ffafa4b Compare April 15, 2021 18:31

mihaiplesa added CI/skip-ios Do not run CI builds for iOS CI/skip-linux CI/skip-macos-x64 Do not run CI builds for macOS x64 labels Apr 15, 2021

mihaiplesa reviewed Apr 16, 2021

View reviewed changes

Update Jenkinsfile

1f332ae

mihaiplesa added CI/skip Do not run CI builds (except noplatform) and removed CI/skip-ios Do not run CI builds for iOS CI/skip-linux CI/skip-macos-x64 Do not run CI builds for macOS x64 labels Apr 16, 2021

mihaiplesa approved these changes Apr 16, 2021

View reviewed changes

wchen342 merged commit c7f0b4b into master Apr 16, 2021

wchen342 deleted the android-bundle branch April 16, 2021 17:20

wchen342 added this to the 1.25.x - Nightly milestone Apr 16, 2021

wchen342 mentioned this pull request May 5, 2021

Android Bundle (1.24.x) #8713

Merged

24 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Android bundle #8421

Android bundle #8421

wchen342 commented Apr 2, 2021 •

edited by mihaiplesa

Loading

SergeyZhukovsky Apr 5, 2021

SergeyZhukovsky Apr 5, 2021

wchen342 Apr 6, 2021

SergeyZhukovsky Apr 8, 2021

SergeyZhukovsky Apr 8, 2021

mkarolin Apr 8, 2021

mkarolin left a comment

AlexeyBarabash left a comment

samartnik left a comment

AlexeyBarabash left a comment

bridiver Apr 12, 2021

wchen342 Apr 12, 2021

bridiver Apr 12, 2021

bridiver Apr 12, 2021

bridiver Apr 12, 2021

bridiver Apr 12, 2021

wchen342 Apr 13, 2021

bridiver Apr 12, 2021

wchen342 Apr 13, 2021

bridiver Apr 12, 2021

bridiver Apr 12, 2021

wchen342 Apr 13, 2021

wchen342 Apr 13, 2021

AlexeyBarabash Apr 13, 2021 •

edited by wchen342

Loading

wchen342 Apr 13, 2021

wchen342 Apr 13, 2021

mihaiplesa Apr 15, 2021

SergeyZhukovsky Apr 16, 2021

SergeyZhukovsky Apr 16, 2021

mihaiplesa Apr 15, 2021

		@@ -0,0 +1,72 @@
		/* Copyright (c) 2020 The Brave Authors. All rights reserved.

		@@ -0,0 +1,70 @@
		/* Copyright (c) 2020 The Brave Authors. All rights reserved.

Android bundle #8421

Android bundle #8421

Conversation

wchen342 commented Apr 2, 2021 • edited by mihaiplesa Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkarolin left a comment

Choose a reason for hiding this comment

AlexeyBarabash left a comment

Choose a reason for hiding this comment

samartnik left a comment

Choose a reason for hiding this comment

AlexeyBarabash left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexeyBarabash Apr 13, 2021 • edited by wchen342 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wchen342 commented Apr 2, 2021 •

edited by mihaiplesa

Loading

AlexeyBarabash Apr 13, 2021 •

edited by wchen342

Loading