Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2812133003:
Android: Add pseudo module for Android Studio (Closed)
|
Created:
3 years, 8 months ago by Peter Wen Modified:
3 years, 7 months ago CC:
chromium-reviews, mikecase+watch_chromium.org, nyquist+watch_chromium.org, jbudorick+watch_chromium.org, wnwen+watch_chromium.org, agrieve+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAndroid: Add module "_all" for Android Studio
Adding all sources to a "_all" pseudo module fixes Studio's code analysis
functions (imports, refactoring).
In order to have things build properly in gradle, the "_all" module has
all sources excluded (gradle applies the filters, studio does not).
Also fix "--all" targets to include tests. Make it easier to make sweeping
java refactors in Android Studio.
BUG=620034
Review-Url: https://codereview.chromium.org/2812133003
Cr-Commit-Position: refs/heads/master@{#465700}
Committed: https://chromium.googlesource.com/chromium/src/+/03427bcc41560b36b5c595eda92e35f563a6303e
Patch Set 1 #
Total comments: 10
Patch Set 2 : Fix per review #
Total comments: 2
Patch Set 3 : Update docs and switch to sorted #
Total comments: 4
Messages
Total messages: 21 (9 generated)
wnwen@chromium.org changed reviewers: + agrieve@chromium.org
Will wait for a couple other fixes to build and AndroidManifests to land before this CL. It's slow (~157 projects) to build all but nice to be able to refactor all the java code in use (except those behind experimental gn flags).
Not sure "pseudo module" is the best term here. Maybe an "all module" https://codereview.chromium.org/2812133003/diff/1/build/android/gradle/genera... File build/android/gradle/generate_gradle.py (right): https://codereview.chromium.org/2812133003/diff/1/build/android/gradle/genera... build/android/gradle/generate_gradle.py:39: _PSEUDO_NAME = 'a_pseudo_module' nit: is the "a" need to be there so it comes alphabetically first? Worth a comment if so. If not, I'd drop it. https://codereview.chromium.org/2812133003/diff/1/build/android/gradle/genera... build/android/gradle/generate_gradle.py:243: java_dirs = set() This is a singleton class, so it's a bit confusing to have these as class variables. Also as a nit: How about "all_java_dirs", or "seen_java_dirs" (likewise for prebuilts) https://codereview.chromium.org/2812133003/diff/1/build/android/gradle/genera... build/android/gradle/generate_gradle.py:552: """Returns the data for a pseudo build.gradle of all dirs.""" what makes it pseudo? Is it not a real module? Maybe just drop "pseudo" here. https://codereview.chromium.org/2812133003/diff/1/build/android/gradle/genera... build/android/gradle/generate_gradle.py:563: source_properties = _ReadPropertiesFile( Please figure out how to re-use rather than copy & paste. Probably this call should be lifted out of _GenerateGradleFile() since it's re-reading the same file for each entry. https://codereview.chromium.org/2812133003/diff/1/build/android/gradle/genera... build/android/gradle/generate_gradle.py:710: help='Prevent an overall pseudo module from being ' Please explain why you might ever want to do this in the help.
Will add docs next week, just wanted to send this out to get your input on the "all module" being default with no way to turn it off. Personally after thinking about it some more I feel that it's worth it to not have yet another option for something that does what one would expect android studio to do anyways, correctly indexing all included java files. https://codereview.chromium.org/2812133003/diff/1/build/android/gradle/genera... File build/android/gradle/generate_gradle.py (right): https://codereview.chromium.org/2812133003/diff/1/build/android/gradle/genera... build/android/gradle/generate_gradle.py:39: _PSEUDO_NAME = 'a_pseudo_module' On 2017/04/12 19:36:29, agrieve wrote: > nit: is the "a" need to be there so it comes alphabetically first? Worth a > comment if so. If not, I'd drop it. Added comment, changed to _all https://codereview.chromium.org/2812133003/diff/1/build/android/gradle/genera... build/android/gradle/generate_gradle.py:243: java_dirs = set() On 2017/04/12 19:36:29, agrieve wrote: > This is a singleton class, so it's a bit confusing to have these as class > variables. > > Also as a nit: How about "all_java_dirs", or "seen_java_dirs" (likewise for > prebuilts) Done. https://codereview.chromium.org/2812133003/diff/1/build/android/gradle/genera... build/android/gradle/generate_gradle.py:552: """Returns the data for a pseudo build.gradle of all dirs.""" On 2017/04/12 19:36:29, agrieve wrote: > what makes it pseudo? Is it not a real module? Maybe just drop "pseudo" here. Changed name to "All" https://codereview.chromium.org/2812133003/diff/1/build/android/gradle/genera... build/android/gradle/generate_gradle.py:563: source_properties = _ReadPropertiesFile( On 2017/04/12 19:36:29, agrieve wrote: > Please figure out how to re-use rather than copy & paste. Probably this call > should be lifted out of _GenerateGradleFile() since it's re-reading the same > file for each entry. Done. https://codereview.chromium.org/2812133003/diff/1/build/android/gradle/genera... build/android/gradle/generate_gradle.py:710: help='Prevent an overall pseudo module from being ' On 2017/04/12 19:36:29, agrieve wrote: > Please explain why you might ever want to do this in the help. Removed. Hmm... just an escape hatch I added, not really necessary to add the code complexity for something that should always be done (it results in the "correct" interpretation of our code in Android Studio with regard to references and imports).
On 2017/04/13 17:23:17, Peter Wen wrote: > Will add docs next week, just wanted to send this out to get your input on the > "all module" being default with no way to turn it off. > > Personally after thinking about it some more I feel that it's worth it to not > have yet another option for something that does what one would expect android > studio to do anyways, correctly indexing all included java files. > > https://codereview.chromium.org/2812133003/diff/1/build/android/gradle/genera... > File build/android/gradle/generate_gradle.py (right): > > https://codereview.chromium.org/2812133003/diff/1/build/android/gradle/genera... > build/android/gradle/generate_gradle.py:39: _PSEUDO_NAME = 'a_pseudo_module' > On 2017/04/12 19:36:29, agrieve wrote: > > nit: is the "a" need to be there so it comes alphabetically first? Worth a > > comment if so. If not, I'd drop it. > > Added comment, changed to _all > > https://codereview.chromium.org/2812133003/diff/1/build/android/gradle/genera... > build/android/gradle/generate_gradle.py:243: java_dirs = set() > On 2017/04/12 19:36:29, agrieve wrote: > > This is a singleton class, so it's a bit confusing to have these as class > > variables. > > > > Also as a nit: How about "all_java_dirs", or "seen_java_dirs" (likewise for > > prebuilts) > > Done. > > https://codereview.chromium.org/2812133003/diff/1/build/android/gradle/genera... > build/android/gradle/generate_gradle.py:552: """Returns the data for a pseudo > build.gradle of all dirs.""" > On 2017/04/12 19:36:29, agrieve wrote: > > what makes it pseudo? Is it not a real module? Maybe just drop "pseudo" here. > > Changed name to "All" > > https://codereview.chromium.org/2812133003/diff/1/build/android/gradle/genera... > build/android/gradle/generate_gradle.py:563: source_properties = > _ReadPropertiesFile( > On 2017/04/12 19:36:29, agrieve wrote: > > Please figure out how to re-use rather than copy & paste. Probably this call > > should be lifted out of _GenerateGradleFile() since it's re-reading the same > > file for each entry. > > Done. > > https://codereview.chromium.org/2812133003/diff/1/build/android/gradle/genera... > build/android/gradle/generate_gradle.py:710: help='Prevent an overall pseudo > module from being ' > On 2017/04/12 19:36:29, agrieve wrote: > > Please explain why you might ever want to do this in the help. > > Removed. > > Hmm... just an escape hatch I added, not really necessary to add the code > complexity for something that should always be done (it results in the "correct" > interpretation of our code in Android Studio with regard to references and > imports). forgot to upload patchset?
Description was changed from ========== Android: Add pseudo module for Android Studio Add a pseudo overall module which includes all directories of targets so that code analysis works properly. It has a simplistic view that ignores the exclude filters. Use exclude filters to remove all code from that pseudo module so that gradle is happy, it understands the exclude filters. Also fix all targets to include tests. Make it easier to make sweeping java refactors in Android Studio. BUG=620034 ========== to ========== Android: Add module "All" for Android Studio Adding all sources to an "All" module fixes Studio's code analysis functions. In order to have things build properly in gradle, the "All" module has all sources excluded (gradle applies the filters, studio does not). Also fix all targets to include tests. Make it easier to make sweeping java refactors in Android Studio. BUG=620034 ==========
lgtm. Agree not having the flag is better. https://codereview.chromium.org/2812133003/diff/20001/build/android/gradle/ge... File build/android/gradle/generate_gradle.py (right): https://codereview.chromium.org/2812133003/diff/20001/build/android/gradle/ge... build/android/gradle/generate_gradle.py:555: java_dirs = list(generator.processed_java_dirs) nit: you can replace: list(foo) foo.sort() with: sorted(foo)
On 2017/04/13 19:26:12, agrieve wrote: > lgtm. Agree not having the flag is better. > > https://codereview.chromium.org/2812133003/diff/20001/build/android/gradle/ge... > File build/android/gradle/generate_gradle.py (right): > > https://codereview.chromium.org/2812133003/diff/20001/build/android/gradle/ge... > build/android/gradle/generate_gradle.py:555: java_dirs = > list(generator.processed_java_dirs) > nit: you can replace: > > list(foo) > foo.sort() > > with: > sorted(foo) FYI - I updated your cl description.
Added docs, sending to CQ https://codereview.chromium.org/2812133003/diff/20001/build/android/gradle/ge... File build/android/gradle/generate_gradle.py (right): https://codereview.chromium.org/2812133003/diff/20001/build/android/gradle/ge... build/android/gradle/generate_gradle.py:555: java_dirs = list(generator.processed_java_dirs) On 2017/04/13 19:26:12, agrieve wrote: > nit: you can replace: > > list(foo) > foo.sort() > > with: > sorted(foo) Done.
The CQ bit was checked by wnwen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from agrieve@chromium.org Link to the patchset: https://codereview.chromium.org/2812133003/#ps40001 (title: "Update docs and switch to sorted")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Android: Add module "All" for Android Studio Adding all sources to an "All" module fixes Studio's code analysis functions. In order to have things build properly in gradle, the "All" module has all sources excluded (gradle applies the filters, studio does not). Also fix all targets to include tests. Make it easier to make sweeping java refactors in Android Studio. BUG=620034 ========== to ========== Android: Add module "_all" for Android Studio Adding all sources to a "_all" pseudo module fixes Studio's code analysis functions (imports, refactoring). In order to have things build properly in gradle, the "_all" module has all sources excluded (gradle applies the filters, studio does not). Also fix all targets to include tests. Make it easier to make sweeping java refactors in Android Studio. BUG=620034 ==========
Description was changed from ========== Android: Add module "_all" for Android Studio Adding all sources to a "_all" pseudo module fixes Studio's code analysis functions (imports, refactoring). In order to have things build properly in gradle, the "_all" module has all sources excluded (gradle applies the filters, studio does not). Also fix all targets to include tests. Make it easier to make sweeping java refactors in Android Studio. BUG=620034 ========== to ========== Android: Add module "_all" for Android Studio Adding all sources to a "_all" pseudo module fixes Studio's code analysis functions (imports, refactoring). In order to have things build properly in gradle, the "_all" module has all sources excluded (gradle applies the filters, studio does not). Also fix "--all" targets to include tests. Make it easier to make sweeping java refactors in Android Studio. BUG=620034 ==========
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1492625736750760,
"parent_rev": "e51782890830ad8e709199ad65ce66e52af90441", "commit_rev":
"03427bcc41560b36b5c595eda92e35f563a6303e"}
Message was sent while issue was closed.
Description was changed from ========== Android: Add module "_all" for Android Studio Adding all sources to a "_all" pseudo module fixes Studio's code analysis functions (imports, refactoring). In order to have things build properly in gradle, the "_all" module has all sources excluded (gradle applies the filters, studio does not). Also fix "--all" targets to include tests. Make it easier to make sweeping java refactors in Android Studio. BUG=620034 ========== to ========== Android: Add module "_all" for Android Studio Adding all sources to a "_all" pseudo module fixes Studio's code analysis functions (imports, refactoring). In order to have things build properly in gradle, the "_all" module has all sources excluded (gradle applies the filters, studio does not). Also fix "--all" targets to include tests. Make it easier to make sweeping java refactors in Android Studio. BUG=620034 Review-Url: https://codereview.chromium.org/2812133003 Cr-Commit-Position: refs/heads/master@{#465700} Committed: https://chromium.googlesource.com/chromium/src/+/03427bcc41560b36b5c595eda92e... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/03427bcc41560b36b5c595eda92e...
Message was sent while issue was closed.
https://codereview.chromium.org/2812133003/diff/40001/build/android/gradle/ge... File build/android/gradle/generate_gradle.py (right): https://codereview.chromium.org/2812133003/diff/40001/build/android/gradle/ge... build/android/gradle/generate_gradle.py:702: assert not args.use_gradle_process_resources, ( Why did the meaning of this check change? Now it asserts that use_gradle_process_resouces is not set when using split_projects. I think this is a bug?
Message was sent while issue was closed.
sakal@chromium.org changed reviewers: + sakal@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2812133003/diff/40001/build/android/gradle/an... File build/android/gradle/android.jinja (right): https://codereview.chromium.org/2812133003/diff/40001/build/android/gradle/an... build/android/gradle/android.jinja:22: {% if variables.jniLibs is defined %} Should be jni_libs not jniLibs.
Message was sent while issue was closed.
Thanks for finding the bugs! https://codereview.chromium.org/2812133003/diff/40001/build/android/gradle/an... File build/android/gradle/android.jinja (right): https://codereview.chromium.org/2812133003/diff/40001/build/android/gradle/an... build/android/gradle/android.jinja:22: {% if variables.jniLibs is defined %} On 2017/05/04 09:37:39, sakal-chromium wrote: > Should be jni_libs not jniLibs. +1 will fix in follow-up CL. https://codereview.chromium.org/2812133003/diff/40001/build/android/gradle/ge... File build/android/gradle/generate_gradle.py (right): https://codereview.chromium.org/2812133003/diff/40001/build/android/gradle/ge... build/android/gradle/generate_gradle.py:702: assert not args.use_gradle_process_resources, ( On 2017/05/04 09:11:00, sakal-chromium wrote: > Why did the meaning of this check change? Now it asserts that > use_gradle_process_resouces is not set when using split_projects. I think this > is a bug? Yes, you are correct, it's a bug. I'll fix it in a follow-up CL. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
