PSA: OWNERS Reviews being Removed for DEPS "include_rules" Additions

[Andrew Grieve]

You currently need to get an OWNERS approval for paths that you add to a DEPS's "include_rules" list. This sometimes makes sense, but is more commonly just a rubber stamp review.

In order to reduce the number of these low-value reviews, I'd like to change the default behavior to not require an OWNERS review for additions to "include_rules" (crbug).

No review will be required by default, but directories can opt in. For example:

If //base/DEPS contains:

new_usages_require_review = True

Then a CL that added:

include_rules = [ "+base/foo/bar" ]

Would require a review from a //base OWNER

Action required:

To opt-in to needing reviews, please add new_usages_require_review = True to your DEPS files.

I'll plan to switch the default on October 24 (two weeks) to give everyone time to add the line, or to shout and convince me this is a terrible idea :P

[Adam Rice]

If //foo/bar requires review but //foo doesn't, can other components bypass the review requirement just by adding "//foo" to their include_rules?

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CABiQX1UbNvrje4GYcHEECVeoQdtWCxwhY___7RDF9PkL8XjrAQ%40mail.gmail.com.

[Colin Blundell]

On Fri, Oct 11, 2024 at 4:58 AM Adam Rice <ri...@chromium.org> wrote:

If //foo/bar requires review but //foo doesn't, can other components bypass the review requirement just by adding "//foo" to their include_rules?

+1 - this seems like a tricky thing to resolve.

 

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

[Andrew Grieve]

That's a good call out! I think this case is covered if //foo/bar also sets noparent=True, but wouldn't catch it in absence of that.

I don't see how this could be implemented in the absence of that, so please add noparent=True if this applies to you. 

[Colin Blundell]

Thanks!

On Tue, Oct 15, 2024 at 6:09 PM Andrew Grieve <agr...@chromium.org> wrote:

That's a good call out! I think this case is covered if //foo/bar also sets noparent=True, but wouldn't catch it in absence of that.

Could you verify (if you haven't) that setting noparent=True in //foo/bar means that a //foo/OWNERS +1 of a new DEP on //foo/bar won't satisfy the tooling?

 

I don't see how this could be implemented in the absence of that, so please add noparent=True if this applies to you. 

I suggest that we make it so that having `new_usages_require_review` *requires* `noparent`. The former is pretty much meaningless without the latter, but that might not be at all obvious when people add the flag (and nothing would necessarily alert them to it).

[Andrew Grieve]

On Tue, Oct 15, 2024 at 12:27 PM Colin Blundell <blun...@chromium.org> wrote:

Thanks!

On Tue, Oct 15, 2024 at 6:09 PM Andrew Grieve <agr...@chromium.org> wrote:

That's a good call out! I think this case is covered if //foo/bar also sets noparent=True, but wouldn't catch it in absence of that.

Could you verify (if you haven't) that setting noparent=True in //foo/bar means that a //foo/OWNERS +1 of a new DEP on //foo/bar won't satisfy the tooling?

The code I'm adding would not catch this at OWNERS approvals level. 

However, the code that actually checks your #includes against DEPS files (//tools/checkdeps), should fail in this scenario. If there's a bug with it, then that's kinda tangential to this change (but there are unit tests and many real usages, so it very likely works fine).

Once the normal checkdeps check forces you to add in the child entry, then the OWNERS check will kick in.

 

 

I don't see how this could be implemented in the absence of that, so please add noparent=True if this applies to you. 

I suggest that we make it so that having `new_usages_require_review` *requires* `noparent`. The former is pretty much meaningless without the latter, but that might not be at all obvious when people add the flag (and nothing would necessarily alert them to it).

This is a good point. It probably works in the other direction as well: why would you care enough to add "noparent=True" and then not care enough to enable "new_usages_require_review=True"?

There are 42 existing noparent=True. 

This brings me to a bit of a dilemma... Should we add checks to ensure both are set, have one flag imply the other, or just do away with one of them in favor of the other...

The 4 newly added new_usages_require_review=True instances do not set noparent=True. The ones that are one level deep probably don't bother because they don't have a meaningful parent, and the other two probably do also want noparent=True for the reasons described here.

So... I guess I'd lean towards removing new_usages_require_review in favor of noparent=True, but would welcome thoughts as to if they might be individually useful.

[Peter Kasting]

Perhaps just one flag, but it's named something semantic and general instead of behavioral and specific like these two flags. 

What common property of a directory results in behaviors like "ignore parents" and "require review of new usage" being true? 

"Usage requires human consideration"? "Sensitive contents"? "Usage is not mechanical"? "Local owners should verify uses"?

PK

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CABiQX1V_wg-0%2B1RrS50GJtuckQfghqYKb_0JMN-hbDzwLz1V2w%40mail.gmail.com.

[Tommy Nyquist]

Oooh, bikeshedding!

How about something like
[restricted|controlled|guarded|moderated]_dependency_management

--

Tommy

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAAHOzFDW%3DO%3Dzqin_Qas1N-EkQ%2BbFuOofJygqC-8c1wNH04jkbA%40mail.gmail.com.

[Andrew Grieve]

On Tue, Oct 15, 2024 at 4:37 PM Tommy Nyquist <nyq...@chromium.org> wrote:

Oooh, bikeshedding!

Indeed :) 

How about something like
[restricted|controlled|guarded|moderated]_dependency_management

All good candidates. I'm currently liking the idea of just having "new_usages_require_review=True" replace "noparent=True". It pretty accurately describes what "noparent=True" means.

[Colin Blundell]

On Wed, Oct 16, 2024 at 12:57 AM Andrew Grieve <agr...@chromium.org> wrote:

On Tue, Oct 15, 2024 at 4:37 PM Tommy Nyquist <nyq...@chromium.org> wrote:

Oooh, bikeshedding!

Indeed :) 

How about something like
[restricted|controlled|guarded|moderated]_dependency_management

All good candidates. I'm currently liking the idea of just having "new_usages_require_review=True" replace "noparent=True". It pretty accurately describes what "noparent=True" means.

 

--

Tommy

On Tue, Oct 15, 2024 at 12:47 PM Peter Kasting <pkas...@chromium.org> wrote:

Perhaps just one flag, but it's named something semantic and general instead of behavioral and specific like these two flags. 

What common property of a directory results in behaviors like "ignore parents" and "require review of new usage" being true? 

"Usage requires human consideration"? "Sensitive contents"? "Usage is not mechanical"? "Local owners should verify uses"?

PK

On Tue, Oct 15, 2024, 11:04 AM Andrew Grieve <agr...@chromium.org> wrote:

On Tue, Oct 15, 2024 at 12:27 PM Colin Blundell <blun...@chromium.org> wrote:

Thanks!

On Tue, Oct 15, 2024 at 6:09 PM Andrew Grieve <agr...@chromium.org> wrote:

That's a good call out! I think this case is covered if //foo/bar also sets noparent=True, but wouldn't catch it in absence of that.

Could you verify (if you haven't) that setting noparent=True in //foo/bar means that a //foo/OWNERS +1 of a new DEP on //foo/bar won't satisfy the tooling?

The code I'm adding would not catch this at OWNERS approvals level. 

However, the code that actually checks your #includes against DEPS files (//tools/checkdeps), should fail in this scenario. If there's a bug with it, then that's kinda tangential to this change (but there are unit tests and many real usages, so it very likely works fine).

Once the normal checkdeps check forces you to add in the child entry, then the OWNERS check will kick in.

I realize now that there was a conceptual overload here: I was thinking about "noparent" in OWNERS files, whereas you're referencing "noparent" in DEPS files.

I verified that checkdeps doesn't complain if a given dir foo has bar in its DEPS entries and then adds an include on a bar/baz that has `noparent` in its DEPS file. To me this makes sense: `noparent` impacts the answer to the question "Is bar/baz allowed to include a given file?", whereas the question we're talking about is "Is a given file allowed to include bar/baz?"

I actually think that these are semantically distinct questions and one question doesn't subsume the other. To me, for the case where `new_usages_require_review` is in bar/baz's DEPS files:

• We'd need to ensure that when a new include is added on bar/baz, bar/ being in DEPS of the dir with the include doesn't satisfy checkdeps.
• We'd need to ensure that when bar/baz is added to a DEPS entry, a +1 from an OWNER of bar/ doesn't satisfy the code review requirements (this is why I was thinking about "noparent" in OWNERS files, but the rule can be standalone regardless of whether "noparent" is in bar/baz/OWNERS).

I would gate making the change from the OP on implementing both of these. I would then just add the new flag separately from "noparent", and folks would be free to add one, the other, or both independently to their DEPS files.

[Andrew Grieve]

Aha, yes! I've been mixing up OWNERS vs DEPS noparent semantics!

I agree, and I'll work on updating checkdeps to handle:

• We'd need to ensure that when a new include is added on bar/baz, bar/ being in DEPS of the dir with the include doesn't satisfy checkdeps.

I think the existing OWNERS noparent should be sufficient to cover:

• We'd need to ensure that when bar/baz is added to a DEPS entry, a +1 from an OWNER of bar/ doesn't satisfy the code review requirements (this is why I was thinking about "noparent" in OWNERS files, but the rule can be standalone regardless of whether "noparent" is in bar/baz/OWNERS).

[Colin Blundell]

On Wed, Oct 16, 2024 at 6:50 PM Andrew Grieve <agr...@chromium.org> wrote:

Aha, yes! I've been mixing up OWNERS vs DEPS noparent semantics!

I agree, and I'll work on updating checkdeps to handle:

• We'd need to ensure that when a new include is added on bar/baz, bar/ being in DEPS of the dir with the include doesn't satisfy checkdeps.

👍

 

I think the existing OWNERS noparent should be sufficient to cover:

• We'd need to ensure that when bar/baz is added to a DEPS entry, a +1 from an OWNER of bar/ doesn't satisfy the code review requirements (this is why I was thinking about "noparent" in OWNERS files, but the rule can be standalone regardless of whether "noparent" is in bar/baz/OWNERS).

Do you know whether we could verify this on the test CL without actually trying to land it? I've only seen these "need +1 from OWNER of directory added in DEPS" check kick in when CQ+2'ing a CL.

[Andrew Grieve]

On Thu, Oct 17, 2024 at 4:58 AM Colin Blundell <blun...@chromium.org> wrote:

On Wed, Oct 16, 2024 at 6:50 PM Andrew Grieve <agr...@chromium.org> wrote:

Aha, yes! I've been mixing up OWNERS vs DEPS noparent semantics!

I agree, and I'll work on updating checkdeps to handle:

• We'd need to ensure that when a new include is added on bar/baz, bar/ being in DEPS of the dir with the include doesn't satisfy checkdeps.

👍

 

I think the existing OWNERS noparent should be sufficient to cover:

• We'd need to ensure that when bar/baz is added to a DEPS entry, a +1 from an OWNER of bar/ doesn't satisfy the code review requirements (this is why I was thinking about "noparent" in OWNERS files, but the rule can be standalone regardless of whether "noparent" is in bar/baz/OWNERS).

Do you know whether we could verify this on the test CL without actually trying to land it? I've only seen these "need +1 from OWNER of directory added in DEPS" check kick in when CQ+2'ing a CL.

This part is just the existing OWNERS noparent logic, so I think can be relied upon? I think without a "noparent=True" in the OWNERS, it should be fine for a parent OWNER to +1 a CL even in the presence of a child's new_usages_require_review=True.

[Colin Blundell]

On Thu, Oct 17, 2024 at 3:41 PM Andrew Grieve <agr...@chromium.org> wrote:

On Thu, Oct 17, 2024 at 4:58 AM Colin Blundell <blun...@chromium.org> wrote:

On Wed, Oct 16, 2024 at 6:50 PM Andrew Grieve <agr...@chromium.org> wrote:

Aha, yes! I've been mixing up OWNERS vs DEPS noparent semantics!

I agree, and I'll work on updating checkdeps to handle:

• We'd need to ensure that when a new include is added on bar/baz, bar/ being in DEPS of the dir with the include doesn't satisfy checkdeps.

👍

 

I think the existing OWNERS noparent should be sufficient to cover:

• We'd need to ensure that when bar/baz is added to a DEPS entry, a +1 from an OWNER of bar/ doesn't satisfy the code review requirements (this is why I was thinking about "noparent" in OWNERS files, but the rule can be standalone regardless of whether "noparent" is in bar/baz/OWNERS).

Do you know whether we could verify this on the test CL without actually trying to land it? I've only seen these "need +1 from OWNER of directory added in DEPS" check kick in when CQ+2'ing a CL.

This part is just the existing OWNERS noparent logic, so I think can be relied upon?

 As long as the DEPS OWNERS checks go through the same code as the regular OWNERS checks, I agree. I don't know offhand if that's the case, although of course it would be logical for it to be the case.

I think without a "noparent=True" in the OWNERS, it should be fine for a parent OWNER to +1 a CL even in the presence of a child's new_usages_require_review=True.

Ah, so now we loop back around to where we had actually started talking about noparent, where I was thinking about noparent in OWNERS files. If someone goes to the effort of adding `new_usages_require_review`, it's hard for me to imagine that they actually mean "new usages require review by anyone who happens to be in the chain of OWNERS all the way leading up to this directory" as opposed to "new usages require review by one of the OWNERS of this directory". However, I'm also not at all sure that folks will always be aware of the fact that they would need to add noparent to the corresponding OWNERS file in the directory in question to avoid that result. Hence I think that it would make sense to have adding `new_usages_require_review` either (1) require that there be an OWNERS file in the same directory with `noparent` in it or (2) alternatively spit out a prominent presubmit warning and then folks could decide, but they'd be aware.

[Andrew Grieve]

This has now landed.

To recap:

1) You no longer need OWNERS approval from the directory that you are adding via "+allow/this/thing", unless "//allow/this/thing" or an ancestor contains "new_usages_require_review=True" in their DEPS

2) If "foo/bar/DEPS" contains "new_usages_require_review=True", a DEPS entry of "+foo" will not permit #includes to "foo/bar". You'd need to additionally add "+foo/bar".

[Colin Blundell]

On Fri, Nov 29, 2024 at 4:22 PM Andrew Grieve <agr...@chromium.org> wrote:

This has now landed.

To recap:

1) You no longer need OWNERS approval from the directory that you are adding via "+allow/this/thing", unless "//allow/this/thing" or an ancestor contains "new_usages_require_review=True" in their DEPS

2) If "foo/bar/DEPS" contains "new_usages_require_review=True", a DEPS entry of "+foo" will not permit #includes to "foo/bar". You'd need to additionally add "+foo/bar".

Great, thanks! I think that this is a good balance between developer productivity and long-term architectural robustness.

[Colin Blundell]

On Mon, Dec 2, 2024 at 2:47 PM Colin Blundell <blun...@chromium.org> wrote:

On Fri, Nov 29, 2024 at 4:22 PM Andrew Grieve <agr...@chromium.org> wrote:

This has now landed.

To recap:

1) You no longer need OWNERS approval from the directory that you are adding via "+allow/this/thing", unless "//allow/this/thing" or an ancestor contains "new_usages_require_review=True" in their DEPS

2) If "foo/bar/DEPS" contains "new_usages_require_review=True", a DEPS entry of "+foo" will not permit #includes to "foo/bar". You'd need to additionally add "+foo/bar".

Great, thanks! I think that this is a good balance between developer productivity and long-term architectural robustness.

A followup question: Did you verify/add logic to enforce that "when foo/bar is added to a DEPS entry, a +1 from an OWNER of foo/ doesn't satisfy the code review requirements"? It's not obvious to me that that condition would a priori be enforced by current OWNERS checks, but it seems important to fully enforce the semantics of this tag.

Best,

Colin

[Andrew Grieve]

On Mon, Dec 2, 2024 at 8:52 AM Colin Blundell <blun...@chromium.org> wrote:

On Mon, Dec 2, 2024 at 2:47 PM Colin Blundell <blun...@chromium.org> wrote:

On Fri, Nov 29, 2024 at 4:22 PM Andrew Grieve <agr...@chromium.org> wrote:

This has now landed.

To recap:

1) You no longer need OWNERS approval from the directory that you are adding via "+allow/this/thing", unless "//allow/this/thing" or an ancestor contains "new_usages_require_review=True" in their DEPS

2) If "foo/bar/DEPS" contains "new_usages_require_review=True", a DEPS entry of "+foo" will not permit #includes to "foo/bar". You'd need to additionally add "+foo/bar".

Great, thanks! I think that this is a good balance between developer productivity and long-term architectural robustness.

A followup question: Did you verify/add logic to enforce that "when foo/bar is added to a DEPS entry, a +1 from an OWNER of foo/ doesn't satisfy the code review requirements"? It's not obvious to me that that condition would a priori be enforced by current OWNERS checks, but it seems important to fully enforce the semantics of this tag.

I verified that if you add an entry for "+foo/bar", then an approval from a "foo" owner is not sufficient so long as foo/bar/OWNERS has noparent=True. 

[Colin Blundell]

On Mon, Dec 2, 2024 at 3:59 PM Andrew Grieve <agr...@chromium.org> wrote:

On Mon, Dec 2, 2024 at 8:52 AM Colin Blundell <blun...@chromium.org> wrote:

On Mon, Dec 2, 2024 at 2:47 PM Colin Blundell <blun...@chromium.org> wrote:

On Fri, Nov 29, 2024 at 4:22 PM Andrew Grieve <agr...@chromium.org> wrote:

This has now landed.

To recap:

1) You no longer need OWNERS approval from the directory that you are adding via "+allow/this/thing", unless "//allow/this/thing" or an ancestor contains "new_usages_require_review=True" in their DEPS

2) If "foo/bar/DEPS" contains "new_usages_require_review=True", a DEPS entry of "+foo" will not permit #includes to "foo/bar". You'd need to additionally add "+foo/bar".

Great, thanks! I think that this is a good balance between developer productivity and long-term architectural robustness.

A followup question: Did you verify/add logic to enforce that "when foo/bar is added to a DEPS entry, a +1 from an OWNER of foo/ doesn't satisfy the code review requirements"? It's not obvious to me that that condition would a priori be enforced by current OWNERS checks, but it seems important to fully enforce the semantics of this tag.

I verified that if you add an entry for "+foo/bar", then an approval from a "foo" owner is not sufficient so long as foo/bar/OWNERS has noparent=True. 

That's definitely good - thanks!

It brings us back to a question I had earlier in this thread though - Wouldn't someone adding "new_usages_require_review" always have in mind "by the folks that own this directory" rather than "by anyone in the tree leading up to this directory"? i.e., why should we require that they also add "noparent" in the OWNERS file to get that semantics? Maybe they don't want "noparent" in the OWNERS file for whatever reason, or they just don't think of the fact that the two would need to be linked to achieve the full semantics.

I could see arguing that it does make sense to link them, but I think that if we go that way we should surface that point in a presubmit message so folks are aware.

[Andrew Grieve]

On Mon, Dec 2, 2024 at 11:13 AM Colin Blundell <blun...@chromium.org> wrote:

On Mon, Dec 2, 2024 at 3:59 PM Andrew Grieve <agr...@chromium.org> wrote:

On Mon, Dec 2, 2024 at 8:52 AM Colin Blundell <blun...@chromium.org> wrote:

On Mon, Dec 2, 2024 at 2:47 PM Colin Blundell <blun...@chromium.org> wrote:

On Fri, Nov 29, 2024 at 4:22 PM Andrew Grieve <agr...@chromium.org> wrote:

This has now landed.

To recap:

1) You no longer need OWNERS approval from the directory that you are adding via "+allow/this/thing", unless "//allow/this/thing" or an ancestor contains "new_usages_require_review=True" in their DEPS

2) If "foo/bar/DEPS" contains "new_usages_require_review=True", a DEPS entry of "+foo" will not permit #includes to "foo/bar". You'd need to additionally add "+foo/bar".

Great, thanks! I think that this is a good balance between developer productivity and long-term architectural robustness.

A followup question: Did you verify/add logic to enforce that "when foo/bar is added to a DEPS entry, a +1 from an OWNER of foo/ doesn't satisfy the code review requirements"? It's not obvious to me that that condition would a priori be enforced by current OWNERS checks, but it seems important to fully enforce the semantics of this tag.

I verified that if you add an entry for "+foo/bar", then an approval from a "foo" owner is not sufficient so long as foo/bar/OWNERS has noparent=True. 

That's definitely good - thanks!

It brings us back to a question I had earlier in this thread though - Wouldn't someone adding "new_usages_require_review" always have in mind "by the folks that own this directory" rather than "by anyone in the tree leading up to this directory"? i.e., why should we require that they also add "noparent" in the OWNERS file to get that semantics? Maybe they don't want "noparent" in the OWNERS file for whatever reason, or they just don't think of the fact that the two would need to be linked to achieve the full semantics.

I could see arguing that it does make sense to link them, but I think that if we go that way we should surface that point in a presubmit message so folks are aware.

IMO the current state makes the most sense. 

• "new_usages_require_review=True" ==> "A reviewer of this directory is required"
• Without noparent=True in the OWNERS, then any ancestor review is "a reviewer of this directory".
• I don't think an implicit "noparent=True" would make sense, because it would mean the algorithm for choosing an OWNER is different from the one used by normal file modifications. 

[Colin Blundell]

On Mon, Dec 2, 2024 at 5:27 PM Andrew Grieve <agr...@chromium.org> wrote:

On Mon, Dec 2, 2024 at 11:13 AM Colin Blundell <blun...@chromium.org> wrote:

On Mon, Dec 2, 2024 at 3:59 PM Andrew Grieve <agr...@chromium.org> wrote:

On Mon, Dec 2, 2024 at 8:52 AM Colin Blundell <blun...@chromium.org> wrote:

On Mon, Dec 2, 2024 at 2:47 PM Colin Blundell <blun...@chromium.org> wrote:

On Fri, Nov 29, 2024 at 4:22 PM Andrew Grieve <agr...@chromium.org> wrote:

This has now landed.

To recap:

1) You no longer need OWNERS approval from the directory that you are adding via "+allow/this/thing", unless "//allow/this/thing" or an ancestor contains "new_usages_require_review=True" in their DEPS

2) If "foo/bar/DEPS" contains "new_usages_require_review=True", a DEPS entry of "+foo" will not permit #includes to "foo/bar". You'd need to additionally add "+foo/bar".

Great, thanks! I think that this is a good balance between developer productivity and long-term architectural robustness.

A followup question: Did you verify/add logic to enforce that "when foo/bar is added to a DEPS entry, a +1 from an OWNER of foo/ doesn't satisfy the code review requirements"? It's not obvious to me that that condition would a priori be enforced by current OWNERS checks, but it seems important to fully enforce the semantics of this tag.

I verified that if you add an entry for "+foo/bar", then an approval from a "foo" owner is not sufficient so long as foo/bar/OWNERS has noparent=True. 

That's definitely good - thanks!

It brings us back to a question I had earlier in this thread though - Wouldn't someone adding "new_usages_require_review" always have in mind "by the folks that own this directory" rather than "by anyone in the tree leading up to this directory"? i.e., why should we require that they also add "noparent" in the OWNERS file to get that semantics? Maybe they don't want "noparent" in the OWNERS file for whatever reason, or they just don't think of the fact that the two would need to be linked to achieve the full semantics.

I could see arguing that it does make sense to link them, but I think that if we go that way we should surface that point in a presubmit message so folks are aware.

IMO the current state makes the most sense. 

• "new_usages_require_review=True" ==> "A reviewer of this directory is required"
• Without noparent=True in the OWNERS, then any ancestor review is "a reviewer of this directory".
• I don't think an implicit "noparent=True" would make sense, because it would mean the algorithm for choosing an OWNER is different from the one used by normal file modifications. 

Oops, my "does" was meant to be "doesn't" in my last sentence above :P. i.e., I agree that it could make sense not to link them because linking them is indeed a bit of "spooky action at a distance", but I then strongly suggest that we add a presubmit message for the case when folks are adding "new_usages_require_review" without "noparent" being present in the OWNERS file to make sure that they're aware of the fact that without "noparent" being present in the OWNERS file they're not guaranteed that new usages will go for review to them. e.g., as a //chrome OWNER I get reviews for "blundell@ for this and that random bit of //chrome on this CL" all the time - on such CLs, my review would also implicitly cover any DEPS additions that fall into this case, whereas surely that wouldn't be the intent of the person adding this flag.

Does that make sense?

[Andrew Grieve]

On Tue, Dec 3, 2024 at 8:13 AM Colin Blundell <blun...@chromium.org> wrote:

On Mon, Dec 2, 2024 at 5:27 PM Andrew Grieve <agr...@chromium.org> wrote:

On Mon, Dec 2, 2024 at 11:13 AM Colin Blundell <blun...@chromium.org> wrote:

On Mon, Dec 2, 2024 at 3:59 PM Andrew Grieve <agr...@chromium.org> wrote:

On Mon, Dec 2, 2024 at 8:52 AM Colin Blundell <blun...@chromium.org> wrote:

On Mon, Dec 2, 2024 at 2:47 PM Colin Blundell <blun...@chromium.org> wrote:

On Fri, Nov 29, 2024 at 4:22 PM Andrew Grieve <agr...@chromium.org> wrote:

This has now landed.

To recap:

1) You no longer need OWNERS approval from the directory that you are adding via "+allow/this/thing", unless "//allow/this/thing" or an ancestor contains "new_usages_require_review=True" in their DEPS

2) If "foo/bar/DEPS" contains "new_usages_require_review=True", a DEPS entry of "+foo" will not permit #includes to "foo/bar". You'd need to additionally add "+foo/bar".

Great, thanks! I think that this is a good balance between developer productivity and long-term architectural robustness.

A followup question: Did you verify/add logic to enforce that "when foo/bar is added to a DEPS entry, a +1 from an OWNER of foo/ doesn't satisfy the code review requirements"? It's not obvious to me that that condition would a priori be enforced by current OWNERS checks, but it seems important to fully enforce the semantics of this tag.

I verified that if you add an entry for "+foo/bar", then an approval from a "foo" owner is not sufficient so long as foo/bar/OWNERS has noparent=True. 

That's definitely good - thanks!

It brings us back to a question I had earlier in this thread though - Wouldn't someone adding "new_usages_require_review" always have in mind "by the folks that own this directory" rather than "by anyone in the tree leading up to this directory"? i.e., why should we require that they also add "noparent" in the OWNERS file to get that semantics? Maybe they don't want "noparent" in the OWNERS file for whatever reason, or they just don't think of the fact that the two would need to be linked to achieve the full semantics.

I could see arguing that it does make sense to link them, but I think that if we go that way we should surface that point in a presubmit message so folks are aware.

IMO the current state makes the most sense. 

• "new_usages_require_review=True" ==> "A reviewer of this directory is required"
• Without noparent=True in the OWNERS, then any ancestor review is "a reviewer of this directory".
• I don't think an implicit "noparent=True" would make sense, because it would mean the algorithm for choosing an OWNER is different from the one used by normal file modifications. 

Oops, my "does" was meant to be "doesn't" in my last sentence above :P. i.e., I agree that it could make sense not to link them because linking them is indeed a bit of "spooky action at a distance", but I then strongly suggest that we add a presubmit message for the case when folks are adding "new_usages_require_review" without "noparent" being present in the OWNERS file to make sure that they're aware of the fact that without "noparent" being present in the OWNERS file they're not guaranteed that new usages will go for review to them. e.g., as a //chrome OWNER I get reviews for "blundell@ for this and that random bit of //chrome on this CL" all the time - on such CLs, my review would also implicitly cover any DEPS additions that fall into this case, whereas surely that wouldn't be the intent of the person adding this flag.

Does that make sense?

It makes sense. I don't think it's worth it though. Before this change, *all* directories were implicitly new_usages_require_review=True, and ancestor OWNERS could approve them. There are now only a handful of new_usages_require_review=True directories, and it's not necessarily the case that ancestor OWNERS are not desirable for them. I don't think the concern is sufficiently common or severe to merit a presubmit check.

[Colin Blundell]

Of course, almost all directories didn't actually need that bit at all in reality, which is the motivation for your work :). What's worth examining is the directories that conceptually actually require that bit, which we can do now thanks to your work (see below).

  

There are now only a handful of new_usages_require_review=True directories, and it's not necessarily the case that ancestor OWNERS are not desirable for them. I don't think the concern is sufficiently common or severe to merit a presubmit check.

I just looked, and indeed there are only 5:

components/os_crypt/sync/DEPS
content/DEPS
crypto/DEPS
device/fido/DEPS

third_party/boringssl/DEPS

//content and //crypto are noparent already by virtue of being top-level modules. The other three fall into the category of "change could be unintentionally/unwantedly approved by a higher-level OWNER" (of //components, //device, //third_party respectively). Given the low absolute number involved here, I'm fine not doing further effort at this point. For posterity in case this becomes relevant in the future: What I was just suggesting was just adding a presubmit warning so that folks would be sure to think about this explicitly when adding the bit to a DEPS file, not making it mandatory that noparent be set on the corresponding OWNERS file.

Best,

Colin