Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1516053002:
[Chromecast][BuildFix] Remove circular dependency in GN. (Closed)
Description[Chromecast] Remove circular dependency in GN.
Remove unnecesary dep from //ui/ozone/platform/cast which was creating a
dep cycle. Add a dep to //ui/app_list to keep `gn check` clean.
Verified by is_chromecast=true build.
BUG=
Committed: https://crrev.com/a29d2dcd6d0a31c1e6cc258e533c9e1b6cb44e33
Cr-Commit-Position: refs/heads/master@{#364817}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Remove redundant sub-dependency. #
Messages
Total messages: 36 (13 generated)
slan@chromium.org changed reviewers: + calamity@chromium.org, halliwell@chromium.org
+halliwell@ for //ui/ozone/platform/cast OWNERS +calamity@ for //ui/app_list OWNERS All, This minor change fixes some breakage for Chromecast GN builds. PTAL.
Description was changed from ========== [Chromecast] Remove circular dependency in GN. Remove unnecesary dep from //ui/ozone/platform/cast which was creating a dep cycle. Add a dep to //ui/app_list to keep `gn check` clean. Verified by is_chromecast=true build. BUG= ========== to ========== [Chromecast] Remove circular dependency in GN. Remove unnecesary dep from //ui/ozone/platform/cast which was creating a dep cycle. Add a dep to //ui/app_list to keep `gn check` clean. Verified by is_chromecast=true build. BUG= ==========
https://codereview.chromium.org/1516053002/diff/1/ui/ozone/platform/cast/BUIL... File ui/ozone/platform/cast/BUILD.gn (left): https://codereview.chromium.org/1516053002/diff/1/ui/ozone/platform/cast/BUIL... ui/ozone/platform/cast/BUILD.gn:42: "//ui/platform_window", Naively, it looks like PlatformWindowCast needs this. What am I missing?
https://codereview.chromium.org/1516053002/diff/1/ui/ozone/platform/cast/BUIL... File ui/ozone/platform/cast/BUILD.gn (left): https://codereview.chromium.org/1516053002/diff/1/ui/ozone/platform/cast/BUIL... ui/ozone/platform/cast/BUILD.gn:42: "//ui/platform_window", On 2015/12/10 21:13:16, halliwell wrote: > Naively, it looks like PlatformWindowCast needs this. What am I missing? Honestly did no further check than making sure it compiles. Let me dig a little deeper.
The CQ bit was checked by halliwell@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/1516053002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1516053002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1516053002/diff/1/ui/ozone/platform/cast/BUIL... File ui/ozone/platform/cast/BUILD.gn (left): https://codereview.chromium.org/1516053002/diff/1/ui/ozone/platform/cast/BUIL... ui/ozone/platform/cast/BUILD.gn:42: "//ui/platform_window", On 2015/12/10 21:14:58, slan wrote: > On 2015/12/10 21:13:16, halliwell wrote: > > Naively, it looks like PlatformWindowCast needs this. What am I missing? > > Honestly did no further check than making sure it compiles. Let me dig a little > deeper. Just circling back on this now. So here's what's going on: We are including //u/p/platform_window.h and //u/p/platform_window_delegate.h, both of which are virtual interfaces. So the headers will be included with our code, but we do not actually need to depend on any target, because there is no base class impl to link. You are right that because we are including this file, gn check should be complaining that there needs to be a dependency. Unfortunately, //ui/ozone is a known mess: https://code.google.com/p/chromium/codesearch#chromium/src/.gn&l=83 And thus this file is not explicitly whitelisted for gn check, so gn will not throw a fit when this is removed. All the other platforms likely have the same issues. This will be corrected when somebody (probably us or CrOS guys) sweeps through and fixes up all the platforms. This most likely involves making a "public" target for platform window, or removing the strict dependencies from ozone => ozone platforms. Either way, the change is outside the scope of this CL IMO. What do you think?
On 2015/12/11 00:13:01, slan wrote: > https://codereview.chromium.org/1516053002/diff/1/ui/ozone/platform/cast/BUIL... > File ui/ozone/platform/cast/BUILD.gn (left): > > https://codereview.chromium.org/1516053002/diff/1/ui/ozone/platform/cast/BUIL... > ui/ozone/platform/cast/BUILD.gn:42: "//ui/platform_window", > On 2015/12/10 21:14:58, slan wrote: > > On 2015/12/10 21:13:16, halliwell wrote: > > > Naively, it looks like PlatformWindowCast needs this. What am I missing? > > > > Honestly did no further check than making sure it compiles. Let me dig a > little > > deeper. > > Just circling back on this now. So here's what's going on: > > We are including //u/p/platform_window.h and //u/p/platform_window_delegate.h, > both of which are virtual interfaces. So the headers will be included with our > code, but we do not actually need to depend on any target, because there is no > base class impl to link. > > You are right that because we are including this file, gn check should be > complaining that there needs to be a dependency. > Unfortunately, //ui/ozone is a known mess: > https://code.google.com/p/chromium/codesearch#chromium/src/.gn&l=83 > > And thus this file is not explicitly whitelisted for gn check, so gn will not > throw a fit when this is removed. All the other platforms likely have the same > issues. This will be corrected when somebody (probably us or CrOS guys) sweeps > through and fixes up all the platforms. This most likely involves making a > "public" target for platform window, or removing the strict dependencies from > ozone => ozone platforms. > > Either way, the change is outside the scope of this CL IMO. What do you think? So if this file isn't whitelisted for gn check, why does it need fixing? Is the circular dependency not part of gn check? (forgive my lack of gn knowledge)
On 2015/12/11 00:20:42, halliwell wrote: > On 2015/12/11 00:13:01, slan wrote: > > > https://codereview.chromium.org/1516053002/diff/1/ui/ozone/platform/cast/BUIL... > > File ui/ozone/platform/cast/BUILD.gn (left): > > > > > https://codereview.chromium.org/1516053002/diff/1/ui/ozone/platform/cast/BUIL... > > ui/ozone/platform/cast/BUILD.gn:42: "//ui/platform_window", > > On 2015/12/10 21:14:58, slan wrote: > > > On 2015/12/10 21:13:16, halliwell wrote: > > > > Naively, it looks like PlatformWindowCast needs this. What am I missing? > > > > > > Honestly did no further check than making sure it compiles. Let me dig a > > little > > > deeper. > > > > Just circling back on this now. So here's what's going on: > > > > We are including //u/p/platform_window.h and //u/p/platform_window_delegate.h, > > both of which are virtual interfaces. So the headers will be included with our > > code, but we do not actually need to depend on any target, because there is no > > base class impl to link. > > > > You are right that because we are including this file, gn check should be > > complaining that there needs to be a dependency. > > Unfortunately, //ui/ozone is a known mess: > > https://code.google.com/p/chromium/codesearch#chromium/src/.gn&l=83 > > > > And thus this file is not explicitly whitelisted for gn check, so gn will not > > throw a fit when this is removed. All the other platforms likely have the same > > issues. This will be corrected when somebody (probably us or CrOS guys) sweeps > > through and fixes up all the platforms. This most likely involves making a > > "public" target for platform window, or removing the strict dependencies from > > ozone => ozone platforms. > > > > Either way, the change is outside the scope of this CL IMO. What do you think? > > So if this file isn't whitelisted for gn check, why does it need fixing? Is the > circular dependency not part of gn check? (forgive my lack of gn knowledge) The circular dependency occurs before gn check can ever execute. The deps are used to determine build order of components, what to rebuild when something is touched, etc. gn check simply maps #includes to the targets that contain these files, then ensures that you are depending on something if you are including it.
On 2015/12/11 00:23:17, slan wrote: > On 2015/12/11 00:20:42, halliwell wrote: > > On 2015/12/11 00:13:01, slan wrote: > > > > > > https://codereview.chromium.org/1516053002/diff/1/ui/ozone/platform/cast/BUIL... > > > File ui/ozone/platform/cast/BUILD.gn (left): > > > > > > > > > https://codereview.chromium.org/1516053002/diff/1/ui/ozone/platform/cast/BUIL... > > > ui/ozone/platform/cast/BUILD.gn:42: "//ui/platform_window", > > > On 2015/12/10 21:14:58, slan wrote: > > > > On 2015/12/10 21:13:16, halliwell wrote: > > > > > Naively, it looks like PlatformWindowCast needs this. What am I > missing? > > > > > > > > Honestly did no further check than making sure it compiles. Let me dig a > > > little > > > > deeper. > > > > > > Just circling back on this now. So here's what's going on: > > > > > > We are including //u/p/platform_window.h and > //u/p/platform_window_delegate.h, > > > both of which are virtual interfaces. So the headers will be included with > our > > > code, but we do not actually need to depend on any target, because there is > no > > > base class impl to link. > > > > > > You are right that because we are including this file, gn check should be > > > complaining that there needs to be a dependency. > > > Unfortunately, //ui/ozone is a known mess: > > > https://code.google.com/p/chromium/codesearch#chromium/src/.gn&l=83 > > > > > > And thus this file is not explicitly whitelisted for gn check, so gn will > not > > > throw a fit when this is removed. All the other platforms likely have the > same > > > issues. This will be corrected when somebody (probably us or CrOS guys) > sweeps > > > through and fixes up all the platforms. This most likely involves making a > > > "public" target for platform window, or removing the strict dependencies > from > > > ozone => ozone platforms. > > > > > > Either way, the change is outside the scope of this CL IMO. What do you > think? > > > > So if this file isn't whitelisted for gn check, why does it need fixing? Is > the > > circular dependency not part of gn check? (forgive my lack of gn knowledge) > > The circular dependency occurs before gn check can ever execute. The deps are > used to determine build order of components, what to rebuild when something is > touched, etc. gn check simply maps #includes to the targets that contain these > files, then ensures that you are depending on something if you are including it. Some more information: The cycle was created here (https://codereview.chromium.org/1501823003) to make //ui/platform_window pass gn check. I think that the file requiring that //ui/platform_window depends on //ui/base/ime can be moved to //ui/base/ime, which will break that dependency and solve our problem. Talking this option over with penghuang@. I think that we should still submit this in the meantime though.
On 2015/12/11 00:32:57, slan wrote: > On 2015/12/11 00:23:17, slan wrote: > > On 2015/12/11 00:20:42, halliwell wrote: > > > On 2015/12/11 00:13:01, slan wrote: > > > > > > > > > > https://codereview.chromium.org/1516053002/diff/1/ui/ozone/platform/cast/BUIL... > > > > File ui/ozone/platform/cast/BUILD.gn (left): > > > > > > > > > > > > > > https://codereview.chromium.org/1516053002/diff/1/ui/ozone/platform/cast/BUIL... > > > > ui/ozone/platform/cast/BUILD.gn:42: "//ui/platform_window", > > > > On 2015/12/10 21:14:58, slan wrote: > > > > > On 2015/12/10 21:13:16, halliwell wrote: > > > > > > Naively, it looks like PlatformWindowCast needs this. What am I > > missing? > > > > > > > > > > Honestly did no further check than making sure it compiles. Let me dig a > > > > little > > > > > deeper. > > > > > > > > Just circling back on this now. So here's what's going on: > > > > > > > > We are including //u/p/platform_window.h and > > //u/p/platform_window_delegate.h, > > > > both of which are virtual interfaces. So the headers will be included with > > our > > > > code, but we do not actually need to depend on any target, because there > is > > no > > > > base class impl to link. > > > > > > > > You are right that because we are including this file, gn check should be > > > > complaining that there needs to be a dependency. > > > > Unfortunately, //ui/ozone is a known mess: > > > > https://code.google.com/p/chromium/codesearch#chromium/src/.gn&l=83 > > > > > > > > And thus this file is not explicitly whitelisted for gn check, so gn will > > not > > > > throw a fit when this is removed. All the other platforms likely have the > > same > > > > issues. This will be corrected when somebody (probably us or CrOS guys) > > sweeps > > > > through and fixes up all the platforms. This most likely involves making a > > > > "public" target for platform window, or removing the strict dependencies > > from > > > > ozone => ozone platforms. > > > > > > > > Either way, the change is outside the scope of this CL IMO. What do you > > think? > > > > > > So if this file isn't whitelisted for gn check, why does it need fixing? Is > > the > > > circular dependency not part of gn check? (forgive my lack of gn knowledge) > > > > The circular dependency occurs before gn check can ever execute. The deps are > > used to determine build order of components, what to rebuild when something is > > touched, etc. gn check simply maps #includes to the targets that contain these > > files, then ensures that you are depending on something if you are including > it. > > Some more information: The cycle was created here > (https://codereview.chromium.org/1501823003) to make //ui/platform_window pass > gn check. I think that the file requiring that //ui/platform_window depends on > //ui/base/ime can be moved to //ui/base/ime, which will break that dependency > and solve our problem. Talking this option over with penghuang@. > > I think that we should still submit this in the meantime though. lgtm, thanks for your patient explanations :)
spang@chromium.org changed reviewers: + spang@chromium.org
https://codereview.chromium.org/1516053002/diff/1/ui/ozone/platform/cast/BUIL... File ui/ozone/platform/cast/BUILD.gn (left): https://codereview.chromium.org/1516053002/diff/1/ui/ozone/platform/cast/BUIL... ui/ozone/platform/cast/BUILD.gn:42: "//ui/platform_window", On 2015/12/11 00:13:01, slan wrote: > On 2015/12/10 21:14:58, slan wrote: > > On 2015/12/10 21:13:16, halliwell wrote: > > > Naively, it looks like PlatformWindowCast needs this. What am I missing? > > > > Honestly did no further check than making sure it compiles. Let me dig a > little > > deeper. > > Just circling back on this now. So here's what's going on: > > We are including //u/p/platform_window.h and //u/p/platform_window_delegate.h, > both of which are virtual interfaces. So the headers will be included with our > code, but we do not actually need to depend on any target, because there is no > base class impl to link. > > You are right that because we are including this file, gn check should be > complaining that there needs to be a dependency. > Unfortunately, //ui/ozone is a known mess: > https://code.google.com/p/chromium/codesearch#chromium/src/.gn&l=83 > > And thus this file is not explicitly whitelisted for gn check, so gn will not > throw a fit when this is removed. All the other platforms likely have the same > issues. This will be corrected when somebody (probably us or CrOS guys) sweeps > through and fixes up all the platforms. This most likely involves making a > "public" target for platform window, or removing the strict dependencies from > ozone => ozone platforms. > > Either way, the change is outside the scope of this CL IMO. What do you think? This cycle seems to have been added in https://codereview.chromium.org/1286383003 Maybe you should file a bug for penghuang to look at?
On 2015/12/11 00:40:26, spang wrote: > https://codereview.chromium.org/1516053002/diff/1/ui/ozone/platform/cast/BUIL... > File ui/ozone/platform/cast/BUILD.gn (left): > > https://codereview.chromium.org/1516053002/diff/1/ui/ozone/platform/cast/BUIL... > ui/ozone/platform/cast/BUILD.gn:42: "//ui/platform_window", > On 2015/12/11 00:13:01, slan wrote: > > On 2015/12/10 21:14:58, slan wrote: > > > On 2015/12/10 21:13:16, halliwell wrote: > > > > Naively, it looks like PlatformWindowCast needs this. What am I missing? > > > > > > Honestly did no further check than making sure it compiles. Let me dig a > > little > > > deeper. > > > > Just circling back on this now. So here's what's going on: > > > > We are including //u/p/platform_window.h and //u/p/platform_window_delegate.h, > > both of which are virtual interfaces. So the headers will be included with our > > code, but we do not actually need to depend on any target, because there is no > > base class impl to link. > > > > You are right that because we are including this file, gn check should be > > complaining that there needs to be a dependency. > > Unfortunately, //ui/ozone is a known mess: > > https://code.google.com/p/chromium/codesearch#chromium/src/.gn&l=83 > > > > And thus this file is not explicitly whitelisted for gn check, so gn will not > > throw a fit when this is removed. All the other platforms likely have the same > > issues. This will be corrected when somebody (probably us or CrOS guys) sweeps > > through and fixes up all the platforms. This most likely involves making a > > "public" target for platform window, or removing the strict dependencies from > > ozone => ozone platforms. > > > > Either way, the change is outside the scope of this CL IMO. What do you think? > > This cycle seems to have been added in > https://codereview.chromium.org/1286383003 > > Maybe you should file a bug for penghuang to look at? Hey Michael, thanks for the note. This cycle was created here (https://codereview.chromium.org/1501823003) as Brett moved through //ui to make gn check pass. I just spoke with Peng - he agreed that we can move a class, breaking the dep from platform_window to base/ime. So I will take a bug out against him to attempt this. Currently, our GN builds are broken, so I would like to get this issue landed, then we can look at Peng's solution for long-term. Thanks!
mgiuca@chromium.org changed reviewers: + mgiuca@chromium.org
Can you share the output of gn check without the change in ui/app_list/BUILD.gn? I'm just interested in what is requiring this dependency. https://codereview.chromium.org/1516053002/diff/1/ui/app_list/BUILD.gn File ui/app_list/BUILD.gn (right): https://codereview.chromium.org/1516053002/diff/1/ui/app_list/BUILD.gn#newcode86 ui/app_list/BUILD.gn:86: "//ui/events:events_base", Does line 85 supersede line 86? Try removing it and see if everything still works.
mgiuca@chromium.org changed reviewers: - calamity@chromium.org
(Subbing myself in for calamity@ as app_list reviewer.)
On 2015/12/11 02:04:32, Matt Giuca wrote:
> Can you share the output of gn check without the change in
ui/app_list/BUILD.gn?
> I'm just interested in what is requiring this dependency.
Looks like events.h is needed:
ERROR at //ui/app_list/pagination_controller.cc:8:11: Can't include this header
from here.
#include "ui/events/event.h"
^----------------
The target:
//ui/app_list:app_list
is including a file from the target:
//ui/events:events
>
> https://codereview.chromium.org/1516053002/diff/1/ui/app_list/BUILD.gn
> File ui/app_list/BUILD.gn (right):
>
>
https://codereview.chromium.org/1516053002/diff/1/ui/app_list/BUILD.gn#newcode86
> ui/app_list/BUILD.gn:86: "//ui/events:events_base",
> Does line 85 supersede line 86? Try removing it and see if everything still
> works.
https://codereview.chromium.org/1516053002/diff/1/ui/app_list/BUILD.gn File ui/app_list/BUILD.gn (right): https://codereview.chromium.org/1516053002/diff/1/ui/app_list/BUILD.gn#newcode86 ui/app_list/BUILD.gn:86: "//ui/events:events_base", On 2015/12/11 02:04:32, Matt Giuca wrote: > Does line 85 supersede line 86? Try removing it and see if everything still > works. Good thought. Done.
The CQ bit was checked by slan@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/1516053002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1516053002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
ping for feedback from ui/app_list OWNERS! Currently, our internal builds are broken because of this.
On 2015/12/11 19:44:21, slan wrote: > ping for feedback from ui/app_list OWNERS! > > Currently, our internal builds are broken because of this. Sorry and thanks for the explanation. lgtm
The CQ bit was checked by slan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from halliwell@chromium.org Link to the patchset: https://codereview.chromium.org/1516053002/#ps20001 (title: "Remove redundant sub-dependency.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1516053002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1516053002/20001
Message was sent while issue was closed.
Description was changed from ========== [Chromecast] Remove circular dependency in GN. Remove unnecesary dep from //ui/ozone/platform/cast which was creating a dep cycle. Add a dep to //ui/app_list to keep `gn check` clean. Verified by is_chromecast=true build. BUG= ========== to ========== [Chromecast] Remove circular dependency in GN. Remove unnecesary dep from //ui/ozone/platform/cast which was creating a dep cycle. Add a dep to //ui/app_list to keep `gn check` clean. Verified by is_chromecast=true build. BUG= ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== [Chromecast] Remove circular dependency in GN. Remove unnecesary dep from //ui/ozone/platform/cast which was creating a dep cycle. Add a dep to //ui/app_list to keep `gn check` clean. Verified by is_chromecast=true build. BUG= ========== to ========== [Chromecast] Remove circular dependency in GN. Remove unnecesary dep from //ui/ozone/platform/cast which was creating a dep cycle. Add a dep to //ui/app_list to keep `gn check` clean. Verified by is_chromecast=true build. BUG= Committed: https://crrev.com/a29d2dcd6d0a31c1e6cc258e533c9e1b6cb44e33 Cr-Commit-Position: refs/heads/master@{#364817} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/a29d2dcd6d0a31c1e6cc258e533c9e1b6cb44e33 Cr-Commit-Position: refs/heads/master@{#364817} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
