Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2480873003:
Revert of Remove self-DEPS from chrome/variations/DEPS. (Closed)
DescriptionRevert of Remove self-DEPS from chrome/variations/DEPS. (patchset #1 id:1 of https://codereview.chromium.org/2473023002/ )
Reason for revert:
Broke checkdeps:
https://build.chromium.org/p/chromium.linux/builders/Linux%20Tests/builds/48559
Original issue's description:
> Remove self-DEPS from chrome/variations/DEPS.
>
> BUG=None
>
> Committed: https://crrev.com/e89fc14086f099160141dc92d8eb94a5c2dd0873
> Cr-Commit-Position: refs/heads/master@{#430120}
TBR=jwd@chromium.org,gab@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=None
Committed: https://crrev.com/e3c657e06340261825f8d52f1dede637be4aa5e8
Cr-Commit-Position: refs/heads/master@{#430143}
Patch Set 1 #
Messages
Total messages: 16 (3 generated)
The CQ bit was checked by mathp@chromium.org
Created Revert of Remove self-DEPS from chrome/variations/DEPS.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Revert of Remove self-DEPS from chrome/variations/DEPS. (patchset #1 id:1 of https://codereview.chromium.org/2473023002/ ) Reason for revert: Broke checkdeps: https://build.chromium.org/p/chromium.linux/builders/Linux%20Tests/builds/48559 Original issue's description: > Remove self-DEPS from chrome/variations/DEPS. > > BUG=None > > Committed: https://crrev.com/e89fc14086f099160141dc92d8eb94a5c2dd0873 > Cr-Commit-Position: refs/heads/master@{#430120} TBR=jwd@chromium.org,gab@chromium.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=None ========== to ========== Revert of Remove self-DEPS from chrome/variations/DEPS. (patchset #1 id:1 of https://codereview.chromium.org/2473023002/ ) Reason for revert: Broke checkdeps: https://build.chromium.org/p/chromium.linux/builders/Linux%20Tests/builds/48559 Original issue's description: > Remove self-DEPS from chrome/variations/DEPS. > > BUG=None > > Committed: https://crrev.com/e89fc14086f099160141dc92d8eb94a5c2dd0873 > Cr-Commit-Position: refs/heads/master@{#430120} TBR=jwd@chromium.org,gab@chromium.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=None ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== Revert of Remove self-DEPS from chrome/variations/DEPS. (patchset #1 id:1 of https://codereview.chromium.org/2473023002/ ) Reason for revert: Broke checkdeps: https://build.chromium.org/p/chromium.linux/builders/Linux%20Tests/builds/48559 Original issue's description: > Remove self-DEPS from chrome/variations/DEPS. > > BUG=None > > Committed: https://crrev.com/e89fc14086f099160141dc92d8eb94a5c2dd0873 > Cr-Commit-Position: refs/heads/master@{#430120} TBR=jwd@chromium.org,gab@chromium.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=None ========== to ========== Revert of Remove self-DEPS from chrome/variations/DEPS. (patchset #1 id:1 of https://codereview.chromium.org/2473023002/ ) Reason for revert: Broke checkdeps: https://build.chromium.org/p/chromium.linux/builders/Linux%20Tests/builds/48559 Original issue's description: > Remove self-DEPS from chrome/variations/DEPS. > > BUG=None > > Committed: https://crrev.com/e89fc14086f099160141dc92d8eb94a5c2dd0873 > Cr-Commit-Position: refs/heads/master@{#430120} TBR=jwd@chromium.org,gab@chromium.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=None Committed: https://crrev.com/e3c657e06340261825f8d52f1dede637be4aa5e8 Cr-Commit-Position: refs/heads/master@{#430143} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/e3c657e06340261825f8d52f1dede637be4aa5e8 Cr-Commit-Position: refs/heads/master@{#430143}
Message was sent while issue was closed.
Ah interesting, FTR: the self DEPS is necessary to counterbalance the "-components" in the same DEPS file. It's kind of weird to have to be explicit about including self but I guess it's a rare enough case to a have to explicitly set DEPS on set per explicitly blocking a path under self. But I'm not going to chase this as I was merely doing a drive-by cleanup and don't really care. PS: Weird that it passed CQ though?! I guess this DEPS "bug" only applies in some corner cases..?
Message was sent while issue was closed.
On 2016/11/07 15:25:12, gab wrote: > Ah interesting, FTR: the self DEPS is necessary to counterbalance the > "-components" in the same DEPS file. It's kind of weird to have to be explicit > about including self but I guess it's a rare enough case to a have to explicitly > set DEPS on set per explicitly blocking a path under self. But I'm not going to > chase this as I was merely doing a drive-by cleanup and don't really care. > > PS: Weird that it passed CQ though?! I guess this DEPS "bug" only applies in > some corner cases..? FWIW, The -components in the same file itself seems redundant since it's in //components/DEPS (i.e., *no* component can blindly depend on //components by design).
Message was sent while issue was closed.
Maybe CQ doesn't check DEPS? Sounds like a bug with CQ - where it's not testing things that the main waterfall tests. Do one of you want to follow up with cq folks? On Mon, Nov 7, 2016 at 7:25 AM, <gab@chromium.org> wrote: > Ah interesting, FTR: the self DEPS is necessary to counterbalance the > "-components" in the same DEPS file. It's kind of weird to have to be > explicit > about including self but I guess it's a rare enough case to a have to > explicitly > set DEPS on set per explicitly blocking a path under self. But I'm not > going to > chase this as I was merely doing a drive-by cleanup and don't really care. > > PS: Weird that it passed CQ though?! I guess this DEPS "bug" only applies > in > some corner cases..? > > https://codereview.chromium.org/2480873003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
Oh I know why it passed CQ: the presubmit check only runs checkdeps *on files with changed includes*, of which there were none in this patch, whereas the main waterfall just blindly runs checkdeps on the codebase after each commit. The former behavior is of course intentional since checkdeps is very slow, but it does mean that presubmit misses CLs that break checkdeps only due to DEPS file changes, rather than source file changes. On 2016/11/07 15:28:14, chromium-reviews wrote: > Maybe CQ doesn't check DEPS? Sounds like a bug with CQ - where it's not > testing things that the main waterfall tests. Do one of you want to follow > up with cq folks? > > On Mon, Nov 7, 2016 at 7:25 AM, <mailto:gab@chromium.org> wrote: > > > Ah interesting, FTR: the self DEPS is necessary to counterbalance the > > "-components" in the same DEPS file. It's kind of weird to have to be > > explicit > > about including self but I guess it's a rare enough case to a have to > > explicitly > > set DEPS on set per explicitly blocking a path under self. But I'm not > > going to > > chase this as I was merely doing a drive-by cleanup and don't really care. > > > > PS: Weird that it passed CQ though?! I guess this DEPS "bug" only applies > > in > > some corner cases..? > > > > https://codereview.chromium.org/2480873003/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
On 2016/11/07 15:33:27, blundell wrote: > Oh I know why it passed CQ: the presubmit check only runs checkdeps *on files > with changed includes*, of which there were none in this patch, whereas the main > waterfall just blindly runs checkdeps on the codebase after each commit. The > former behavior is of course intentional since checkdeps is very slow, but it > does mean that presubmit misses CLs that break checkdeps only due to DEPS file > changes, rather than source file changes. > > On 2016/11/07 15:28:14, chromium-reviews wrote: > > Maybe CQ doesn't check DEPS? Sounds like a bug with CQ - where it's not > > testing things that the main waterfall tests. Do one of you want to follow > > up with cq folks? > > > > On Mon, Nov 7, 2016 at 7:25 AM, <mailto:gab@chromium.org> wrote: > > > > > Ah interesting, FTR: the self DEPS is necessary to counterbalance the > > > "-components" in the same DEPS file. It's kind of weird to have to be > > > explicit > > > about including self but I guess it's a rare enough case to a have to > > > explicitly > > > set DEPS on set per explicitly blocking a path under self. But I'm not > > > going to > > > chase this as I was merely doing a drive-by cleanup and don't really care. > > > > > > PS: Weird that it passed CQ though?! I guess this DEPS "bug" only applies > > > in > > > some corner cases..? > > > > > > https://codereview.chromium.org/2480873003/ > > > > > > > -- > > You received this message because you are subscribed to the Google Groups > > "Chromium-reviews" group. > > To unsubscribe from this group and stop receiving emails from it, send an > email > > to mailto:chromium-reviews+unsubscribe@chromium.org. Feels like ideally checkdeps should have run for the whole of //components/variations during CQ (even if no files were changed), no?
Message was sent while issue was closed.
On 2016/11/07 15:34:58, Mathieu Perreault wrote: > On 2016/11/07 15:33:27, blundell wrote: > > Oh I know why it passed CQ: the presubmit check only runs checkdeps *on files > > with changed includes*, of which there were none in this patch, whereas the > main > > waterfall just blindly runs checkdeps on the codebase after each commit. The > > former behavior is of course intentional since checkdeps is very slow, but it > > does mean that presubmit misses CLs that break checkdeps only due to DEPS file > > changes, rather than source file changes. > > > > On 2016/11/07 15:28:14, chromium-reviews wrote: > > > Maybe CQ doesn't check DEPS? Sounds like a bug with CQ - where it's not > > > testing things that the main waterfall tests. Do one of you want to follow > > > up with cq folks? > > > > > > On Mon, Nov 7, 2016 at 7:25 AM, <mailto:gab@chromium.org> wrote: > > > > > > > Ah interesting, FTR: the self DEPS is necessary to counterbalance the > > > > "-components" in the same DEPS file. It's kind of weird to have to be > > > > explicit > > > > about including self but I guess it's a rare enough case to a have to > > > > explicitly > > > > set DEPS on set per explicitly blocking a path under self. But I'm not > > > > going to > > > > chase this as I was merely doing a drive-by cleanup and don't really care. > > > > > > > > PS: Weird that it passed CQ though?! I guess this DEPS "bug" only applies > > > > in > > > > some corner cases..? > > > > > > > > https://codereview.chromium.org/2480873003/ > > > > > > > > > > -- > > > You received this message because you are subscribed to the Google Groups > > > "Chromium-reviews" group. > > > To unsubscribe from this group and stop receiving emails from it, send an > > email > > > to mailto:chromium-reviews+unsubscribe@chromium.org. > > Feels like ideally checkdeps should have run for the whole of > //components/variations during CQ (even if no files were changed), no? Yep. I think it would be just a matter of changing the logic here: https://cs.chromium.org/chromium/src/PRESUBMIT.py?q=CheckUnwanted&sq=package:..., to additionally run the entire directory through checkdeps for any directory with a modified DEPS file in the CL.
Message was sent while issue was closed.
On 2016/11/07 15:27:12, blundell wrote: > On 2016/11/07 15:25:12, gab wrote: > > Ah interesting, FTR: the self DEPS is necessary to counterbalance the > > "-components" in the same DEPS file. It's kind of weird to have to be explicit > > about including self but I guess it's a rare enough case to a have to > explicitly > > set DEPS on set per explicitly blocking a path under self. But I'm not going > to > > chase this as I was merely doing a drive-by cleanup and don't really care. > > > > PS: Weird that it passed CQ though?! I guess this DEPS "bug" only applies in > > some corner cases..? > > FWIW, The -components in the same file itself seems redundant since it's in > //components/DEPS (i.e., *no* component can blindly depend on //components by > design). I had the same thought but before making this change I checked whether other components were also self-including and the few I checked weren't... maybe the DEPS check bug is that it requires explicit self-include when there's an explicit broader self-exclude in the *same* DEPS file?
Message was sent while issue was closed.
On 2016/11/07 15:44:42, gab wrote: > On 2016/11/07 15:27:12, blundell wrote: > > On 2016/11/07 15:25:12, gab wrote: > > > Ah interesting, FTR: the self DEPS is necessary to counterbalance the > > > "-components" in the same DEPS file. It's kind of weird to have to be > explicit > > > about including self but I guess it's a rare enough case to a have to > > explicitly > > > set DEPS on set per explicitly blocking a path under self. But I'm not going > > to > > > chase this as I was merely doing a drive-by cleanup and don't really care. > > > > > > PS: Weird that it passed CQ though?! I guess this DEPS "bug" only applies in > > > some corner cases..? > > > > FWIW, The -components in the same file itself seems redundant since it's in > > //components/DEPS (i.e., *no* component can blindly depend on //components by > > design). > > I had the same thought but before making this change I checked whether other > components were also self-including and the few I checked weren't... maybe the > DEPS check bug is that it requires explicit self-include when there's an > explicit broader self-exclude in the *same* DEPS file? Yes, I think the the hierarchy goes like this, with higher overriding lower: - deps set explicitly in the current DEPS file - implicit allowance on current directory - deps inherited from ancestors' DEPS files So if -components was removed from //components/variations, there would be no need for +components/variations and also no change in what's allowed within //components/variations.
Message was sent while issue was closed.
On 2016/11/07 15:48:36, blundell wrote: > On 2016/11/07 15:44:42, gab wrote: > > On 2016/11/07 15:27:12, blundell wrote: > > > On 2016/11/07 15:25:12, gab wrote: > > > > Ah interesting, FTR: the self DEPS is necessary to counterbalance the > > > > "-components" in the same DEPS file. It's kind of weird to have to be > > explicit > > > > about including self but I guess it's a rare enough case to a have to > > > explicitly > > > > set DEPS on set per explicitly blocking a path under self. But I'm not > going > > > to > > > > chase this as I was merely doing a drive-by cleanup and don't really care. > > > > > > > > PS: Weird that it passed CQ though?! I guess this DEPS "bug" only applies > in > > > > some corner cases..? > > > > > > FWIW, The -components in the same file itself seems redundant since it's in > > > //components/DEPS (i.e., *no* component can blindly depend on //components > by > > > design). > > > > I had the same thought but before making this change I checked whether other > > components were also self-including and the few I checked weren't... maybe the > > DEPS check bug is that it requires explicit self-include when there's an > > explicit broader self-exclude in the *same* DEPS file? > > Yes, I think the the hierarchy goes like this, with higher overriding lower: > > - deps set explicitly in the current DEPS file > - implicit allowance on current directory > - deps inherited from ancestors' DEPS files > > So if -components was removed from //components/variations, there would be no > need for +components/variations and also no change in what's allowed within > //components/variations. I see, well I'll let the components/variations/OWNERS fix it if they care, like I said I was mostly just doing a drive-by fix in a split second but don't have the intention to drive this further (but would like to be CC'ed out of curiosity if someone does this). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
