|
|
Created:
4 years, 5 months ago by agrieve Modified:
4 years, 5 months ago Reviewers:
jbudorick CC:
chromium-reviews, jbudorick+watch_chromium.org, mikecase+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionInitial version of Android Studio project generation
This supports Java auto-completion, but probably not many
more advanced features since it doesn't include resources,
assets, etc.
It also likely doesn't work for test apks yet (haven't tried it).
Usage:
build/android/generate_gradle.py \
--output-directory out/Default \
--target //chrome/android:chrome_public_apk \
--project-dir my-project-dir
BUG=620034
Committed: https://crrev.com/0fa1a09f4f5418b05248c48e995ce7434970e6cc
Cr-Commit-Position: refs/heads/master@{#405431}
Patch Set 1 #
Total comments: 28
Patch Set 2 : comments 1 #
Total comments: 13
Patch Set 3 : comments 2 #
Messages
Total messages: 19 (8 generated)
Description was changed from ========== Initial version of Android Studio project generation Supports java editing fairly well (it compiles), but haven's yet explored setting resources / assets / native libraries. Usage: build/android/generate_gradle.py \ --output-directory out/Default \ --target //chrome/android:chrome_public_apk \ --project-dir my-project-dir BUG=620034 ========== to ========== Initial version of Android Studio project generation Supports java editing fairly well (it compiles), but haven's yet explored setting resources / assets / native libraries. Usage: build/android/generate_gradle.py \ --output-directory out/Default \ --target //chrome/android:chrome_public_apk \ --project-dir my-project-dir BUG=620034 ==========
agrieve@chromium.org changed reviewers: + jbudorick@chromium.org
On 2016/07/07 20:46:43, agrieve wrote: > mailto:agrieve@chromium.org changed reviewers: > + mailto:jbudorick@chromium.org
Trying to think of a clean way that you could run this as its own build target ("chrome_public_apk_gradle" or w/e), but the gradle_output_dir is a stumbling block there :( https://codereview.chromium.org/2130933002/diff/1/build/android/BUILD.gn File build/android/BUILD.gn (right): https://codereview.chromium.org/2130933002/diff/1/build/android/BUILD.gn#newc... build/android/BUILD.gn:45: _rebased_android_sdk_root = rebase_path(android_sdk_root, root_build_dir) Should this be a part of a group or action rather than just top-level //build/android/BUILD.gn (% enable_java_templates, which is always on in chromium)? https://codereview.chromium.org/2130933002/diff/1/build/android/build.gradle.... File build/android/build.gradle.jinja (right): https://codereview.chromium.org/2130933002/diff/1/build/android/build.gradle.... build/android/build.gradle.jinja:1: {# Copyright 2016 The Chromium Authors. All rights reserved. #} Can we put all of this in a //build/android/gradle/ directory? https://codereview.chromium.org/2130933002/diff/1/build/android/generate_grad... File build/android/generate_gradle.py (right): https://codereview.chromium.org/2130933002/diff/1/build/android/generate_grad... build/android/generate_gradle.py:2: # Copyright 2016 The Chromium Authors. All rights reserved. Again, //build/android/gradle/? https://codereview.chromium.org/2130933002/diff/1/build/android/generate_grad... build/android/generate_gradle.py:26: _JINJA_TEMPLATE_PATH = os.path.join( nit: sort https://codereview.chromium.org/2130933002/diff/1/build/android/generate_grad... build/android/generate_gradle.py:95: while os.path.basename(path_root) not in ('javax', 'org', 'com'): nit: I think this is a bit simpler than while/break/else: while os.path.basename(path_root) not in ('com', 'javax', 'org', 'src'): assert path_root path_root = os.path.dirname(path_root) if os.path.basename(path_root) != 'src': path_root = os.path.dirname(path_root) https://codereview.chromium.org/2130933002/diff/1/build/android/generate_grad... build/android/generate_gradle.py:114: shutil.rmtree(symlink_dir) I would expect this case to be an error. We could possibly nuke directories behind a flag, but I don't think that should be the default behavior. https://codereview.chromium.org/2130933002/diff/1/build/android/generate_grad... build/android/generate_gradle.py:116: for target_path in desired_files: Are the target_paths relative or absolute? https://codereview.chromium.org/2130933002/diff/1/build/android/generate_grad... build/android/generate_gradle.py:118: subpath = target_path[len(prefix) + 1:] os.path.relpath(target_path, prefix) ? https://codereview.chromium.org/2130933002/diff/1/build/android/generate_grad... build/android/generate_gradle.py:123: relpath = os.path.relpath(os.path.join(output_dir, target_path), ... if the target paths are absolute, this seems odd. https://codereview.chromium.org/2130933002/diff/1/build/android/generate_grad... build/android/generate_gradle.py:176: if deps_info['requires_android']: I'm a bit surprised that we write both of these as 'java_library' now. https://codereview.chromium.org/2130933002/diff/1/build/android/generate_grad... build/android/generate_gradle.py:235: shutil.rmtree(extracted_path) Same comment as before about making destruction non-default. https://codereview.chromium.org/2130933002/diff/1/build/android/generate_grad... build/android/generate_gradle.py:316: gradle_dir = os.path.join(gradle_output_dir, entry.GradleSubdir()) optional nit: maybe gradle_subdir instead of gradle_dir to avoid confusion w/ gradle_output_dir? https://codereview.chromium.org/2130933002/diff/1/build/config/android/intern... File build/config/android/internal_rules.gni (right): https://codereview.chromium.org/2130933002/diff/1/build/config/android/intern... build/config/android/internal_rules.gni:332: rebase_path(invoker.bundled_srcjars, root_build_dir) Does this work with a list of paths? https://codereview.chromium.org/2130933002/diff/1/build/config/android/intern... build/config/android/internal_rules.gni:2170: _dep_name = get_label_info(d, "name") Is this used?
I actually had started off with this as a GN rule, but switched to a standalone script halfway through. I think it's a bit nicer stand-alone so we can add --flags to it. https://codereview.chromium.org/2130933002/diff/1/build/android/BUILD.gn File build/android/BUILD.gn (right): https://codereview.chromium.org/2130933002/diff/1/build/android/BUILD.gn#newc... build/android/BUILD.gn:45: _rebased_android_sdk_root = rebase_path(android_sdk_root, root_build_dir) On 2016/07/07 22:32:29, jbudorick wrote: > Should this be a part of a group or action rather than just top-level > //build/android/BUILD.gn (% enable_java_templates, which is always on in > chromium)? If we made it its own action, then "gn gen" would need to write this information to a .ninja file, which then when executed would pass them as args to a script, which would then convert them from args to a file on disk. Much more efficient to have "gn gen" just write them straight to disk :) https://codereview.chromium.org/2130933002/diff/1/build/android/build.gradle.... File build/android/build.gradle.jinja (right): https://codereview.chromium.org/2130933002/diff/1/build/android/build.gradle.... build/android/build.gradle.jinja:1: {# Copyright 2016 The Chromium Authors. All rights reserved. #} On 2016/07/07 22:32:29, jbudorick wrote: > Can we put all of this in a //build/android/gradle/ directory? Done. https://codereview.chromium.org/2130933002/diff/1/build/android/generate_grad... File build/android/generate_gradle.py (right): https://codereview.chromium.org/2130933002/diff/1/build/android/generate_grad... build/android/generate_gradle.py:2: # Copyright 2016 The Chromium Authors. All rights reserved. On 2016/07/07 22:32:29, jbudorick wrote: > Again, //build/android/gradle/? Done. https://codereview.chromium.org/2130933002/diff/1/build/android/generate_grad... build/android/generate_gradle.py:26: _JINJA_TEMPLATE_PATH = os.path.join( On 2016/07/07 22:32:29, jbudorick wrote: > nit: sort Done. https://codereview.chromium.org/2130933002/diff/1/build/android/generate_grad... build/android/generate_gradle.py:95: while os.path.basename(path_root) not in ('javax', 'org', 'com'): On 2016/07/07 22:32:30, jbudorick wrote: > nit: I think this is a bit simpler than while/break/else: > > while os.path.basename(path_root) not in ('com', 'javax', 'org', 'src'): > assert path_root > path_root = os.path.dirname(path_root) > if os.path.basename(path_root) != 'src': > path_root = os.path.dirname(path_root) > > Done. https://codereview.chromium.org/2130933002/diff/1/build/android/generate_grad... build/android/generate_gradle.py:114: shutil.rmtree(symlink_dir) On 2016/07/07 22:32:30, jbudorick wrote: > I would expect this case to be an error. We could possibly nuke directories > behind a flag, but I don't think that should be the default behavior. It happens when regenerating. There are .idea files in the top-level project dir once you get going that Android Studio doesn't like being removed, so we can't just delete the entire output directory. It can also cause excessive re-sync times if you delete all of the build/ directories. Added an assert to at least make sure we're in the project directory. https://codereview.chromium.org/2130933002/diff/1/build/android/generate_grad... build/android/generate_gradle.py:116: for target_path in desired_files: On 2016/07/07 22:32:29, jbudorick wrote: > Are the target_paths relative or absolute? Reworked this to be less brain-hurting. https://codereview.chromium.org/2130933002/diff/1/build/android/generate_grad... build/android/generate_gradle.py:118: subpath = target_path[len(prefix) + 1:] On 2016/07/07 22:32:29, jbudorick wrote: > os.path.relpath(target_path, prefix) > > ? Done. https://codereview.chromium.org/2130933002/diff/1/build/android/generate_grad... build/android/generate_gradle.py:123: relpath = os.path.relpath(os.path.join(output_dir, target_path), On 2016/07/07 22:32:29, jbudorick wrote: > ... if the target paths are absolute, this seems odd. Done. https://codereview.chromium.org/2130933002/diff/1/build/android/generate_grad... build/android/generate_gradle.py:176: if deps_info['requires_android']: On 2016/07/07 22:32:29, jbudorick wrote: > I'm a bit surprised that we write both of these as 'java_library' now. Meh, in rules.gni, android_library is just a shortcut for java_library{requires_android=true} https://codereview.chromium.org/2130933002/diff/1/build/android/generate_grad... build/android/generate_gradle.py:235: shutil.rmtree(extracted_path) On 2016/07/07 22:32:29, jbudorick wrote: > Same comment as before about making destruction non-default. Done. https://codereview.chromium.org/2130933002/diff/1/build/android/generate_grad... build/android/generate_gradle.py:316: gradle_dir = os.path.join(gradle_output_dir, entry.GradleSubdir()) On 2016/07/07 22:32:29, jbudorick wrote: > optional nit: maybe gradle_subdir instead of gradle_dir to avoid confusion w/ > gradle_output_dir? changed to entry_output_dir https://codereview.chromium.org/2130933002/diff/1/build/config/android/intern... File build/config/android/internal_rules.gni (right): https://codereview.chromium.org/2130933002/diff/1/build/config/android/intern... build/config/android/internal_rules.gni:332: rebase_path(invoker.bundled_srcjars, root_build_dir) On 2016/07/07 22:32:30, jbudorick wrote: > Does this work with a list of paths? yes https://codereview.chromium.org/2130933002/diff/1/build/config/android/intern... build/config/android/internal_rules.gni:2170: _dep_name = get_label_info(d, "name") On 2016/07/07 22:32:30, jbudorick wrote: > Is this used? On the next line :P
Description was changed from ========== Initial version of Android Studio project generation Supports java editing fairly well (it compiles), but haven's yet explored setting resources / assets / native libraries. Usage: build/android/generate_gradle.py \ --output-directory out/Default \ --target //chrome/android:chrome_public_apk \ --project-dir my-project-dir BUG=620034 ========== to ========== Initial version of Android Studio project generation This supports Java auto-completion, but probably not many more advanced features since it doesn't include resources, assets, etc. It also likely doesn't work for test apks yet (haven't tried it). Usage: build/android/generate_gradle.py \ --output-directory out/Default \ --target //chrome/android:chrome_public_apk \ --project-dir my-project-dir BUG=620034 ==========
On 2016/07/08 19:07:29, agrieve wrote: > I actually had started off with this as a GN rule, but switched to a standalone > script halfway through. I think it's a bit nicer stand-alone so we can add > --flags to it. > > https://codereview.chromium.org/2130933002/diff/1/build/android/BUILD.gn > File build/android/BUILD.gn (right): > > https://codereview.chromium.org/2130933002/diff/1/build/android/BUILD.gn#newc... > build/android/BUILD.gn:45: _rebased_android_sdk_root = > rebase_path(android_sdk_root, root_build_dir) > On 2016/07/07 22:32:29, jbudorick wrote: > > Should this be a part of a group or action rather than just top-level > > //build/android/BUILD.gn (% enable_java_templates, which is always on in > > chromium)? > > If we made it its own action, then "gn gen" would need to write this information > to a .ninja file, which then when executed would pass them as args to a script, > which would then convert them from args to a file on disk. > > Much more efficient to have "gn gen" just write them straight to disk :) > > https://codereview.chromium.org/2130933002/diff/1/build/android/build.gradle.... > File build/android/build.gradle.jinja (right): > > https://codereview.chromium.org/2130933002/diff/1/build/android/build.gradle.... > build/android/build.gradle.jinja:1: {# Copyright 2016 The Chromium Authors. All > rights reserved. #} > On 2016/07/07 22:32:29, jbudorick wrote: > > Can we put all of this in a //build/android/gradle/ directory? > > Done. > > https://codereview.chromium.org/2130933002/diff/1/build/android/generate_grad... > File build/android/generate_gradle.py (right): > > https://codereview.chromium.org/2130933002/diff/1/build/android/generate_grad... > build/android/generate_gradle.py:2: # Copyright 2016 The Chromium Authors. All > rights reserved. > On 2016/07/07 22:32:29, jbudorick wrote: > > Again, //build/android/gradle/? > > Done. > > https://codereview.chromium.org/2130933002/diff/1/build/android/generate_grad... > build/android/generate_gradle.py:26: _JINJA_TEMPLATE_PATH = os.path.join( > On 2016/07/07 22:32:29, jbudorick wrote: > > nit: sort > > Done. > > https://codereview.chromium.org/2130933002/diff/1/build/android/generate_grad... > build/android/generate_gradle.py:95: while os.path.basename(path_root) not in > ('javax', 'org', 'com'): > On 2016/07/07 22:32:30, jbudorick wrote: > > nit: I think this is a bit simpler than while/break/else: > > > > while os.path.basename(path_root) not in ('com', 'javax', 'org', 'src'): > > assert path_root > > path_root = os.path.dirname(path_root) > > if os.path.basename(path_root) != 'src': > > path_root = os.path.dirname(path_root) > > > > > > Done. > > https://codereview.chromium.org/2130933002/diff/1/build/android/generate_grad... > build/android/generate_gradle.py:114: shutil.rmtree(symlink_dir) > On 2016/07/07 22:32:30, jbudorick wrote: > > I would expect this case to be an error. We could possibly nuke directories > > behind a flag, but I don't think that should be the default behavior. > > It happens when regenerating. There are .idea files in the top-level project dir > once you get going that Android Studio doesn't like being removed, so we can't > just delete the entire output directory. It can also cause excessive re-sync > times if you delete all of the build/ directories. > > Added an assert to at least make sure we're in the project directory. > > https://codereview.chromium.org/2130933002/diff/1/build/android/generate_grad... > build/android/generate_gradle.py:116: for target_path in desired_files: > On 2016/07/07 22:32:29, jbudorick wrote: > > Are the target_paths relative or absolute? > > Reworked this to be less brain-hurting. > > https://codereview.chromium.org/2130933002/diff/1/build/android/generate_grad... > build/android/generate_gradle.py:118: subpath = target_path[len(prefix) + 1:] > On 2016/07/07 22:32:29, jbudorick wrote: > > os.path.relpath(target_path, prefix) > > > > ? > > Done. > > https://codereview.chromium.org/2130933002/diff/1/build/android/generate_grad... > build/android/generate_gradle.py:123: relpath = > os.path.relpath(os.path.join(output_dir, target_path), > On 2016/07/07 22:32:29, jbudorick wrote: > > ... if the target paths are absolute, this seems odd. > > Done. > > https://codereview.chromium.org/2130933002/diff/1/build/android/generate_grad... > build/android/generate_gradle.py:176: if deps_info['requires_android']: > On 2016/07/07 22:32:29, jbudorick wrote: > > I'm a bit surprised that we write both of these as 'java_library' now. > > Meh, in rules.gni, android_library is just a shortcut for > java_library{requires_android=true} > > https://codereview.chromium.org/2130933002/diff/1/build/android/generate_grad... > build/android/generate_gradle.py:235: shutil.rmtree(extracted_path) > On 2016/07/07 22:32:29, jbudorick wrote: > > Same comment as before about making destruction non-default. > > Done. > > https://codereview.chromium.org/2130933002/diff/1/build/android/generate_grad... > build/android/generate_gradle.py:316: gradle_dir = > os.path.join(gradle_output_dir, entry.GradleSubdir()) > On 2016/07/07 22:32:29, jbudorick wrote: > > optional nit: maybe gradle_subdir instead of gradle_dir to avoid confusion w/ > > gradle_output_dir? > > changed to entry_output_dir > > https://codereview.chromium.org/2130933002/diff/1/build/config/android/intern... > File build/config/android/internal_rules.gni (right): > > https://codereview.chromium.org/2130933002/diff/1/build/config/android/intern... > build/config/android/internal_rules.gni:332: > rebase_path(invoker.bundled_srcjars, root_build_dir) > On 2016/07/07 22:32:30, jbudorick wrote: > > Does this work with a list of paths? > > yes > > https://codereview.chromium.org/2130933002/diff/1/build/config/android/intern... > build/config/android/internal_rules.gni:2170: _dep_name = get_label_info(d, > "name") > On 2016/07/07 22:32:30, jbudorick wrote: > > Is this used? > > On the next line :P I can't build there.
https://codereview.chromium.org/2130933002/diff/20001/build/android/PRESUBMIT.py File build/android/PRESUBMIT.py (left): https://codereview.chromium.org/2130933002/diff/20001/build/android/PRESUBMIT... build/android/PRESUBMIT.py:24: r'incremental_install/.*\.py', Yay! https://codereview.chromium.org/2130933002/diff/20001/build/android/gradle/ge... File build/android/gradle/generate_gradle.py (right): https://codereview.chromium.org/2130933002/diff/20001/build/android/gradle/ge... build/android/gradle/generate_gradle.py:21: from pylib.utils import run_tests_helper from devil.utils import run_tests_helper pylib.utils.run_tests_helper is a transitional module that is going away today % the CQ coming back to life https://codereview.chromium.org/2130933002/diff/20001/build/android/gradle/ge... build/android/gradle/generate_gradle.py:79: assert gn_target.startswith('//') and ':' in gn_target, gn_target Is the second half of this condition necessarily true? GN targets can omit the : part if the target name is the same as the directory name, right? (e.g., //base is //base:base) https://codereview.chromium.org/2130933002/diff/20001/build/android/gradle/ge... build/android/gradle/generate_gradle.py:181: unwanted_java_files = set(found_java_files) - set(java_files) Is it possible for there to be missing files? (i.e., for java_files - found_java_files to not be empty) https://codereview.chromium.org/2130933002/diff/20001/build/android/gradle/ge... build/android/gradle/generate_gradle.py:194: '# Generated by //build/android/generate_gradle.py', nit: update this path https://codereview.chromium.org/2130933002/diff/20001/build/android/gradle/ge... build/android/gradle/generate_gradle.py:228: nit: odd to have a line break here rather than one line down https://codereview.chromium.org/2130933002/diff/20001/build/android/gradle/ge... build/android/gradle/generate_gradle.py:249: lines.append('// Generated by //build/android/generate_gradle.py') nit: update this path
Thanks as always for your attention to detail! https://codereview.chromium.org/2130933002/diff/20001/build/android/gradle/ge... File build/android/gradle/generate_gradle.py (right): https://codereview.chromium.org/2130933002/diff/20001/build/android/gradle/ge... build/android/gradle/generate_gradle.py:21: from pylib.utils import run_tests_helper On 2016/07/12 19:20:17, jbudorick wrote: > from devil.utils import run_tests_helper > > pylib.utils.run_tests_helper is a transitional module that is going away today % > the CQ coming back to life Done. https://codereview.chromium.org/2130933002/diff/20001/build/android/gradle/ge... build/android/gradle/generate_gradle.py:79: assert gn_target.startswith('//') and ':' in gn_target, gn_target On 2016/07/12 19:20:17, jbudorick wrote: > Is the second half of this condition necessarily true? GN targets can omit the : > part if the target name is the same as the directory name, right? (e.g., //base > is //base:base) Done. https://codereview.chromium.org/2130933002/diff/20001/build/android/gradle/ge... build/android/gradle/generate_gradle.py:181: unwanted_java_files = set(found_java_files) - set(java_files) On 2016/07/12 19:20:17, jbudorick wrote: > Is it possible for there to be missing files? (i.e., for java_files - > found_java_files to not be empty) It's possible, but I would expect "gn gen" to fail in that case. Added a check here for it anyways. https://codereview.chromium.org/2130933002/diff/20001/build/android/gradle/ge... build/android/gradle/generate_gradle.py:194: '# Generated by //build/android/generate_gradle.py', On 2016/07/12 19:20:17, jbudorick wrote: > nit: update this path Done. https://codereview.chromium.org/2130933002/diff/20001/build/android/gradle/ge... build/android/gradle/generate_gradle.py:228: On 2016/07/12 19:20:17, jbudorick wrote: > nit: odd to have a line break here rather than one line down Done. https://codereview.chromium.org/2130933002/diff/20001/build/android/gradle/ge... build/android/gradle/generate_gradle.py:249: lines.append('// Generated by //build/android/generate_gradle.py') On 2016/07/12 19:20:17, jbudorick wrote: > nit: update this path Done.
lgtm
The CQ bit was checked by agrieve@chromium.org
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: 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 agrieve@chromium.org
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 ========== Initial version of Android Studio project generation This supports Java auto-completion, but probably not many more advanced features since it doesn't include resources, assets, etc. It also likely doesn't work for test apks yet (haven't tried it). Usage: build/android/generate_gradle.py \ --output-directory out/Default \ --target //chrome/android:chrome_public_apk \ --project-dir my-project-dir BUG=620034 ========== to ========== Initial version of Android Studio project generation This supports Java auto-completion, but probably not many more advanced features since it doesn't include resources, assets, etc. It also likely doesn't work for test apks yet (haven't tried it). Usage: build/android/generate_gradle.py \ --output-directory out/Default \ --target //chrome/android:chrome_public_apk \ --project-dir my-project-dir BUG=620034 ==========
Message was sent while issue was closed.
Description was changed from ========== Initial version of Android Studio project generation This supports Java auto-completion, but probably not many more advanced features since it doesn't include resources, assets, etc. It also likely doesn't work for test apks yet (haven't tried it). Usage: build/android/generate_gradle.py \ --output-directory out/Default \ --target //chrome/android:chrome_public_apk \ --project-dir my-project-dir BUG=620034 ========== to ========== Initial version of Android Studio project generation This supports Java auto-completion, but probably not many more advanced features since it doesn't include resources, assets, etc. It also likely doesn't work for test apks yet (haven't tried it). Usage: build/android/generate_gradle.py \ --output-directory out/Default \ --target //chrome/android:chrome_public_apk \ --project-dir my-project-dir BUG=620034 Committed: https://crrev.com/0fa1a09f4f5418b05248c48e995ce7434970e6cc Cr-Commit-Position: refs/heads/master@{#405431} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/0fa1a09f4f5418b05248c48e995ce7434970e6cc Cr-Commit-Position: refs/heads/master@{#405431} |