All Chromium targets that depend on Aura or Content end up with test_ime_driver in data-deps |
|||||
Issue descriptionWhile porting Chromium to Fuchsia we've been building the headless_shell, content_shell and cast_shell (audio-only) targets, without any graphics rendering enabled. All of these end up with data dependencies on e.g. test_ime_driver, even though they never use it, via their Aura and Content dependencies. e.g. for the audio-only cast_shell we see: //chromecast:cast_shell --[private]--> //content/public/app:both --[private]--> //content/public/renderer:renderer_sources --[private]--> //content/renderer:renderer --[private]--> //services/ui/public/cpp:cpp --[data]--> //services/ui:ui --[data]--> //services/ui/ime/test_ime_driver:test_ime_driver and //chromecast:cast_shell --[private]--> //chromecast:cast_shell_lib --[private]--> //chromecast/browser:browser --[private]--> //ui/aura:aura --[private]--> //services/ui/public/cpp:cpp --[data]--> //services/ui:ui --[data]--> //services/ui/ime/test_ime_driver:test_ime_driver though there are also data-dependencies from //ui/aura to //services/ui: //chromecast:cast_shell --[private]--> //chromecast:cast_shell_lib --[private]--> //chromecast/browser:browser --[private]--> //ui/aura:aura --[data]--> //services/ui:ui --[data]--> //services/ui/ime/test_ime_driver:test_ime_driver While it is expected that building-in UI components even though we never use them at run-time will cause some unused dependencies, there nonetheless appear to be the following issues: 1. //services/ui:ui has a data dependency on test_ime_driver, which is almost certainly bogus, assuming test_ime_driver is correctly-named. :) 2. //services/ui/public/cpp:cpp has a data dependency on the whole of //services/ui, which pulls in all possible UI services, regardless of whether the depending binary actually needs them. 3. Presumably it should also be possible to e.g. build headless_shell, content_shell or the non-audio-only cast_shell with the Viz service built-in to the main binary, rather than as dynamically loaded binary.
,
Nov 10 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5cab3191bdd3e3203e1bca2474572af22d5e528d commit 5cab3191bdd3e3203e1bca2474572af22d5e528d Author: Scott Violet <sky@chromium.org> Date: Fri Nov 10 18:50:17 2017 removes test_ime_driver from services/ui deps test_ime_driver is really only needed in tests, so it shouldn't be built everywhere. This removes the data_dep from services/ui:ui and makes tests that add the flag have the data_dep. BUG= 783326 TEST=none Change-Id: I27262b400be2d32d36ac3eaee46f850fe5608bf2 Reviewed-on: https://chromium-review.googlesource.com/762258 Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org> Commit-Queue: Scott Violet <sky@chromium.org> Cr-Commit-Position: refs/heads/master@{#515612} [modify] https://crrev.com/5cab3191bdd3e3203e1bca2474572af22d5e528d/ash/mus/BUILD.gn [modify] https://crrev.com/5cab3191bdd3e3203e1bca2474572af22d5e528d/services/ui/BUILD.gn [modify] https://crrev.com/5cab3191bdd3e3203e1bca2474572af22d5e528d/services/ui/common/switches.cc [modify] https://crrev.com/5cab3191bdd3e3203e1bca2474572af22d5e528d/services/ui/demo/BUILD.gn [modify] https://crrev.com/5cab3191bdd3e3203e1bca2474572af22d5e528d/services/ui/ws/BUILD.gn [modify] https://crrev.com/5cab3191bdd3e3203e1bca2474572af22d5e528d/ui/views/mus/BUILD.gn
,
Nov 10 2017
I removed the test_ime_driver explicit dep and made tests that need it depend upon it. So, your immediate concern has been addressed. That said, I left the //services/ui dep from ui/aura though. We plan on migrating aura to only use the UI service as some point, so we're keeping the dep. It may be that the services/ui dep should really be in executables/tests and aura shouldn't directly depend on services/ui.
,
Nov 10 2017
Thanks for the quick fix for test_ime_driver, Scott. Regarding the //ui/aura -> //services/ui dependency - will we still allow an "in-process" UI service, statically linked into embedders' main binaries, in future?
,
Nov 10 2017
Definitely. The more I think about this the more I think the dep on services/ui is wrong. The packaging should be left up to executables/tests. I will try to remove that dep shortly.
,
Nov 10 2017
,
Nov 10 2017
Yes, I think pushing these depenencies to the exe/test consumers makes most sense. Thanks!
,
Dec 8 2017
,
Jan 22 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/dd1c41e18906012be2b0f1145fbbb563aaa497a3 commit dd1c41e18906012be2b0f1145fbbb563aaa497a3 Author: Scott Violet <sky@chromium.org> Date: Mon Jan 22 22:53:14 2018 aura: push deps on ui service to exe targets Currently a handful of low level places depend upon //services/ui. This was done at a time when the only way to launch a service was via the service. Now that services may be packaged in other ways (such as baked into a binary), these dependencies on the service may result in unnecessarily building the target. This patch pushes the dependencies into the actual exe targets as they are the best place to make this decision as to whether the ui service is needed. BUG= 783326 TEST=none, build only changes Change-Id: I40f5773400b9d22a694b56f66e92b8965e9cc3e9 Reviewed-on: https://chromium-review.googlesource.com/879108 Reviewed-by: Michael Wasserman <msw@chromium.org> Reviewed-by: Scott Violet <sky@chromium.org> Commit-Queue: Scott Violet <sky@chromium.org> Cr-Commit-Position: refs/heads/master@{#531036} [modify] https://crrev.com/dd1c41e18906012be2b0f1145fbbb563aaa497a3/ash/autoclick/mus/BUILD.gn [modify] https://crrev.com/dd1c41e18906012be2b0f1145fbbb563aaa497a3/ash/touch_hud/mus/BUILD.gn [modify] https://crrev.com/dd1c41e18906012be2b0f1145fbbb563aaa497a3/content/shell/BUILD.gn [modify] https://crrev.com/dd1c41e18906012be2b0f1145fbbb563aaa497a3/content/test/BUILD.gn [modify] https://crrev.com/dd1c41e18906012be2b0f1145fbbb563aaa497a3/mash/catalog_viewer/BUILD.gn [modify] https://crrev.com/dd1c41e18906012be2b0f1145fbbb563aaa497a3/mash/example/window_type_launcher/BUILD.gn [modify] https://crrev.com/dd1c41e18906012be2b0f1145fbbb563aaa497a3/services/ui/public/cpp/BUILD.gn [modify] https://crrev.com/dd1c41e18906012be2b0f1145fbbb563aaa497a3/services/ui/ws/BUILD.gn [modify] https://crrev.com/dd1c41e18906012be2b0f1145fbbb563aaa497a3/ui/aura/BUILD.gn
,
Jan 22 2018
Only executable targets now depend upon //services/ui. In talking with Ken it seems we should make the executable targets package the ui service just as chrome does. But that's for a separate bug.
,
Feb 26 2018
|
|||||
►
Sign in to add a comment |
|||||
Comment 1 by w...@chromium.org
, Nov 9 2017