Issue 379333003: Add beginnings of android_apk and unittest_apk templates

Issue 379333003: Add beginnings of android_apk and unittest_apk templates (Closed)

Created:
6 years, 5 months ago by cjhopman

Modified:
6 years, 4 months ago

Reviewers:
brettw, newt (away)

CC:
chromium-reviews, klundberg+watch_chromium.org, erikwright+watch_chromium.org, yfriedman+watch_chromium.org, ilevy-cc_chromium.org

Base URL:
https://chromium.googlesource.com/chromium/src.git@gn-filearg

Project:
chromium

Visibility:
Public.

More Reviews

Description

Add beginnings of android_apk and unittest_apk templates This adds two templates, android_apk and unittest_apk. unittest_apk is a simple wrapper of android_apk for packaging gtests into apks. android_apk currently just does the java compilation, processing resources, and build_config stuff for apks. BUG=359249 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=286031

Patch Set 1 : #

Patch Set 2 : 80-column lines #

Total comments: 6

Patch Set 3 : #

Patch Set 4 : Rebase #

Created: 6 years, 4 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+230 lines, -35 lines)			Patch
M	base/BUILD.gn	View	1 2 3	1 chunk	+16 lines, -0 lines	0 comments	Download
M	build/android/gyp/jar_toc.py	View		1 chunk	+5 lines, -0 lines	0 comments	Download
M	build/android/gyp/process_resources.py	View	1 2	2 chunks	+9 lines, -0 lines	0 comments	Download
M	build/android/gyp/write_build_config.py	View		4 chunks	+27 lines, -16 lines	0 comments	Download
M	build/config/android/internal_rules.gni	View	1 2	9 chunks	+37 lines, -17 lines	0 comments	Download
M	build/config/android/rules.gni	View	1 2 3	4 chunks	+136 lines, -2 lines	0 comments	Download

Messages

Total messages: 11 (0 generated)

Expand Messages | Collapse Messages

newt (away)

lgtm https://codereview.chromium.org/379333003/diff/40001/build/android/gyp/write_build_config.py File build/android/gyp/write_build_config.py (right): https://codereview.chromium.org/379333003/diff/40001/build/android/gyp/write_build_config.py#newcode102 build/android/gyp/write_build_config.py:102: unknown_deps = [ s/unknown/missing or s/unknown/nonexistent ? wait... ...

6 years, 5 months ago (2014-07-17 20:11:47 UTC) #3

cjhopman

brettw: for OWNERS https://codereview.chromium.org/379333003/diff/40001/build/android/gyp/write_build_config.py File build/android/gyp/write_build_config.py (right): https://codereview.chromium.org/379333003/diff/40001/build/android/gyp/write_build_config.py#newcode102 build/android/gyp/write_build_config.py:102: unknown_deps = [ On 2014/07/17 20:11:47, ...

6 years, 5 months ago (2014-07-18 00:58:01 UTC) #4

brettw: for OWNERS

https://codereview.chromium.org/379333003/diff/40001/build/android/gyp/write_...
File build/android/gyp/write_build_config.py (right):

https://codereview.chromium.org/379333003/diff/40001/build/android/gyp/write_...
build/android/gyp/write_build_config.py:102: unknown_deps = [
On 2014/07/17 20:11:47, newt wrote:
> s/unknown/missing or s/unknown/nonexistent ?
> 
> wait... are these deps that haven't been written yet? or deps that will never
be
> written?

So, possible_deps_configs would be the path to the .build_config for everything
listed in deps. However, only some types of targets actually write a
build_config file. So, right now, for libraries and resources I just call that
an error (because I don't know of any case where one of those types of targets
needs such a dep). For android_apk, deps could include things like
shared_library or something that generates an asset into asset_dir, so I allowed
that.

Ideally, your target just lists its deps in deps and things just work. Also, we
don't want making it work to require a lot of magic. This here is a bit of
magic, but I don't know if it will cause issues or not. Also, it's possible that
this write_build_config.py will be built into (or have more hooks into) gn such
that it could have access to the entire dependency graph and so understand
exactly what to do with the different deps (and be a bit less magical).

(did that make sense?)

Maybe unrecognized_deps would be better?

https://codereview.chromium.org/379333003/diff/40001/build/config/android/int...
File build/config/android/internal_rules.gni (right):

https://codereview.chromium.org/379333003/diff/40001/build/config/android/int...
build/config/android/internal_rules.gni:234: jar_path = invoker.jar_path
On 2014/07/17 20:11:47, newt wrote:
> is jar_path still visible to the java_library template below?

It is. As discussed, I've changed it to make it more clear what is used where.

https://codereview.chromium.org/379333003/diff/40001/build/config/android/rul...
File build/config/android/rules.gni (right):

https://codereview.chromium.org/379333003/diff/40001/build/config/android/rul...
build/config/android/rules.gni:500: assert(!defined(invoker.native_libs)
On 2014/07/17 20:11:47, newt wrote:
> why not just add "|| true" ?

Done.

newt (away)

On 2014/07/18 00:58:01, cjhopman wrote: > brettw: for OWNERS > > https://codereview.chromium.org/379333003/diff/40001/build/android/gyp/write_build_config.py > File build/android/gyp/write_build_config.py ...

6 years, 5 months ago (2014-07-18 01:25:59 UTC) #5

On 2014/07/18 00:58:01, cjhopman wrote:
> brettw: for OWNERS
> 
>
https://codereview.chromium.org/379333003/diff/40001/build/android/gyp/write_...
> File build/android/gyp/write_build_config.py (right):
> 
>
https://codereview.chromium.org/379333003/diff/40001/build/android/gyp/write_...
> build/android/gyp/write_build_config.py:102: unknown_deps = [
> On 2014/07/17 20:11:47, newt wrote:
> > s/unknown/missing or s/unknown/nonexistent ?
> > 
> > wait... are these deps that haven't been written yet? or deps that will
never
> be
> > written?
> 
> So, possible_deps_configs would be the path to the .build_config for
everything
> listed in deps. However, only some types of targets actually write a
> build_config file. So, right now, for libraries and resources I just call that
> an error (because I don't know of any case where one of those types of targets
> needs such a dep). For android_apk, deps could include things like
> shared_library or something that generates an asset into asset_dir, so I
allowed
> that.
> 
> Ideally, your target just lists its deps in deps and things just work. Also,
we
> don't want making it work to require a lot of magic. This here is a bit of
> magic, but I don't know if it will cause issues or not. Also, it's possible
that
> this write_build_config.py will be built into (or have more hooks into) gn
such
> that it could have access to the entire dependency graph and so understand
> exactly what to do with the different deps (and be a bit less magical).
> 
> (did that make sense?)
> 
> Maybe unrecognized_deps would be better?

Makes sense. Any of those naming options sound decent to me, though a comment
might explain things better.

> 
>
https://codereview.chromium.org/379333003/diff/40001/build/config/android/int...
> File build/config/android/internal_rules.gni (right):
> 
>
https://codereview.chromium.org/379333003/diff/40001/build/config/android/int...
> build/config/android/internal_rules.gni:234: jar_path = invoker.jar_path
> On 2014/07/17 20:11:47, newt wrote:
> > is jar_path still visible to the java_library template below?
> 
> It is. As discussed, I've changed it to make it more clear what is used where.
> 
>
https://codereview.chromium.org/379333003/diff/40001/build/config/android/rul...
> File build/config/android/rules.gni (right):
> 
>
https://codereview.chromium.org/379333003/diff/40001/build/config/android/rul...
> build/config/android/rules.gni:500: assert(!defined(invoker.native_libs)
> On 2014/07/17 20:11:47, newt wrote:
> > why not just add "|| true" ?
> 
> Done.

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/cjhopman@chromium.org/379333003/80001

6 years, 4 months ago (2014-07-28 20:44:32 UTC) #9

commit-bot: I haz the power

FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_gpu_triggered_tests on tryserver.chromium.gpu ...

6 years, 4 months ago (2014-07-28 22:12:20 UTC) #10

Message was sent while issue was closed.

Change committed as 286031

Expand Messages | Collapse Messages