GN (Android): Fix packaging excess libs when target_cpu is changed

GN (Android): Fix packaging excess libs when target_cpu is changed

E.g.: Build for x86, then build for arm without first deleting the out
directory would result in an apk with libs for both architectures.

Committed: https://crrev.com/6be715d1ca1fad4dc80f194a4aa710f1eb6f9399
Cr-Commit-Position: refs/heads/master@{#345436}

[agrieve, 2015-08-12 03:18:15]

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

[agrieve, 2015-08-14 17:27:52]

agrieve@chromium.org changed reviewers:
+ dpranke@chromium.org
- cjhopman@chromium.org

[agrieve, 2015-08-17 20:58:12]

On 2015/08/12 03:18:16, agrieve wrote:

bump

[Dirk Pranke, 2015-08-17 21:17:53]

dpranke@chromium.org changed reviewers:
+ brettw@chromium.org

[Dirk Pranke, 2015-08-17 21:17:54]

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

https://codereview.chromium.org/1290643002/diff/1/build/config/android/rules....
build/config/android/rules.gni:1455: _native_libs_dir = gen_dir +
"/lib-$target_cpu"
Brett, is there a better convention for toolchain-specific generated files?

[Dirk Pranke, 2015-08-19 20:14:49]

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

https://codereview.chromium.org/1290643002/diff/1/build/config/android/rules....
build/config/android/rules.gni:1455: _native_libs_dir = gen_dir +
"/lib-$target_cpu"
Brett points out to me that this should actually be okay; ninja should note that
the file was generated by a particular command line, and so if you change the
target_cpu, shouldn't you have a different command line and automatically
rebuild?

[agrieve, 2015-08-19 20:28:39]

On 2015/08/19 20:14:49, Dirk Pranke wrote:
>
https://codereview.chromium.org/1290643002/diff/1/build/config/android/rules.gni
> File build/config/android/rules.gni (right):
> 
>
https://codereview.chromium.org/1290643002/diff/1/build/config/android/rules....
> build/config/android/rules.gni:1455: _native_libs_dir = gen_dir +
> "/lib-$target_cpu"
> Brett points out to me that this should actually be okay; ninja should note
that
> the file was generated by a particular command line, and so if you change the
> target_cpu, shouldn't you have a different command line and automatically
> rebuild?

From what I can tell, the main target is generated in out/Debug, and if you
change the toolset via a target:toolset label, then those files will end up in
out/Debug/toolset/...

In this case, we're doing one build with target_cpu="arm", and then doing
another build with target_cpu="x86" without wiping the output directory
in-between. Both times files go into out/Debug.

If we changed GN to put *all* outputs into toolchain subdirectories I think this
would be fine, but it'd be a pretty big change from how GYP works.

[Dirk Pranke, 2015-08-19 20:46:10]

On 2015/08/19 20:28:39, agrieve wrote:
> On 2015/08/19 20:14:49, Dirk Pranke wrote:
> >
>
https://codereview.chromium.org/1290643002/diff/1/build/config/android/rules.gni
> > File build/config/android/rules.gni (right):
> > 
> >
>
https://codereview.chromium.org/1290643002/diff/1/build/config/android/rules....
> > build/config/android/rules.gni:1455: _native_libs_dir = gen_dir +
> > "/lib-$target_cpu"
> > Brett points out to me that this should actually be okay; ninja should note
> that
> > the file was generated by a particular command line, and so if you change
the
> > target_cpu, shouldn't you have a different command line and automatically
> > rebuild?
> 
> From what I can tell, the main target is generated in out/Debug, and if you
> change the toolset via a target:toolset label, then those files will end up in
> out/Debug/toolset/...
> 
> In this case, we're doing one build with target_cpu="arm", and then doing
> another build with target_cpu="x86" without wiping the output directory
> in-between. Both times files go into out/Debug.

This is correct.

However, if the files have the same name, wouldn't it be
rebuilt because the command lines would be different?

[agrieve, 2015-08-20 00:31:14]

On 2015/08/19 20:46:10, Dirk Pranke wrote:
> On 2015/08/19 20:28:39, agrieve wrote:
> > On 2015/08/19 20:14:49, Dirk Pranke wrote:
> > >
> >
>
https://codereview.chromium.org/1290643002/diff/1/build/config/android/rules.gni
> > > File build/config/android/rules.gni (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1290643002/diff/1/build/config/android/rules....
> > > build/config/android/rules.gni:1455: _native_libs_dir = gen_dir +
> > > "/lib-$target_cpu"
> > > Brett points out to me that this should actually be okay; ninja should
note
> > that
> > > the file was generated by a particular command line, and so if you change
> the
> > > target_cpu, shouldn't you have a different command line and automatically
> > > rebuild?
> > 
> > From what I can tell, the main target is generated in out/Debug, and if you
> > change the toolset via a target:toolset label, then those files will end up
in
> > out/Debug/toolset/...
> > 
> > In this case, we're doing one build with target_cpu="arm", and then doing
> > another build with target_cpu="x86" without wiping the output directory
> > in-between. Both times files go into out/Debug.
> 
> This is correct.
> 
> However, if the files have the same name, wouldn't it be
> rebuilt because the command lines would be different?

Aha - not in this case because the files live within an $android_app_abi
subdirectory within _native_libs_dir. So, alternative fix would be to change the
"${_template_name}__prepare_native" rule to take in the native_lib_dir and
android_app_abi as separate parameters so that it can rm -r the directory itself
(it currently rm -r's the android_app_abi subdirectory, but siblings to it
remain). Is that a better option?

[agrieve, 2015-08-25 15:40:46]

On 2015/08/20 00:31:14, agrieve wrote:
> On 2015/08/19 20:46:10, Dirk Pranke wrote:
> > On 2015/08/19 20:28:39, agrieve wrote:
> > > On 2015/08/19 20:14:49, Dirk Pranke wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/1290643002/diff/1/build/config/android/rules.gni
> > > > File build/config/android/rules.gni (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1290643002/diff/1/build/config/android/rules....
> > > > build/config/android/rules.gni:1455: _native_libs_dir = gen_dir +
> > > > "/lib-$target_cpu"
> > > > Brett points out to me that this should actually be okay; ninja should
> note
> > > that
> > > > the file was generated by a particular command line, and so if you
change
> > the
> > > > target_cpu, shouldn't you have a different command line and
automatically
> > > > rebuild?
> > > 
> > > From what I can tell, the main target is generated in out/Debug, and if
you
> > > change the toolset via a target:toolset label, then those files will end
up
> in
> > > out/Debug/toolset/...
> > > 
> > > In this case, we're doing one build with target_cpu="arm", and then doing
> > > another build with target_cpu="x86" without wiping the output directory
> > > in-between. Both times files go into out/Debug.
> > 
> > This is correct.
> > 
> > However, if the files have the same name, wouldn't it be
> > rebuilt because the command lines would be different?
> 
> Aha - not in this case because the files live within an $android_app_abi
> subdirectory within _native_libs_dir. So, alternative fix would be to change
the
> "${_template_name}__prepare_native" rule to take in the native_lib_dir and
> android_app_abi as separate parameters so that it can rm -r the directory
itself
> (it currently rm -r's the android_app_abi subdirectory, but siblings to it
> remain). Is that a better option?

bump

[Dirk Pranke, 2015-08-25 20:06:46]

got it, lgtm.

[agrieve, 2015-08-25 20:44:21]

The CQ bit was checked by agrieve@chromium.org

[commit-bot: I haz the power, 2015-08-25 20:44:59]

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

[commit-bot: I haz the power, 2015-08-25 21:16:59]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-08-25 21:17:44]

Patchset 1 (id:??) landed as
https://crrev.com/6be715d1ca1fad4dc80f194a4aa710f1eb6f9399
Cr-Commit-Position: refs/heads/master@{#345436}