Add checkdeps presubmit check. Warns on new #includes of dependencies

Add checkdeps presubmit check. Warns on new #includes of dependencies
we are working to eliminate, and errors on new #includes of disallowed
dependencies.

BUG=138280
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=149167

[Jói, 2012-07-20 19:52:24]

I've tested this manually in a local branch I keep around especially for testing
presubmit scripts (it has a bunch of local changes to trigger various presubmit
errors and warnings).

Cheers,
Jói

[Jói, 2012-07-20 19:54:04]

BTW, this depends on http://codereview.chromium.org/10805042/.  CCing
reviewers of that change.

On Fri, Jul 20, 2012 at 7:52 PM,  <joi@chromium.org> wrote:
> Reviewers: Marc-Antoine Ruel,
>
> Message:
> I've tested this manually in a local branch I keep around especially for
> testing
> presubmit scripts (it has a bunch of local changes to trigger various
> presubmit
> errors and warnings).
>
> Cheers,
> Jói
>
>
> Description:
> Add checkdeps presubmit check. Warns on new #includes of dependencies
> we are working to eliminate, and errors on new #includes of disallowed
> dependencies.
>
> BUG=138280
>
>
> Please review this at http://codereview.chromium.org/10806049/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>
> Affected files:
>   M PRESUBMIT.py
>
>
> Index: PRESUBMIT.py
> diff --git a/PRESUBMIT.py b/PRESUBMIT.py
> index
>
009df4ce5a5cd0d55f1307976cb6f8d87d3c6bf5..bdedc49ab1f15be5beff1f5519a75902e5ae07c6
> 100644
> --- a/PRESUBMIT.py
> +++ b/PRESUBMIT.py
> @@ -9,7 +9,9 @@ for more details about the presubmit API built into gcl.
>  """
>
>
> +import os
>  import re
> +import sys
>
>
>  _EXCLUDED_PATHS = (
> @@ -374,6 +376,54 @@ def _CheckNoPragmaOnce(input_api, output_api):
>    return []
>
>
> +def _CheckUnwantedDependencies(input_api, output_api):
> +  """Runs checkdeps on #include statements added in this
> +  change. Breaking - rules is an error, breaking ! rules is a
> +  warning.
> +  """
> +  # We need to wait until we have an input_api object and use this
> +  # roundabout construct to import checkdeps because this file is
> +  # eval-ed and thus doesn't have __file__.
> +  sys.path.append(os.path.join(input_api.change.RepositoryRoot(),
> +                               'tools/checkdeps'))
> +  import checkdeps
> +
> +  added_includes = []
> +  for f in input_api.AffectedFiles():
> +    if not checkdeps.ShouldCheckFile(f.LocalPath()):
> +      continue
> +
> +    include_lines = []
> +    for line_num, line in f.ChangedContents():
> +      if not checkdeps.EXTRACT_INCLUDE_PATH.match(line):
> +        continue
> +      include_lines.append(line)
> +    added_includes.append([f.LocalPath(), include_lines])
> +
> +  error_descriptions = []
> +  warning_descriptions = []
> +  for path, rule_type, rule_description in checkdeps.CheckAddedIncludes(
> +      added_includes):
> +    description_with_path = '%s\n    %s' % (path, rule_description)
> +    if rule_type == checkdeps.Rule.DISALLOW:
> +      error_descriptions.append(description_with_path)
> +    else:
> +      warning_descriptions.append(description_with_path)
> +
> +  results = []
> +  if error_descriptions:
> +    results.append(output_api.PresubmitError(
> +        'You added one or more #includes that violate checkdeps rules.',
> +        error_descriptions))
> +  if warning_descriptions:
> +    results.append(output_api.PresubmitPromptWarning(
> +        'You added one or more #includes of files that are temporarily\n'
> +        'allowed but being removed. Can you avoid introducing the\n'
> +        '#include? See relevant DEPS file(s) for details and contacts.',
> +        warning_descriptions))
> +  return results
> +
> +
>  def _CommonChecks(input_api, output_api):
>    """Checks common to both upload and commit."""
>    results = []
> @@ -388,6 +438,7 @@ def _CommonChecks(input_api, output_api):
>    results.extend(_CheckNoDEPSGIT(input_api, output_api))
>    results.extend(_CheckNoBannedFunctions(input_api, output_api))
>    results.extend(_CheckNoPragmaOnce(input_api, output_api))
> +  results.extend(_CheckUnwantedDependencies(input_api, output_api))
>    return results
>
>
>
>

[Jói, 2012-07-25 17:25:12]

Marc-Antoine:  Could you also take a look at this one now?  I've merged in the
updated parent change, http://codereview.chromium.org/10805042/.

Cheers,
Jói

[Jói, 2012-07-26 10:04:21]

maruel: Friendly ping.

On Wed, Jul 25, 2012 at 5:25 PM,  <joi@chromium.org> wrote:
> Marc-Antoine:  Could you also take a look at this one now?  I've merged in
> the
> updated parent change, http://codereview.chromium.org/10805042/.
>
> Cheers,
> Jói
>
>
> http://codereview.chromium.org/10806049/

[M-A Ruel, 2012-07-26 12:37:41]

http://codereview.chromium.org/10806049/diff/4001/PRESUBMIT.py
File PRESUBMIT.py (right):

http://codereview.chromium.org/10806049/diff/4001/PRESUBMIT.py#newcode384
PRESUBMIT.py:384: # We need to wait until we have an input_api object and use
this
old_sys = sys.path
try:
  sys.path = sys.path + [os.path.join(input_api.PresubmitLocalPath(), 'tools',
'checkdeps')]

# This creates a new list.

finally:
  sys.path = old_sys_path

http://codereview.chromium.org/10806049/diff/4001/PRESUBMIT.py#newcode396
PRESUBMIT.py:396: include_lines = []
include_lines = [line for line_num, line in f.ChangedContents()]

but is that what you want to do?

http://codereview.chromium.org/10806049/diff/4001/PRESUBMIT.py#newcode422
PRESUBMIT.py:422: return results
Personally, I would probably have just input_api.subprocess.check_call() the
script with the list of files to check. To work around command line length
limitation, use '-' as parameter and have the script files =
sys.stdin.readlines(). Feels easier to me.

[Jói, 2012-07-26 13:00:34]

Thanks!  I will wait for your LGTM and until you've also finished reviewing the
parent change (http://codereview.chromium.org/10805042/).

Cheers,
Jói

http://codereview.chromium.org/10806049/diff/4001/PRESUBMIT.py
File PRESUBMIT.py (right):

http://codereview.chromium.org/10806049/diff/4001/PRESUBMIT.py#newcode384
PRESUBMIT.py:384: # We need to wait until we have an input_api object and use
this
On 2012/07/26 12:37:42, Marc-Antoine Ruel wrote:
> old_sys = sys.path
> try:
>   sys.path = sys.path + [os.path.join(input_api.PresubmitLocalPath(), 'tools',
> 'checkdeps')]
> 
> # This creates a new list.
> 
> finally:
>   sys.path = old_sys_path

Done.

http://codereview.chromium.org/10806049/diff/4001/PRESUBMIT.py#newcode396
PRESUBMIT.py:396: include_lines = []
On 2012/07/26 12:37:42, Marc-Antoine Ruel wrote:
> include_lines = [line for line_num, line in f.ChangedContents()]
> 
> but is that what you want to do?

It should be named changed_lines rather than include_lines; I fixed that and
used your suggestion.

http://codereview.chromium.org/10806049/diff/4001/PRESUBMIT.py#newcode422
PRESUBMIT.py:422: return results
On 2012/07/26 12:37:42, Marc-Antoine Ruel wrote:
> Personally, I would probably have just input_api.subprocess.check_call() the
> script with the list of files to check. To work around command line length
> limitation, use '-' as parameter and have the script files =
> sys.stdin.readlines(). Feels easier to me.

It's more efficient to check only the changed lines, so coupling via Python
calls seems a lot easier than packing everything into a script invocation.  If
you don't object I will leave this as an import thing with a direct method call.

[M-A Ruel, 2012-07-26 13:09:38]

http://codereview.chromium.org/10806049/diff/4002/PRESUBMIT.py
File PRESUBMIT.py (right):

http://codereview.chromium.org/10806049/diff/4002/PRESUBMIT.py#newcode12
PRESUBMIT.py:12: import os
Remove

http://codereview.chromium.org/10806049/diff/4002/PRESUBMIT.py#newcode389
PRESUBMIT.py:389: sys.path = sys.path + [os.path.join(
input_api.os_path.join(

http://codereview.chromium.org/10806049/diff/4002/PRESUBMIT.py#newcode390
PRESUBMIT.py:390: input_api.change.RepositoryRoot(), 'tools', 'checkdeps')]
It is relatively unsafe to use RepositoryRoot(). With svn, it could be an
arbitrary directory. It's better to use PresubmitLocalPath() as I noted earlier.

http://codereview.chromium.org/10806049/diff/4002/PRESUBMIT.py#newcode420
PRESUBMIT.py:420: results.append(output_api.PresubmitPromptWarning(
On commit, it would need to be a notification otherwise it'll break the CQ.

[Jói, 2012-07-26 13:37:21]

Thanks Marc-Antoine, see new patch and responses below.

Cheers,
Jói

http://codereview.chromium.org/10806049/diff/4002/PRESUBMIT.py
File PRESUBMIT.py (right):

http://codereview.chromium.org/10806049/diff/4002/PRESUBMIT.py#newcode12
PRESUBMIT.py:12: import os
On 2012/07/26 13:09:38, Marc-Antoine Ruel wrote:
> Remove

Done.

http://codereview.chromium.org/10806049/diff/4002/PRESUBMIT.py#newcode389
PRESUBMIT.py:389: sys.path = sys.path + [os.path.join(
On 2012/07/26 13:09:38, Marc-Antoine Ruel wrote:
> input_api.os_path.join(

Done.

http://codereview.chromium.org/10806049/diff/4002/PRESUBMIT.py#newcode390
PRESUBMIT.py:390: input_api.change.RepositoryRoot(), 'tools', 'checkdeps')]
On 2012/07/26 13:09:38, Marc-Antoine Ruel wrote:
> It is relatively unsafe to use RepositoryRoot(). With svn, it could be an
> arbitrary directory. It's better to use PresubmitLocalPath() as I noted
earlier.

Ah, sorry I missed that.  Thanks for catching.

http://codereview.chromium.org/10806049/diff/4002/PRESUBMIT.py#newcode420
PRESUBMIT.py:420: results.append(output_api.PresubmitPromptWarning(
On 2012/07/26 13:09:38, Marc-Antoine Ruel wrote:
> On commit, it would need to be a notification otherwise it'll break the CQ.

I would prefer if people need to jump through hoops to land a change that adds a
dependency we are trying to get rid of.  I wanted to go mid-way by making it a
warning, John suggested making it an error.  I'd prefer not to have this be a
no-problem event when submitting via the CQ.

[M-A Ruel, 2012-07-26 13:52:00]

Can you delete patchset 4 and upload again? Rietveld ate your patch.

[Jói, 2012-07-26 13:55:17]

On 2012/07/26 13:52:00, Marc-Antoine Ruel wrote:
> Can you delete patchset 4 and upload again? Rietveld ate your patch.

Done.

[M-A Ruel, 2012-07-26 14:03:28]

lgtm

http://codereview.chromium.org/10806049/diff/11002/tools/checkdeps/checkdeps.py
File tools/checkdeps/checkdeps.py (right):

http://codereview.chromium.org/10806049/diff/11002/tools/checkdeps/checkdeps....
tools/checkdeps/checkdeps.py:234: def ShouldCheckAddedIncludesInFile(file_path):
ShouldcheckIncludesInCppFile ?

Just to be clearer. Otherwise Java-people looking that the PRESUBMIT.py could be
confused about it.

This is taken for granted that java people are easily confused so this nit is
optional.

http://codereview.chromium.org/10806049/diff/11002/tools/checkdeps/checkdeps....
tools/checkdeps/checkdeps.py:239: def CheckAddedIncludes(added_includes):
I'd like to have Cpp here too but this is outside the scope of this CL.

[Jói, 2012-07-26 14:33:49]

I addressed all of the comments.  Will commit once the parent change
is ready and I have merged to it.

On Thu, Jul 26, 2012 at 2:03 PM,  <maruel@chromium.org> wrote:
> lgtm
>
>
>
http://codereview.chromium.org/10806049/diff/11002/tools/checkdeps/checkdeps.py
> File tools/checkdeps/checkdeps.py (right):
>
>
http://codereview.chromium.org/10806049/diff/11002/tools/checkdeps/checkdeps....
> tools/checkdeps/checkdeps.py:234: def
> ShouldCheckAddedIncludesInFile(file_path):
> ShouldcheckIncludesInCppFile ?
>
> Just to be clearer. Otherwise Java-people looking that the PRESUBMIT.py
> could be confused about it.
>
> This is taken for granted that java people are easily confused so this
> nit is optional.
>
>
http://codereview.chromium.org/10806049/diff/11002/tools/checkdeps/checkdeps....
> tools/checkdeps/checkdeps.py:239: def
> CheckAddedIncludes(added_includes):
> I'd like to have Cpp here too but this is outside the scope of this CL.
>
> http://codereview.chromium.org/10806049/

[Jói, 2012-07-26 17:18:09]

Just FYI, I uploaded a small patch with a merge to the latest parent.  It is
slightly different but preserves the spirit of what you asked for in review
comments about making it explicit that we are checking only C++ files.

Cheers,
Jói

[M-A Ruel, 2012-07-26 18:31:12]

lgtm