Support multiple proguard flags from java_apk.gypi. Proguard supports multiple proguard configurati…

Support multiple proguard flags from java_apk.gypi. Proguard supports multiple proguard configuration files but java_apk and apk-build.xml supports only one proguard configuration.

BUG=181470
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=191806

[Jungshik Jang, 2013-03-25 08:09:27]

Hi all. 

I'd like you to do code review on this.
This is to support multiple proguard configurations in java_apk build.
Please let me know if you have any questions. 

Thanks.

[ycheo (away), 2013-03-25 12:32:23]

lgtm

[Yaron, 2013-03-27 23:56:31]

-me, +cjhopman

[cjhopman, 2013-03-28 00:08:12]

Looks like you lost the change to the ant script (the relevant part has moved to
build/android/ant/apk-obfuscate.xml).

Also, how would you feel about making a simple .apk target to test that proguard
works? We don't currently have any coverage in chromium to protect this logic.

[jayjang, 2013-03-28 01:12:54]

On 2013/03/28 00:08:12, cjhopman wrote:
> Looks like you lost the change to the ant script (the relevant part has moved
to
> build/android/ant/apk-obfuscate.xml).
> 
> Also, how would you feel about making a simple .apk target to test that
proguard
> works? We don't currently have any coverage in chromium to protect this logic.
Thanks for the review. 
I found out that I don't need to touch ant configuration because of gyp change. 
Let me add test apk and ping you once updated it.

[Jungshik Jang, 2013-03-29 12:49:15]

Hi Chris. 

I added dummy package which tests multiple proguard files.
Also, I revisited ant-obfuscate.xml file because path element can handle well
for two or more files given as input. 

Please take another look at it. 
Thanks.

[cjhopman, 2013-03-29 14:43:46]

Adding ilevy@ to approve the new target for android_builder_tests

This is good, just a few points to clean up the apk target.

https://codereview.chromium.org/12913017/diff/13001/build/android/tests/multi...
File build/android/tests/multiple_proguards/multiple_proguards.gyp (right):

https://codereview.chromium.org/12913017/diff/13001/build/android/tests/multi...
build/android/tests/multiple_proguards/multiple_proguards.gyp:15:
'app_manifest_version_code%': '<(android_app_version_code)',
You shouldn't need the app_manifest_version* variables here, the defaults should
be fine.

https://codereview.chromium.org/12913017/diff/13001/build/android/tests/multi...
build/android/tests/multiple_proguards/multiple_proguards.gyp:16: 'java_in_dir':
'../multiple_proguards',
change this to:
'java_in_dir': 'src',

https://codereview.chromium.org/12913017/diff/13001/build/android/tests/multi...
build/android/tests/multiple_proguards/multiple_proguards.gyp:19:
'<(DEPTH)/build/android/tests/multiple_proguards/proguard1.flags',
These paths can be relative to this directory, i.e:
'proguard1.flags',
'proguard2.flags',

https://codereview.chromium.org/12913017/diff/13001/build/android/tests/multi...
build/android/tests/multiple_proguards/multiple_proguards.gyp:23:
'R_package_relpath': 'dummy',
I don't think you need these R_package* variables defined here (it should work
without them, but I'm not certain).

[jayjang, 2013-03-29 15:05:19]

Thanks Chris for your review. 

I applied your comments to the change. 
Please review it again

Thanks.

https://codereview.chromium.org/12913017/diff/13001/build/android/tests/multi...
File build/android/tests/multiple_proguards/multiple_proguards.gyp (right):

https://codereview.chromium.org/12913017/diff/13001/build/android/tests/multi...
build/android/tests/multiple_proguards/multiple_proguards.gyp:15:
'app_manifest_version_code%': '<(android_app_version_code)',
On 2013/03/29 14:43:46, cjhopman wrote:
> You shouldn't need the app_manifest_version* variables here, the defaults
should
> be fine.

Done.

https://codereview.chromium.org/12913017/diff/13001/build/android/tests/multi...
build/android/tests/multiple_proguards/multiple_proguards.gyp:16: 'java_in_dir':
'../multiple_proguards',
On 2013/03/29 14:43:46, cjhopman wrote:
> change this to:
> 'java_in_dir': 'src',
it couldn't be 'src'. In fact it searches manifest file from java_in_dir. it
should be '.'

https://codereview.chromium.org/12913017/diff/13001/build/android/tests/multi...
build/android/tests/multiple_proguards/multiple_proguards.gyp:19:
'<(DEPTH)/build/android/tests/multiple_proguards/proguard1.flags',
On 2013/03/29 14:43:46, cjhopman wrote:
> These paths can be relative to this directory, i.e:
> 'proguard1.flags',
> 'proguard2.flags',

Done.

https://codereview.chromium.org/12913017/diff/13001/build/android/tests/multi...
build/android/tests/multiple_proguards/multiple_proguards.gyp:23:
'R_package_relpath': 'dummy',
On 2013/03/29 14:43:46, cjhopman wrote:
> I don't think you need these R_package* variables defined here (it should work
> without them, but I'm not certain).

Unfortunately it's required because some variables like additional_R_text_files
in java_apk.gypi created when R_package name specified

[Jungshik Jang, 2013-04-01 07:18:45]

On 2013/03/29 15:05:19, jayjang wrote:
> Thanks Chris for your review. 
> 
> I applied your comments to the change. 
> Please review it again
> 
> Thanks.
> 
>
https://codereview.chromium.org/12913017/diff/13001/build/android/tests/multi...
> File build/android/tests/multiple_proguards/multiple_proguards.gyp (right):
> 
>
https://codereview.chromium.org/12913017/diff/13001/build/android/tests/multi...
> build/android/tests/multiple_proguards/multiple_proguards.gyp:15:
> 'app_manifest_version_code%': '<(android_app_version_code)',
> On 2013/03/29 14:43:46, cjhopman wrote:
> > You shouldn't need the app_manifest_version* variables here, the defaults
> should
> > be fine.
> 
> Done.
> 
>
https://codereview.chromium.org/12913017/diff/13001/build/android/tests/multi...
> build/android/tests/multiple_proguards/multiple_proguards.gyp:16:
'java_in_dir':
> '../multiple_proguards',
> On 2013/03/29 14:43:46, cjhopman wrote:
> > change this to:
> > 'java_in_dir': 'src',
> it couldn't be 'src'. In fact it searches manifest file from java_in_dir. it
> should be '.'
> 
>
https://codereview.chromium.org/12913017/diff/13001/build/android/tests/multi...
> build/android/tests/multiple_proguards/multiple_proguards.gyp:19:
> '<(DEPTH)/build/android/tests/multiple_proguards/proguard1.flags',
> On 2013/03/29 14:43:46, cjhopman wrote:
> > These paths can be relative to this directory, i.e:
> > 'proguard1.flags',
> > 'proguard2.flags',
> 
> Done.
> 
>
https://codereview.chromium.org/12913017/diff/13001/build/android/tests/multi...
> build/android/tests/multiple_proguards/multiple_proguards.gyp:23:
> 'R_package_relpath': 'dummy',
> On 2013/03/29 14:43:46, cjhopman wrote:
> > I don't think you need these R_package* variables defined here (it should
work
> > without them, but I'm not certain).
> 
> Unfortunately it's required because some variables like
additional_R_text_files
> in java_apk.gypi created when R_package name specified

Hi ilevy
Could you take a look at this change?
There are pending cls depending on this.

[cjhopman, 2013-04-01 20:33:58]

lgtm

[Jungshik Jang, 2013-04-02 06:03:09]

On 2013/04/01 20:33:58, cjhopman wrote:
> lgtm

Thanks

[commit-bot: I haz the power, 2013-04-02 06:03:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jayjang@chromium.org/12913017/18001

[commit-bot: I haz the power, 2013-04-02 06:03:48]

Failed to apply patch for build/java_apk.gypi:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file build/java_apk.gypi
  Hunk #2 succeeded at 346 (offset 78 lines).
  Hunk #3 succeeded at 367 (offset 78 lines).
  Hunk #4 FAILED at 308.
  1 out of 4 hunks FAILED -- saving rejects to file build/java_apk.gypi.rej

Patch:       build/java_apk.gypi
Index: build/java_apk.gypi
diff --git a/build/java_apk.gypi b/build/java_apk.gypi
index
bfe8d9c1c3f39008ef7ba0a0199632104776a658..cfd136824efb190119646794a5ff2b87dd59f9c9
100644
--- a/build/java_apk.gypi
+++ b/build/java_apk.gypi
@@ -59,7 +59,7 @@
     'app_manifest_version_name%': '<(android_app_version_name)',
     'app_manifest_version_code%': '<(android_app_version_code)',
     'proguard_enabled%': 'false',
-    'proguard_flags_path%': '<(DEPTH)/build/android/empty_proguard.flags',
+    'proguard_flags_paths%': ['<(DEPTH)/build/android/empty_proguard.flags'],
     'native_libs_paths': [],
     'jar_name': 'chromium_apk_<(_target_name).jar',
     'resource_dir%':'<(DEPTH)/build/android/ant/empty/res',
@@ -268,7 +268,7 @@
         '<(DEPTH)/build/android/ant/apk-obfuscate.xml',
         '<(DEPTH)/build/android/ant/create-test-jar.js',
         '<(compile_stamp)',
-        '<(proguard_flags_path)',
+        '>@(proguard_flags_paths)',
       ],
       'outputs': [
         '<(obfuscate_stamp)',
@@ -289,7 +289,7 @@
         '-DOBFUSCATED_JAR_PATH=<(obfuscated_jar_path)',
         '-DOUT_DIR=<(intermediate_dir)',
         '-DPROGUARD_ENABLED=<(proguard_enabled)',
-        '-DPROGUARD_FLAGS=<(proguard_flags_path)',
+        '-DPROGUARD_FLAGS=<(proguard_flags_paths)',
         '-DTEST_JAR_PATH=<(PRODUCT_DIR)/test.lib.java/<(apk_name).jar',
 
         '-DSTAMP=<(obfuscate_stamp)',
@@ -308,7 +308,7 @@
       'message': 'Dexing <(_target_name) jar',
       'variables': {
         'conditions': [
-          ['proguard_enabled==1', {
+          ['proguard_enabled=="true" and CONFIGURATION_NAME=="Release"', {
             'dex_inputs': [ '<(obfuscated_jar_path)' ],
             'dex_generated_inputs': [],
           }, {

[commit-bot: I haz the power, 2013-04-02 06:28:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jayjang@chromium.org/12913017/26002

[commit-bot: I haz the power, 2013-04-02 09:10:55]

Change committed as 191806

[kjyoun, 2013-04-02 10:11:31]

On 2013/04/02 09:10:55, I haz the power (commit-bot) wrote:
> Change committed as 191806

This CL was reverted due to build breakage on chrome.
Upload new CL to fix build breakage on chrome.
https://codereview.chromium.org/13470004

[kjyoun, 2013-04-02 10:16:00]

Ignore my last update.
(I put the comment on the wrong CL)

On 2013/04/02 10:11:31, kjyoun wrote:
> On 2013/04/02 09:10:55, I haz the power (commit-bot) wrote:
> > Change committed as 191806
> 
> This CL was reverted due to build breakage on chrome.
> Upload new CL to fix build breakage on chrome.
> https://codereview.chromium.org/13470004