GN(android): Use list of libraries rather than native_lib_dir in all places

GN(android): Use list of libraries rather than native_lib_dir in all places

Addresses a TODO and makes rules more well suited for adding in
loadable_modules

BUG=559289
Committed: https://crrev.com/7867a73b9d808755d40b8278b312122269720462
Cr-Commit-Position: refs/heads/master@{#362463}

[agrieve, 2015-11-27 21:26:32]

Description was changed from

==========
GN(android): Use list of libraries rather than native_lib_dir in all places

Addresses a TODO and makes rules more well suited for adding in
loadable_modules

BUG=559289
==========

to

==========
GN(android): Use list of libraries rather than native_lib_dir in all places

Addresses a TODO and makes rules more well suited for adding in
loadable_modules

BUG=559289
==========

[agrieve, 2015-11-27 21:26:32]

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

[agrieve, 2015-11-27 21:27:32]

On 2015/11/27 21:26:32, agrieve wrote:
> mailto:agrieve@chromium.org changed reviewers:
> + mailto:pkotwicz@chromium.org

Sadly, not too nice thanks to the need for FileArg(), still better than using a
directory though.

[pkotwicz, 2015-11-30 20:28:41]

LGTM with nits

https://codereview.chromium.org/1483683002/diff/1/build/android/gyp/apkbuilde...
File build/android/gyp/apkbuilder.py (right):

https://codereview.chromium.org/1483683002/diff/1/build/android/gyp/apkbuilde...
build/android/gyp/apkbuilder.py:48: # TODO(agrieve): Switch this to be a list of
files rather than a directory.
You should more proud of yourself. Remove the TODO now that you have completed
it!

https://codereview.chromium.org/1483683002/diff/1/build/config/android/rules.gni
File build/config/android/rules.gni (left):

https://codereview.chromium.org/1483683002/diff/1/build/config/android/rules....
build/config/android/rules.gni:1600: "data_deps",
This change may be correct. Any reason that we are not forwarding "data_deps"
any more?

https://codereview.chromium.org/1483683002/diff/1/build/config/android/rules.gni
File build/config/android/rules.gni (right):

https://codereview.chromium.org/1483683002/diff/1/build/config/android/rules....
build/config/android/rules.gni:1628: _extra_native_libs = []
I think that it would be clearer if |_extra_native_libs_| also had the contents
of |_extra_native_libs_even_when_incremental|

That would allow you to have |_extra_native_libs| and
|_extra_native_libs_incremental|
This will make the "create_apk" template in particular clearer

https://codereview.chromium.org/1483683002/diff/1/build/config/android/rules....
build/config/android/rules.gni:1765: _native_libs_file_arg_dep,
Should this depend on |_extra_native_libs_deps| ?

https://codereview.chromium.org/1483683002/diff/1/build/config/android/rules....
build/config/android/rules.gni:1795: _rebased_native_libs =
rebase_path(_extra_native_libs, root_build_dir)
Nit: _rebased_native_libs -> _rebased_extra_native_libs

https://codereview.chromium.org/1483683002/diff/1/build/config/android/rules....
build/config/android/rules.gni:1816: _native_libs_deps + _extra_native_libs_deps
I must be missing something. How come |_native_libs_deps| and
|_extra_native_libs_deps| are "data_deps" instead of regular "deps" ?

[pkotwicz, 2015-11-30 20:36:38]

https://codereview.chromium.org/1483683002/diff/1/build/config/android/rules.gni
File build/config/android/rules.gni (right):

https://codereview.chromium.org/1483683002/diff/1/build/config/android/rules....
build/config/android/rules.gni:1631: _extra_native_libs_even_when_incremental =
[]
Can you please add |_extra_native_libs_even_when_incremental| as part of a
separate CL?

As a general rule of thumb whenever you feel like you are trying to sneak
something in, it is usually time to split the CL into two. It usually does not
take longer to review two smaller CLs than one larger CL. (If I am currently
taking longer to review multiple smallers CLs than one big CLs, please tell me
and I will try to improve)

I like multiple smaller CLs because it makes the git revision history cleaner
(and I don't need to wonder whether a CL titled "Refactor X" actually has some
functionality changed snuck in as well)

[agrieve, 2015-12-01 15:34:22]

https://codereview.chromium.org/1483683002/diff/1/build/android/gyp/apkbuilde...
File build/android/gyp/apkbuilder.py (right):

https://codereview.chromium.org/1483683002/diff/1/build/android/gyp/apkbuilde...
build/android/gyp/apkbuilder.py:48: # TODO(agrieve): Switch this to be a list of
files rather than a directory.
On 2015/11/30 20:28:40, pkotwicz wrote:
> You should more proud of yourself. Remove the TODO now that you have completed
> it!

\o/

https://codereview.chromium.org/1483683002/diff/1/build/config/android/rules.gni
File build/config/android/rules.gni (left):

https://codereview.chromium.org/1483683002/diff/1/build/config/android/rules....
build/config/android/rules.gni:1600: "data_deps",
On 2015/11/30 20:28:41, pkotwicz wrote:
> This change may be correct. Any reason that we are not forwarding "data_deps"
> any more?

Since data_deps just means that the targets will be built "at some point", it
makes no sense to forward them on to the sub-actions. data_deps is already
forwarded to the main group() targets, so a bit odd that it was listed here.

https://codereview.chromium.org/1483683002/diff/1/build/config/android/rules.gni
File build/config/android/rules.gni (right):

https://codereview.chromium.org/1483683002/diff/1/build/config/android/rules....
build/config/android/rules.gni:1628: _extra_native_libs = []
On 2015/11/30 20:28:40, pkotwicz wrote:
> I think that it would be clearer if |_extra_native_libs_| also had the
contents
> of |_extra_native_libs_even_when_incremental|
> 
> That would allow you to have |_extra_native_libs| and
> |_extra_native_libs_incremental|
> This will make the "create_apk" template in particular clearer

Tried this out, but can't do it because extra_native_libs is needed (without
extra_native_libs_even_when_incremental) in the __incremental_script rule

https://codereview.chromium.org/1483683002/diff/1/build/config/android/rules....
build/config/android/rules.gni:1631: _extra_native_libs_even_when_incremental =
[]
On 2015/11/30 20:36:38, pkotwicz wrote:
> Can you please add |_extra_native_libs_even_when_incremental| as part of a
> separate CL?
> 
> As a general rule of thumb whenever you feel like you are trying to sneak
> something in, it is usually time to split the CL into two. It usually does not
> take longer to review two smaller CLs than one larger CL. (If I am currently
> taking longer to review multiple smallers CLs than one big CLs, please tell me
> and I will try to improve)
> 
> I like multiple smaller CLs because it makes the git revision history cleaner
> (and I don't need to wonder whether a CL titled "Refactor X" actually has some
> functionality changed snuck in as well) 

Done.

https://codereview.chromium.org/1483683002/diff/1/build/config/android/rules....
build/config/android/rules.gni:1765: _native_libs_file_arg_dep,
On 2015/11/30 20:28:40, pkotwicz wrote:
> Should this depend on |_extra_native_libs_deps| ?

nope. Not needed to generate the script.

https://codereview.chromium.org/1483683002/diff/1/build/config/android/rules....
build/config/android/rules.gni:1795: _rebased_native_libs =
rebase_path(_extra_native_libs, root_build_dir)
On 2015/11/30 20:28:40, pkotwicz wrote:
> Nit: _rebased_native_libs -> _rebased_extra_native_libs

Done.

https://codereview.chromium.org/1483683002/diff/1/build/config/android/rules....
build/config/android/rules.gni:1816: _native_libs_deps + _extra_native_libs_deps
On 2015/11/30 20:28:40, pkotwicz wrote:
> I must be missing something. How come |_native_libs_deps| and
> |_extra_native_libs_deps| are "data_deps" instead of regular "deps" ?

Because for incremental install, they are not needed until install-time

[agrieve, 2015-12-01 15:34:32]

The CQ bit was checked by agrieve@chromium.org

[agrieve, 2015-12-01 15:34:32]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkotwicz@chromium.org
Link to the patchset: https://codereview.chromium.org/1483683002/#ps20001
(title: "review coments")

[commit-bot: I haz the power, 2015-12-01 15:35:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1483683002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1483683002/20001

[commit-bot: I haz the power, 2015-12-01 18:53:09]

Description was changed from

==========
GN(android): Use list of libraries rather than native_lib_dir in all places

Addresses a TODO and makes rules more well suited for adding in
loadable_modules

BUG=559289
==========

to

==========
GN(android): Use list of libraries rather than native_lib_dir in all places

Addresses a TODO and makes rules more well suited for adding in
loadable_modules

BUG=559289
==========

[commit-bot: I haz the power, 2015-12-01 18:53:09]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-12-01 18:54:45]

Description was changed from

==========
GN(android): Use list of libraries rather than native_lib_dir in all places

Addresses a TODO and makes rules more well suited for adding in
loadable_modules

BUG=559289
==========

to

==========
GN(android): Use list of libraries rather than native_lib_dir in all places

Addresses a TODO and makes rules more well suited for adding in
loadable_modules

BUG=559289

Committed: https://crrev.com/7867a73b9d808755d40b8278b312122269720462
Cr-Commit-Position: refs/heads/master@{#362463}
==========

[commit-bot: I haz the power, 2015-12-01 18:54:49]

Patchset 2 (id:??) landed as
https://crrev.com/7867a73b9d808755d40b8278b312122269720462
Cr-Commit-Position: refs/heads/master@{#362463}

[no sievers, 2015-12-01 22:15:02]

Hmm, this breaks the build for me:

ERROR at //build/config/android/rules.gni:1636:7: Replacing nonempty list.
      _extra_native_libs =
      ^-----------------
This overwrites a previously-defined nonempty list (length 1).
See //build/config/android/rules.gni:1632:28: for previous definition
      _extra_native_libs = [ android_gdbserver ]
                           ^-------------------
with another one (length 1). Did you mean "+=" to append instead? If you
really want to do this, do
  _extra_native_libs = []
before reassigning.
See //content/shell/android/BUILD.gn:175:3: whence it was called.
  android_apk("chromium_linker_test_apk") {

[no sievers, 2015-12-01 22:15:13]

Description was changed from

==========
GN(android): Use list of libraries rather than native_lib_dir in all places

Addresses a TODO and makes rules more well suited for adding in
loadable_modules

BUG=559289

Committed: https://crrev.com/7867a73b9d808755d40b8278b312122269720462
Cr-Commit-Position: refs/heads/master@{#362463}
==========

to

==========
GN(android): Use list of libraries rather than native_lib_dir in all places

Addresses a TODO and makes rules more well suited for adding in
loadable_modules

BUG=559289

Committed: https://crrev.com/7867a73b9d808755d40b8278b312122269720462
Cr-Commit-Position: refs/heads/master@{#362463}
==========

[jbudorick, 2015-12-01 22:32:45]

On 2015/12/01 22:15:02, sievers wrote:
> Hmm, this breaks the build for me:
> 
> ERROR at //build/config/android/rules.gni:1636:7: Replacing nonempty list.
>       _extra_native_libs =
>       ^-----------------
> This overwrites a previously-defined nonempty list (length 1).
> See //build/config/android/rules.gni:1632:28: for previous definition
>       _extra_native_libs = [ android_gdbserver ]
>                            ^-------------------
> with another one (length 1). Did you mean "+=" to append instead? If you
> really want to do this, do
>   _extra_native_libs = []
> before reassigning.
> See //content/shell/android/BUILD.gn:175:3: whence it was called.
>   android_apk("chromium_linker_test_apk") {

this also breaks presubmit in build/android/ :(

** Presubmit ERRORS **
Pylint (241 files using ['--disable=cyclic-import'] on 8 cores) (12.11s) failed
************* Module pylib.local.device.local_device_gtest_run
E: 99, 4: Unexpected keyword argument 'lib_dir' in function call
(unexpected-keyword-arg)

[agrieve, 2015-12-01 23:02:16]

On 2015/12/01 22:32:45, jbudorick wrote:
> On 2015/12/01 22:15:02, sievers wrote:
> > Hmm, this breaks the build for me:
> > 
> > ERROR at //build/config/android/rules.gni:1636:7: Replacing nonempty list.
> >       _extra_native_libs =
> >       ^-----------------
> > This overwrites a previously-defined nonempty list (length 1).
> > See //build/config/android/rules.gni:1632:28: for previous definition
> >       _extra_native_libs = [ android_gdbserver ]
> >                            ^-------------------
> > with another one (length 1). Did you mean "+=" to append instead? If you
> > really want to do this, do
> >   _extra_native_libs = []
> > before reassigning.
> > See //content/shell/android/BUILD.gn:175:3: whence it was called.
> >   android_apk("chromium_linker_test_apk") {
> 
> this also breaks presubmit in build/android/ :(
> 
> ** Presubmit ERRORS **
> Pylint (241 files using ['--disable=cyclic-import'] on 8 cores) (12.11s)
failed
> ************* Module pylib.local.device.local_device_gtest_run
> E: 99, 4: Unexpected keyword argument 'lib_dir' in function call
> (unexpected-keyword-arg)

Yerg, a double-break. Achievement unlocked? :S

John's got a fix in the CQ for the presubmit error:
https://codereview.chromium.org/1486303002/

The array reassigning bug is fixed up in:
https://codereview.chromium.org/1489883002/