Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1103013002:
Refactor proguard scripts (Closed)
|
Created:
5 years, 8 months ago by cjhopman Modified:
5 years, 7 months ago CC:
chromium-reviews, klundberg+watch_chromium.org, yfriedman+watch_chromium.org, jbudorick+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRefactor proguard scripts
Currently, we use proguard from two places: proguard.py and apk_obfuscate.py.
These are used to preprocess libraries and to process full apks, respectively.
This extracts construction/running/filtering output of the actual proguard
command to a simple builder class that is then used in both places. This makes
some parts of how proguard is run to be more consistent between the two.
proguard.py now supports running proguard in the way needed for apks. (GN will
be using proguard.py instead of apk_obfuscate.py)
BUG=359249, 478319
Committed: https://crrev.com/e852f8d50a4c65f0de6fcc3753abdc8ba3b22a9e
Cr-Commit-Position: refs/heads/master@{#328439}
Patch Set 1 #
Total comments: 16
Patch Set 2 : #Patch Set 3 : #
Total comments: 5
Patch Set 4 : #
Messages
Total messages: 38 (14 generated)
cjhopman@chromium.org changed reviewers: + newt@chromium.org
newt: *
Please expand the commit message a bit. What's the motivation for this change? Clean-up, consistency, or enabling GN to work with proguard? All of the above? https://codereview.chromium.org/1103013002/diff/1/build/android/gyp/apk_obfus... File build/android/gyp/apk_obfuscate.py (right): https://codereview.chromium.org/1103013002/diff/1/build/android/gyp/apk_obfus... build/android/gyp/apk_obfuscate.py:89: options.tested_apk_obfuscated_jar_path != '/'): why would the path be "/"? https://codereview.chromium.org/1103013002/diff/1/build/android/gyp/apk_obfus... build/android/gyp/apk_obfuscate.py:98: proguard.testoptions(True) testoptions() sounds like it's going to take a list of options, but in fact it takes a boolean. How about calling it "is_test()" or something like that? https://codereview.chromium.org/1103013002/diff/1/build/android/gyp/proguard.py File build/android/gyp/proguard.py (right): https://codereview.chromium.org/1103013002/diff/1/build/android/gyp/proguard.... build/android/gyp/proguard.py:47: parser.add_option('--mapping', help='Path to proguard mapping to apply.') --mapping and --is-test aren't used anywhere. Is that intentional? https://codereview.chromium.org/1103013002/diff/1/build/android/gyp/proguard.... build/android/gyp/proguard.py:50: + 'added.') "+" isn't needed. https://codereview.chromium.org/1103013002/diff/1/build/android/gyp/util/prog... File build/android/gyp/util/proguard_util.py (right): https://codereview.chromium.org/1103013002/diff/1/build/android/gyp/util/prog... build/android/gyp/util/proguard_util.py:29: Please move the extraneous newline from java_apk.gpyi to here. https://codereview.chromium.org/1103013002/diff/1/build/android/gyp/util/prog... build/android/gyp/util/proguard_util.py:55: for f in paths: why f? why not p? https://codereview.chromium.org/1103013002/diff/1/build/android/gyp/util/prog... build/android/gyp/util/proguard_util.py:72: assert os.path.exists(self._proguard_jar_path) why not put this assertion in __init__? https://codereview.chromium.org/1103013002/diff/1/build/android/gyp/util/prog... build/android/gyp/util/proguard_util.py:78: '-forceprocessing', Previously, proguard.py wasn't using this argument; now it is. Is that change OK? Same goes for -dump, -printseeds, -printusage, and -printmapping
Patchset #2 (id:20001) has been deleted
I've expanded the description a bit. https://codereview.chromium.org/1103013002/diff/1/build/android/gyp/apk_obfus... File build/android/gyp/apk_obfuscate.py (right): https://codereview.chromium.org/1103013002/diff/1/build/android/gyp/apk_obfus... build/android/gyp/apk_obfuscate.py:89: options.tested_apk_obfuscated_jar_path != '/'): On 2015/04/28 04:22:14, newt wrote: > why would the path be "/"? Done. https://codereview.chromium.org/1103013002/diff/1/build/android/gyp/apk_obfus... build/android/gyp/apk_obfuscate.py:98: proguard.testoptions(True) On 2015/04/28 04:22:14, newt wrote: > testoptions() sounds like it's going to take a list of options, but in fact it > takes a boolean. How about calling it "is_test()" or something like that? Done. https://codereview.chromium.org/1103013002/diff/1/build/android/gyp/proguard.py File build/android/gyp/proguard.py (right): https://codereview.chromium.org/1103013002/diff/1/build/android/gyp/proguard.... build/android/gyp/proguard.py:47: parser.add_option('--mapping', help='Path to proguard mapping to apply.') On 2015/04/28 04:22:14, newt wrote: > --mapping and --is-test aren't used anywhere. Is that intentional? Yeah, they're only needed for apks. Gyp uses apk_obfuscate.py for those, but GN will use this. https://codereview.chromium.org/1103013002/diff/1/build/android/gyp/proguard.... build/android/gyp/proguard.py:50: + 'added.') On 2015/04/28 04:22:14, newt wrote: > "+" isn't needed. Done. https://codereview.chromium.org/1103013002/diff/1/build/android/gyp/util/prog... File build/android/gyp/util/proguard_util.py (right): https://codereview.chromium.org/1103013002/diff/1/build/android/gyp/util/prog... build/android/gyp/util/proguard_util.py:29: On 2015/04/28 04:22:14, newt wrote: > Please move the extraneous newline from java_apk.gpyi to here. Done. https://codereview.chromium.org/1103013002/diff/1/build/android/gyp/util/prog... build/android/gyp/util/proguard_util.py:55: for f in paths: On 2015/04/28 04:22:14, newt wrote: > why f? why not p? Done. https://codereview.chromium.org/1103013002/diff/1/build/android/gyp/util/prog... build/android/gyp/util/proguard_util.py:72: assert os.path.exists(self._proguard_jar_path) On 2015/04/28 04:22:14, newt wrote: > why not put this assertion in __init__? Done. https://codereview.chromium.org/1103013002/diff/1/build/android/gyp/util/prog... build/android/gyp/util/proguard_util.py:78: '-forceprocessing', On 2015/04/28 04:22:14, newt wrote: > Previously, proguard.py wasn't using this argument; now it is. Is that change > OK? > > Same goes for -dump, -printseeds, -printusage, and -printmapping Yeah, -forceprocessing just makes proguard skip its internal up-to-date check and the others are just extra output about what proguard did to the output.
lgtm, thanks for the expanded explanation
The CQ bit was checked by cjhopman@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1103013002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
cjhopman@chromium.org changed reviewers: + dtu@chromium.org
dtu: for TELEMETRY_DEPS changes
https://codereview.chromium.org/1103013002/diff/60001/tools/telemetry/telemet... File tools/telemetry/telemetry/TELEMETRY_DEPS (right): https://codereview.chromium.org/1103013002/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/TELEMETRY_DEPS:21: "build/android/AndroidManifest.xml", Really, I think we should just whitelist build/android but most of my changes will be withing build/android/gyp so I don't care as much about the bits outside there.
aiolos@chromium.org changed reviewers: + aiolos@chromium.org
https://codereview.chromium.org/1103013002/diff/60001/tools/telemetry/telemet... File tools/telemetry/telemetry/TELEMETRY_DEPS (right): https://codereview.chromium.org/1103013002/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/TELEMETRY_DEPS:21: "build/android/AndroidManifest.xml", On 2015/05/04 20:38:05, cjhopman wrote: > Really, I think we should just whitelist build/android but most of my changes > will be withing build/android/gyp so I don't care as much about the bits outside > there. We are actually in the process of moving telemetry out of the chromium repo, and are trying to reduce/remove the build/android dependancies. I would prefer to keep this the way it is to help track the removed dependancies unless there is a really good reason to change it.
https://codereview.chromium.org/1103013002/diff/60001/tools/telemetry/telemet... File tools/telemetry/telemetry/TELEMETRY_DEPS (right): https://codereview.chromium.org/1103013002/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/TELEMETRY_DEPS:21: "build/android/AndroidManifest.xml", On 2015/05/04 20:59:58, aiolos wrote: > On 2015/05/04 20:38:05, cjhopman wrote: > > Really, I think we should just whitelist build/android but most of my changes > > will be withing build/android/gyp so I don't care as much about the bits > outside > > there. > > We are actually in the process of moving telemetry out of the chromium repo, and > are trying to reduce/remove the build/android dependancies. I would prefer to > keep this the way it is to help track the removed dependancies unless there is a > really good reason to change it. What's the timeline for that? Having to update this whitelist for pretty trivial changes is kind of annoying.
https://codereview.chromium.org/1103013002/diff/60001/tools/telemetry/telemet... File tools/telemetry/telemetry/TELEMETRY_DEPS (right): https://codereview.chromium.org/1103013002/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/TELEMETRY_DEPS:21: "build/android/AndroidManifest.xml", On 2015/05/04 21:14:31, cjhopman wrote: > On 2015/05/04 20:59:58, aiolos wrote: > > On 2015/05/04 20:38:05, cjhopman wrote: > > > Really, I think we should just whitelist build/android but most of my > changes > > > will be withing build/android/gyp so I don't care as much about the bits > > outside > > > there. > > > > We are actually in the process of moving telemetry out of the chromium repo, > and > > are trying to reduce/remove the build/android dependancies. I would prefer to > > keep this the way it is to help track the removed dependancies unless there is > a > > really good reason to change it. > > What's the timeline for that? Having to update this whitelist for pretty trivial > changes is kind of annoying. This month.
On 2015/05/04 21:15:23, aiolos wrote: > https://codereview.chromium.org/1103013002/diff/60001/tools/telemetry/telemet... > File tools/telemetry/telemetry/TELEMETRY_DEPS (right): > > https://codereview.chromium.org/1103013002/diff/60001/tools/telemetry/telemet... > tools/telemetry/telemetry/TELEMETRY_DEPS:21: > "build/android/AndroidManifest.xml", > On 2015/05/04 21:14:31, cjhopman wrote: > > On 2015/05/04 20:59:58, aiolos wrote: > > > On 2015/05/04 20:38:05, cjhopman wrote: > > > > Really, I think we should just whitelist build/android but most of my > > changes > > > > will be withing build/android/gyp so I don't care as much about the bits > > > outside > > > > there. > > > > > > We are actually in the process of moving telemetry out of the chromium repo, > > and > > > are trying to reduce/remove the build/android dependancies. I would prefer > to > > > keep this the way it is to help track the removed dependancies unless there > is > > a > > > really good reason to change it. > > > > What's the timeline for that? Having to update this whitelist for pretty > trivial > > changes is kind of annoying. > > This month. Sounds reasonable. I'll change this to just whitelist the one new file then.
https://codereview.chromium.org/1103013002/diff/60001/tools/telemetry/telemet... File tools/telemetry/telemetry/TELEMETRY_DEPS (right): https://codereview.chromium.org/1103013002/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/TELEMETRY_DEPS:21: "build/android/AndroidManifest.xml", On 2015/05/04 21:15:23, aiolos wrote: > On 2015/05/04 21:14:31, cjhopman wrote: > > On 2015/05/04 20:59:58, aiolos wrote: > > > On 2015/05/04 20:38:05, cjhopman wrote: > > > > Really, I think we should just whitelist build/android but most of my > > changes > > > > will be withing build/android/gyp so I don't care as much about the bits > > > outside > > > > there. > > > > > > We are actually in the process of moving telemetry out of the chromium repo, > > and > > > are trying to reduce/remove the build/android dependancies. I would prefer > to > > > keep this the way it is to help track the removed dependancies unless there > is > > a > > > really good reason to change it. > > > > What's the timeline for that? Having to update this whitelist for pretty > trivial > > changes is kind of annoying. > > This month. Done.
The CQ bit was checked by cjhopman@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from newt@chromium.org Link to the patchset: https://codereview.chromium.org/1103013002/#ps80001 (title: " ")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1103013002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
On 2015/05/04 22:55:03, I haz the power (commit-bot) wrote: > Dry run: Try jobs failed on following builders: > linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...) lgtm
The CQ bit was checked by cjhopman@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1103013002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by cjhopman@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1103013002/80001
The CQ bit was unchecked by cjhopman@chromium.org
dtu: still need OWNERS for tools/telemetry
lgtm !
The CQ bit was checked by cjhopman@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1103013002/80001
Message was sent while issue was closed.
Committed patchset #4 (id:80001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/e852f8d50a4c65f0de6fcc3753abdc8ba3b22a9e Cr-Commit-Position: refs/heads/master@{#328439} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
