Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1411913005:
Make Google API keys available for the Java codebase (Closed)
|
Created:
5 years, 2 months ago by dvh Modified:
5 years, 1 month 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. |
DescriptionMake Google API keys available for the Java codebase.
BUG=546038
Committed: https://crrev.com/310b3905a97a06b30514e0db0a46c7d8af4dcb60
Cr-Commit-Position: refs/heads/master@{#357774}
Patch Set 1 #Patch Set 2 : #
Total comments: 1
Patch Set 3 : #Patch Set 4 : #
Total comments: 5
Patch Set 5 : #Patch Set 6 : #Patch Set 7 : #
Total comments: 39
Patch Set 8 : #Patch Set 9 : #Patch Set 10 : #Patch Set 11 : #Patch Set 12 : #
Total comments: 8
Patch Set 13 : #Patch Set 14 : #
Total comments: 14
Patch Set 15 : #
Total comments: 2
Patch Set 16 : #
Messages
Total messages: 85 (31 generated)
dvh@chromium.org changed reviewers: + pasko@chromium.org
pasko@ Could you please take a look? Thanks!
I can rubberstamp after a committer knowledgeable in api keys can approve it. I would suggest to start with rogerta@. General comments: * The bug lacks details on API keys, and the commit description is minimal, this makes it hard to understand the direction of this specific change. Please create a bug that describes the big picture of generating API key constants in Chrome (why do we need the new mechanism? will existing clients migrate to it? any overhead associated?). Then please make the bug a blocker to your umbrella bug. Then please answer performance/overhead questions in the bug (the ones lizeb@ and I asked you in a private thread) * Please do *not* pollute the chrome src with autogenerated files, all generated stuff should go to the 'gen' directory in output (like out/Release/gen or out/Debug/gen). The GYP/GN define where to put the autogenerated files, and the generating scripts take the filename as a parameter. * Please add a unittest and make sure it runs either as a PRESUBMIT or as part of some existing suite. Feel free to ask me for details over various communication channels of your choice.
Description was changed from ========== Script to generate Google API keys constants for Java. BUG=529962 ========== to ========== Script to generate Google API keys constants for Java. BUG=546038 ==========
dvh@chromium.org changed reviewers: + thestig@chromium.org
thestig@chromium.org: Please review changes in chrome/*.gyp*
https://codereview.chromium.org/1411913005/diff/20001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/1411913005/diff/20001/chrome/chrome_browser.g... chrome/chrome_browser.gypi:4073: # TODO(dvh): write GN rule to generate Java Google API Keys Are GN bots going to be upset if you don't do this now?
The CQ bit was checked by dvh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1411913005/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1411913005/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
The CQ bit was checked by dvh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1411913005/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1411913005/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2015/10/22 20:40:41, Lei Zhang wrote: > https://codereview.chromium.org/1411913005/diff/20001/chrome/chrome_browser.gypi > File chrome/chrome_browser.gypi (right): > > https://codereview.chromium.org/1411913005/diff/20001/chrome/chrome_browser.g... > chrome/chrome_browser.gypi:4073: # TODO(dvh): write GN rule to generate Java > Google API Keys > Are GN bots going to be upset if you don't do this now? I'm trying to make the change for GN and build with GN but I got build issues unrelated to my changes. Is the GN supposed to be maintained on Android? See logs below. ~/Chromium/clank/src/chrome/android$ gn gen --args='target_os="android"' out_mobile_gn/Debug Done. Wrote 7617 targets from 748 files in 2674ms ~/Chromium/clank/src/chrome/android$ ninja -C out_mobile_gn/Debug chrome_apk ninja: Entering directory `out_mobile_gn/Debug' [2975/17567] ACTION //third_party/opus...sembler(//build/toolchain/android:arm) perl /usr/local/google/home/dvh/Chromium/clank/src/third_party/opus/src/celt/arm/arm2gnu.pl /usr/local/google/home/dvh/Chromium/clank/src/third_party/opus/src/celt/arm/celt_pitch_xcorr_arm.s | sed "s/OPUS_ARM_MAY_HAVE_[A-Z]*/1/g" | sed "/.include/d" > gen/third_party/opus/celt_pitch_xcorr_arm_gnu.S [6830/17567] ACTION //third_party/andr...__javac(//build/toolchain/android:arm) ../../../../third_party/android_data_chart/java/src/org/chromium/third_party/android/datausagechart/ChartDataUsageView.java:25: warning: [deprecation] Time in android.text.format has been deprecated import android.text.format.Time; ^ ../../../../third_party/android_data_chart/java/src/org/chromium/third_party/android/datausagechart/ChartDataUsageView.java:309: warning: [deprecation] Time in android.text.format has been deprecated final Time time = new Time(); ^ ../../../../third_party/android_data_chart/java/src/org/chromium/third_party/android/datausagechart/ChartDataUsageView.java:309: warning: [deprecation] Time in android.text.format has been deprecated final Time time = new Time(); ^ 3 warnings [6853/17567] ACTION //third_party/andr...__javac(//build/toolchain/android:arm) ../../../../third_party/android_protobuf/src/java/src/main/java/com/google/protobuf/nano/ExtendableMessageNano.java:165: warning: [unchecked] unchecked cast M cloned = (M) super.clone(); ^ required: M found: MessageNano where M is a type-variable: M extends ExtendableMessageNano<M> declared in class ExtendableMessageNano ../../../../third_party/android_protobuf/src/java/src/main/java/com/google/protobuf/nano/FieldData.java:89: warning: [unchecked] unchecked cast return (T) value; ^ required: T found: Object where T is a type-variable: T extends Object declared in method <T>getValue(Extension<?,T>) 2 warnings [8704/17567] ACTION //third_party/andr...__javac(//build/toolchain/android:arm) ../../../../third_party/android_swipe_refresh/java/src/org/chromium/third_party/android/swiperefresh/SwipeRefreshLayout.java:457: warning: [deprecation] getColor(int) in Resources has been deprecated setProgressBackgroundColorSchemeColor(getResources().getColor(colorRes)); ^ ../../../../third_party/android_swipe_refresh/java/src/org/chromium/third_party/android/swiperefresh/SwipeRefreshLayout.java:489: warning: [deprecation] getColor(int) in Resources has been deprecated colorRes[i] = res.getColor(colorResIds[i]); ^ ../../../../third_party/android_swipe_refresh/java/src/org/chromium/third_party/android/swiperefresh/CircleImageView.java:133: warning: [deprecation] getColor(int) in Resources has been deprecated setBackgroundColor(getContext().getResources().getColor(colorRes)); ^ 3 warnings [10880/17567] ACTION //content/public/...__javac(//build/toolchain/android:arm) ../../../../content/public/android/java/src/org/chromium/content/browser/input/SelectPopupDialog.java:48: warning: [deprecation] setInverseBackgroundForced(boolean) in Builder has been deprecated .setInverseBackgroundForced(true); ^ 1 warning [11580/17567] ACTION //sync/android:sy...__javac(//build/toolchain/android:arm) ../../../../sync/android/java/src/org/chromium/sync/signin/AccountManagerHelper.java:165: warning: [deprecation] getAccountsByType(String) in AccountManagerDelegate has been deprecated return mAccountManager.getAccountsByType(GOOGLE_ACCOUNT_TYPE); ^ ../../../../sync/android/java/src/org/chromium/sync/signin/SystemAccountManagerDelegate.java:45: warning: [deprecation] getAccountsByType(String) in AccountManagerDelegate has been deprecated public Account[] getAccountsByType(String type) { ^ 2 warnings ... + more errors
On 2015/10/26 19:45:42, dvh wrote: > On 2015/10/22 20:40:41, Lei Zhang wrote: > > > https://codereview.chromium.org/1411913005/diff/20001/chrome/chrome_browser.gypi > > File chrome/chrome_browser.gypi (right): > > > > > https://codereview.chromium.org/1411913005/diff/20001/chrome/chrome_browser.g... > > chrome/chrome_browser.gypi:4073: # TODO(dvh): write GN rule to generate Java > > Google API Keys > > Are GN bots going to be upset if you don't do this now? > > I'm trying to make the change for GN and build with GN but I got build issues > unrelated to my changes. > Is the GN supposed to be maintained on Android? See logs below. That's a good question. I don't know for sure. Someone asked this on the internal mailing list, and the reply was "The GN build still runs properly if you use Release as long as you do not set dcheck_always_on=true"
dvh@chromium.org changed reviewers: + scottmg@chromium.org
scottmg@chromium.org: Please review changes in Could you review the change in build/config/android/* Thanks!
Description was changed from ========== Script to generate Google API keys constants for Java. BUG=546038 ========== to ========== Make Google API keys available for the Java codebase. BUG=546038 ==========
https://codereview.chromium.org/1411913005/diff/60001/chrome/chrome.gyp File chrome/chrome.gyp (right): https://codereview.chromium.org/1411913005/diff/60001/chrome/chrome.gyp#newco... chrome/chrome.gyp:538: 'google_api_keys_java', nit: alphabetical order https://codereview.chromium.org/1411913005/diff/60001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/1411913005/diff/60001/chrome/chrome_browser.g... chrome/chrome_browser.gypi:4073: # TODO(dvh): write GN rule to generate Java Google API Keys remove now?
scottmg@chromium.org changed reviewers: + agrieve@chromium.org, brettw@chromium.org - scottmg@chromium.org
scottmg@chromium.org changed reviewers: + scottmg@chromium.org
Passing build/config/android/rules.gni to either brettw or agrieve.
https://codereview.chromium.org/1411913005/diff/60001/chrome/chrome.gyp File chrome/chrome.gyp (right): https://codereview.chromium.org/1411913005/diff/60001/chrome/chrome.gyp#newco... chrome/chrome.gyp:538: 'google_api_keys_java', On 2015/10/27 20:16:05, Lei Zhang wrote: > nit: alphabetical order Done. That said, the current alphabetical order is weird. https://codereview.chromium.org/1411913005/diff/60001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/1411913005/diff/60001/chrome/chrome_browser.g... chrome/chrome_browser.gypi:4073: # TODO(dvh): write GN rule to generate Java Google API Keys On 2015/10/27 20:16:05, Lei Zhang wrote: > remove now? Done.
On 2015/10/27 20:17:41, scottmg wrote: > Passing build/config/android/rules.gni to either brettw or agrieve. Since agrieve is OOO, brettw@, could you review those changes? Thanks!
https://codereview.chromium.org/1411913005/diff/60001/chrome/chrome.gyp File chrome/chrome.gyp (right): https://codereview.chromium.org/1411913005/diff/60001/chrome/chrome.gyp#newco... chrome/chrome.gyp:538: 'google_api_keys_java', On 2015/10/27 20:22:52, dvh wrote: > On 2015/10/27 20:16:05, Lei Zhang wrote: > > nit: alphabetical order > > Done. That said, the current alphabetical order is weird. Indeed! Feel free to sort it. I don't see a warning telling us not to.
On 2015/10/27 20:25:17, Lei Zhang wrote: > Indeed! Feel free to sort it. I don't see a warning telling us not to. Done.
The CQ bit was checked by dvh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1411913005/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1411913005/120001
Hi all, pasko@, could you take a look again at the changes in these files? build/android/gyp/java_google_api_keys.py build/android/gyp/java_google_api_keys_tests.py build/android/java_cpp_enum.gypi => java_google_api_keys.gypi build/android/pylib/constants/__init__.py thestig@, for these files? chrome/chrome.gyp chrome/chrome_browser.gypi and agrieve@ or brettw@ for these files? build/config/android/rules.gni chrome/android/BUILD.gn Thanks!
chrome/ lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1411913005/diff/120001/build/android/gyp/java... File build/android/gyp/java_google_api_keys.py (right): https://codereview.chromium.org/1411913005/diff/120001/build/android/gyp/java... build/android/gyp/java_google_api_keys.py:10: from string import Template style guide is against importing classes (here and below): https://engdoc.corp.google.com/eng/doc/devguide/py/styleguide.shtml?cl=head#s... https://codereview.chromium.org/1411913005/diff/120001/build/android/gyp/java... build/android/gyp/java_google_api_keys.py:17: two lines between top-level declarations https://codereview.chromium.org/1411913005/diff/120001/build/android/gyp/java... build/android/gyp/java_google_api_keys.py:18: def GetScriptName(): I think this function would be better to use: os.path.relpath(__file__, constants.DIR_SOURCE_ROOT) https://codereview.chromium.org/1411913005/diff/120001/build/android/gyp/java... build/android/gyp/java_google_api_keys.py:18: def GetScriptName(): Make this private (_GetScriptName) https://codereview.chromium.org/1411913005/diff/120001/build/android/gyp/java... build/android/gyp/java_google_api_keys.py:43: constant_entries_string = [] nit: don't call this _string since it holds a list https://codereview.chromium.org/1411913005/diff/120001/build/android/gyp/java... build/android/gyp/java_google_api_keys.py:61: def DoWriteOutput(output_path, constant_definition): nit: make private https://codereview.chromium.org/1411913005/diff/120001/build/android/gyp/java... build/android/gyp/java_google_api_keys.py:62: with open(output_path + os.path.sep + 'GoogleAPIKeys.java', 'w') as out_file: use os.path.join() rather than os.path.sep https://codereview.chromium.org/1411913005/diff/120001/build/android/gyp/java... build/android/gyp/java_google_api_keys.py:80: DoWriteOutput(argv[0], values) pass this is as a --flag and use argparse so that you'll get arg checking. https://codereview.chromium.org/1411913005/diff/120001/build/android/gyp/java... File build/android/gyp/java_google_api_keys_tests.py (right): https://codereview.chromium.org/1411913005/diff/120001/build/android/gyp/java... build/android/gyp/java_google_api_keys_tests.py:48: parser = optparse.OptionParser() nit: optparse is deprecated, use argparse (almost identical syntax) https://codereview.chromium.org/1411913005/diff/120001/build/android/java_goo... File build/android/java_google_api_keys.gypi (right): https://codereview.chromium.org/1411913005/diff/120001/build/android/java_goo... build/android/java_google_api_keys.gypi:35: # '<(source_file)', meant to be commented out? https://codereview.chromium.org/1411913005/diff/120001/build/config/android/r... File build/config/android/rules.gni (right): https://codereview.chromium.org/1411913005/diff/120001/build/config/android/r... build/config/android/rules.gni:409: template("java_google_api_keys") { This is not really a general-purpose template (given that it takes no parameters), so it should either be defined in the file that uses it, or just skip defining it as a template and add in the two rules (action+zip) directly. https://codereview.chromium.org/1411913005/diff/120001/build/config/android/r... build/config/android/rules.gni:410: set_sources_assignment_filter([]) No need to do this since the template doesn't look for a sources parameter https://codereview.chromium.org/1411913005/diff/120001/build/config/android/r... build/config/android/rules.gni:413: generate_google_api_keys_target_name = nit: It has become customary to prefix all local variables with _ (that is, those that are not used as parameters to rules) https://codereview.chromium.org/1411913005/diff/120001/build/config/android/r... build/config/android/rules.gni:422: check_includes = false this isn't applicable since there are no sources https://codereview.chromium.org/1411913005/diff/120001/build/config/android/r... build/config/android/rules.gni:427: get_path_info(rebase_path("GoogleAPIKeys.java", ".", gen_dir), "abspath"), Nit: I think rebase_path(foo, "", bar) will return an abspath, so you could use that and remove get_path_info() https://codereview.chromium.org/1411913005/diff/120001/build/config/android/r... build/config/android/rules.gni:433: generate_google_api_keys_outputs = nit: this is used only within zip(), so put it within zip() https://codereview.chromium.org/1411913005/diff/120001/build/config/android/r... build/config/android/rules.gni:435: base_gen_dir = I believe this is equivalent to: base_gen_dir = target_gen_dir
build/android rubberstamp lgtm with comments by agrieve@ addressed
agrieve@ I have some questions related to your comments. And please take a look at the tweaks I made. https://codereview.chromium.org/1411913005/diff/120001/build/android/gyp/java... File build/android/gyp/java_google_api_keys.py (right): https://codereview.chromium.org/1411913005/diff/120001/build/android/gyp/java... build/android/gyp/java_google_api_keys.py:10: from string import Template On 2015/10/30 00:55:13, agrieve wrote: > style guide is against importing classes (here and below): > https://engdoc.corp.google.com/eng/doc/devguide/py/styleguide.shtml?cl=head#s... Done. https://codereview.chromium.org/1411913005/diff/120001/build/android/gyp/java... build/android/gyp/java_google_api_keys.py:17: On 2015/10/30 00:55:13, agrieve wrote: > two lines between top-level declarations Done. https://codereview.chromium.org/1411913005/diff/120001/build/android/gyp/java... build/android/gyp/java_google_api_keys.py:18: def GetScriptName(): On 2015/10/30 00:55:13, agrieve wrote: > Make this private (_GetScriptName) It's exported for the unit test. https://codereview.chromium.org/1411913005/diff/120001/build/android/gyp/java... build/android/gyp/java_google_api_keys.py:18: def GetScriptName(): On 2015/10/30 00:55:13, agrieve wrote: > I think this function would be better to use: > os.path.relpath(__file__, constants.DIR_SOURCE_ROOT) constants didn't work. I tried to import it. It hasn't been found. https://codereview.chromium.org/1411913005/diff/120001/build/android/gyp/java... build/android/gyp/java_google_api_keys.py:43: constant_entries_string = [] On 2015/10/30 00:55:13, agrieve wrote: > nit: don't call this _string since it holds a list Done. https://codereview.chromium.org/1411913005/diff/120001/build/android/gyp/java... build/android/gyp/java_google_api_keys.py:61: def DoWriteOutput(output_path, constant_definition): On 2015/10/30 00:55:13, agrieve wrote: > nit: make private Done. https://codereview.chromium.org/1411913005/diff/120001/build/android/gyp/java... build/android/gyp/java_google_api_keys.py:62: with open(output_path + os.path.sep + 'GoogleAPIKeys.java', 'w') as out_file: On 2015/10/30 00:55:13, agrieve wrote: > use os.path.join() rather than os.path.sep Done. https://codereview.chromium.org/1411913005/diff/120001/build/android/gyp/java... build/android/gyp/java_google_api_keys.py:80: DoWriteOutput(argv[0], values) On 2015/10/30 00:55:13, agrieve wrote: > pass this is as a --flag and use argparse so that you'll get arg checking. Done. https://codereview.chromium.org/1411913005/diff/120001/build/android/gyp/java... File build/android/gyp/java_google_api_keys_tests.py (right): https://codereview.chromium.org/1411913005/diff/120001/build/android/gyp/java... build/android/gyp/java_google_api_keys_tests.py:48: parser = optparse.OptionParser() On 2015/10/30 00:55:13, agrieve wrote: > nit: optparse is deprecated, use argparse (almost identical syntax) Done. https://codereview.chromium.org/1411913005/diff/120001/build/android/java_goo... File build/android/java_google_api_keys.gypi (right): https://codereview.chromium.org/1411913005/diff/120001/build/android/java_goo... build/android/java_google_api_keys.gypi:35: # '<(source_file)', On 2015/10/30 00:55:13, agrieve wrote: > meant to be commented out? I removed it. https://codereview.chromium.org/1411913005/diff/120001/build/config/android/r... File build/config/android/rules.gni (right): https://codereview.chromium.org/1411913005/diff/120001/build/config/android/r... build/config/android/rules.gni:409: template("java_google_api_keys") { On 2015/10/30 00:55:13, agrieve wrote: > This is not really a general-purpose template (given that it takes no > parameters), so it should either be defined in the file that uses it, or just > skip defining it as a template and add in the two rules (action+zip) directly. Are there examples of non-general-purpose things I could use to create my rule? https://codereview.chromium.org/1411913005/diff/120001/build/config/android/r... build/config/android/rules.gni:410: set_sources_assignment_filter([]) On 2015/10/30 00:55:14, agrieve wrote: > No need to do this since the template doesn't look for a sources parameter Done. https://codereview.chromium.org/1411913005/diff/120001/build/config/android/r... build/config/android/rules.gni:413: generate_google_api_keys_target_name = On 2015/10/30 00:55:13, agrieve wrote: > nit: It has become customary to prefix all local variables with _ (that is, > those that are not used as parameters to rules) Done. https://codereview.chromium.org/1411913005/diff/120001/build/config/android/r... build/config/android/rules.gni:422: check_includes = false On 2015/10/30 00:55:13, agrieve wrote: > this isn't applicable since there are no sources Done. https://codereview.chromium.org/1411913005/diff/120001/build/config/android/r... build/config/android/rules.gni:427: get_path_info(rebase_path("GoogleAPIKeys.java", ".", gen_dir), "abspath"), On 2015/10/30 00:55:13, agrieve wrote: > Nit: I think rebase_path(foo, "", bar) will return an abspath, so you could use > that and remove get_path_info() Done. https://codereview.chromium.org/1411913005/diff/120001/build/config/android/r... build/config/android/rules.gni:433: generate_google_api_keys_outputs = On 2015/10/30 00:55:14, agrieve wrote: > nit: this is used only within zip(), so put it within zip() Done. https://codereview.chromium.org/1411913005/diff/120001/build/config/android/r... build/config/android/rules.gni:435: base_gen_dir = On 2015/10/30 00:55:14, agrieve wrote: > I believe this is equivalent to: > base_gen_dir = target_gen_dir Could you tell me more about that? I'm not even sure to understand what get_label_info() does eventually. Is there some doc I could read?
The CQ bit was checked by dvh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1411913005/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1411913005/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
The CQ bit was checked by dvh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1411913005/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1411913005/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1411913005/diff/120001/build/android/gyp/java... File build/android/gyp/java_google_api_keys.py (right): https://codereview.chromium.org/1411913005/diff/120001/build/android/gyp/java... build/android/gyp/java_google_api_keys.py:10: from string import Template On 2015/10/30 20:56:20, dvh wrote: > On 2015/10/30 00:55:13, agrieve wrote: > > style guide is against importing classes (here and below): > > > https://engdoc.corp.google.com/eng/doc/devguide/py/styleguide.shtml?cl=head#s... > > Done. Please fix all imports that do this (google_apk_keys and java_google_api_keys as well) https://codereview.chromium.org/1411913005/diff/120001/build/android/gyp/java... build/android/gyp/java_google_api_keys.py:18: def GetScriptName(): On 2015/10/30 20:56:20, dvh wrote: > On 2015/10/30 00:55:13, agrieve wrote: > > I think this function would be better to use: > > os.path.relpath(__file__, constants.DIR_SOURCE_ROOT) > > constants didn't work. I tried to import it. It hasn't been found. You need to tweak the sys.path (not a great practice, but it is what it is...) E.g. https://code.google.com/p/chromium/codesearch#chromium/src/build/android/gyp/... https://codereview.chromium.org/1411913005/diff/120001/build/config/android/r... File build/config/android/rules.gni (right): https://codereview.chromium.org/1411913005/diff/120001/build/config/android/r... build/config/android/rules.gni:409: template("java_google_api_keys") { On 2015/10/30 20:56:21, dvh wrote: > On 2015/10/30 00:55:13, agrieve wrote: > > This is not really a general-purpose template (given that it takes no > > parameters), so it should either be defined in the file that uses it, or just > > skip defining it as a template and add in the two rules (action+zip) directly. > > Are there examples of non-general-purpose things I could use to create my rule? What I think would be best is to avoid declaring a template() altogether, and just declare an action() and zip() within chrome/android/BUILD.gn https://codereview.chromium.org/1411913005/diff/120001/build/config/android/r... build/config/android/rules.gni:435: base_gen_dir = On 2015/10/30 20:56:21, dvh wrote: > On 2015/10/30 00:55:14, agrieve wrote: > > I believe this is equivalent to: > > base_gen_dir = target_gen_dir > > Could you tell me more about that? > I'm not even sure to understand what get_label_info() does eventually. > Is there some doc I could read? The command "gn help" has pretty exhaustive docs. Try doing: print(target_gen_dir) both inside of that action() and outside of the action(), and I think they'll print the same value https://codereview.chromium.org/1411913005/diff/160001/build/android/gyp/java... File build/android/gyp/java_google_api_keys.py (right): https://codereview.chromium.org/1411913005/diff/160001/build/android/gyp/java... build/android/gyp/java_google_api_keys.py:25: # script_components = os.path.abspath(sys.argv[0]).split(os.path.sep) Delete comments https://codereview.chromium.org/1411913005/diff/160001/build/android/gyp/java... build/android/gyp/java_google_api_keys.py:66: nit: two blank lines between top-level statements https://codereview.chromium.org/1411913005/diff/160001/build/android/gyp/java... build/android/gyp/java_google_api_keys.py:73: parser.add_argument("--out", help="Output folder.") nit: set required=True https://codereview.chromium.org/1411913005/diff/160001/build/android/gyp/java... build/android/gyp/java_google_api_keys.py:73: parser.add_argument("--out", help="Output folder.") GYP and GN need to know that actual output file to declare the script outputs. Might make more sense to specify the output file here rather than parent directory. https://codereview.chromium.org/1411913005/diff/160001/build/android/java_goo... File build/android/java_google_api_keys.gypi (right): https://codereview.chromium.org/1411913005/diff/160001/build/android/java_goo... build/android/java_google_api_keys.gypi:1: # Copyright 2014 The Chromium Authors. All rights reserved. nit: 2015 https://codereview.chromium.org/1411913005/diff/160001/build/android/java_goo... build/android/java_google_api_keys.gypi:45: 'python', '<(generator_path)', '--out', '<@(generator_args)' nit: <@ is for lists, but generator_args isn't a list. Just use <(generator_args).
https://codereview.chromium.org/1411913005/diff/120001/build/config/android/r... File build/config/android/rules.gni (right): https://codereview.chromium.org/1411913005/diff/120001/build/config/android/r... build/config/android/rules.gni:409: template("java_google_api_keys") { On 2015/10/31 02:09:37, agrieve wrote: > What I think would be best is to avoid declaring a template() altogether, and > just declare an action() and zip() within chrome/android/BUILD.gn +1
https://codereview.chromium.org/1411913005/diff/160001/build/android/gyp/java... File build/android/gyp/java_google_api_keys.py (right): https://codereview.chromium.org/1411913005/diff/160001/build/android/gyp/java... build/android/gyp/java_google_api_keys.py:25: # script_components = os.path.abspath(sys.argv[0]).split(os.path.sep) On 2015/10/31 02:09:37, agrieve wrote: > Delete comments Done. https://codereview.chromium.org/1411913005/diff/160001/build/android/gyp/java... build/android/gyp/java_google_api_keys.py:66: On 2015/10/31 02:09:37, agrieve wrote: > nit: two blank lines between top-level statements Done. https://codereview.chromium.org/1411913005/diff/160001/build/android/gyp/java... build/android/gyp/java_google_api_keys.py:73: parser.add_argument("--out", help="Output folder.") On 2015/10/31 02:09:37, agrieve wrote: > GYP and GN need to know that actual output file to declare the script outputs. > Might make more sense to specify the output file here rather than parent > directory. Done. https://codereview.chromium.org/1411913005/diff/160001/build/android/java_goo... File build/android/java_google_api_keys.gypi (right): https://codereview.chromium.org/1411913005/diff/160001/build/android/java_goo... build/android/java_google_api_keys.gypi:1: # Copyright 2014 The Chromium Authors. All rights reserved. On 2015/10/31 02:09:37, agrieve wrote: > nit: 2015 Done. https://codereview.chromium.org/1411913005/diff/160001/build/android/java_goo... build/android/java_google_api_keys.gypi:45: 'python', '<(generator_path)', '--out', '<@(generator_args)' On 2015/10/31 02:09:37, agrieve wrote: > nit: <@ is for lists, but generator_args isn't a list. Just use > <(generator_args). Done.
The CQ bit was checked by dvh@chromium.org to run a CQ dry run
Patchset #9 (id:160001) has been deleted
The CQ bit was checked by dvh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1411913005/190001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1411913005/190001
On 2015/11/02 05:29:18, brettw wrote: > https://codereview.chromium.org/1411913005/diff/120001/build/config/android/r... > File build/config/android/rules.gni (right): > > https://codereview.chromium.org/1411913005/diff/120001/build/config/android/r... > build/config/android/rules.gni:409: template("java_google_api_keys") { > On 2015/10/31 02:09:37, agrieve wrote: > > What I think would be best is to avoid declaring a template() altogether, and > > just declare an action() and zip() within chrome/android/BUILD.gn > > +1 I'd like to understand how to do that. Are there any similar examples?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
The CQ bit was checked by dvh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1411913005/210001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1411913005/210001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
agrieve, I think I managed to move out the template. Please could you take a look? Thanks!
The CQ bit was checked by dvh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1411913005/230001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1411913005/230001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1411913005/diff/230001/build/android/gyp/java... File build/android/gyp/java_google_api_keys.py (right): https://codereview.chromium.org/1411913005/diff/230001/build/android/gyp/java... build/android/gyp/java_google_api_keys.py:16: from google_api_keys import (GetAPIKey, GetAPIKeyRemoting, GetClientID, Don't import functions. https://codereview.chromium.org/1411913005/diff/230001/build/android/gyp/java... build/android/gyp/java_google_api_keys.py:22: build_index = script_components.index('build') use constants.DIR_SOURCE_ROOT https://codereview.chromium.org/1411913005/diff/230001/build/android/gyp/java... build/android/gyp/java_google_api_keys.py:24: 2 blank lines https://codereview.chromium.org/1411913005/diff/230001/chrome/android/BUILD.gn File chrome/android/BUILD.gn (right): https://codereview.chromium.org/1411913005/diff/230001/chrome/android/BUILD.g... chrome/android/BUILD.gn:202: zip("chrome_android_java_google_api_keys_srcjar") { yep, this looks good :) One more thing you should do to simplify though, is to fold zip() into java_google_api_keys.py. Can you add a --srcjar arg that works similarly to https://code.google.com/p/chromium/codesearch#chromium/src/build/android/gyp/... You should be able to avoid writing intermediate files entirely by using zipfile.writestr(). You should also use HERMETIC_TIMESTAMP from here: https://code.google.com/p/chromium/codesearch#chromium/src/build/android/gyp/... (and also HERMETIC_FILE_ATTR) so that identical .srcjar files will be created given identical inputs. The other thing this will fix is that the path should include the java package name in it, or else compilation with enable_incremental_javac = true will fail. This doesn't exist for GYP, so just leave as-is for GYP. Note: I see that java_cpp_enum() doesn't follow my advice here, but I will fix that up :)
Please take a look. I also moved gyp build from template to a single target. https://codereview.chromium.org/1411913005/diff/230001/build/android/gyp/java... File build/android/gyp/java_google_api_keys.py (right): https://codereview.chromium.org/1411913005/diff/230001/build/android/gyp/java... build/android/gyp/java_google_api_keys.py:16: from google_api_keys import (GetAPIKey, GetAPIKeyRemoting, GetClientID, On 2015/11/03 16:22:42, agrieve wrote: > Don't import functions. Done. https://codereview.chromium.org/1411913005/diff/230001/build/android/gyp/java... build/android/gyp/java_google_api_keys.py:22: build_index = script_components.index('build') On 2015/11/03 16:22:42, agrieve wrote: > use constants.DIR_SOURCE_ROOT Done. https://codereview.chromium.org/1411913005/diff/230001/build/android/gyp/java... build/android/gyp/java_google_api_keys.py:24: On 2015/11/03 16:22:42, agrieve wrote: > 2 blank lines Done. https://codereview.chromium.org/1411913005/diff/230001/chrome/android/BUILD.gn File chrome/android/BUILD.gn (right): https://codereview.chromium.org/1411913005/diff/230001/chrome/android/BUILD.g... chrome/android/BUILD.gn:202: zip("chrome_android_java_google_api_keys_srcjar") { On 2015/11/03 16:22:42, agrieve wrote: > yep, this looks good :) > > One more thing you should do to simplify though, is to fold zip() into > java_google_api_keys.py. > > Can you add a --srcjar arg that works similarly to > https://code.google.com/p/chromium/codesearch#chromium/src/build/android/gyp/... > > You should be able to avoid writing intermediate files entirely by using > zipfile.writestr(). > > You should also use HERMETIC_TIMESTAMP from here: > https://code.google.com/p/chromium/codesearch#chromium/src/build/android/gyp/... > > (and also HERMETIC_FILE_ATTR) so that identical .srcjar files will be created > given identical inputs. > > The other thing this will fix is that the path should include the java package > name in it, or else compilation with enable_incremental_javac = true will fail. > This doesn't exist for GYP, so just leave as-is for GYP. > > Note: I see that java_cpp_enum() doesn't follow my advice here, but I will fix > that up :) Done.
The CQ bit was checked by dvh@chromium.org to run a CQ dry run
thestig@, you might also want to take a look again at my changes in the gyp files.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1411913005/270001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1411913005/270001
https://codereview.chromium.org/1411913005/diff/270001/build/android/gyp/java... File build/android/gyp/java_google_api_keys.py (right): https://codereview.chromium.org/1411913005/diff/270001/build/android/gyp/java... build/android/gyp/java_google_api_keys.py:75: if len(folder) > 0 and not os.path.exists(folder): nit: len(folder) > 0 -> folder https://codereview.chromium.org/1411913005/diff/270001/build/android/gyp/java... build/android/gyp/java_google_api_keys.py:82: srcjar.writestr(arcname, GenerateOutput(constant_definition)) arcname -> zipinfo https://codereview.chromium.org/1411913005/diff/270001/build/android/gyp/java... build/android/gyp/java_google_api_keys.py:109: if options.srcjar is not None: nit: if options.srcjar is not None: -> if options.srcjar: https://codereview.chromium.org/1411913005/diff/270001/build/android/gyp/java... File build/android/gyp/java_google_api_keys_tests.py (right): https://codereview.chromium.org/1411913005/diff/270001/build/android/gyp/java... build/android/gyp/java_google_api_keys_tests.py:19: from java_google_api_keys import GenerateOutput, GetScriptName don't import functions. https://codereview.chromium.org/1411913005/diff/270001/build/android/gyp/java... build/android/gyp/java_google_api_keys_tests.py:23: 2 blanks https://codereview.chromium.org/1411913005/diff/270001/build/android/gyp/java... build/android/gyp/java_google_api_keys_tests.py:46: 2 blanks https://codereview.chromium.org/1411913005/diff/270001/build/android/java_goo... File build/android/java_google_api_keys.gyp (right): https://codereview.chromium.org/1411913005/diff/270001/build/android/java_goo... build/android/java_google_api_keys.gyp:38: 'python', '<(generator_path)', '--srcjar', '<(output_file)' I don't think GYP supports srcjars. Might have looked like it worked if you have stale files in your output directory?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
agrieve@, thestig@, could you please take a look? Thanks! https://codereview.chromium.org/1411913005/diff/270001/build/android/gyp/java... File build/android/gyp/java_google_api_keys.py (right): https://codereview.chromium.org/1411913005/diff/270001/build/android/gyp/java... build/android/gyp/java_google_api_keys.py:75: if len(folder) > 0 and not os.path.exists(folder): On 2015/11/03 20:27:34, agrieve wrote: > nit: len(folder) > 0 -> folder Done. https://codereview.chromium.org/1411913005/diff/270001/build/android/gyp/java... build/android/gyp/java_google_api_keys.py:82: srcjar.writestr(arcname, GenerateOutput(constant_definition)) On 2015/11/03 20:27:34, agrieve wrote: > arcname -> zipinfo Done. https://codereview.chromium.org/1411913005/diff/270001/build/android/gyp/java... build/android/gyp/java_google_api_keys.py:109: if options.srcjar is not None: On 2015/11/03 20:27:34, agrieve wrote: > nit: if options.srcjar is not None: -> if options.srcjar: Done. https://codereview.chromium.org/1411913005/diff/270001/build/android/gyp/java... File build/android/gyp/java_google_api_keys_tests.py (right): https://codereview.chromium.org/1411913005/diff/270001/build/android/gyp/java... build/android/gyp/java_google_api_keys_tests.py:19: from java_google_api_keys import GenerateOutput, GetScriptName On 2015/11/03 20:27:35, agrieve wrote: > don't import functions. Done. https://codereview.chromium.org/1411913005/diff/270001/build/android/gyp/java... build/android/gyp/java_google_api_keys_tests.py:23: On 2015/11/03 20:27:35, agrieve wrote: > 2 blanks Done. https://codereview.chromium.org/1411913005/diff/270001/build/android/gyp/java... build/android/gyp/java_google_api_keys_tests.py:46: On 2015/11/03 20:27:35, agrieve wrote: > 2 blanks Done. https://codereview.chromium.org/1411913005/diff/270001/build/android/java_goo... File build/android/java_google_api_keys.gyp (right): https://codereview.chromium.org/1411913005/diff/270001/build/android/java_goo... build/android/java_google_api_keys.gyp:38: 'python', '<(generator_path)', '--srcjar', '<(output_file)' On 2015/11/03 20:27:35, agrieve wrote: > I don't think GYP supports srcjars. Might have looked like it worked if you have > stale files in your output directory? Done.
The CQ bit was checked by dvh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1411913005/290001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1411913005/290001
On 2015/11/04 00:08:43, dvh wrote: > agrieve@, thestig@, could you please take a look? chrome/chrome.gyp still lgtm
lgtm with one final change! https://codereview.chromium.org/1411913005/diff/290001/chrome/android/BUILD.gn File chrome/android/BUILD.gn (right): https://codereview.chromium.org/1411913005/diff/290001/chrome/android/BUILD.g... chrome/android/BUILD.gn:188: _output_path = rebase_path("${target_name}.srcjar", ".", target_gen_dir) These two lines are a bit weird. I copied this part in and print()ed _outtput_path, and it gives: ../../out-gn/Debug/gen/chrome/android/chrome_android_java_google_api_keys_srcjar.srcjar It actually only even works because chrome/android is two levels deep, and out-gn/Debug is also two levels deep. Better would be: _output_path = "$target_gen_dir/$target_name.srcjar" outputs = [ _output_path ] args = [ "--srcjar", rebase_path(_output_path, root_build_dir) ]
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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_...)
https://codereview.chromium.org/1411913005/diff/290001/chrome/android/BUILD.gn File chrome/android/BUILD.gn (right): https://codereview.chromium.org/1411913005/diff/290001/chrome/android/BUILD.g... chrome/android/BUILD.gn:188: _output_path = rebase_path("${target_name}.srcjar", ".", target_gen_dir) On 2015/11/04 02:09:11, agrieve wrote: > These two lines are a bit weird. I copied this part in and print()ed > _outtput_path, and it gives: > > ../../out-gn/Debug/gen/chrome/android/chrome_android_java_google_api_keys_srcjar.srcjar > > It actually only even works because chrome/android is two levels deep, and > out-gn/Debug is also two levels deep. > > Better would be: > _output_path = "$target_gen_dir/$target_name.srcjar" > outputs = [ _output_path ] > args = [ "--srcjar", rebase_path(_output_path, root_build_dir) ] Done.
The CQ bit was checked by dvh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pasko@chromium.org, thestig@chromium.org, agrieve@chromium.org Link to the patchset: https://codereview.chromium.org/1411913005/#ps300001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1411913005/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1411913005/300001
Message was sent while issue was closed.
Committed patchset #16 (id:300001)
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/310b3905a97a06b30514e0db0a46c7d8af4dcb60 Cr-Commit-Position: refs/heads/master@{#357774} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
