From 7c7503390271018572571ca67acc0fb6ab417d62 Mon Sep 17 00:00:00 2001
From: "joi@chromium.org"
 <joi@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98>
Date: Thu, 6 Dec 2012 17:26:36 +0000
Subject: Fix a bug in checkdeps due to a shallow copy.

Add regression test that now passes and would have caught the bug.

Kudos to jam@ for spotting the symptoms of the bug, which primarily
would manifest when specific_include_rules are being used, and only (I
think) when running presubmit or doing checkdeps for a specific
directory since in that case rules get built up in child->parent order
rather than the parent->child order that the normal checkdeps run
would use.

BUG=None


Review URL: https://chromiumcodereview.appspot.com/11440013

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@171516 0039d316-1c4b-4281-b951-d872f2087c98
---
 tools/checkdeps/checkdeps.py                       |  2 +-
 tools/checkdeps/checkdeps_test.py                  | 23 ++++++++++++++++++++++
 tools/checkdeps/testdata/allowed/DEPS              |  1 +
 .../checkdeps/testdata/disallowed/foo_unittest.cc  | 10 ++++++++++
 4 files changed, 35 insertions(+), 1 deletion(-)
 create mode 100644 tools/checkdeps/testdata/disallowed/foo_unittest.cc

(limited to 'tools')

diff --git a/tools/checkdeps/checkdeps.py b/tools/checkdeps/checkdeps.py
index b7b109a..8333ef6 100755
--- a/tools/checkdeps/checkdeps.py
+++ b/tools/checkdeps/checkdeps.py
@@ -178,7 +178,7 @@ class DepsChecker(object):
     Returns: A new set of rules combining the existing_rules with the other
              arguments.
     """
-    rules = copy.copy(existing_rules)
+    rules = copy.deepcopy(existing_rules)
 
     # First apply the implicit "allow" rule for the current directory.
     if cur_dir.startswith(
diff --git a/tools/checkdeps/checkdeps_test.py b/tools/checkdeps/checkdeps_test.py
index 2ce80fa..e8835a5 100755
--- a/tools/checkdeps/checkdeps_test.py
+++ b/tools/checkdeps/checkdeps_test.py
@@ -149,6 +149,29 @@ class CheckDepsTest(unittest.TestCase):
       ]])
     self.failUnless(problems)
 
+  def testCopyIsDeep(self):
+    # Regression test for a bug where we were making shallow copies of
+    # Rules objects and therefore all Rules objects shared the same
+    # dictionary for specific rules.
+    #
+    # The first pair should bring in a rule from testdata/allowed/DEPS
+    # into that global dictionary that allows the
+    # temp_allowed_for_tests.h file to be included in files ending
+    # with _unittest.cc, and the second pair should completely fail
+    # once the bug is fixed, but succeed (with a temporary allowance)
+    # if the bug is in place.
+    problems = self.deps_checker.CheckAddedCppIncludes(
+      [['tools/checkdeps/testdata/allowed/test.cc',
+        ['#include "tools/checkdeps/testdata/disallowed/temporarily_allowed.h"']
+       ],
+       ['tools/checkdeps/testdata/disallowed/foo_unittest.cc',
+        ['#include "tools/checkdeps/testdata/bongo/temp_allowed_for_tests.h"']
+       ]])
+    # With the bug in place, there would be two problems reported, and
+    # the second would be for foo_unittest.cc.
+    self.failUnless(len(problems) == 1)
+    self.failUnless(problems[0][0].endswith('/test.cc'))
+
 
 if __name__ == '__main__':
   unittest.main()
diff --git a/tools/checkdeps/testdata/allowed/DEPS b/tools/checkdeps/testdata/allowed/DEPS
index 362623c..8fb0905 100644
--- a/tools/checkdeps/testdata/allowed/DEPS
+++ b/tools/checkdeps/testdata/allowed/DEPS
@@ -7,5 +7,6 @@ include_rules = [
 specific_include_rules = {
   ".*_unittest\.cc": [
     "+tools/checkdeps/testdata/disallowed/teststuff",
+    "!tools/checkdeps/testdata/bongo/temp_allowed_for_tests.h",
   ]
 }
diff --git a/tools/checkdeps/testdata/disallowed/foo_unittest.cc b/tools/checkdeps/testdata/disallowed/foo_unittest.cc
new file mode 100644
index 0000000..144c05e
--- /dev/null
+++ b/tools/checkdeps/testdata/disallowed/foo_unittest.cc
@@ -0,0 +1,10 @@
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// Not allowed for code under disallowed/ but temporarily allowed
+// specifically for test code under allowed/.  This regression tests a
+// bug where we were taking shallow copies of rules when generating
+// rules for subdirectories, so all rule objects were getting the same
+// dictionary for specific rules.
+#include "tools/checkdeps/testdata/disallowed/temp_allowed_for_tests.h"
-- 
cgit v1.1