Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2827793005:
Get Proguard flags from GMS clients (upstream part) (Closed)
|
Created:
3 years, 8 months ago by paulmiller Modified:
3 years, 8 months ago CC:
chromium-reviews, mikecase+watch_chromium.org, jbudorick+watch_chromium.org, agrieve+watch_chromium.org, dgn, Maria Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionGet Proguard flags from GMS clients (upstream part)
Update the GMS preprocessing script to enumerate the proguard files that
come with the client libs, and put them in a generated .gni file. Use
the generated file to pull in the recommended flags, and remove them
from apk_proguard.flags. This will preclude some human error in the GMS
roll process.
In v9, each GMS client has its own proguard.txt file, with common flags
duplicated accross clients. In v10, most clients don't have their own
proguard.txt, with common flags only in the "basement" client.
BUG=708349
Review-Url: https://codereview.chromium.org/2827793005
Cr-Commit-Position: refs/heads/master@{#465814}
Committed: https://chromium.googlesource.com/chromium/src/+/3278d07f94c85839bac14d4f9495056c82977b56
Patch Set 1 #
Total comments: 4
Patch Set 2 : . #
Total comments: 2
Patch Set 3 : constant copyright year #
Total comments: 7
Patch Set 4 : absolute GN paths #Messages
Total messages: 22 (8 generated)
Description was changed from ========== Get Proguard flags from GMS clients (upstream part) Update the GMS preprocessing script to enumerate the proguard files that come with the client libs, and put them in a generated .gni file. Remove the manually curated apk_proguard.flags file, and instead use the generated .gni to pull in the Proguard flags. This will preclude some human error in the GMS roll process. In v9, each GMS client has its own proguard.txt file, with common flags duplicated accross clients. In v10, most clients don't have their own proguard.txt, with common flags only in the "basement" client. BUG=708349 ========== to ========== Get Proguard flags from GMS clients (upstream part) Update the GMS preprocessing script to enumerate the proguard files that come with the client libs, and put them in a generated .gni file. Remove the manually curated apk_proguard.flags file, and instead use the generated .gni to pull in the Proguard flags. This will preclude some human error in the GMS roll process. In v9, each GMS client has its own proguard.txt file, with common flags duplicated accross clients. In v10, most clients don't have their own proguard.txt, with common flags only in the "basement" client. BUG=708349 ==========
paulmiller@chromium.org changed reviewers: + jbudorick@chromium.org
On 2017/04/18 23:38:49, paulmiller wrote: > mailto:paulmiller@chromium.org changed reviewers: > + mailto:jbudorick@chromium.org PTAL
https://codereview.chromium.org/2827793005/diff/1/build/android/play_services... File build/android/play_services/preprocess.py (right): https://codereview.chromium.org/2827793005/diff/1/build/android/play_services... build/android/play_services/preprocess.py:203: gni_text += 'gms_proguard_configs = [\n' Build the (sorted) list of extant proguard files, then just use something like gni_text += 'gms_proguard_configs = %s' % pprint.pformat(gms_proguard_configs) The resulting .gni will need to be formatted, but I'd rather than be the case w/ less to maintain here.
mariakhomenko@chromium.org changed reviewers: + mariakhomenko@chromium.org
lgtm https://codereview.chromium.org/2827793005/diff/1/build/android/play_services... File build/android/play_services/preprocess.py (right): https://codereview.chromium.org/2827793005/diff/1/build/android/play_services... build/android/play_services/preprocess.py:202: gni_text = '# This file generated by ' + script_name + '\n' Copyright should go here I guess.
https://codereview.chromium.org/2827793005/diff/1/build/android/play_services... File build/android/play_services/preprocess.py (right): https://codereview.chromium.org/2827793005/diff/1/build/android/play_services... build/android/play_services/preprocess.py:203: gni_text += 'gms_proguard_configs = [\n' On 2017/04/18 23:59:12, jbudorick wrote: > Build the (sorted) list of extant proguard files, then just use something like > > gni_text += 'gms_proguard_configs = %s' % pprint.pformat(gms_proguard_configs) > > The resulting .gni will need to be formatted, but I'd rather than be the case w/ > less to maintain here. It looks like pformat emits single quotes. Not sure if GN will accept that.
How's this? (patch set 2) I wasn't sure how to make pprint work, but if we're worried about maintaining the code generation, I figure the template style is clearer.
https://codereview.chromium.org/2827793005/diff/20001/build/android/play_serv... File build/android/play_services/preprocess.py (right): https://codereview.chromium.org/2827793005/diff/20001/build/android/play_serv... build/android/play_services/preprocess.py:199: # Copyright {year} The Chromium Authors. All rights reserved. not sure that year should be computed. Generally copyright stays at the year the file was first added?
https://codereview.chromium.org/2827793005/diff/20001/build/android/play_serv... File build/android/play_services/preprocess.py (right): https://codereview.chromium.org/2827793005/diff/20001/build/android/play_serv... build/android/play_services/preprocess.py:199: # Copyright {year} The Chromium Authors. All rights reserved. On 2017/04/19 18:20:22, Maria wrote: > not sure that year should be computed. Generally copyright stays at the year the > file was first added? Done.
https://codereview.chromium.org/2827793005/diff/1/build/android/play_services... File build/android/play_services/preprocess.py (right): https://codereview.chromium.org/2827793005/diff/1/build/android/play_services... build/android/play_services/preprocess.py:203: gni_text += 'gms_proguard_configs = [\n' On 2017/04/19 17:30:13, paulmiller wrote: > On 2017/04/18 23:59:12, jbudorick wrote: > > Build the (sorted) list of extant proguard files, then just use something like > > > > gni_text += 'gms_proguard_configs = %s' % > pprint.pformat(gms_proguard_configs) > > > > The resulting .gni will need to be formatted, but I'd rather than be the case > w/ > > less to maintain here. > > It looks like pformat emits single quotes. Not sure if GN will accept that. Ah, yeah, you're right. https://codereview.chromium.org/2827793005/diff/40001/build/android/play_serv... File build/android/play_services/preprocess.py (right): https://codereview.chromium.org/2827793005/diff/40001/build/android/play_serv... build/android/play_services/preprocess.py:197: gni_template = '''\ nits: _GNI_TEMPLATE + use textwrap.dedent + indent the lines below Using a format string here is a nice change, though. https://codereview.chromium.org/2827793005/diff/40001/build/android/play_serv... build/android/play_services/preprocess.py:217: relative = os.path.relpath(proguard_path, gni_dir) I'm a bit concerned about the case in which out_dir isn't a descendent of gni_dir. GN style typically favors src/-rooted paths to relative paths that go up the directory tree (i.e., '//foo/bar/baz' vs '../bar/baz') https://codereview.chromium.org/2827793005/diff/40001/build/android/play_serv... build/android/play_services/preprocess.py:218: gni_lines.append(' "' + relative + '",') nit: instead of the string appends, do gni_lines.append(' "%s",' % relative)
https://codereview.chromium.org/2827793005/diff/40001/build/android/play_serv... File build/android/play_services/preprocess.py (right): https://codereview.chromium.org/2827793005/diff/40001/build/android/play_serv... build/android/play_services/preprocess.py:197: gni_template = '''\ On 2017/04/19 18:42:26, jbudorick wrote: > nits: _GNI_TEMPLATE + use textwrap.dedent + indent the lines below > > Using a format string here is a nice change, though. Thanks, I didn't know about dedent. But if I can indent it, then I might as well make it a local variable. Aside, it seems odd to use the ALL_CAPS style in a language that doesn't support constants. https://codereview.chromium.org/2827793005/diff/40001/build/android/play_serv... build/android/play_services/preprocess.py:217: relative = os.path.relpath(proguard_path, gni_dir) On 2017/04/19 18:42:26, jbudorick wrote: > I'm a bit concerned about the case in which out_dir isn't a descendent of > gni_dir. GN style typically favors src/-rooted paths to relative paths that go > up the directory tree (i.e., '//foo/bar/baz' vs '../bar/baz') I'll make the paths absolute, then. https://codereview.chromium.org/2827793005/diff/40001/build/android/play_serv... build/android/play_services/preprocess.py:218: gni_lines.append(' "' + relative + '",') On 2017/04/19 18:42:26, jbudorick wrote: > nit: instead of the string appends, do > gni_lines.append(' "%s",' % relative) Done.
https://codereview.chromium.org/2827793005/diff/40001/build/android/play_serv... File build/android/play_services/preprocess.py (right): https://codereview.chromium.org/2827793005/diff/40001/build/android/play_serv... build/android/play_services/preprocess.py:197: gni_template = '''\ On 2017/04/19 21:28:10, paulmiller wrote: > On 2017/04/19 18:42:26, jbudorick wrote: > > nits: _GNI_TEMPLATE + use textwrap.dedent + indent the lines below > > > > Using a format string here is a nice change, though. > > Thanks, I didn't know about dedent. But if I can indent it, then I might as well > make it a local variable. > > Aside, it seems odd to use the ALL_CAPS style in a language that doesn't support > constants. heh, it's at least a reminder to people working in the code that it is conceptually, if not logically, a constant.
Description was changed from ========== Get Proguard flags from GMS clients (upstream part) Update the GMS preprocessing script to enumerate the proguard files that come with the client libs, and put them in a generated .gni file. Remove the manually curated apk_proguard.flags file, and instead use the generated .gni to pull in the Proguard flags. This will preclude some human error in the GMS roll process. In v9, each GMS client has its own proguard.txt file, with common flags duplicated accross clients. In v10, most clients don't have their own proguard.txt, with common flags only in the "basement" client. BUG=708349 ========== to ========== Get Proguard flags from GMS clients (upstream part) Update the GMS preprocessing script to enumerate the proguard files that come with the client libs, and put them in a generated .gni file. Use the generated file to pull in the recommended flags, and remove them from apk_proguard.flags. This will preclude some human error in the GMS roll process. In v9, each GMS client has its own proguard.txt file, with common flags duplicated accross clients. In v10, most clients don't have their own proguard.txt, with common flags only in the "basement" client. BUG=708349 ==========
PTAL at #4
lgtm
The CQ bit was checked by paulmiller@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mariakhomenko@chromium.org Link to the patchset: https://codereview.chromium.org/2827793005/#ps60001 (title: "absolute GN paths")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1492641461908930,
"parent_rev": "f2a0b8e1353add967a367b5ce57b41ddbc1546e3", "commit_rev":
"3278d07f94c85839bac14d4f9495056c82977b56"}
Message was sent while issue was closed.
Description was changed from ========== Get Proguard flags from GMS clients (upstream part) Update the GMS preprocessing script to enumerate the proguard files that come with the client libs, and put them in a generated .gni file. Use the generated file to pull in the recommended flags, and remove them from apk_proguard.flags. This will preclude some human error in the GMS roll process. In v9, each GMS client has its own proguard.txt file, with common flags duplicated accross clients. In v10, most clients don't have their own proguard.txt, with common flags only in the "basement" client. BUG=708349 ========== to ========== Get Proguard flags from GMS clients (upstream part) Update the GMS preprocessing script to enumerate the proguard files that come with the client libs, and put them in a generated .gni file. Use the generated file to pull in the recommended flags, and remove them from apk_proguard.flags. This will preclude some human error in the GMS roll process. In v9, each GMS client has its own proguard.txt file, with common flags duplicated accross clients. In v10, most clients don't have their own proguard.txt, with common flags only in the "basement" client. BUG=708349 Review-Url: https://codereview.chromium.org/2827793005 Cr-Commit-Position: refs/heads/master@{#465814} Committed: https://chromium.googlesource.com/chromium/src/+/3278d07f94c85839bac14d4f9495... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/3278d07f94c85839bac14d4f9495... |
