Bug 49192 – Add a step to check-webkit-style to check include paths

• Status: NEW
• Component: New Bugs

[James Robinson]

Add a step to check-webkit-style to check include paths

[James Robinson]

Created attachment 73254 [details]
Patch

[James Robinson]

Initial patch for feedback.  The names and code organization aren't very good and I need to add tests.

Sample output:

$ ./WebKitTools/Scripts/check-webkit-style WebCore/platform/ContextMenu.cpp 
WebCore/platform/ContextMenu.cpp:33:  Should not include BackForwardController.h from here: WebCore/platform should not depending on anything else in WebCore.  [build/include_rules] [4]
WebCore/platform/ContextMenu.cpp:34:  Should not include ContextMenuController.h from here: WebCore/platform should not depending on anything else in WebCore.  [build/include_rules] [4]
WebCore/platform/ContextMenu.cpp:35:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/platform/ContextMenu.cpp:35:  Should not include ContextMenuClient.h from here: WebCore/platform should not depending on anything else in WebCore.  [build/include_rules] [4]
WebCore/platform/ContextMenu.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/platform/ContextMenu.cpp:36:  Should not include CSSComputedStyleDeclaration.h from here: WebCore/platform should not depending on anything else in WebCore.  [build/include_rules] [4]
WebCore/platform/ContextMenu.cpp:37:  Should not include CSSProperty.h from here: WebCore/platform should not depending on anything else in WebCore.  [build/include_rules] [4]
WebCore/platform/ContextMenu.cpp:39:  Should not include Document.h from here: WebCore/platform should not depending on anything else in WebCore.  [build/include_rules] [4]
WebCore/platform/ContextMenu.cpp:40:  Should not include DocumentLoader.h from here: WebCore/platform should not depending on anything else in WebCore.  [build/include_rules] [4]
WebCore/platform/ContextMenu.cpp:41:  Should not include Editor.h from here: WebCore/platform should not depending on anything else in WebCore.  [build/include_rules] [4]
WebCore/platform/ContextMenu.cpp:42:  Should not include Frame.h from here: WebCore/platform should not depending on anything else in WebCore.  [build/include_rules] [4]
WebCore/platform/ContextMenu.cpp:43:  Should not include FrameLoader.h from here: WebCore/platform should not depending on anything else in WebCore.  [build/include_rules] [4]
WebCore/platform/ContextMenu.cpp:44:  Should not include InspectorController.h from here: WebCore/platform should not depending on anything else in WebCore.  [build/include_rules] [4]
WebCore/platform/ContextMenu.cpp:47:  Should not include Node.h from here: WebCore/platform should not depending on anything else in WebCore.  [build/include_rules] [4]
WebCore/platform/ContextMenu.cpp:48:  Should not include Page.h from here: WebCore/platform should not depending on anything else in WebCore.  [build/include_rules] [4]
WebCore/platform/ContextMenu.cpp:50:  Should not include SelectionController.h from here: WebCore/platform should not depending on anything else in WebCore.  [build/include_rules] [4]
WebCore/platform/ContextMenu.cpp:51:  Should not include Settings.h from here: WebCore/platform should not depending on anything else in WebCore.  [build/include_rules] [4]
WebCore/platform/ContextMenu.cpp:52:  Should not include TextIterator.h from here: WebCore/platform should not depending on anything else in WebCore.  [build/include_rules] [4]
WebCore/platform/ContextMenu.cpp:442:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/platform/ContextMenu.cpp:619:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Total errors found: 20 in 1 files

[Adam Barth]

Comment on attachment 73254 [details]
Patch

Cool idea.  Thanks.

[Tony Gentilcore]

This seems like a really good idea. Are you still interested in landing it?

[Eric Seidel (no email)]

I support this product and/or service and wish to buy!

[James Robinson]

(In reply to comment #4)
> This seems like a really good idea. Are you still interested in landing it?

I don't think that I'm likely to have time to finish this up and land it, although I think it'd be nice to have.

[James Robinson]

Created attachment 111191 [details]
Patch

[James Robinson]

Updated the patch to work with ToT, but I haven't really cleaned much up.  I think in order to land this I should switch over to something more testable than directly using os.walk and add some tests - I'm guessing that the webkitpy/common/filesystem stuff is intended for this.  Any comments/feedback are welcome while I look into that.

[Adam Barth]

Comment on attachment 111191 [details]
Patch

I'd really like to have this feature, but this patch needs a bit more iteration.  The main include_path.py should be in webkitpy.common (as a peer to the watchlist mechanism).  Also, the config file should be in common.config (as a peer to the watchlist mechanism).  Also, we'll want to use the filesystem abstraction to make this more testable.

I talked a bit with jamesr about this patch, and I might take a pass at integrating it more with webkitpy because he's got other things on his plate.

[Adam Barth]

Created attachment 111523 [details]
work in progress

[Adam Barth]

@dave_levin: A couple questions:

1) Where should I stick the dependency_checker object on the style machinery?  It takes a noticeable time to start because it needs to build a map of all the headers in the project.

2) Is there a way to restrict running the dependency check to just the style bot?  The check is somewhat slow and I don't want everyone to need to wait all the time.

The best answer to (1) and (2) might be to not integrate with the style checker at all and to use a similar mechanism to watchlists.

Thoughts?

[David Levin]

(In reply to comment #11)
> @dave_levin: A couple questions:
> 
> 1) Where should I stick the dependency_checker object on the style machinery?  It takes a noticeable time to start because it needs to build a map of all the headers in the project.

I feel like we could make this on demand load instead of loading on initialization.

We could also add a little bit of logic in the style checker to ask the style_error_handler if a line matters and if it doesn't skip the check.

Those items together would then mean that the check would only run when someone adds an include which would reduce how often it hits people.

> 
> 2) Is there a way to restrict running the dependency check to just the style bot?  The check is somewhat slow and I don't want everyone to need to wait all the time.

I guess we could have a separate command line option that needs to be passed explicitly to the style checker and have the bots pass in that command.


> 
> The best answer to (1) and (2) might be to not integrate with the style checker at all and to use a similar mechanism to watchlists.
> 
> Thoughts?

Here's another idea.

We could build the list once and check it in and then only try to build things from disk if there is a header that we don't know about from the checked in version.

We'd need to update the checked in list once in a while.

Does that make sense? (It is slightly more work to make it function this way though.)

[Adam Barth]

I'm not sure we want to check in a list of every header in the project.  That seems like it would be a maintenance burden.

I think I'm going to make this a separate command for now.  We can change it around later if we want.

[David Levin]

(In reply to comment #13)
> I'm not sure we want to check in a list of every header in the project.  That seems like it would be a maintenance burden.
> 
> I think I'm going to make this a separate command for now.  We can change it around later if we want.

Sounds reasonable.