gclient flatten: correctness fixes for OS-specific recursedeps

We need to pass OS info to recursively called _flatten_dep.
Regular deps entries in OS-specific DEPS file recursed into
need to be marked as OS-specific in the flattened file.

Bug: 570091
Change-Id: If3055b84143d8a52d10d8753113893b5054b4d07
Reviewed-on: https://chromium-review.googlesource.com/621046Reviewed-by: Michael Moss <mmoss@chromium.org>
Commit-Queue: Paweł Hajdan Jr. <phajdan.jr@chromium.org>

gclient.py

@@ -1794,12 +1794,29 @@ class Flattener(object):
     if dep.url:
       self._deps[dep.name] = dep
 
-  def _flatten_dep(self, dep):
+  def _add_os_dep(self, os_dep, dep_os):
+    """Helper to add an OS-specific dependency to flattened DEPS.
+
+    Arguments:
+      os_dep (Dependency): dependency to add
+      dep_os (str): name of the OS
+    """
+    assert (
+        os_dep.name not in self._deps_os.get(dep_os, {}) or
+        self._deps_os.get(dep_os, {}).get(os_dep.name) == os_dep), (
+            os_dep.name, self._deps_os.get(dep_os, {}).get(os_dep.name))
+    if os_dep.url:
+      self._deps_os.setdefault(dep_os, {})[os_dep.name] = os_dep
+
+  def _flatten_dep(self, dep, dep_os=None):
     """Visits a dependency in order to flatten it (see CMDflatten).
 
     Arguments:
       dep (Dependency): dependency to process
+      dep_os (str or None): name of the OS |dep| is specific to
     """
+    logging.debug('_flatten_dep(%s, %s)', dep.name, dep_os)
+
     if not dep.deps_parsed:
       dep.ParseDepsFile()
 
@@ -1811,31 +1828,42 @@ class Flattener(object):
       assert key not in self._vars or self._vars[key][1] == value
       self._vars[key] = (dep, value)
 
-    self._hooks.extend([(dep, hook) for hook in dep.deps_hooks])
     self._pre_deps_hooks.extend([(dep, hook) for hook in dep.pre_deps_hooks])
 
+    if dep_os:
+      if dep.deps_hooks:
+        self._hooks_os.setdefault(dep_os, []).extend(
+            [(dep, hook) for hook in dep.deps_hooks])
+    else:
+      self._hooks.extend([(dep, hook) for hook in dep.deps_hooks])
+
     for sub_dep in dep.dependencies:
-      self._add_dep(sub_dep)
+      if dep_os:
+        self._add_os_dep(sub_dep, dep_os)
+      else:
+        self._add_dep(sub_dep)
 
     for hook_os, os_hooks in dep.os_deps_hooks.iteritems():
       self._hooks_os.setdefault(hook_os, []).extend(
           [(dep, hook) for hook in os_hooks])
 
-    for dep_os, os_deps in dep.os_dependencies.iteritems():
+    for sub_dep_os, os_deps in dep.os_dependencies.iteritems():
       for os_dep in os_deps:
-        self._deps_os.setdefault(dep_os, {})[os_dep.name] = os_dep
-
-    # Process recursedeps.
-    deps_by_name = dict((d.name, d) for d in dep.dependencies)
-    # Allow recursedeps entries that refer to deps_os entries.
-    # In case there are multiple entries with the same name,
-    # we have to pick something - e.g. the first one.
-    for os_deps in dep.os_dependencies.itervalues():
+        self._add_os_dep(os_dep, sub_dep_os)
+
+    # Process recursedeps. |deps_by_name| is a map where keys are dependency
+    # names, and values are maps of OS names to |Dependency| instances.
+    # |None| in place of OS name means the dependency is not OS-specific.
+    deps_by_name = dict((d.name, {None: d}) for d in dep.dependencies)
+    for sub_dep_os, os_deps in dep.os_dependencies.iteritems():
       for os_dep in os_deps:
-        if os_dep.name not in deps_by_name:
-          deps_by_name[os_dep.name] = os_dep
+        assert sub_dep_os not in deps_by_name.get(os_dep.name, {}), (
+            os_dep.name, sub_dep_os)
+        deps_by_name.setdefault(os_dep.name, {})[sub_dep_os] = os_dep
     for recurse_dep_name in (dep.recursedeps or []):
-      self._flatten_dep(deps_by_name[recurse_dep_name])
+      dep_info = deps_by_name[recurse_dep_name]
+      for sub_dep_os, os_dep in dep_info.iteritems():
+        self._flatten_dep(os_dep, dep_os=(sub_dep_os or dep_os))
 
 
 def CMDflatten(parser, args):

testing_support/fake_repos.py

@@ -594,6 +594,9 @@ deps = {
   'src/repo6': '/repo_6',
 }
 deps_os = {
+  'mac': {
+    'src/repo11': '/repo_11',
+  },
   'ios': {
     'src/repo11': '/repo_11',
   }
@@ -608,7 +611,7 @@ recursedeps = [
     self._commit_git('repo_11', {
       'DEPS': """
 deps = {
-  'src/repo12': '/repo12',
+  'src/repo12': '/repo_12',
 }""",
       'origin': 'git/repo_11@1\n',
     })

tests/gclient_smoketest.py

@@ -892,11 +892,6 @@ class GClientSmokeGIT(GClientSmokeBase):
         '    "url": "git://127.0.0.1:20000/git/repo_10",',
         '  },',
         '',
-        '  # src -> src/repo11 -> src/repo12',
-        '  "src/repo12": {',
-        '    "url": "/repo12",',
-        '  },',
-        '',
         '  # src -> src/repo6',
         '  "src/repo6": {',
         '    "url": "/repo_6",',
@@ -934,6 +929,11 @@ class GClientSmokeGIT(GClientSmokeBase):
         '      "url": "/repo_11",',
         '    },',
         '',
+        '    # src -> src/repo11 -> src/repo12',
+        '    "src/repo12": {',
+        '      "url": "/repo_12",',
+        '    },',
+        '',
         '  },',
         '',
         '  "mac": {',
@@ -942,6 +942,16 @@ class GClientSmokeGIT(GClientSmokeBase):
         '      "url": "/repo_5",',
         '    },',
         '',
+        '    # src -> src/repo11',
+        '    "src/repo11": {',
+        '      "url": "/repo_11",',
+        '    },',
+        '',
+        '    # src -> src/repo11 -> src/repo12',
+        '    "src/repo12": {',
+        '      "url": "/repo_12",',
+        '    },',
+        '',
         '  },',
         '',
         '  "unix": {',