|
|
Created:
5 years, 1 month ago by Hadi Modified:
5 years, 1 month ago Reviewers:
mfomitchev, no sievers CC:
chromium-reviews, darin-cc_chromium.org, jam Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAura on Android:Remove unnecessary dependencies on ui/android.
BUG=NONE
Committed: https://crrev.com/d2ff47131f1a53a712adf860275a2f7320d9da9f
Cr-Commit-Position: refs/heads/master@{#360835}
Patch Set 1 : Minimal change to fix the build. #Patch Set 2 : Put ui_android inside if (!use_aura). #Patch Set 3 : Use assert(!use_aura). #
Total comments: 3
Patch Set 4 : Revert to patch set 1. #Patch Set 5 : Fix content/app/BUILD.gn on Android when use_aura is not defined explicitly. #
Total comments: 3
Patch Set 6 : address feedback from ps5. #
Created: 5 years, 1 month ago
Messages
Total messages: 21 (7 generated)
Description was changed from ========== Aura on Android: Remove dependency on //ui/android. BUG=NONE ========== to ========== Aura on Android: Remove dependency on //ui/android. BUG=NONE ==========
moshayedi@chromium.org changed reviewers: + mfomitchev@chromium.org, sievers@chromium.org
Patch set #2 puts ui_android inside "if (!use_aura)" so we can catch if something depends on ui_android. Patch set #3 adds "assert(!use_aura)" inside component(android). GN gives error for this one, since ui/android/BUILD.gn is included in some other places for things like ui_java, which causes evaluation of this file, and we don't know the component("android") block evaluates to ui_android without evaluating that block.
https://codereview.chromium.org/1460633005/diff/40001/components/web_contents... File components/web_contents_delegate_android/BUILD.gn (right): https://codereview.chromium.org/1460633005/diff/40001/components/web_contents... components/web_contents_delegate_android/BUILD.gn:33: deps += [ "//ui/android" ] this should not be needed since the aura build should not depend on web_contents_delegate_android
https://codereview.chromium.org/1460633005/diff/40001/ui/android/BUILD.gn File ui/android/BUILD.gn (right): https://codereview.chromium.org/1460633005/diff/40001/ui/android/BUILD.gn#new... ui/android/BUILD.gn:11: assert(!use_aura) I think this doesn't work, since we don't know if this block evaluates to "ui_android" without actually evaluating it. Please see patch set #2 as an alternative for making sure we don't depend on ui_android.
On 2015/11/19 22:23:10, sievers wrote: > https://codereview.chromium.org/1460633005/diff/40001/components/web_contents... > File components/web_contents_delegate_android/BUILD.gn (right): > > https://codereview.chromium.org/1460633005/diff/40001/components/web_contents... > components/web_contents_delegate_android/BUILD.gn:33: deps += [ "//ui/android" ] > this should not be needed since the aura build should not depend on > web_contents_delegate_android Yes, I think you're right. We're depending on web_contents_delegate_android from different places, so I thought I can do this as a temporary quick fix. If I comment out the web_contents_delegate_android, I get the following errors: //chrome/browser:browser(//build/toolchain/android:arm) needs //components/web_contents_delegate_android:web_contents_delegate_android(//build/toolchain/android:arm) //android_webview:common(//build/toolchain/android:arm) needs //components/web_contents_delegate_android:web_contents_delegate_android(//build/toolchain/android:arm) //chrome/android:chrome_shared_test_java__build_config(//build/toolchain/android:arm) needs //components/web_contents_delegate_android:web_contents_delegate_android_java(//build/toolchain/android:arm) //android_webview/glue:glue_resource_rewriter__build_config(//build/toolchain/android:arm) needs //components/web_contents_delegate_android:web_contents_delegate_android_java_resources(//build/toolchain/android:arm) //android_webview/native:native(//build/toolchain/android:arm) needs //components/web_contents_delegate_android:web_contents_delegate_android(//build/toolchain/android:arm) //android_webview:android_webview_java__build_config(//build/toolchain/android:arm) needs //components/web_contents_delegate_android:web_contents_delegate_android_java(//build/toolchain/android:arm) //android_webview/glue:glue__build_config(//build/toolchain/android:arm) needs //components/web_contents_delegate_android:web_contents_delegate_android_java_resources(//build/toolchain/android:arm) //chrome/android:chrome_java__build_config(//build/toolchain/android:arm) needs //components/web_contents_delegate_android:web_contents_delegate_android_java(//build/toolchain/android:arm) //chrome/test/android:chrome_java_test_support__build_config(//build/toolchain/android:arm) needs //components/web_contents_delegate_android:web_contents_delegate_android_java(//build/toolchain/android:arm) Probably we don't need some of these targets too, and I think removing each of them will require some other changes so it will be a bigger project. Maybe I can tackle fixing the dependency tree and removing dependency on targets we don't need (including ui_android) as one CL if it looks high priority.
https://codereview.chromium.org/1460633005/diff/40001/components/web_contents... File components/web_contents_delegate_android/BUILD.gn (right): https://codereview.chromium.org/1460633005/diff/40001/components/web_contents... components/web_contents_delegate_android/BUILD.gn:33: deps += [ "//ui/android" ] On 2015/11/19 22:23:09, sievers wrote: > this should not be needed since the aura build should not depend on > web_contents_delegate_android (I replied in the discussion thread in the bottom which didn't appear in the inlined comments, so I am repeating it here). Yes, I think you're right. We're depending on web_contents_delegate_android from different places, so I thought I can do this as a temporary quick fix. If I comment out the web_contents_delegate_android, I get the following errors: //chrome/browser:browser(//build/toolchain/android:arm) needs //components/web_contents_delegate_android:web_contents_delegate_android(//build/toolchain/android:arm) //android_webview:common(//build/toolchain/android:arm) needs //components/web_contents_delegate_android:web_contents_delegate_android(//build/toolchain/android:arm) //chrome/android:chrome_shared_test_java__build_config(//build/toolchain/android:arm) needs //components/web_contents_delegate_android:web_contents_delegate_android_java(//build/toolchain/android:arm) //android_webview/glue:glue_resource_rewriter__build_config(//build/toolchain/android:arm) needs //components/web_contents_delegate_android:web_contents_delegate_android_java_resources(//build/toolchain/android:arm) //android_webview/native:native(//build/toolchain/android:arm) needs //components/web_contents_delegate_android:web_contents_delegate_android(//build/toolchain/android:arm) //android_webview:android_webview_java__build_config(//build/toolchain/android:arm) needs //components/web_contents_delegate_android:web_contents_delegate_android_java(//build/toolchain/android:arm) //android_webview/glue:glue__build_config(//build/toolchain/android:arm) needs //components/web_contents_delegate_android:web_contents_delegate_android_java_resources(//build/toolchain/android:arm) //chrome/android:chrome_java__build_config(//build/toolchain/android:arm) needs //components/web_contents_delegate_android:web_contents_delegate_android_java(//build/toolchain/android:arm) //chrome/test/android:chrome_java_test_support__build_config(//build/toolchain/android:arm) needs //components/web_contents_delegate_android:web_contents_delegate_android_java(//build/toolchain/android:arm) Probably we don't need some of these targets too, and I think removing each of them will require some other changes so it will be a bigger project. Maybe I can tackle fixing the dependency tree and removing dependency on targets we don't need (including ui_android) as one CL if it looks high priority.
It's not that the GN file "evaluates" to ui_java or android. ui/android/BUILD.gn defines multiple "components"/"build targets". The main component is ui/android (defined by component("android")), but there's also ui/android:ui_java (defined by android_library("ui_java")). And if somebody depends on ui/android:ui_java (or any ui/android:XYZ, really), then ui/android/BUILD.gn gets evaluated, and then, yeah, we can't really put an assert there. In that light, I think your second approach makes sense, however I am wondering if we can get rid of all ui/android:XYZ dependencies for Android Aura? I don't see a good reason for Android Aura code to depend on ui/android:ui_java for example.
ps1 lgtm
Description was changed from ========== Aura on Android: Remove dependency on //ui/android. BUG=NONE ========== to ========== Aura on Android: Remove dependency of content_unittests on //ui/android. https://codereview.chromium.org/1419843002 which changed //ui/android broke the compilation of content_unittests on Aura Android. But Aura Android shouldn't depend on //ui/android. This change removes the dependency of content_unittests on //ui/android to fix that. BUG=NONE ==========
https://codereview.chromium.org/1460633005/diff/80001/content/app/BUILD.gn File content/app/BUILD.gn (right): https://codereview.chromium.org/1460633005/diff/80001/content/app/BUILD.gn#ne... content/app/BUILD.gn:53: if (!defined(use_aura) || !use_aura) { mfomitchev@/sievers@ do you know from top of your head why use_aura may not be defined here? It's not defined on Android when we don't pass use_aura explicitly. (See the tryjob errors in ps4).
Can you also please update the title: it's not just content_unittests that is affected - it's content/browser, content/app, and content/test. Maybe just say "Remove unnecessary dependencies on ui/android". https://codereview.chromium.org/1460633005/diff/80001/content/app/BUILD.gn File content/app/BUILD.gn (right): https://codereview.chromium.org/1460633005/diff/80001/content/app/BUILD.gn#ne... content/app/BUILD.gn:53: if (!defined(use_aura) || !use_aura) { On 2015/11/20 15:42:42, Hadi wrote: > mfomitchev@/sievers@ do you know from top of your head why use_aura may not be > defined here? It's not defined on Android when we don't pass use_aura > explicitly. (See the tryjob errors in ps4). I think you need to add 'import("//build/config/ui.gni")'. Adding a defined is not the right fix.
Description was changed from ========== Aura on Android: Remove dependency of content_unittests on //ui/android. https://codereview.chromium.org/1419843002 which changed //ui/android broke the compilation of content_unittests on Aura Android. But Aura Android shouldn't depend on //ui/android. This change removes the dependency of content_unittests on //ui/android to fix that. BUG=NONE ========== to ========== Aura on Android:Remove unnecessary dependencies on ui/android BUG=NONE ==========
https://codereview.chromium.org/1460633005/diff/80001/content/app/BUILD.gn File content/app/BUILD.gn (right): https://codereview.chromium.org/1460633005/diff/80001/content/app/BUILD.gn#ne... content/app/BUILD.gn:53: if (!defined(use_aura) || !use_aura) { On 2015/11/20 15:55:03, mfomitchev wrote: > On 2015/11/20 15:42:42, Hadi wrote: > > mfomitchev@/sievers@ do you know from top of your head why use_aura may not be > > defined here? It's not defined on Android when we don't pass use_aura > > explicitly. (See the tryjob errors in ps4). > > I think you need to add 'import("//build/config/ui.gni")'. > Adding a defined is not the right fix. Done.
Description was changed from ========== Aura on Android:Remove unnecessary dependencies on ui/android BUG=NONE ========== to ========== Aura on Android:Remove unnecessary dependencies on ui/android. BUG=NONE ==========
The CQ bit was checked by moshayedi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sievers@chromium.org Link to the patchset: https://codereview.chromium.org/1460633005/#ps100001 (title: "address feedback from ps5.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1460633005/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1460633005/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/d2ff47131f1a53a712adf860275a2f7320d9da9f Cr-Commit-Position: refs/heads/master@{#360835}
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:100001) has been created in https://codereview.chromium.org/1646693002/ by mfomitchev@chromium.org. The reason for reverting is: Reverting, since Android Aura has been cancelled.. |