Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2123753005:
Creating new gn arg: enable_all_proguard_optimizations. (Closed)
DescriptionCreating new gn arg: enable_all_proguard_optimizations.
If we wish to turn on optimizations for our main code base, we need to
first make sure that these optimizations don't break our instrumentation
tests when inlining functions. To do this, we make a gn arg which will guard
our instrumentation tests and only allow them to run without this
flag. This flag will then be the thing that turns on these Proguard
optimizations.
BUG=625704
Committed: https://crrev.com/05c799ee764fef9d63b4e45c4fba22145f96fed3
Cr-Commit-Position: refs/heads/master@{#405509}
Patch Set 1 #Patch Set 2 #Patch Set 3 : Rebased #Patch Set 4 : Renamed proguard_under_test.flags to proguard_for_test.flags #
Total comments: 22
Patch Set 5 : Addressing comments #
Total comments: 11
Patch Set 6 : Addressing comments from last patch set #
Total comments: 2
Patch Set 7 : Changing from 2 apks to gn arg which controls optimizations #
Total comments: 5
Patch Set 8 : Addressing Andrew's comments #Patch Set 9 : Removing guard for instrumentation tests which do not use Proguard #Patch Set 10 : Preventing any regression if enable_all_proguard_optimizations is never turned on #
Total comments: 10
Patch Set 11 : Adding assert to ensure sanity with enable_all_proguard_optimizations and is_java_debug #Patch Set 12 : Moving testing-specific proguard configs to chrome_public_apk_tmpl #Patch Set 13 : rebased #Patch Set 14 : Rebased again #
Messages
Total messages: 87 (46 generated)
The CQ bit was checked by smaier@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
The CQ bit was checked by smaier@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
smaier@chromium.org changed reviewers: + agrieve@chromium.org, yfriedman@chromium.org
yfriedman@chromium.org changed reviewers: + jbudorick@chromium.org
+jbudorick who should review https://codereview.chromium.org/2123753005/diff/60001/build/config/android/ru... File build/config/android/rules.gni (right): https://codereview.chromium.org/2123753005/diff/60001/build/config/android/ru... build/config/android/rules.gni:1619: _is_testable_apk = _proguard_enabled && !defined(invoker.apk_under_test) Should at minimum have a comment but I feel like in general this is hard to grok (or will be after the fact). Rather than assuming there will be other differences, it might make it easier to understand if we just call this the "unoptimized" apk. https://codereview.chromium.org/2123753005/diff/60001/build/config/android/ru... build/config/android/rules.gni:2131: # We are going to create a second apk, very similiar to the above seems like an awful lot of copy/paste :( is using a new template unwieldy? https://codereview.chromium.org/2123753005/diff/60001/chrome/android/java/pro... File chrome/android/java/proguard_for_test.flags (right): https://codereview.chromium.org/2123753005/diff/60001/chrome/android/java/pro... chrome/android/java/proguard_for_test.flags:1: -dontoptimize seems like this should be under testing/android
https://codereview.chromium.org/2123753005/diff/60001/build/config/android/ru... File build/config/android/rules.gni (right): https://codereview.chromium.org/2123753005/diff/60001/build/config/android/ru... build/config/android/rules.gni:1619: _is_testable_apk = _proguard_enabled && !defined(invoker.apk_under_test) nit: non-proguarded apks are testable as well. How about "_create_apk_for_test" This is probably also a good spot to add a comment saying why it's necessary to do this. https://codereview.chromium.org/2123753005/diff/60001/build/config/android/ru... build/config/android/rules.gni:1626: final_dex_for_test_path = "$gen_dir/classes_for_test.dex" nit: prefix with _ https://codereview.chromium.org/2123753005/diff/60001/build/config/android/ru... build/config/android/rules.gni:1629: "Cannot set final_apk_path for tested apk") nit: this message misses the "why". How about: "Setting final_apk_path for proguarded apks is not yet implemented." https://codereview.chromium.org/2123753005/diff/60001/build/config/android/ru... build/config/android/rules.gni:1631: "$root_build_dir/apks/${invoker.apk_name}_for_test.apk" nit: apk_names are AllInCamelCase. So how about: ${invoker.apk_name}ForTest.apk https://codereview.chromium.org/2123753005/diff/60001/build/config/android/ru... build/config/android/rules.gni:1940: _dex_sources_for_test = [ _proguard_output_jar_for_test_path ] nit: these are both used only once. Would be better to inline them. https://codereview.chromium.org/2123753005/diff/60001/build/config/android/ru... build/config/android/rules.gni:1973: final_dex_for_test_target_name = "${final_dex_target_name}_for_test" nit: prefix with _ https://codereview.chromium.org/2123753005/diff/60001/build/config/android/ru... build/config/android/rules.gni:2135: create_apk("${_template_name_for_test}") { We can eliminate this copy / paste using a nested template() I think (e.g. define it at line 2055). It would have to go something like: template("create_main_apk") { _parent_invoker = invoker.invoker ... } create_main_apk(_template_name_for_test) { apk_path = ... base_path = ... dex_path = ... deps = ... } https://codereview.chromium.org/2123753005/diff/60001/build/config/android/ru... build/config/android/rules.gni:2205: forward_variables_from(invoker, [ "native_lib_placeholders" ]) nit: since you're here, can you put this in with the forward_variables_from() all above, and move the note about http://crbug.com/395038 to appear in the template argument comments on line 1429? https://codereview.chromium.org/2123753005/diff/60001/chrome/android/java/pro... File chrome/android/java/proguard_for_test.flags (right): https://codereview.chromium.org/2123753005/diff/60001/chrome/android/java/pro... chrome/android/java/proguard_for_test.flags:1: -dontoptimize nit: add copyright header. and comment about why we don't optimize for_test apks
The CQ bit was checked by smaier@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2123753005/diff/60001/build/config/android/ru... File build/config/android/rules.gni (right): https://codereview.chromium.org/2123753005/diff/60001/build/config/android/ru... build/config/android/rules.gni:1619: _is_testable_apk = _proguard_enabled && !defined(invoker.apk_under_test) On 2016/07/06 14:18:49, agrieve wrote: > nit: non-proguarded apks are testable as well. How about "_create_apk_for_test" > > This is probably also a good spot to add a comment saying why it's necessary to > do this. Done. https://codereview.chromium.org/2123753005/diff/60001/build/config/android/ru... build/config/android/rules.gni:1626: final_dex_for_test_path = "$gen_dir/classes_for_test.dex" On 2016/07/06 14:18:49, agrieve wrote: > nit: prefix with _ Done. https://codereview.chromium.org/2123753005/diff/60001/build/config/android/ru... build/config/android/rules.gni:1629: "Cannot set final_apk_path for tested apk") On 2016/07/06 14:18:49, agrieve wrote: > nit: this message misses the "why". How about: > > "Setting final_apk_path for proguarded apks is not yet implemented." Done. https://codereview.chromium.org/2123753005/diff/60001/build/config/android/ru... build/config/android/rules.gni:1631: "$root_build_dir/apks/${invoker.apk_name}_for_test.apk" On 2016/07/06 14:18:49, agrieve wrote: > nit: apk_names are AllInCamelCase. So how about: ${invoker.apk_name}ForTest.apk Done. https://codereview.chromium.org/2123753005/diff/60001/build/config/android/ru... build/config/android/rules.gni:1940: _dex_sources_for_test = [ _proguard_output_jar_for_test_path ] On 2016/07/06 14:18:49, agrieve wrote: > nit: these are both used only once. Would be better to inline them. Done. https://codereview.chromium.org/2123753005/diff/60001/build/config/android/ru... build/config/android/rules.gni:1973: final_dex_for_test_target_name = "${final_dex_target_name}_for_test" On 2016/07/06 14:18:49, agrieve wrote: > nit: prefix with _ Done. https://codereview.chromium.org/2123753005/diff/60001/build/config/android/ru... build/config/android/rules.gni:2131: # We are going to create a second apk, very similiar to the above On 2016/07/06 14:01:41, Yaron wrote: > seems like an awful lot of copy/paste :( > is using a new template unwieldy? Done. https://codereview.chromium.org/2123753005/diff/60001/build/config/android/ru... build/config/android/rules.gni:2135: create_apk("${_template_name_for_test}") { On 2016/07/06 14:18:49, agrieve wrote: > We can eliminate this copy / paste using a nested template() I think (e.g. > define it at line 2055). > > It would have to go something like: > > template("create_main_apk") { > _parent_invoker = invoker.invoker > ... > } > > create_main_apk(_template_name_for_test) { > apk_path = ... > base_path = ... > dex_path = ... > deps = ... > } Done. https://codereview.chromium.org/2123753005/diff/60001/build/config/android/ru... build/config/android/rules.gni:2205: forward_variables_from(invoker, [ "native_lib_placeholders" ]) On 2016/07/06 14:18:49, agrieve wrote: > nit: since you're here, can you put this in with the forward_variables_from() > all above, and move the note about http://crbug.com/395038 to appear in the > template argument comments on line 1429? Done. https://codereview.chromium.org/2123753005/diff/60001/chrome/android/java/pro... File chrome/android/java/proguard_for_test.flags (right): https://codereview.chromium.org/2123753005/diff/60001/chrome/android/java/pro... chrome/android/java/proguard_for_test.flags:1: -dontoptimize On 2016/07/06 14:18:49, agrieve wrote: > nit: add copyright header. and comment about why we don't optimize for_test apks Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
https://codereview.chromium.org/2123753005/diff/80001/build/config/android/ru... File build/config/android/rules.gni (right): https://codereview.chromium.org/2123753005/diff/80001/build/config/android/ru... build/config/android/rules.gni:1920: proguard(_proguard_for_test_target) { I liked how the create_apk() template turned out. Can you do the same for proguard()? e.g. proguard_helper(_proguard_for_test_target) { output_jar_path = ... proguard_configs = _proguard_configs + [ "//testing/android/proguard_for_test.flags" ] } https://codereview.chromium.org/2123753005/diff/80001/build/config/android/ru... build/config/android/rules.gni:2066: assert(_extra_native_libs_even_when_incremental == [] || nit: move below comment. https://codereview.chromium.org/2123753005/diff/80001/testing/android/proguar... File testing/android/proguard_for_test.flags (right): https://codereview.chromium.org/2123753005/diff/80001/testing/android/proguar... testing/android/proguard_for_test.flags:1: # Copyright (c) 2016 The Chromium Authors. All rights reserved. nit: we don't put the (c) in anymore :P
On 2016/07/07 01:32:13, agrieve wrote: > https://codereview.chromium.org/2123753005/diff/80001/build/config/android/ru... > File build/config/android/rules.gni (right): > > https://codereview.chromium.org/2123753005/diff/80001/build/config/android/ru... > build/config/android/rules.gni:1920: proguard(_proguard_for_test_target) { > I liked how the create_apk() template turned out. Can you do the same for > proguard()? e.g. > > proguard_helper(_proguard_for_test_target) { > output_jar_path = ... > proguard_configs = _proguard_configs + [ > "//testing/android/proguard_for_test.flags" ] > } > > https://codereview.chromium.org/2123753005/diff/80001/build/config/android/ru... > build/config/android/rules.gni:2066: > assert(_extra_native_libs_even_when_incremental == [] || > nit: move below comment. > > https://codereview.chromium.org/2123753005/diff/80001/testing/android/proguar... > File testing/android/proguard_for_test.flags (right): > > https://codereview.chromium.org/2123753005/diff/80001/testing/android/proguar... > testing/android/proguard_for_test.flags:1: # Copyright (c) 2016 The Chromium > Authors. All rights reserved. > nit: we don't put the (c) in anymore :P Note: there was a bot outage which is why your one tryjob failed.
I'm wondering about the merits of this vs either: - building the current contents of test apk & apk under test into a single apk. There's limited value in having a separate APK if it's not the one we're shipping (and since we support multidex, which we didn't when we made the splits) - putting optimizations behind a gn flag that would disable test apk targets -- i.e., you either build w/o optimizations and get apk under test + test apk, or you build w/ optimizations and can only build apk under test. We aren't testing the optimized apk in either scenario, but I think the GN is significantly simpler. https://codereview.chromium.org/2123753005/diff/80001/build/config/android/ru... File build/config/android/rules.gni (right): https://codereview.chromium.org/2123753005/diff/80001/build/config/android/ru... build/config/android/rules.gni:2087: _parent_invoker = invoker.invoker :| I'm not crazy about the double invoker thing -- build/config/android appears to be the only place in which it's used at all. Can we instead forward variables down normally? https://codereview.chromium.org/2123753005/diff/80001/testing/android/proguar... File testing/android/proguard_for_test.flags (right): https://codereview.chromium.org/2123753005/diff/80001/testing/android/proguar... testing/android/proguard_for_test.flags:12: -dontoptimize Does this need to be in its own file? Can we pass it inline?
On 2016/07/07 01:48:52, jbudorick wrote: > I'm wondering about the merits of this vs either: > - building the current contents of test apk & apk under test into a single apk. > There's limited value in having a separate APK if it's not the one we're > shipping (and since we support multidex, which we didn't when we made the > splits) Main reasons I can see to keep them separate is shorter build times when you're changing only test code > - putting optimizations behind a gn flag that would disable test apk targets -- > i.e., you either build w/o optimizations and get apk under test + test apk, or > you build w/ optimizations and can only build apk under test. We aren't testing > the optimized apk in either scenario, but I think the GN is significantly > simpler. I hadn't considered this approach! I think the added GN complexity might be worth it though. A GN arg that disabled optimizations would mean that people couldn't run tests without enabling the GN arg. Not sure how many run tests with is_debug=false though. I also think there's value in having "chrome_public_apk" always builds the version that we ship, and not the one we use for testing. > > https://codereview.chromium.org/2123753005/diff/80001/build/config/android/ru... > File build/config/android/rules.gni (right): > > https://codereview.chromium.org/2123753005/diff/80001/build/config/android/ru... > build/config/android/rules.gni:2087: _parent_invoker = invoker.invoker > :| I'm not crazy about the double invoker thing -- build/config/android appears > to be the only place in which it's used at all. Can we instead forward variables > down normally? > > https://codereview.chromium.org/2123753005/diff/80001/testing/android/proguar... > File testing/android/proguard_for_test.flags (right): > > https://codereview.chromium.org/2123753005/diff/80001/testing/android/proguar... > testing/android/proguard_for_test.flags:12: -dontoptimize > Does this need to be in its own file? Can we pass it inline?
https://codereview.chromium.org/2123753005/diff/80001/build/config/android/ru... File build/config/android/rules.gni (right): https://codereview.chromium.org/2123753005/diff/80001/build/config/android/ru... build/config/android/rules.gni:2087: _parent_invoker = invoker.invoker On 2016/07/07 01:48:52, jbudorick wrote: > :| I'm not crazy about the double invoker thing -- build/config/android appears > to be the only place in which it's used at all. Can we instead forward variables > down normally? Hmm, I suggested using invoker.invoker and think it's not the worst since it still refers to the main template's invoker. But, looking at this again, I wonder if we could get rid a lot of the Mark as used asserts by doing: _create_apk_vars = [ "alternative_android_sdk_jar", "android_aapt_path", "app_as_shared_lib", "deps", "extensions_to_not_compress", "language_splits", "native_lib_placeholders", "page_align_shared_libraries", "public_deps", "secondary_native_libs", "shared_resources", "uncompress_shared_libraries", "write_asset_list", ] Then in create_main_apk: forward_variables_from(invoker, _create_apk_vars + [ ... ]) Then when calling it: create_main_apk("${_template_name}__create") { forward_variables_from(invoker, _create_apk_vars) } https://codereview.chromium.org/2123753005/diff/80001/testing/android/proguar... File testing/android/proguard_for_test.flags (right): https://codereview.chromium.org/2123753005/diff/80001/testing/android/proguar... testing/android/proguard_for_test.flags:12: -dontoptimize On 2016/07/07 01:48:52, jbudorick wrote: > Does this need to be in its own file? Can we pass it inline? I made the same comment in person, and Sam pointed out that we'll also want to move the @VisibleForTesting -keeps in here next (and friends)
https://codereview.chromium.org/2123753005/diff/80001/build/config/android/ru... File build/config/android/rules.gni (right): https://codereview.chromium.org/2123753005/diff/80001/build/config/android/ru... build/config/android/rules.gni:1920: proguard(_proguard_for_test_target) { On 2016/07/07 01:32:12, agrieve wrote: > I liked how the create_apk() template turned out. Can you do the same for > proguard()? e.g. > > proguard_helper(_proguard_for_test_target) { > output_jar_path = ... > proguard_configs = _proguard_configs + [ > "//testing/android/proguard_for_test.flags" ] > } Done. https://codereview.chromium.org/2123753005/diff/80001/build/config/android/ru... build/config/android/rules.gni:2066: assert(_extra_native_libs_even_when_incremental == [] || On 2016/07/07 01:32:12, agrieve wrote: > nit: move below comment. Done. https://codereview.chromium.org/2123753005/diff/80001/build/config/android/ru... build/config/android/rules.gni:2087: _parent_invoker = invoker.invoker On 2016/07/07 13:37:57, agrieve wrote: > On 2016/07/07 01:48:52, jbudorick wrote: > > :| I'm not crazy about the double invoker thing -- build/config/android > appears > > to be the only place in which it's used at all. Can we instead forward > variables > > down normally? > > Hmm, I suggested using invoker.invoker and think it's not the worst since it > still refers to the main template's invoker. But, looking at this again, I > wonder if we could get rid a lot of the Mark as used asserts by doing: > > _create_apk_vars = [ > "alternative_android_sdk_jar", > "android_aapt_path", > "app_as_shared_lib", > "deps", > "extensions_to_not_compress", > "language_splits", > "native_lib_placeholders", > "page_align_shared_libraries", > "public_deps", > "secondary_native_libs", > "shared_resources", > "uncompress_shared_libraries", > "write_asset_list", > ] > > Then in create_main_apk: > forward_variables_from(invoker, _create_apk_vars + [ ... ]) > > Then when calling it: > create_main_apk("${_template_name}__create") { > forward_variables_from(invoker, _create_apk_vars) > } Done. https://codereview.chromium.org/2123753005/diff/80001/testing/android/proguar... File testing/android/proguard_for_test.flags (right): https://codereview.chromium.org/2123753005/diff/80001/testing/android/proguar... testing/android/proguard_for_test.flags:1: # Copyright (c) 2016 The Chromium Authors. All rights reserved. On 2016/07/07 01:32:13, agrieve wrote: > nit: we don't put the (c) in anymore :P Done.
The CQ bit was checked by smaier@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm. Please wait for John's lgtm as well though. https://codereview.chromium.org/2123753005/diff/100001/build/config/android/r... File build/config/android/rules.gni (right): https://codereview.chromium.org/2123753005/diff/100001/build/config/android/r... build/config/android/rules.gni:2104: # This target generates the input file _all_resources_zip_path. nit: can you remove this comment? It doesn't add much value and "This target" is ambiguous. https://codereview.chromium.org/2123753005/diff/100001/build/config/android/r... build/config/android/rules.gni:2151: create_main_apk("${_template_name}__create") { nit: "${var}" -> var
On 2016/07/07 02:35:25, agrieve wrote: > On 2016/07/07 01:48:52, jbudorick wrote: > > I'm wondering about the merits of this vs either: > > - building the current contents of test apk & apk under test into a single > apk. > > There's limited value in having a separate APK if it's not the one we're > > shipping (and since we support multidex, which we didn't when we made the > > splits) > > Main reasons I can see to keep them separate is shorter build times when you're > changing only test code I wonder how much shorter it actually is in this case. (I should also note that this change would be quite a bit more involved.) > > > - putting optimizations behind a gn flag that would disable test apk targets > -- > > i.e., you either build w/o optimizations and get apk under test + test apk, or > > you build w/ optimizations and can only build apk under test. We aren't > testing > > the optimized apk in either scenario, but I think the GN is significantly > > simpler. > > I hadn't considered this approach! I think the added GN complexity might be > worth it though. > A GN arg that disabled optimizations would mean that people couldn't run tests > without enabling the GN arg. Not sure how many run tests with is_debug=false > though. > I also think there's value in having "chrome_public_apk" always builds the > version that we ship, and not the one we use for testing. I think the default would be the opposite -- people wouldn't build optimized without enabling the gn arg. > > > > > > https://codereview.chromium.org/2123753005/diff/80001/build/config/android/ru... > > File build/config/android/rules.gni (right): > > > > > https://codereview.chromium.org/2123753005/diff/80001/build/config/android/ru... > > build/config/android/rules.gni:2087: _parent_invoker = invoker.invoker > > :| I'm not crazy about the double invoker thing -- build/config/android > appears > > to be the only place in which it's used at all. Can we instead forward > variables > > down normally? > > > > > https://codereview.chromium.org/2123753005/diff/80001/testing/android/proguar... > > File testing/android/proguard_for_test.flags (right): > > > > > https://codereview.chromium.org/2123753005/diff/80001/testing/android/proguar... > > testing/android/proguard_for_test.flags:12: -dontoptimize > > Does this need to be in its own file? Can we pass it inline?
On 2016/07/07 15:40:46, jbudorick wrote: > On 2016/07/07 02:35:25, agrieve wrote: > > On 2016/07/07 01:48:52, jbudorick wrote: > > > I'm wondering about the merits of this vs either: > > > - building the current contents of test apk & apk under test into a single > > apk. > > > There's limited value in having a separate APK if it's not the one we're > > > shipping (and since we support multidex, which we didn't when we made the > > > splits) > > > > Main reasons I can see to keep them separate is shorter build times when > you're > > changing only test code > > I wonder how much shorter it actually is in this case. > > (I should also note that this change would be quite a bit more involved.) > > > > > > - putting optimizations behind a gn flag that would disable test apk > targets > > -- > > > i.e., you either build w/o optimizations and get apk under test + test apk, > or > > > you build w/ optimizations and can only build apk under test. We aren't > > testing > > > the optimized apk in either scenario, but I think the GN is significantly > > > simpler. > > > > I hadn't considered this approach! I think the added GN complexity might be > > worth it though. > > A GN arg that disabled optimizations would mean that people couldn't run tests > > without enabling the GN arg. Not sure how many run tests with is_debug=false > > though. > > I also think there's value in having "chrome_public_apk" always builds the > > version that we ship, and not the one we use for testing. > > I think the default would be the opposite -- people wouldn't build optimized > without enabling the gn arg. I should note that I think the two-apk solution is more confusing w.r.t. build products (e.g., "why do I have two ChromePublic apks in out/Release/apks?") in addition to being more complex in gn. > > > > > > > > > > > > https://codereview.chromium.org/2123753005/diff/80001/build/config/android/ru... > > > File build/config/android/rules.gni (right): > > > > > > > > > https://codereview.chromium.org/2123753005/diff/80001/build/config/android/ru... > > > build/config/android/rules.gni:2087: _parent_invoker = invoker.invoker > > > :| I'm not crazy about the double invoker thing -- build/config/android > > appears > > > to be the only place in which it's used at all. Can we instead forward > > variables > > > down normally? > > > > > > > > > https://codereview.chromium.org/2123753005/diff/80001/testing/android/proguar... > > > File testing/android/proguard_for_test.flags (right): > > > > > > > > > https://codereview.chromium.org/2123753005/diff/80001/testing/android/proguar... > > > testing/android/proguard_for_test.flags:12: -dontoptimize > > > Does this need to be in its own file? Can we pass it inline?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
On 2016/07/07 17:11:04, commit-bot: I haz the power wrote:
> Dry run: Try jobs failed on following builders:
> linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
> (JOB_TIMED_OUT, no build URL)
Talked it through with John offline and I think I've come around to agreeing
it'd be better to just have a separate GN arg for this.
Specifically:
enable_full_proguard_optimizations = is_official_build
Then, add an assert to instrumentation_test_apk() {
assert(!enable_full_proguard_optimizations) }
And guard all instrumentation_test_apk() behind if
(!enable_full_proguard_optimizations) {}
To not break the build, I think you'll need to add the assert as the last step
(after if guards are added up & downstream)
The CQ bit was checked by smaier@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
couple nits + fix the missing imports = lgtm! https://codereview.chromium.org/2123753005/diff/120001/android_webview/tools/... File android_webview/tools/system_webview_shell/BUILD.gn (right): https://codereview.chromium.org/2123753005/diff/120001/android_webview/tools/... android_webview/tools/system_webview_shell/BUILD.gn:68: if (!enable_all_proguard_optimizations) { nit: combine with previous if. https://codereview.chromium.org/2123753005/diff/120001/build/config/android/c... File build/config/android/config.gni (right): https://codereview.chromium.org/2123753005/diff/120001/build/config/android/c... build/config/android/config.gni:121: # enabled for instrumentation tests, as they break the tests. nit: change ", as they break the tests" -> "since they cause code required by the tests to be removed" https://codereview.chromium.org/2123753005/diff/120001/build/config/android/r... File build/config/android/rules.gni (right): https://codereview.chromium.org/2123753005/diff/120001/build/config/android/r... build/config/android/rules.gni:1842: assert(_proguard_configs != []) # Mark as used. still necessary?
On 2016/07/08 14:26:20, agrieve wrote: > couple nits + fix the missing imports = lgtm! > > https://codereview.chromium.org/2123753005/diff/120001/android_webview/tools/... > File android_webview/tools/system_webview_shell/BUILD.gn (right): > > https://codereview.chromium.org/2123753005/diff/120001/android_webview/tools/... > android_webview/tools/system_webview_shell/BUILD.gn:68: if > (!enable_all_proguard_optimizations) { > nit: combine with previous if. > > https://codereview.chromium.org/2123753005/diff/120001/build/config/android/c... > File build/config/android/config.gni (right): > > https://codereview.chromium.org/2123753005/diff/120001/build/config/android/c... > build/config/android/config.gni:121: # enabled for instrumentation tests, as > they break the tests. > nit: change ", as they break the tests" -> "since they cause code required by > the tests to be removed" > > https://codereview.chromium.org/2123753005/diff/120001/build/config/android/r... > File build/config/android/rules.gni (right): > > https://codereview.chromium.org/2123753005/diff/120001/build/config/android/r... > build/config/android/rules.gni:1842: assert(_proguard_configs != []) # Mark as > used. > still necessary? (+ update commit message)
The CQ bit was checked by smaier@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2123753005/diff/120001/android_webview/tools/... File android_webview/tools/system_webview_shell/BUILD.gn (right): https://codereview.chromium.org/2123753005/diff/120001/android_webview/tools/... android_webview/tools/system_webview_shell/BUILD.gn:68: if (!enable_all_proguard_optimizations) { On 2016/07/08 14:26:20, agrieve wrote: > nit: combine with previous if. Done. https://codereview.chromium.org/2123753005/diff/120001/build/config/android/c... File build/config/android/config.gni (right): https://codereview.chromium.org/2123753005/diff/120001/build/config/android/c... build/config/android/config.gni:121: # enabled for instrumentation tests, as they break the tests. On 2016/07/08 14:26:20, agrieve wrote: > nit: change ", as they break the tests" -> "since they cause code required by > the tests to be removed" Done.
The CQ bit was checked by smaier@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Creating separate .apk for instrumentation tests in release. If we wish to turn on optimizations for our main code base, we need to first make sure that these optimizations don't break our instrumentation tests when inlining functions. To do this, we make a second, similiar .apk which has another proguard flags file which we will use to avoid optimizations which could damage our tests. BUG=625704 ========== to ========== Creating new gn arg: enable_all_proguard_optimizations. If we wish to turn on optimizations for our main code base, we need to first make sure that these optimizations don't break our instrumentation tests when inlining functions. To do this, we make a gn arg which will guard our instrumentation tests and only allow them to run without this flag. This flag will then be the thing that turns on these Proguard optimizations. BUG=625704 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by smaier@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2123753005/diff/180001/chrome/android/BUILD.gn File chrome/android/BUILD.gn (right): https://codereview.chromium.org/2123753005/diff/180001/chrome/android/BUILD.g... chrome/android/BUILD.gn:555: if (!enable_all_proguard_optimizations) { since you made the rest of the change global (i.e in the templates), could you assert in instrumentation_test_apk that this flag isn't set and output a helpful message?
https://codereview.chromium.org/2123753005/diff/180001/chrome/android/BUILD.gn File chrome/android/BUILD.gn (right): https://codereview.chromium.org/2123753005/diff/180001/chrome/android/BUILD.g... chrome/android/BUILD.gn:555: if (!enable_all_proguard_optimizations) { On 2016/07/12 03:40:04, Yaron wrote: > since you made the rest of the change global (i.e in the templates), could you > assert in instrumentation_test_apk that this flag isn't set and output a helpful > message? That's the plan, but we want to ensure that we land this CL and a matching downstream one first to avoid breaking the build.
ok lgtm
The CQ bit was checked by smaier@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from agrieve@chromium.org Link to the patchset: https://codereview.chromium.org/2123753005/#ps180001 (title: "Preventing any regression if enable_all_proguard_optimizations is never turned on")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
smaier@chromium.org changed reviewers: + mef@chromium.org
mef@chromium.org: Please review components/cronet/android/BUILD.gn
https://codereview.chromium.org/2123753005/diff/180001/components/cronet/andr... File components/cronet/android/BUILD.gn (right): https://codereview.chromium.org/2123753005/diff/180001/components/cronet/andr... components/cronet/android/BUILD.gn:363: proguard_enabled = true Should proguard_enabled be conditioned on enable_all_proguard_optimizations here? https://codereview.chromium.org/2123753005/diff/180001/components/cronet/andr... components/cronet/android/BUILD.gn:381: instrumentation_test_apk("cronet_sample_test_apk") { So, if enable_all_proguard_optimizations is true, then cronet_sample_test_apk will not build. This seems wrong, am I missing something? https://codereview.chromium.org/2123753005/diff/180001/components/cronet/andr... components/cronet/android/BUILD.gn:402: proguard_enabled = !is_java_debug Should proguard_enabled be conditioned on enable_all_proguard_optimizations here?
https://codereview.chromium.org/2123753005/diff/180001/components/cronet/andr... File components/cronet/android/BUILD.gn (right): https://codereview.chromium.org/2123753005/diff/180001/components/cronet/andr... components/cronet/android/BUILD.gn:363: proguard_enabled = true On 2016/07/13 15:12:07, mef wrote: > Should proguard_enabled be conditioned on enable_all_proguard_optimizations > here? No - we can still proguard without all proguard optimizations on. enable_all_proguard_optimizations is a flag to set when we want to squeeze every last byte out of our .apk - it will typically be only for release configs. https://codereview.chromium.org/2123753005/diff/180001/components/cronet/andr... components/cronet/android/BUILD.gn:381: instrumentation_test_apk("cronet_sample_test_apk") { On 2016/07/13 15:12:07, mef wrote: > So, if enable_all_proguard_optimizations is true, then cronet_sample_test_apk > will not build. > > This seems wrong, am I missing something? You have this correct - again, we typically only have this on is_official_build=true. https://codereview.chromium.org/2123753005/diff/180001/components/cronet/andr... components/cronet/android/BUILD.gn:402: proguard_enabled = !is_java_debug On 2016/07/13 15:12:07, mef wrote: > Should proguard_enabled be conditioned on enable_all_proguard_optimizations > here? Added an assert in config.gni to ensure sanity here.
The CQ bit was checked by smaier@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2123753005/diff/180001/components/cronet/andr... File components/cronet/android/BUILD.gn (right): https://codereview.chromium.org/2123753005/diff/180001/components/cronet/andr... components/cronet/android/BUILD.gn:381: instrumentation_test_apk("cronet_sample_test_apk") { On 2016/07/13 15:31:16, smaier wrote: > On 2016/07/13 15:12:07, mef wrote: > > So, if enable_all_proguard_optimizations is true, then cronet_sample_test_apk > > will not build. > > > > This seems wrong, am I missing something? > > You have this correct - again, we typically only have this on > is_official_build=true. Just to clarify - the main purpose of cronet_sample_test_apk is to ensure that fully optimized/proguarded Cronet in CronetSample is functional despite obfuscations and removals. Disabling this test when proguard optimizations are enabled defeats this purpose.
The CQ bit was checked by smaier@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2123753005/diff/180001/components/cronet/andr... File components/cronet/android/BUILD.gn (right): https://codereview.chromium.org/2123753005/diff/180001/components/cronet/andr... components/cronet/android/BUILD.gn:381: instrumentation_test_apk("cronet_sample_test_apk") { On 2016/07/13 16:49:40, mef wrote: > On 2016/07/13 15:31:16, smaier wrote: > > On 2016/07/13 15:12:07, mef wrote: > > > So, if enable_all_proguard_optimizations is true, then > cronet_sample_test_apk > > > will not build. > > > > > > This seems wrong, am I missing something? > > > > You have this correct - again, we typically only have this on > > is_official_build=true. > > Just to clarify - the main purpose of cronet_sample_test_apk is to ensure that > fully optimized/proguarded Cronet in CronetSample is functional despite > obfuscations and removals. > > Disabling this test when proguard optimizations are enabled defeats this > purpose. Agreed - this helped us discover an issue with when we were distributing our "test specific" flags. We won't be changing any Cronet code for this change, as I believe is the more correct thing to do in this case.
On 2016/07/13 18:13:31, smaier wrote: > https://codereview.chromium.org/2123753005/diff/180001/components/cronet/andr... > File components/cronet/android/BUILD.gn (right): > > https://codereview.chromium.org/2123753005/diff/180001/components/cronet/andr... > components/cronet/android/BUILD.gn:381: > instrumentation_test_apk("cronet_sample_test_apk") { > On 2016/07/13 16:49:40, mef wrote: > > On 2016/07/13 15:31:16, smaier wrote: > > > On 2016/07/13 15:12:07, mef wrote: > > > > So, if enable_all_proguard_optimizations is true, then > > cronet_sample_test_apk > > > > will not build. > > > > > > > > This seems wrong, am I missing something? > > > > > > You have this correct - again, we typically only have this on > > > is_official_build=true. > > > > Just to clarify - the main purpose of cronet_sample_test_apk is to ensure that > > fully optimized/proguarded Cronet in CronetSample is functional despite > > obfuscations and removals. > > > > Disabling this test when proguard optimizations are enabled defeats this > > purpose. > > Agreed - this helped us discover an issue with when we were distributing our > "test specific" flags. We won't be changing any Cronet code for this change, as > I believe is the more correct thing to do in this case. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by smaier@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
The CQ bit was checked by smaier@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by smaier@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yfriedman@chromium.org, agrieve@chromium.org Link to the patchset: https://codereview.chromium.org/2123753005/#ps260001 (title: "Rebased again")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Creating new gn arg: enable_all_proguard_optimizations. If we wish to turn on optimizations for our main code base, we need to first make sure that these optimizations don't break our instrumentation tests when inlining functions. To do this, we make a gn arg which will guard our instrumentation tests and only allow them to run without this flag. This flag will then be the thing that turns on these Proguard optimizations. BUG=625704 ========== to ========== Creating new gn arg: enable_all_proguard_optimizations. If we wish to turn on optimizations for our main code base, we need to first make sure that these optimizations don't break our instrumentation tests when inlining functions. To do this, we make a gn arg which will guard our instrumentation tests and only allow them to run without this flag. This flag will then be the thing that turns on these Proguard optimizations. BUG=625704 ==========
Message was sent while issue was closed.
Committed patchset #14 (id:260001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Creating new gn arg: enable_all_proguard_optimizations. If we wish to turn on optimizations for our main code base, we need to first make sure that these optimizations don't break our instrumentation tests when inlining functions. To do this, we make a gn arg which will guard our instrumentation tests and only allow them to run without this flag. This flag will then be the thing that turns on these Proguard optimizations. BUG=625704 ========== to ========== Creating new gn arg: enable_all_proguard_optimizations. If we wish to turn on optimizations for our main code base, we need to first make sure that these optimizations don't break our instrumentation tests when inlining functions. To do this, we make a gn arg which will guard our instrumentation tests and only allow them to run without this flag. This flag will then be the thing that turns on these Proguard optimizations. BUG=625704 Committed: https://crrev.com/05c799ee764fef9d63b4e45c4fba22145f96fed3 Cr-Commit-Position: refs/heads/master@{#405509} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/05c799ee764fef9d63b4e45c4fba22145f96fed3 Cr-Commit-Position: refs/heads/master@{#405509} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
