New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 724264 link

Starred by 5 users

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: May 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

DEPS does not handle #include "../" properly

Project Member Reported by aelias@chromium.org, May 18 2017

Issue description

#include "../" is a very rare pattern in the codebase that DEPS processors don't appear capable of handling properly.  Instead of changing the DEPS processing to handle it, it's probably better to eliminate the pattern from the codebase.

rlanday@ already landed https://codereview.chromium.org/2887233003 for this.

Another patch up at: https://codereview.chromium.org/2892703002/
 

Comment 1 by aelias@chromium.org, May 18 2017

It looks like after the latter change, in total there will remain only these (outside of git subrepositories):

gpu/command_buffer/common/gles2_cmd_format.h:311:#include "../common/gles2_cmd_format_autogen.h"
gpu/command_buffer/common/gles2_cmd_utils.h:260:  #include "../common/gles2_cmd_utils_autogen.h"
ppapi/lib/gl/include/GLES2/gl2.h:476:#include "../command_buffer/client/gles2_lib.h"
third_party/WebKit/public/platform/WebTouchPoint.h:34:#include "../platform/WebCommon.h"
third_party/WebKit/public/platform/WebTouchPoint.h:35:#include "../platform/WebFloatPoint.h"
third_party/WebKit/public/platform/WebTouchPoint.h:36:#include "../platform/WebPointerProperties.h"
third_party/WebKit/public/platform/linux/WebFallbackFont.h:34:#include "../WebCString.h"
third_party/WebKit/public/platform/linux/WebFallbackFont.h:35:#include "../WebCommon.h"
third_party/WebKit/public/platform/linux/WebFontRenderStyle.h:34:#include "../WebCommon.h"
third_party/WebKit/public/platform/linux/WebSandboxSupport.h:34:#include "../WebCommon.h"
third_party/WebKit/public/platform/linux/WebSandboxSupport.h:35:#include "../WebString.h"

How about removing them as well, then adding a PRESUBMIT rule so that the pattern cannot recur?
Project Member

Comment 2 by bugdroid1@chromium.org, May 19 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/38a14eb81c041f7ae7bff36226318bca3d161e79

commit 38a14eb81c041f7ae7bff36226318bca3d161e79
Author: rlanday <rlanday@chromium.org>
Date: Fri May 19 21:38:01 2017

Finish ../platform => public/platform refactor

In https://codereview.chromium.org/2887233003, I changed most of the header
includes in the third_party/WebKit/public/web directory that started with
"../platform/" to instead start with "public/platform/" in order to make
these header includes valid under a DEPS rule that's flagging new includes
of the form "../platform/" as errors. I did not change these headers in that CL
because some build targets were not building properly. I've determined that the
problem was that these targets are missing a dependency in their BUILD.gn files.
This CL adds this dependency to those build files and finishes the codemod.

Note: I have not run git cl format to put the headers in the correct order
because I ran into problems with the original CL due to this breaking fragile
code.

BUG= 724264 

Review-Url: https://codereview.chromium.org/2892703002
Cr-Commit-Position: refs/heads/master@{#473333}

[modify] https://crrev.com/38a14eb81c041f7ae7bff36226318bca3d161e79/chrome/browser/BUILD.gn
[modify] https://crrev.com/38a14eb81c041f7ae7bff36226318bca3d161e79/chrome/test/BUILD.gn
[modify] https://crrev.com/38a14eb81c041f7ae7bff36226318bca3d161e79/components/autofill/content/renderer/BUILD.gn
[modify] https://crrev.com/38a14eb81c041f7ae7bff36226318bca3d161e79/headless/BUILD.gn
[modify] https://crrev.com/38a14eb81c041f7ae7bff36226318bca3d161e79/third_party/WebKit/public/web/WebConsoleMessage.h
[modify] https://crrev.com/38a14eb81c041f7ae7bff36226318bca3d161e79/third_party/WebKit/public/web/WebElement.h
[modify] https://crrev.com/38a14eb81c041f7ae7bff36226318bca3d161e79/third_party/WebKit/public/web/WebFormControlElement.h
[modify] https://crrev.com/38a14eb81c041f7ae7bff36226318bca3d161e79/third_party/WebKit/public/web/WebFormElement.h
[modify] https://crrev.com/38a14eb81c041f7ae7bff36226318bca3d161e79/third_party/WebKit/public/web/WebNode.h
[modify] https://crrev.com/38a14eb81c041f7ae7bff36226318bca3d161e79/third_party/WebKit/public/web/WebPrintParams.h

Comment 3 by tkent@chromium.org, May 21 2017

Cc: tkent@chromium.org
 Issue 350097  has been merged into this issue.
Project Member

Comment 4 by bugdroid1@chromium.org, May 22 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/764476c65b8205086820a5ea02c10a645d6a0518

commit 764476c65b8205086820a5ea02c10a645d6a0518
Author: rlanday <rlanday@chromium.org>
Date: Mon May 22 23:27:58 2017

Eliminate remaining ../ header includes

I previously changed all the header includes in third_party/WebKit/public/web
that started with "../" to no longer use relative paths because include paths of
this pattern can include headers that aren't actually specified as build
dependencies (and also, engineers adding new includes of this form can run into
lint errors in some cases). aelias@ pointed out on crbug/724264 that there are
only a small number of these relative include paths in other directories
(ignoring other repos that get pulled in). This CL fixes these remaining
includes.

BUG= 724264 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2897503002
Cr-Commit-Position: refs/heads/master@{#473741}

[modify] https://crrev.com/764476c65b8205086820a5ea02c10a645d6a0518/chrome/browser/BUILD.gn
[modify] https://crrev.com/764476c65b8205086820a5ea02c10a645d6a0518/chrome/browser/chromeos/BUILD.gn
[modify] https://crrev.com/764476c65b8205086820a5ea02c10a645d6a0518/gpu/command_buffer/common/gles2_cmd_format.h
[modify] https://crrev.com/764476c65b8205086820a5ea02c10a645d6a0518/gpu/command_buffer/common/gles2_cmd_utils.h
[modify] https://crrev.com/764476c65b8205086820a5ea02c10a645d6a0518/headless/BUILD.gn
[modify] https://crrev.com/764476c65b8205086820a5ea02c10a645d6a0518/third_party/WebKit/public/platform/WebTouchPoint.h
[modify] https://crrev.com/764476c65b8205086820a5ea02c10a645d6a0518/third_party/WebKit/public/platform/linux/WebFallbackFont.h
[modify] https://crrev.com/764476c65b8205086820a5ea02c10a645d6a0518/third_party/WebKit/public/platform/linux/WebFontRenderStyle.h
[modify] https://crrev.com/764476c65b8205086820a5ea02c10a645d6a0518/third_party/WebKit/public/platform/linux/WebSandboxSupport.h

Project Member

Comment 5 by bugdroid1@chromium.org, May 24 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/90089e1929d8ec7bda755456e7a54808a2c843ea

commit 90089e1929d8ec7bda755456e7a54808a2c843ea
Author: rlanday <rlanday@chromium.org>
Date: Wed May 24 19:52:15 2017

Add presubmit rule banning relative header includes

Relative header includes (#include path containing "../") can be used to cheat
the dependency system because they're not checked properly. This CL adds a
presubmit rule to catch these.

This rules applies to third_party/WebKit, but not anywhere else in third_party/.
There's one currently existing file I know of that would fail this rule:
ppapi/lib/gl/include/GLES2/gl2.h

I did not change this file when cleaning up the other headers since it appears
to be code imported from a third-party repo. I guess whoever updates this file
will have to bypass the rule.

BUG= 724264 

Review-Url: https://codereview.chromium.org/2900173003
Cr-Commit-Position: refs/heads/master@{#474396}

[modify] https://crrev.com/90089e1929d8ec7bda755456e7a54808a2c843ea/PRESUBMIT.py

Project Member

Comment 6 by bugdroid1@chromium.org, May 24 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c5cf3f2778b406509d532111bb7005f25d3dfcee

commit c5cf3f2778b406509d532111bb7005f25d3dfcee
Author: danakj <danakj@chromium.org>
Date: Wed May 24 20:14:12 2017

Revert of Add presubmit rule banning relative header includes (patchset #1 id:1 of https://codereview.chromium.org/2900173003/ )

Reason for revert:
Fails when it should not.

Original issue's description:
> Add presubmit rule banning relative header includes
>
> Relative header includes (#include path containing "../") can be used to cheat
> the dependency system because they're not checked properly. This CL adds a
> presubmit rule to catch these.
>
> This rules applies to third_party/WebKit, but not anywhere else in third_party/.
> There's one currently existing file I know of that would fail this rule:
> ppapi/lib/gl/include/GLES2/gl2.h
>
> I did not change this file when cleaning up the other headers since it appears
> to be code imported from a third-party repo. I guess whoever updates this file
> will have to bypass the rule.
>
> BUG= 724264 
>
> Review-Url: https://codereview.chromium.org/2900173003
> Cr-Commit-Position: refs/heads/master@{#474396}
> Committed: https://chromium.googlesource.com/chromium/src/+/90089e1929d8ec7bda755456e7a54808a2c843ea

TBR=jochen@chromium.org,rlanday@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 724264 

Review-Url: https://codereview.chromium.org/2897383002
Cr-Commit-Position: refs/heads/master@{#474400}

[modify] https://crrev.com/c5cf3f2778b406509d532111bb7005f25d3dfcee/PRESUBMIT.py

Status: Fixed (was: Assigned)

Sign in to add a comment