Add incremental_apk_by_default GN arg.

Add incremental_apk_by_default GN arg.

Setting this arg allows _incremental targets and scripts to be used
without explicitly building them. With this arg set to true,
non-incremental targets will build their incremental counterparts if
they exist (i.e. the chrome_public_apk target will actually build
chrome_public_apk_incremental).

This works for .apks, test runner scripts, and install scripts for
instrumentation_test_apks, android_apks, and unittest_apks

BUG=634430
Committed: https://crrev.com/ce844392ab42112ce1fb92240de7ae102ed4c868
Cr-Commit-Position: refs/heads/master@{#438901}

[estevenson, 2016-12-09 22:27:57]

The CQ bit was checked by estevenson@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-09 22:28:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-10 01:58:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-10 01:58:28]

Dry run: This issue passed the CQ dry run.

[estevenson, 2016-12-12 17:48:59]

Description was changed from

==========
Add incremental_apks_by_default gn arg.

Setting this arg will change non-incremental targets to build their
incremental counterparts if they exist.

BUG=634430
==========

to

==========
Add incremental_apk_by_default gn arg.

Setting this arg will change non-incremental targets to build their
incremental counterparts if they exist (i.e. the chrome_public_apk
target will actually build chrome_public_apk_incremental if this arg is
set). This works for instrumentation_test_apks, android_apks, and
unittest_apks.

BUG=634430
==========

[estevenson, 2016-12-12 17:52:57]

estevenson@chromium.org changed reviewers:
+ agrieve@chromium.org

[estevenson, 2016-12-12 17:52:58]

ptal Andrew!

The reason this failed when we tested it previously was because I was running a
test that didn't exist :|. This approach looks better to me than the
never_incremental approach we talked about.

[agrieve, 2016-12-12 18:07:56]

On 2016/12/12 17:52:58, estevenson wrote:
> ptal Andrew!
> 
> The reason this failed when we tested it previously was because I was running
a
> test that didn't exist :|. This approach looks better to me than the
> never_incremental approach we talked about.

Here's the flags we pass for incremental tests in the wrapper script:
https://cs.chromium.org/chromium/src/build/android/gyp/create_test_runner_scr...

I think without never_incremental, we'd need to add a
--additional-apk-incremental-install-scripts, and update the test runner to use
them.

[estevenson, 2016-12-12 22:07:25]

Description was changed from

==========
Add incremental_apk_by_default gn arg.

Setting this arg will change non-incremental targets to build their
incremental counterparts if they exist (i.e. the chrome_public_apk
target will actually build chrome_public_apk_incremental if this arg is
set). This works for instrumentation_test_apks, android_apks, and
unittest_apks.

BUG=634430
==========

to

==========
Add incremental_apk_by_default GN arg.

Setting this arg allows _incremental targets and scripts to be used
without explicitly building them. With this arg set to true,
non-incremental targets will build their incremental counterparts if
they exist (i.e. the chrome_public_apk target will actually build
chrome_public_apk_incremental). 

This works for .apks, test runner scripts, and install scripts for
instrumentation_test_apks, android_apks, and unittest_apks

BUG=634430
==========

[estevenson, 2016-12-12 22:08:09]

ptal Andrew!

[agrieve, 2016-12-13 15:15:52]

lgtm

https://codereview.chromium.org/2562063003/diff/80001/chrome/android/webapk/l...
File
chrome/android/webapk/libs/runtime_library/javatests/apk_with_webapk_service/BUILD.gn
(right):

https://codereview.chromium.org/2562063003/diff/80001/chrome/android/webapk/l...
chrome/android/webapk/libs/runtime_library/javatests/apk_with_webapk_service/BUILD.gn:9:
never_incremental = true
nit: Can you add a comment to all of these:  # Used as an additional_apk in test
scripts

[estevenson, 2016-12-13 15:57:46]

estevenson@chromium.org changed reviewers:
+ phajdan.jr@chromium.org, xunjieli@chromium.org

[estevenson, 2016-12-13 15:57:47]

xunjieli@chromium.org: Please review changes in //net

phajdan.jr@chromium.org: Please review changes in //chrome/test/ and //testing/

https://codereview.chromium.org/2562063003/diff/80001/chrome/android/webapk/l...
File
chrome/android/webapk/libs/runtime_library/javatests/apk_with_webapk_service/BUILD.gn
(right):

https://codereview.chromium.org/2562063003/diff/80001/chrome/android/webapk/l...
chrome/android/webapk/libs/runtime_library/javatests/apk_with_webapk_service/BUILD.gn:9:
never_incremental = true
On 2016/12/13 15:15:52, agrieve wrote:
> nit: Can you add a comment to all of these:  # Used as an additional_apk in
test
> scripts

Done.

[xunjieli, 2016-12-14 15:44:21]

On 2016/12/13 15:57:47, estevenson wrote:
> mailto:xunjieli@chromium.org: Please review changes in //net


What happens if "net_test_support_apk" does not set "never_incremental" to true?
Will anything break?

[agrieve, 2016-12-14 15:53:03]

For a clean build, I think the test would fail to install the apk, since it
would be looking for the non-incremental one, which is not built.

For a build which already had the apk lying around from a previous gn arg build,
then I think the error would be glossed over (although these apks don't change
very often).

I can't think of a way in GN to error out early if an apk listed as an
additional_apk is not marked as never_incremental. Perhaps it could be added to
the write-test-wrapper python script though? (e.g. ensure that no
"incremental_apk_path" key exists in the .build_config)

https://codereview.chromium.org/2562063003/diff/100001/build/config/android/r...
File build/config/android/rules.gni (right):

https://codereview.chromium.org/2562063003/diff/100001/build/config/android/r...
build/config/android/rules.gni:1402: #   never_incremental: If true,
|incremental_apk_by_default| will be ignored.
Just realized - we should set this for monochrome_apk as well (with a comment
pointing to http://crbug.com/663492)

https://codereview.chromium.org/2562063003/diff/100001/build/config/android/r...
build/config/android/rules.gni:2227: group("${target_name}_incremental") {
Realized too - we should not define this target when never_incremental is set.

[estevenson, 2016-12-14 15:58:04]

On 2016/12/14 15:44:21, xunjieli wrote:
> On 2016/12/13 15:57:47, estevenson wrote:
> > mailto:xunjieli@chromium.org: Please review changes in //net
> 
> 
> What happens if "net_test_support_apk" does not set "never_incremental" to
true?
> Will anything break?

Yes, if apks that are listed as additional_apks for any targets don't set
never_incremental, tests won't run with the incremental_apk_by_default arg set.
The test_runner_script template in internal_rules.gni uses the buildconfig to
get the additional apk path which is incorrect when incremental_apk_by_default
is set (since we don't build the regular apk in this case). Patch set 2
(https://codereview.chromium.org/2562063003/diff/20001/build/config/android/in...)
uses this approach, but it would also require changes to other test runner
scripts and we decided that the never_incremental approach is better since there
aren't that many test support apks and we gain very little with them using
incremental installs.

[xunjieli, 2016-12-14 16:11:10]

On 2016/12/14 15:53:03, agrieve wrote:
> For a clean build, I think the test would fail to install the apk, since it
> would be looking for the non-incremental one, which is not built.
> 
> For a build which already had the apk lying around from a previous gn arg
build,
> then I think the error would be glossed over (although these apks don't change
> very often).
> 
> I can't think of a way in GN to error out early if an apk listed as an
> additional_apk is not marked as never_incremental. Perhaps it could be added
to
> the write-test-wrapper python script though? (e.g. ensure that no
> "incremental_apk_path" key exists in the .build_config)


If the problem is in the install script, wouldn't it be better for the install
script be updated? 
Presumably the install script knows the build_config, it should be able to check
whether |incremental_apk_by_default| is set right?

[xunjieli, 2016-12-14 16:16:31]

On 2016/12/14 15:58:04, estevenson wrote:
> On 2016/12/14 15:44:21, xunjieli wrote:
> > On 2016/12/13 15:57:47, estevenson wrote:
> > > mailto:xunjieli@chromium.org: Please review changes in //net
> > 
> > 
> > What happens if "net_test_support_apk" does not set "never_incremental" to
> true?
> > Will anything break?
> 
> Yes, if apks that are listed as additional_apks for any targets don't set
> never_incremental, tests won't run with the incremental_apk_by_default arg
set.
> The test_runner_script template in internal_rules.gni uses the buildconfig to
> get the additional apk path which is incorrect when incremental_apk_by_default
> is set (since we don't build the regular apk in this case). Patch set 2
>
(https://codereview.chromium.org/2562063003/diff/20001/build/config/android/in...)
> uses this approach, but it would also require changes to other test runner
> scripts and we decided that the never_incremental approach is better since
there
> aren't that many test support apks and we gain very little with them using
> incremental installs.

Just saw this. I agree that incremental build for test support apks don't bring
much benefits since those apks are small. My concern is on the maintenance cost.
If someone want to write a support apk, they need to know about
"never_incremental." It will be good if we can add an check somewhere to enforce
this.

[estevenson, 2016-12-14 20:14:32]

On 2016/12/14 16:16:31, xunjieli wrote:
> On 2016/12/14 15:58:04, estevenson wrote:
> > On 2016/12/14 15:44:21, xunjieli wrote:
> > > On 2016/12/13 15:57:47, estevenson wrote:
> > > > mailto:xunjieli@chromium.org: Please review changes in //net
> > > 
> > > 
> > > What happens if "net_test_support_apk" does not set "never_incremental" to
> > true?
> > > Will anything break?
> > 
> > Yes, if apks that are listed as additional_apks for any targets don't set
> > never_incremental, tests won't run with the incremental_apk_by_default arg
> set.
> > The test_runner_script template in internal_rules.gni uses the buildconfig
to
> > get the additional apk path which is incorrect when
incremental_apk_by_default
> > is set (since we don't build the regular apk in this case). Patch set 2
> >
>
(https://codereview.chromium.org/2562063003/diff/20001/build/config/android/in...)
> > uses this approach, but it would also require changes to other test runner
> > scripts and we decided that the never_incremental approach is better since
> there
> > aren't that many test support apks and we gain very little with them using
> > incremental installs.
> 
> Just saw this. I agree that incremental build for test support apks don't
bring
> much benefits since those apks are small. My concern is on the maintenance
cost.
> If someone want to write a support apk, they need to know about
> "never_incremental." It will be good if we can add an check somewhere to
enforce
> this.

I'll add support for additional_apks to be incremental as well so that we don't
need to worry about adding never_incremental to new apks in the future. We'll
still need the never_incremental flag for monochrome as agrieve@ mentioned
above.

[estevenson, 2016-12-14 20:25:31]

On 2016/12/14 20:14:32, estevenson wrote:
> On 2016/12/14 16:16:31, xunjieli wrote:
> > On 2016/12/14 15:58:04, estevenson wrote:
> > > On 2016/12/14 15:44:21, xunjieli wrote:
> > > > On 2016/12/13 15:57:47, estevenson wrote:
> > > > > mailto:xunjieli@chromium.org: Please review changes in //net
> > > > 
> > > > 
> > > > What happens if "net_test_support_apk" does not set "never_incremental"
to
> > > true?
> > > > Will anything break?
> > > 
> > > Yes, if apks that are listed as additional_apks for any targets don't set
> > > never_incremental, tests won't run with the incremental_apk_by_default arg
> > set.
> > > The test_runner_script template in internal_rules.gni uses the buildconfig
> to
> > > get the additional apk path which is incorrect when
> incremental_apk_by_default
> > > is set (since we don't build the regular apk in this case). Patch set 2
> > >
> >
>
(https://codereview.chromium.org/2562063003/diff/20001/build/config/android/in...)
> > > uses this approach, but it would also require changes to other test runner
> > > scripts and we decided that the never_incremental approach is better since
> > there
> > > aren't that many test support apks and we gain very little with them using
> > > incremental installs.
> > 
> > Just saw this. I agree that incremental build for test support apks don't
> bring
> > much benefits since those apks are small. My concern is on the maintenance
> cost.
> > If someone want to write a support apk, they need to know about
> > "never_incremental." It will be good if we can add an check somewhere to
> enforce
> > this.
> 
> I'll add support for additional_apks to be incremental as well so that we
don't
> need to worry about adding never_incremental to new apks in the future. We'll
> still need the never_incremental flag for monochrome as agrieve@ mentioned
> above.

Discussed with agrieve@ offline: Probably still makes more sense to just add a
check to fail builds when additional_apks are built incrementally.

[xunjieli, 2016-12-14 21:37:44]

On 2016/12/14 20:25:31, estevenson wrote:
> On 2016/12/14 20:14:32, estevenson wrote:
> > On 2016/12/14 16:16:31, xunjieli wrote:
> > > On 2016/12/14 15:58:04, estevenson wrote:
> > > > On 2016/12/14 15:44:21, xunjieli wrote:
> > > > > On 2016/12/13 15:57:47, estevenson wrote:
> > > > > > mailto:xunjieli@chromium.org: Please review changes in //net
> > > > > 
> > > > > 
> > > > > What happens if "net_test_support_apk" does not set
"never_incremental"
> to
> > > > true?
> > > > > Will anything break?
> > > > 
> > > > Yes, if apks that are listed as additional_apks for any targets don't
set
> > > > never_incremental, tests won't run with the incremental_apk_by_default
arg
> > > set.
> > > > The test_runner_script template in internal_rules.gni uses the
buildconfig
> > to
> > > > get the additional apk path which is incorrect when
> > incremental_apk_by_default
> > > > is set (since we don't build the regular apk in this case). Patch set 2
> > > >
> > >
> >
>
(https://codereview.chromium.org/2562063003/diff/20001/build/config/android/in...)
> > > > uses this approach, but it would also require changes to other test
runner
> > > > scripts and we decided that the never_incremental approach is better
since
> > > there
> > > > aren't that many test support apks and we gain very little with them
using
> > > > incremental installs.
> > > 
> > > Just saw this. I agree that incremental build for test support apks don't
> > bring
> > > much benefits since those apks are small. My concern is on the maintenance
> > cost.
> > > If someone want to write a support apk, they need to know about
> > > "never_incremental." It will be good if we can add an check somewhere to
> > enforce
> > > this.
> > 
> > I'll add support for additional_apks to be incremental as well so that we
> don't
> > need to worry about adding never_incremental to new apks in the future.
We'll
> > still need the never_incremental flag for monochrome as agrieve@ mentioned
> > above.
> 
> Discussed with agrieve@ offline: Probably still makes more sense to just add a
> check to fail builds when additional_apks are built incrementally.

SGTM! Once that is in, I will rubber stamp this. Thanks.

[estevenson, 2016-12-14 22:26:03]

ptal Andrew! The added error message formatting logic might be overkill and
shouldn't even really be in that file but I wasn't sure where else to add it..

https://codereview.chromium.org/2562063003/diff/100001/build/config/android/r...
File build/config/android/rules.gni (right):

https://codereview.chromium.org/2562063003/diff/100001/build/config/android/r...
build/config/android/rules.gni:1402: #   never_incremental: If true,
|incremental_apk_by_default| will be ignored.
On 2016/12/14 15:53:03, agrieve wrote:
> Just realized - we should set this for monochrome_apk as well (with a comment
> pointing to http://crbug.com/663492)

Done.

https://codereview.chromium.org/2562063003/diff/100001/build/config/android/r...
build/config/android/rules.gni:2227: group("${target_name}_incremental") {
On 2016/12/14 15:53:03, agrieve wrote:
> Realized too - we should not define this target when never_incremental is set.

Done.

[agrieve, 2016-12-15 16:22:30]

lgtm

https://codereview.chromium.org/2562063003/diff/120001/build/android/gyp/crea...
File build/android/gyp/create_test_runner_script.py (right):

https://codereview.chromium.org/2562063003/diff/120001/build/android/gyp/crea...
build/android/gyp/create_test_runner_script.py:47: def
_GetTargetFromIncrementalApk(incremental_apk):
We have a convention that makes the apk_name reflect the target_name, but I
don't think GN actually enforces this.

Even having a message like this is fairly generous, so I think it'd be fine to
just have the message complain about the file names rather than target_names

https://codereview.chromium.org/2562063003/diff/120001/build/android/gyp/crea...
build/android/gyp/create_test_runner_script.py:55: 'another target cannot be
incremental. Please add never_incremental to the '
nit: indent 4 spaces

[xunjieli, 2016-12-15 16:23:58]

net/android/BUILD.gn lgtm!

[Paweł Hajdan Jr., 2016-12-15 17:11:43]

LGTM

[estevenson, 2016-12-15 18:19:07]

https://codereview.chromium.org/2562063003/diff/120001/build/android/gyp/crea...
File build/android/gyp/create_test_runner_script.py (right):

https://codereview.chromium.org/2562063003/diff/120001/build/android/gyp/crea...
build/android/gyp/create_test_runner_script.py:47: def
_GetTargetFromIncrementalApk(incremental_apk):
On 2016/12/15 16:22:30, agrieve wrote:
> We have a convention that makes the apk_name reflect the target_name, but I
> don't think GN actually enforces this.
> 
> Even having a message like this is fairly generous, so I think it'd be fine to
> just have the message complain about the file names rather than target_names

Done.

https://codereview.chromium.org/2562063003/diff/120001/build/android/gyp/crea...
build/android/gyp/create_test_runner_script.py:55: 'another target cannot be
incremental. Please add never_incremental to the '
On 2016/12/15 16:22:30, agrieve wrote:
> nit: indent 4 spaces

Done.

[estevenson, 2016-12-15 18:21:55]

The CQ bit was checked by estevenson@chromium.org

[estevenson, 2016-12-15 18:21:55]

The patchset sent to the CQ was uploaded after l-g-t-m from
xunjieli@chromium.org, agrieve@chromium.org, phajdan.jr@chromium.org
Link to the patchset: https://codereview.chromium.org/2562063003/#ps140001
(title: "Remove error formatting")

[commit-bot: I haz the power, 2016-12-15 18:22:25]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-15 19:58:02]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1481826115099320,
"parent_rev": "483f4a8668fa6500a53368f03d8db431ee44b748", "commit_rev":
"9f5772d39b002be5996c5e1b107141424717d01b"}

[commit-bot: I haz the power, 2016-12-15 19:58:29]

Description was changed from

==========
Add incremental_apk_by_default GN arg.

Setting this arg allows _incremental targets and scripts to be used
without explicitly building them. With this arg set to true,
non-incremental targets will build their incremental counterparts if
they exist (i.e. the chrome_public_apk target will actually build
chrome_public_apk_incremental). 

This works for .apks, test runner scripts, and install scripts for
instrumentation_test_apks, android_apks, and unittest_apks

BUG=634430
==========

to

==========
Add incremental_apk_by_default GN arg.

Setting this arg allows _incremental targets and scripts to be used
without explicitly building them. With this arg set to true,
non-incremental targets will build their incremental counterparts if
they exist (i.e. the chrome_public_apk target will actually build
chrome_public_apk_incremental). 

This works for .apks, test runner scripts, and install scripts for
instrumentation_test_apks, android_apks, and unittest_apks

BUG=634430

Review-Url: https://codereview.chromium.org/2562063003
==========

[commit-bot: I haz the power, 2016-12-15 19:58:31]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-12-15 20:00:44]

Description was changed from

==========
Add incremental_apk_by_default GN arg.

Setting this arg allows _incremental targets and scripts to be used
without explicitly building them. With this arg set to true,
non-incremental targets will build their incremental counterparts if
they exist (i.e. the chrome_public_apk target will actually build
chrome_public_apk_incremental). 

This works for .apks, test runner scripts, and install scripts for
instrumentation_test_apks, android_apks, and unittest_apks

BUG=634430

Review-Url: https://codereview.chromium.org/2562063003
==========

to

==========
Add incremental_apk_by_default GN arg.

Setting this arg allows _incremental targets and scripts to be used
without explicitly building them. With this arg set to true,
non-incremental targets will build their incremental counterparts if
they exist (i.e. the chrome_public_apk target will actually build
chrome_public_apk_incremental).

This works for .apks, test runner scripts, and install scripts for
instrumentation_test_apks, android_apks, and unittest_apks

BUG=634430

Committed: https://crrev.com/ce844392ab42112ce1fb92240de7ae102ed4c868
Cr-Commit-Position: refs/heads/master@{#438901}
==========

[commit-bot: I haz the power, 2016-12-15 20:00:45]

Patchset 8 (id:??) landed as
https://crrev.com/ce844392ab42112ce1fb92240de7ae102ed4c868
Cr-Commit-Position: refs/heads/master@{#438901}