Do per-file OWNERS check for per-file DEPS changes.

Do per-file OWNERS check for per-file DEPS changes.

Before this, an OWNER of the entire directory that a new DEPS rule was pointing to would be required, even when the DEPS rule specified an individual file for which there are per-file OWNERS in the OWNERS file.

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

[Jói, 2014-01-02 15:13:40]

Finally got around to this on this most quiet of working days. Happy New Year :)

Cheers,
Jói

[Jói, 2014-01-07 08:40:52]

stepco: Gentle ping.

On Thu, Jan 2, 2014 at 3:13 PM,  <joi@chromium.org> wrote:
> Reviewers: stepco,
>
> Message:
> Finally got around to this on this most quiet of working days. Happy New
> Year :)
>
> Cheers,
> Jói
>
>
> Description:
> Do per-file OWNERS check for per-file DEPS changes.
>
> Before this, an OWNER of the entire directory that a new DEPS rule was
> pointing
> to would be required, even when the DEPS rule specified an individual file
> for
> which there are per-file OWNERS in the OWNERS file.
>
> BUG=290401
>
> Please review this at https://codereview.chromium.org/116443011/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>
> Affected files (+33, -13 lines):
>   M PRESUBMIT.py
>   M PRESUBMIT_test.py
>
>
> Index: PRESUBMIT.py
> diff --git a/PRESUBMIT.py b/PRESUBMIT.py
> index
>
04cea10dffee5aed22c9e0372b8bae7fb6e03974..1615cf1e6bc6154da985b2a28c480362887fac5c
> 100644
> --- a/PRESUBMIT.py
> +++ b/PRESUBMIT.py
> @@ -783,9 +783,14 @@ def _CheckNoAbbreviationInPngFileName(input_api,
> output_api):
>    return results
>
>
> -def _DepsFilesToCheck(re, changed_lines):
> +def _FilesToCheckForIncomingDeps(re, changed_lines):
>    """Helper method for _CheckAddedDepsHaveTargetApprovals. Returns
> -  a set of DEPS entries that we should look up."""
> +  a set of DEPS entries that we should look up.
> +
> +  For a directory (rather than a specific filename) we fake a path to
> +  a specific filename by adding /DEPS. This is chosen as a file that
> +  will seldom or never be subject to per-file include_rules.
> +  """
>    # We ignore deps entries on auto-generated directories.
>    AUTO_GENERATED_DIRS = ['grit', 'jni']
>
> @@ -801,7 +806,10 @@ def _DepsFilesToCheck(re, changed_lines):
>      if m:
>        path = m.group(1)
>        if path.split('/')[0] not in AUTO_GENERATED_DIRS:
> -        results.add('%s/DEPS' % m.group(1))
> +        if m.group(2):
> +          results.add('%s%s' % (path, m.group(2)))
> +        else:
> +          results.add('%s/DEPS' % path)
>    return results
>
>
> @@ -821,7 +829,8 @@ def _CheckAddedDepsHaveTargetApprovals(input_api,
> output_api):
>    if not changed_lines:
>      return []
>
> -  virtual_depended_on_files = _DepsFilesToCheck(input_api.re,
> changed_lines)
> +  virtual_depended_on_files = _FilesToCheckForIncomingDeps(input_api.re,
> +                                                           changed_lines)
>    if not virtual_depended_on_files:
>      return []
>
> @@ -850,12 +859,22 @@ def _CheckAddedDepsHaveTargetApprovals(input_api,
> output_api):
>      reviewers_plus_owner.add(owner_email)
>    missing_files = owners_db.files_not_covered_by(virtual_depended_on_files,
>                                                   reviewers_plus_owner)
> -  unapproved_dependencies = ["'+%s'," % path[:-len('/DEPS')]
> +
> +  # We strip the /DEPS part that was added by
> +  # _FilesToCheckForIncomingDeps to fake a path to a file in a
> +  # directory.
> +  def StripDeps(path):
> +    start_deps = path.rfind('/DEPS')
> +    if start_deps != -1:
> +      return path[:start_deps]
> +    else:
> +      return path
> +  unapproved_dependencies = ["'+%s'," % StripDeps(path)
>                               for path in missing_files]
>
>    if unapproved_dependencies:
>      output_list = [
> -      output('Missing LGTM from OWNERS of directories added to DEPS:\n
> %s' %
> +      output('Missing LGTM from OWNERS of dependencies added to DEPS:\n
> %s' %
>               '\n    '.join(sorted(unapproved_dependencies)))]
>      if not input_api.is_committing:
>        suggested_owners = owners_db.reviewers_for(missing_files,
> owner_email)
> Index: PRESUBMIT_test.py
> diff --git a/PRESUBMIT_test.py b/PRESUBMIT_test.py
> index
>
1c7990ce898bb824b1b7068d7787826b9250a0d1..509c45c891b95ebeb138e9901043eb7008da69b7
> 100755
> --- a/PRESUBMIT_test.py
> +++ b/PRESUBMIT_test.py
> @@ -394,7 +394,7 @@ class InvalidOSMacroNamesTest(unittest.TestCase):
>
>
>  class CheckAddedDepsHaveTetsApprovalsTest(unittest.TestCase):
> -  def testDepsFilesToCheck(self):
> +  def testFilesToCheckForIncomingDeps(self):
>      changed_lines = [
>        '"+breakpad",',
>        '"+chrome/installer",',
> @@ -404,6 +404,7 @@ class
> CheckAddedDepsHaveTetsApprovalsTest(unittest.TestCase):
>        '"+components/breakpad",',
>        '"+components/nacl/common",',
>        '"+content/public/browser/render_process_host.h",',
> +      '"+jni/fooblat.h",',
>        '"+grit",  # For generated headers',
>        '"+grit/generated_resources.h",',
>        '"+grit/",',
> @@ -412,20 +413,20 @@ class
> CheckAddedDepsHaveTetsApprovalsTest(unittest.TestCase):
>        '"+tools/memory_watcher",',
>        '"+third_party/lss/linux_syscall_support.h",',
>      ]
> -    files_to_check = PRESUBMIT._DepsFilesToCheck(re, changed_lines)
> +    files_to_check = PRESUBMIT._FilesToCheckForIncomingDeps(re,
> changed_lines)
>      expected = set([
>        'breakpad/DEPS',
>        'chrome/installer/DEPS',
> -      'chrome/plugin/DEPS',
> -      'chrome/utility/DEPS',
> -      'chromeos/DEPS',
> +      'chrome/plugin/chrome_content_plugin_client.h',
> +      'chrome/utility/chrome_content_utility_client.h',
> +      'chromeos/chromeos_paths.h',
>        'components/breakpad/DEPS',
>        'components/nacl/common/DEPS',
> -      'content/public/browser/DEPS',
> +      'content/public/browser/render_process_host.h',
>        'policy/DEPS',
>        'sandbox/DEPS',
>        'tools/memory_watcher/DEPS',
> -      'third_party/lss/DEPS',
> +      'third_party/lss/linux_syscall_support.h',
>      ])
>      self.assertEqual(expected, files_to_check);
>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Steve Condie, 2014-01-07 18:25:25]

lgtm

[commit-bot: I haz the power, 2014-01-07 21:30:33]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a lowly provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[Jói, 2014-01-07 21:36:02]

+maruel for rubber-stamp LGTM.

[M-A Ruel, 2014-01-07 21:39:02]

lgtm

[commit-bot: I haz the power, 2014-01-07 22:23:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/116443011/20001

[commit-bot: I haz the power, 2014-01-08 01:15:51]

Change committed as 243459