Reviewing DEPS changes

Matt Falkenhagen

unread,

May 26, 2021, 7:58:55 PM5/26/21

to content-owners

Hi,

I occasionally get a CL to approve a DEPS change for something taking a dependency on //content /public (typically from //components).

I wonder if it'd be useful to put together a list of what to look out for for these changes.

Here is my current practice:

Try to consider whether it makes architectural sense for the directory to live above //content.
//components can take a dependency on //content/public.
Confirm the code is running in the correct process: e.g., //component/foo/browser can only add //content/public/browser. Some components run only in one process and don't have the explicit directory name.
Circular DEPS are not allowed. One way to check this is "git gs <directory>" inside //content. I don't know if presubmit automatically detects circular DEPS well.
Full guidelines for //components are in the README.

Anything else people look out for?

Kentaro Hara

unread,

May 26, 2021, 10:24:00 PM5/26/21

to Matt Falkenhagen, content-owners

Circular DEPS are not allowed. One way to check this is "git gs <directory>" inside //content. I don't know if presubmit automatically detects circular DEPS well.

(Slightly related topic) I sometimes struggle with reviewing DEPS changes in Blink that add a dependency from Blink to //components/ because it might introduce a circular dependency. It would be nice if circular dependencies are checked via tools. If we have one, would it break a component build maybe?

--
You received this message because you are subscribed to the Google Groups "content-owners" group.
To unsubscribe from this group and stop receiving emails from it, send an email to content-owner...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/content-owners/CAJ_xCimt0vwi2T%2BbYvY_wwKUtHd-qv1qW-eEmWfxHjx2iH_%2Byg%40mail.gmail.com.

--

Kentaro Hara, Tokyo

dan...@chromium.org

unread,

May 27, 2021, 10:57:26 AM5/27/21

to Kentaro Hara, Matt Falkenhagen, content-owners

On Wed, May 26, 2021 at 10:24 PM Kentaro Hara <har...@chromium.org> wrote:

Circular DEPS are not allowed. One way to check this is "git gs <directory>" inside //content. I don't know if presubmit automatically detects circular DEPS well.

(Slightly related topic) I sometimes struggle with reviewing DEPS changes in Blink that add a dependency from Blink to //components/ because it might introduce a circular dependency. It would be nice if circular dependencies are checked via tools. If we have one, would it break a component build maybe?

I know this was previously litigated, but if it's becoming a problem, perhaps we could revisit the decision to have no layering structure in the components directory. If we separated "things content consumes" from "things that consume content" and the same for blink,we could put a DEPS in some ancestor directory and not need to revisit this all the time.

On Thu, May 27, 2021 at 8:58 AM Matt Falkenhagen <fal...@chromium.org> wrote:
Hi,

I occasionally get a CL to approve a DEPS change for something taking a dependency on //content /public (typically from //components).

I wonder if it'd be useful to put together a list of what to look out for for these changes.

Here is my current practice:
Try to consider whether it makes architectural sense for the directory to live above //content.
//components can take a dependency on //content/public.
Confirm the code is running in the correct process: e.g., //component/foo/browser can only add //content/public/browser. Some components run only in one process and don't have the explicit directory name.
Circular DEPS are not allowed. One way to check this is "git gs <directory>" inside //content. I don't know if presubmit automatically detects circular DEPS well.
Full guidelines for //components are in the README.
Anything else people look out for?

--
You received this message because you are subscribed to the Google Groups "content-owners" group.
To unsubscribe from this group and stop receiving emails from it, send an email to content-owner...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/content-owners/CAJ_xCimt0vwi2T%2BbYvY_wwKUtHd-qv1qW-eEmWfxHjx2iH_%2Byg%40mail.gmail.com.

--
Kentaro Hara, Tokyo

--
You received this message because you are subscribed to the Google Groups "content-owners" group.
To unsubscribe from this group and stop receiving emails from it, send an email to content-owner...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/content-owners/CABg10jxWfVqo%3DjQNa%2BNjzSEu%2BUqb-X1CqZyi1nPLaVMT4ssUSg%40mail.gmail.com.

John Abd-El-Malek

unread,

May 27, 2021, 7:41:53 PM5/27/21

to Dana Jansens, Kentaro Hara, Matt Falkenhagen, content-owners

On Thu, May 27, 2021 at 7:57 AM <dan...@chromium.org> wrote:

On Wed, May 26, 2021 at 10:24 PM Kentaro Hara <har...@chromium.org> wrote:
Circular DEPS are not allowed. One way to check this is "git gs <directory>" inside //content. I don't know if presubmit automatically detects circular DEPS well.

(Slightly related topic) I sometimes struggle with reviewing DEPS changes in Blink that add a dependency from Blink to //components/ because it might introduce a circular dependency. It would be nice if circular dependencies are checked via tools.

Note that most of what we get from DEPS is not needed anymore once gn check is enabled everywhere.

gn should catch circular dependencies as well as DEPS right (although with both of course subdirs can work around this).

If we have one, would it break a component build maybe?

I know this was previously litigated, but if it's becoming a problem, perhaps we could revisit the decision to have no layering structure in the components directory. If we separated "things content consumes" from "things that consume content" and the same for blink,we could put a DEPS in some ancestor directory and not need to revisit this all the time.

On Thu, May 27, 2021 at 8:58 AM Matt Falkenhagen <fal...@chromium.org> wrote:
Hi,

I occasionally get a CL to approve a DEPS change for something taking a dependency on //content /public (typically from //components).

I wonder if it'd be useful to put together a list of what to look out for for these changes.

+1

Here is my current practice:
Try to consider whether it makes architectural sense for the directory to live above //content.
//components can take a dependency on //content/public.
Confirm the code is running in the correct process: e.g., //component/foo/browser can only add //content/public/browser. Some components run only in one process and don't have the explicit directory name.
Circular DEPS are not allowed. One way to check this is "git gs <directory>" inside //content. I don't know if presubmit automatically detects circular DEPS well.
Full guidelines for //components are in the README.
Anything else people look out for?

This is a bit of a combination of your first 2, but one other thing is to figure out if code really needs content/. Rarely I've seen cases where some code depends on content just for BrowserThread, where you could instead inject the main task runner (or they can grab it from the static getter on creation of their object).

--
You received this message because you are subscribed to the Google Groups "content-owners" group.
To unsubscribe from this group and stop receiving emails from it, send an email to content-owner...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/content-owners/CAJ_xCimt0vwi2T%2BbYvY_wwKUtHd-qv1qW-eEmWfxHjx2iH_%2Byg%40mail.gmail.com.

--
Kentaro Hara, Tokyo

--
You received this message because you are subscribed to the Google Groups "content-owners" group.
To unsubscribe from this group and stop receiving emails from it, send an email to content-owner...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/content-owners/CABg10jxWfVqo%3DjQNa%2BNjzSEu%2BUqb-X1CqZyi1nPLaVMT4ssUSg%40mail.gmail.com.

--
You received this message because you are subscribed to the Google Groups "content-owners" group.
To unsubscribe from this group and stop receiving emails from it, send an email to content-owner...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/content-owners/CAHtyhaR%2Bjv0Z0YAkqHtn29rmsJm34FGnoPnnOjcCm3%2BADrXxqQ%40mail.gmail.com.

dan...@chromium.org

unread,

Jun 1, 2021, 10:35:37 AM6/1/21

to John Abd-El-Malek, Kentaro Hara, Matt Falkenhagen, content-owners

On Thu, May 27, 2021 at 7:41 PM John Abd-El-Malek <j...@chromium.org> wrote:

On Thu, May 27, 2021 at 7:57 AM <dan...@chromium.org> wrote:
On Wed, May 26, 2021 at 10:24 PM Kentaro Hara <har...@chromium.org> wrote:
Circular DEPS are not allowed. One way to check this is "git gs <directory>" inside //content. I don't know if presubmit automatically detects circular DEPS well.

(Slightly related topic) I sometimes struggle with reviewing DEPS changes in Blink that add a dependency from Blink to //components/ because it might introduce a circular dependency. It would be nice if circular dependencies are checked via tools.

Note that most of what we get from DEPS is not needed anymore once gn check is enabled everywhere.

gn should catch circular dependencies as well as DEPS right (although with both of course subdirs can work around this).

Component builds in //content turn off GN checking, do you include that in "gn check is enabled everywhere"? Is there some documented path to getting to that point? While release (non-component) builds typically give you the safety needed on the bots, it won't guard anything that's only built on bots in component builds. That may or may not be enough for you.

Also note that adding a directory to DEPS requires the owners from that directory to agree. Adding a GN dependency has no such review requirement. While they're both useful I don't think they really provide the same thing at this point.

John Abd-El-Malek

unread,

Jun 1, 2021, 12:51:05 PM6/1/21

to Dana Jansens, Kentaro Hara, Matt Falkenhagen, content-owners

On Tue, Jun 1, 2021 at 7:35 AM <dan...@chromium.org> wrote:

On Thu, May 27, 2021 at 7:41 PM John Abd-El-Malek <j...@chromium.org> wrote:

On Thu, May 27, 2021 at 7:57 AM <dan...@chromium.org> wrote:
On Wed, May 26, 2021 at 10:24 PM Kentaro Hara <har...@chromium.org> wrote:
Circular DEPS are not allowed. One way to check this is "git gs <directory>" inside //content. I don't know if presubmit automatically detects circular DEPS well.

(Slightly related topic) I sometimes struggle with reviewing DEPS changes in Blink that add a dependency from Blink to //components/ because it might introduce a circular dependency. It would be nice if circular dependencies are checked via tools.

Note that most of what we get from DEPS is not needed anymore once gn check is enabled everywhere.

gn should catch circular dependencies as well as DEPS right (although with both of course subdirs can work around this).

Component builds in //content turn off GN checking, do you include that in "gn check is enabled everywhere"? Is there some documented path to getting to that point? While release (non-component) builds typically give you the safety needed on the bots, it won't guard anything that's only built on bots in component builds. That may or may not be enough for you.

My hunch is that the checking on non-component builds is sufficient. Do we have any examples where the component builds caught issues that weren't caught elsewhere?

Also note that adding a directory to DEPS requires the owners from that directory to agree. Adding a GN dependency has no such review requirement. While they're both useful I don't think they really provide the same thing at this point.

Yep, although I believe we can mostly accomplish the same thing with gn visibility rules? When we added that review restriction, it was only a subset of directories that needed this.

dan...@chromium.org

unread,

Jun 1, 2021, 12:58:35 PM6/1/21

to John Abd-El-Malek, Kentaro Hara, Matt Falkenhagen, content-owners

On Tue, Jun 1, 2021 at 12:51 PM John Abd-El-Malek <j...@chromium.org> wrote:

On Tue, Jun 1, 2021 at 7:35 AM <dan...@chromium.org> wrote:
On Thu, May 27, 2021 at 7:41 PM John Abd-El-Malek <j...@chromium.org> wrote:

On Thu, May 27, 2021 at 7:57 AM <dan...@chromium.org> wrote:
On Wed, May 26, 2021 at 10:24 PM Kentaro Hara <har...@chromium.org> wrote:
Circular DEPS are not allowed. One way to check this is "git gs <directory>" inside //content. I don't know if presubmit automatically detects circular DEPS well.

(Slightly related topic) I sometimes struggle with reviewing DEPS changes in Blink that add a dependency from Blink to //components/ because it might introduce a circular dependency. It would be nice if circular dependencies are checked via tools.

Note that most of what we get from DEPS is not needed anymore once gn check is enabled everywhere.

gn should catch circular dependencies as well as DEPS right (although with both of course subdirs can work around this).

Component builds in //content turn off GN checking, do you include that in "gn check is enabled everywhere"? Is there some documented path to getting to that point? While release (non-component) builds typically give you the safety needed on the bots, it won't guard anything that's only built on bots in component builds. That may or may not be enough for you.

My hunch is that the checking on non-component builds is sufficient. Do we have any examples where the component builds caught issues that weren't caught elsewhere?

If we always build our tests with component build, then removing DEPS files could allow writing a test against content internals that shouldn't be.

We've seen incorrect DEPS into content/ internals before, so I guess GN did not catch those. There are still some.

It would seem to me that DEPS mostly prevents this, except in the cases where we've accidently added content incorrectly, and that we'd have no checks at all without DEPS.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/content-owners/CALhVsw0x81xdSDZ4LR22DyzFfU-_o6Soe4qDAr9jKdWntAtmTw%40mail.gmail.com.

John Abd-El-Malek

unread,

Jun 1, 2021, 3:22:29 PM6/1/21

to Dana Jansens, Kentaro Hara, Matt Falkenhagen, content-owners

On Tue, Jun 1, 2021 at 9:58 AM <dan...@chromium.org> wrote:

On Tue, Jun 1, 2021 at 12:51 PM John Abd-El-Malek <j...@chromium.org> wrote:

On Tue, Jun 1, 2021 at 7:35 AM <dan...@chromium.org> wrote:
On Thu, May 27, 2021 at 7:41 PM John Abd-El-Malek <j...@chromium.org> wrote:

On Thu, May 27, 2021 at 7:57 AM <dan...@chromium.org> wrote:
On Wed, May 26, 2021 at 10:24 PM Kentaro Hara <har...@chromium.org> wrote:
Circular DEPS are not allowed. One way to check this is "git gs <directory>" inside //content. I don't know if presubmit automatically detects circular DEPS well.

(Slightly related topic) I sometimes struggle with reviewing DEPS changes in Blink that add a dependency from Blink to //components/ because it might introduce a circular dependency. It would be nice if circular dependencies are checked via tools.

Note that most of what we get from DEPS is not needed anymore once gn check is enabled everywhere.

gn should catch circular dependencies as well as DEPS right (although with both of course subdirs can work around this).

Component builds in //content turn off GN checking, do you include that in "gn check is enabled everywhere"? Is there some documented path to getting to that point? While release (non-component) builds typically give you the safety needed on the bots, it won't guard anything that's only built on bots in component builds. That may or may not be enough for you.

My hunch is that the checking on non-component builds is sufficient. Do we have any examples where the component builds caught issues that weren't caught elsewhere?

If we always build our tests with component build, then removing DEPS files could allow writing a test against content internals that shouldn't be.

To make sure I understand: are you wondering about a scenario where there are tests that are only built in debug config? I'm not aware of any. I'd say even if there's an exception, almost all of our test configs run similarly on debug/release bots, e.g. with CQ doing rel+dcheck while waterfall does normal release + debug.

We've seen incorrect DEPS into content/ internals before, so I guess GN did not catch those. There are still some.

Yeah unfortunately some have leaked in due to reviewer not noticing. We have fixed most of them.

It would seem to me that DEPS mostly prevents this, except in the cases where we've accidently added content incorrectly, and that we'd have no checks at all without DEPS.

In this specific scenario, I've found gn visibility is better. This is because we can put a big comment say in content/test's GN file saying "DO NOT ALLOW CODE OUTSIDE OF CONTENT/". For DEPS allows, those are scattered all over the codebase (e.g. ui/, chrome/) and so it's easier for a content/owner to mistakenly allow it.

dan...@chromium.org

unread,

Jun 1, 2021, 3:29:59 PM6/1/21

to John Abd-El-Malek, Kentaro Hara, Matt Falkenhagen, content-owners

On Tue, Jun 1, 2021 at 3:22 PM John Abd-El-Malek <j...@chromium.org> wrote:

On Tue, Jun 1, 2021 at 9:58 AM <dan...@chromium.org> wrote:
On Tue, Jun 1, 2021 at 12:51 PM John Abd-El-Malek <j...@chromium.org> wrote:

On Tue, Jun 1, 2021 at 7:35 AM <dan...@chromium.org> wrote:
On Thu, May 27, 2021 at 7:41 PM John Abd-El-Malek <j...@chromium.org> wrote:

On Thu, May 27, 2021 at 7:57 AM <dan...@chromium.org> wrote:
On Wed, May 26, 2021 at 10:24 PM Kentaro Hara <har...@chromium.org> wrote:
Circular DEPS are not allowed. One way to check this is "git gs <directory>" inside //content. I don't know if presubmit automatically detects circular DEPS well.

(Slightly related topic) I sometimes struggle with reviewing DEPS changes in Blink that add a dependency from Blink to //components/ because it might introduce a circular dependency. It would be nice if circular dependencies are checked via tools.

Note that most of what we get from DEPS is not needed anymore once gn check is enabled everywhere.

gn should catch circular dependencies as well as DEPS right (although with both of course subdirs can work around this).

Component builds in //content turn off GN checking, do you include that in "gn check is enabled everywhere"? Is there some documented path to getting to that point? While release (non-component) builds typically give you the safety needed on the bots, it won't guard anything that's only built on bots in component builds. That may or may not be enough for you.

My hunch is that the checking on non-component builds is sufficient. Do we have any examples where the component builds caught issues that weren't caught elsewhere?

If we always build our tests with component build, then removing DEPS files could allow writing a test against content internals that shouldn't be.

To make sure I understand: are you wondering about a scenario where there are tests that are only built in debug config? I'm not aware of any. I'd say even if there's an exception, almost all of our test configs run similarly on debug/release bots, e.g. with CQ doing rel+dcheck while waterfall does normal release + debug.

No, component build disables the checks, not debug build. We mostly build release+component on bots, I believe. The fact that these tests are building I think says that GN is not catching it.

We've seen incorrect DEPS into content/ internals before, so I guess GN did not catch those. There are still some.

Yeah unfortunately some have leaked in due to reviewer not noticing. We have fixed most of them.

It would seem to me that DEPS mostly prevents this, except in the cases where we've accidently added content incorrectly, and that we'd have no checks at all without DEPS.

In this specific scenario, I've found gn visibility is better. This is because we can put a big comment say in content/test's GN file saying "DO NOT ALLOW CODE OUTSIDE OF CONTENT/". For DEPS allows, those are scattered all over the codebase (e.g. ui/, chrome/) and so it's easier for a content/owner to mistakenly allow it.

It's better for content internals certainly, which has very few consumers. Though it doesn't seem to be enabled enough to actually verify.

I'm not sure it scales for other places, where you would have to add a visibility for every build target that wants to use it. DEPS works for whole subtrees at a time. Answering can I use X from Y requires reading a (potentially a few) text files. In that world you'd always expect to have to add your target. Without extensive comments, you'd have to try add the new target to a BUILD.gn and seek a reviewer to tell you.

John Abd-El-Malek

unread,

Jun 1, 2021, 3:56:19 PM6/1/21

to Dana Jansens, Kentaro Hara, Matt Falkenhagen, content-owners

On Tue, Jun 1, 2021 at 12:29 PM <dan...@chromium.org> wrote:

On Tue, Jun 1, 2021 at 3:22 PM John Abd-El-Malek <j...@chromium.org> wrote:

On Tue, Jun 1, 2021 at 9:58 AM <dan...@chromium.org> wrote:
On Tue, Jun 1, 2021 at 12:51 PM John Abd-El-Malek <j...@chromium.org> wrote:

On Tue, Jun 1, 2021 at 7:35 AM <dan...@chromium.org> wrote:
On Thu, May 27, 2021 at 7:41 PM John Abd-El-Malek <j...@chromium.org> wrote:

On Thu, May 27, 2021 at 7:57 AM <dan...@chromium.org> wrote:
On Wed, May 26, 2021 at 10:24 PM Kentaro Hara <har...@chromium.org> wrote:
Circular DEPS are not allowed. One way to check this is "git gs <directory>" inside //content. I don't know if presubmit automatically detects circular DEPS well.

(Slightly related topic) I sometimes struggle with reviewing DEPS changes in Blink that add a dependency from Blink to //components/ because it might introduce a circular dependency. It would be nice if circular dependencies are checked via tools.

Note that most of what we get from DEPS is not needed anymore once gn check is enabled everywhere.

gn should catch circular dependencies as well as DEPS right (although with both of course subdirs can work around this).

Component builds in //content turn off GN checking, do you include that in "gn check is enabled everywhere"? Is there some documented path to getting to that point? While release (non-component) builds typically give you the safety needed on the bots, it won't guard anything that's only built on bots in component builds. That may or may not be enough for you.

My hunch is that the checking on non-component builds is sufficient. Do we have any examples where the component builds caught issues that weren't caught elsewhere?

If we always build our tests with component build, then removing DEPS files could allow writing a test against content internals that shouldn't be.

To make sure I understand: are you wondering about a scenario where there are tests that are only built in debug config? I'm not aware of any. I'd say even if there's an exception, almost all of our test configs run similarly on debug/release bots, e.g. with CQ doing rel+dcheck while waterfall does normal release + debug.

No, component build disables the checks, not debug build.

Right, sorry I was using debug as a proxy for component.

We mostly build release+component on bots, I believe.

Commit queue builds release non component. Waterfall builds debug (which is component) and release (non component). I don't believe we build release+component on waterfall or CQ.

dan...@chromium.org

unread,

Jun 1, 2021, 4:03:08 PM6/1/21

to John Abd-El-Malek, Kentaro Hara, Matt Falkenhagen, content-owners

On Tue, Jun 1, 2021 at 3:56 PM John Abd-El-Malek <j...@chromium.org> wrote:

On Tue, Jun 1, 2021 at 12:29 PM <dan...@chromium.org> wrote:
On Tue, Jun 1, 2021 at 3:22 PM John Abd-El-Malek <j...@chromium.org> wrote:

On Tue, Jun 1, 2021 at 9:58 AM <dan...@chromium.org> wrote:
On Tue, Jun 1, 2021 at 12:51 PM John Abd-El-Malek <j...@chromium.org> wrote:

On Tue, Jun 1, 2021 at 7:35 AM <dan...@chromium.org> wrote:
On Thu, May 27, 2021 at 7:41 PM John Abd-El-Malek <j...@chromium.org> wrote:

On Thu, May 27, 2021 at 7:57 AM <dan...@chromium.org> wrote:
On Wed, May 26, 2021 at 10:24 PM Kentaro Hara <har...@chromium.org> wrote:
Circular DEPS are not allowed. One way to check this is "git gs <directory>" inside //content. I don't know if presubmit automatically detects circular DEPS well.

(Slightly related topic) I sometimes struggle with reviewing DEPS changes in Blink that add a dependency from Blink to //components/ because it might introduce a circular dependency. It would be nice if circular dependencies are checked via tools.

Note that most of what we get from DEPS is not needed anymore once gn check is enabled everywhere.

gn should catch circular dependencies as well as DEPS right (although with both of course subdirs can work around this).

Component builds in //content turn off GN checking, do you include that in "gn check is enabled everywhere"? Is there some documented path to getting to that point? While release (non-component) builds typically give you the safety needed on the bots, it won't guard anything that's only built on bots in component builds. That may or may not be enough for you.

My hunch is that the checking on non-component builds is sufficient. Do we have any examples where the component builds caught issues that weren't caught elsewhere?

If we always build our tests with component build, then removing DEPS files could allow writing a test against content internals that shouldn't be.

To make sure I understand: are you wondering about a scenario where there are tests that are only built in debug config? I'm not aware of any. I'd say even if there's an exception, almost all of our test configs run similarly on debug/release bots, e.g. with CQ doing rel+dcheck while waterfall does normal release + debug.

No, component build disables the checks, not debug build.

Right, sorry I was using debug as a proxy for component.

We mostly build release+component on bots, I believe.

Commit queue builds release non component. Waterfall builds debug (which is component) and release (non component). I don't believe we build release+component on waterfall or CQ.

I see, it seems like making content/test:test_support have reduced visibility in GN would be a good experiment to see how it feels to replace DEPS with visibility. I think we've derailed a bit though.

GN visbiility doesn't really change the question a content owner has to answer. It changes the question to be about a build target instead of a directory. But "is it right to depend on content/public" is still a question content owners have to answer.

It would be less work if this question could be answered fewer times (splitting components for example, so it's answered once for the whole tree), rather than more times. If changing the number of times the question needs to be asked isn't feasible, then a clear algorithm would help.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/content-owners/CALhVsw3KW-ZY-CSZ4qU_Tw7s0VMmhgDW8vh8Gq8ENZ0b7Fr2OQ%40mail.gmail.com.

John Abd-El-Malek

unread,

Jun 3, 2021, 7:20:48 PM6/3/21

to Dana Jansens, Kentaro Hara, Matt Falkenhagen, content-owners

@Matt: are you planning on adding the guidelines so far and put them in some content/ docs?

On Tue, Jun 1, 2021 at 1:03 PM <dan...@chromium.org> wrote:

On Tue, Jun 1, 2021 at 3:56 PM John Abd-El-Malek <j...@chromium.org> wrote:

On Tue, Jun 1, 2021 at 12:29 PM <dan...@chromium.org> wrote:
On Tue, Jun 1, 2021 at 3:22 PM John Abd-El-Malek <j...@chromium.org> wrote:

On Tue, Jun 1, 2021 at 9:58 AM <dan...@chromium.org> wrote:
On Tue, Jun 1, 2021 at 12:51 PM John Abd-El-Malek <j...@chromium.org> wrote:

On Tue, Jun 1, 2021 at 7:35 AM <dan...@chromium.org> wrote:
On Thu, May 27, 2021 at 7:41 PM John Abd-El-Malek <j...@chromium.org> wrote:

On Thu, May 27, 2021 at 7:57 AM <dan...@chromium.org> wrote:
On Wed, May 26, 2021 at 10:24 PM Kentaro Hara <har...@chromium.org> wrote:
Circular DEPS are not allowed. One way to check this is "git gs <directory>" inside //content. I don't know if presubmit automatically detects circular DEPS well.

(Slightly related topic) I sometimes struggle with reviewing DEPS changes in Blink that add a dependency from Blink to //components/ because it might introduce a circular dependency. It would be nice if circular dependencies are checked via tools.

Note that most of what we get from DEPS is not needed anymore once gn check is enabled everywhere.

gn should catch circular dependencies as well as DEPS right (although with both of course subdirs can work around this).

Component builds in //content turn off GN checking, do you include that in "gn check is enabled everywhere"? Is there some documented path to getting to that point? While release (non-component) builds typically give you the safety needed on the bots, it won't guard anything that's only built on bots in component builds. That may or may not be enough for you.

My hunch is that the checking on non-component builds is sufficient. Do we have any examples where the component builds caught issues that weren't caught elsewhere?

If we always build our tests with component build, then removing DEPS files could allow writing a test against content internals that shouldn't be.

To make sure I understand: are you wondering about a scenario where there are tests that are only built in debug config? I'm not aware of any. I'd say even if there's an exception, almost all of our test configs run similarly on debug/release bots, e.g. with CQ doing rel+dcheck while waterfall does normal release + debug.

No, component build disables the checks, not debug build.

Right, sorry I was using debug as a proxy for component.

We mostly build release+component on bots, I believe.

Commit queue builds release non component. Waterfall builds debug (which is component) and release (non component). I don't believe we build release+component on waterfall or CQ.

I see, it seems like making content/test:test_support have reduced visibility in GN would be a good experiment to see how it feels to replace DEPS with visibility. I think we've derailed a bit though.

GN visbiility doesn't really change the question a content owner has to answer. It changes the question to be about a build target instead of a directory. But "is it right to depend on content/public" is still a question content owners have to answer.

/me nods

It would be less work if this question could be answered fewer times (splitting components for example, so it's answered once for the whole tree), rather than more times. If changing the number of times the question needs to be asked isn't feasible, then a clear algorithm would help.

Won't we still need to answer this per component, just in different times/places? e.g. if someone wants to add a new component that depends on content (and so would wonder which directory to put it in), or make an existing component depend on content (and so is wondering if they should move their component)?

Matt Falkenhagen

unread,

Jun 7, 2021, 5:29:23 AM6/7/21

to John Abd-El-Malek, Dana Jansens, Kentaro Hara, content-owners

2021年6月4日(金) 8:20 John Abd-El-Malek <j...@chromium.org>:

@Matt: are you planning on adding the guidelines so far and put them in some content/ docs?

Yep, I'm planning to upload a CL soon. Thanks for the discussion on this.

Matt Falkenhagen

unread,

Sep 7, 2021, 4:49:06 AM9/7/21

to John Abd-El-Malek, Dana Jansens, Kentaro Hara, content-owners

2021年6月7日(月) 18:29 Matt Falkenhagen <fal...@chromium.org>:

2021年6月4日(金) 8:20 John Abd-El-Malek <j...@chromium.org>:
@Matt: are you planning on adding the guidelines so far and put them in some content/ docs?

Yep, I'm planning to upload a CL soon. Thanks for the discussion on this.

I uploaded a CL for review here: https://chromium-review.googlesource.com/c/chromium/src/+/3144673

Comments welcome. Sorry for the delay.

Reply all

Reply to author

Forward