Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1288023003:
Create *_managed targets for android_apk()s (Closed)
DescriptionCreate *_incremental targets for android_apk()s
These are not depended on by the main rule, but can be built explicitly.
They are meant to be used to decrease adb install times (see bug)
BUG=520082
Committed: https://crrev.com/fd28ec0ae16a28fba11d0e3b72870f286b990d46
Cr-Commit-Position: refs/heads/master@{#345513}
Patch Set 1 #
Total comments: 2
Patch Set 2 : add template comments #
Total comments: 7
Patch Set 3 : review comments 1 #Patch Set 4 : more forwarded vars #
Total comments: 2
Patch Set 5 : managed -> incremental #Patch Set 6 : rebase #Patch Set 7 : fix gn gen #Messages
Total messages: 27 (11 generated)
agrieve@chromium.org changed reviewers: + dpranke@chromium.org
dpranke@chromium.org changed reviewers: + cjhopman@chromium.org
Chris is probably a much better reviewer for this than I. https://codereview.chromium.org/1288023003/diff/1/build/config/android/intern... File build/config/android/internal_rules.gni (right): https://codereview.chromium.org/1288023003/diff/1/build/config/android/intern... build/config/android/internal_rules.gni:512: template("package_apk") { This should have comments describing what this rule does and what parameters are required and what parameters are optional. See most of the other templates for examples.
On 2015/08/12 18:55:20, Dirk Pranke wrote: > Chris is probably a much better reviewer for this than I. > > https://codereview.chromium.org/1288023003/diff/1/build/config/android/intern... > File build/config/android/internal_rules.gni (right): > > https://codereview.chromium.org/1288023003/diff/1/build/config/android/intern... > build/config/android/internal_rules.gni:512: template("package_apk") { > This should have comments describing what this rule does and what parameters are > required and what parameters are optional. > > See most of the other templates for examples. I've been collaborating with Chris about the approach, but sadly this is his last day at Google :(.
On 2015/08/14 17:42:13, agrieve wrote: > On 2015/08/12 18:55:20, Dirk Pranke wrote: > > Chris is probably a much better reviewer for this than I. > > > > > https://codereview.chromium.org/1288023003/diff/1/build/config/android/intern... > > File build/config/android/internal_rules.gni (right): > > > > > https://codereview.chromium.org/1288023003/diff/1/build/config/android/intern... > > build/config/android/internal_rules.gni:512: template("package_apk") { > > This should have comments describing what this rule does and what parameters > are > > required and what parameters are optional. > > > > See most of the other templates for examples. > > I've been collaborating with Chris about the approach, but sadly this is his > last day at Google :(. bump
Sorry for the delay This basically looks good to me, but I'd like Brett to take a look at it as well to see if he sees stuff I don't, since neither of us is Chris :(. https://codereview.chromium.org/1288023003/diff/20001/build/config/android/in... File build/config/android/internal_rules.gni (right): https://codereview.chromium.org/1288023003/diff/20001/build/config/android/in... build/config/android/internal_rules.gni:517: # resource_packaged_apk_path: Path to .ap_ to use. is `.ap_` a typo? https://codereview.chromium.org/1288023003/diff/20001/build/config/android/in... build/config/android/internal_rules.gni:537: } you can (and should) use forward_variables_from(invoker, ["dex_path", "testonly"]) now, instead. https://codereview.chromium.org/1288023003/diff/20001/build/config/android/in... build/config/android/internal_rules.gni:800: deps += invoker.deps same comment https://codereview.chromium.org/1288023003/diff/20001/build/config/android/ru... File build/config/android/rules.gni (right): https://codereview.chromium.org/1288023003/diff/20001/build/config/android/ru... build/config/android/rules.gni:1812: } same comment
dpranke@chromium.org changed reviewers: + brettw@chromium.org - cjhopman@chromium.org
https://codereview.chromium.org/1288023003/diff/1/build/config/android/intern... File build/config/android/internal_rules.gni (right): https://codereview.chromium.org/1288023003/diff/1/build/config/android/intern... build/config/android/internal_rules.gni:512: template("package_apk") { On 2015/08/12 18:55:20, Dirk Pranke wrote: > This should have comments describing what this rule does and what parameters are > required and what parameters are optional. > > See most of the other templates for examples. Done. https://codereview.chromium.org/1288023003/diff/20001/build/config/android/in... File build/config/android/internal_rules.gni (right): https://codereview.chromium.org/1288023003/diff/20001/build/config/android/in... build/config/android/internal_rules.gni:517: # resource_packaged_apk_path: Path to .ap_ to use. On 2015/08/17 21:14:13, Dirk Pranke wrote: > is `.ap_` a typo? Awesomely, no. That's the actual extension chosen for android resource-only apks. https://codereview.chromium.org/1288023003/diff/20001/build/config/android/in... build/config/android/internal_rules.gni:537: } On 2015/08/17 21:14:13, Dirk Pranke wrote: > you can (and should) use > > forward_variables_from(invoker, ["dex_path", "testonly"]) > > now, instead. Done. https://codereview.chromium.org/1288023003/diff/20001/build/config/android/in... build/config/android/internal_rules.gni:800: deps += invoker.deps On 2015/08/17 21:14:13, Dirk Pranke wrote: > same comment Done.
On 2015/08/18 14:48:47, agrieve wrote: > https://codereview.chromium.org/1288023003/diff/1/build/config/android/intern... > File build/config/android/internal_rules.gni (right): > > https://codereview.chromium.org/1288023003/diff/1/build/config/android/intern... > build/config/android/internal_rules.gni:512: template("package_apk") { > On 2015/08/12 18:55:20, Dirk Pranke wrote: > > This should have comments describing what this rule does and what parameters > are > > required and what parameters are optional. > > > > See most of the other templates for examples. > > Done. > > https://codereview.chromium.org/1288023003/diff/20001/build/config/android/in... > File build/config/android/internal_rules.gni (right): > > https://codereview.chromium.org/1288023003/diff/20001/build/config/android/in... > build/config/android/internal_rules.gni:517: # resource_packaged_apk_path: > Path to .ap_ to use. > On 2015/08/17 21:14:13, Dirk Pranke wrote: > > is `.ap_` a typo? > > Awesomely, no. That's the actual extension chosen for android resource-only > apks. > > https://codereview.chromium.org/1288023003/diff/20001/build/config/android/in... > build/config/android/internal_rules.gni:537: } > On 2015/08/17 21:14:13, Dirk Pranke wrote: > > you can (and should) use > > > > forward_variables_from(invoker, ["dex_path", "testonly"]) > > > > now, instead. > > Done. > > https://codereview.chromium.org/1288023003/diff/20001/build/config/android/in... > build/config/android/internal_rules.gni:800: deps += invoker.deps > On 2015/08/17 21:14:13, Dirk Pranke wrote: > > same comment > > Done. bump
lgtm w/ last comment addressed. https://codereview.chromium.org/1288023003/diff/60001/build/config/android/in... File build/config/android/internal_rules.gni (right): https://codereview.chromium.org/1288023003/diff/60001/build/config/android/in... build/config/android/internal_rules.gni:584: template("finalize_apk") { this still needs comments.
https://codereview.chromium.org/1288023003/diff/60001/build/config/android/in... File build/config/android/internal_rules.gni (right): https://codereview.chromium.org/1288023003/diff/60001/build/config/android/in... build/config/android/internal_rules.gni:584: template("finalize_apk") { On 2015/08/25 20:04:27, Dirk Pranke wrote: > this still needs comments. Done.
The CQ bit was checked by agrieve@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org Link to the patchset: https://codereview.chromium.org/1288023003/#ps80001 (title: "managed -> incremental")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1288023003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1288023003/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by agrieve@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org Link to the patchset: https://codereview.chromium.org/1288023003/#ps100001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1288023003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1288023003/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
The CQ bit was checked by agrieve@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org Link to the patchset: https://codereview.chromium.org/1288023003/#ps120001 (title: "fix gn gen")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1288023003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1288023003/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/fd28ec0ae16a28fba11d0e3b72870f286b990d46 Cr-Commit-Position: refs/heads/master@{#345513} |
