[Findit] Factoring the gitiles (etc) stuff out into its own directory

appengine/findit/common/deps_parser.py

Index: appengine/findit/common/deps_parser.py
diff --git a/appengine/findit/common/deps_parser.py b/appengine/findit/common/deps_parser.py
index b2b80b5639bd864814a9cfa7c176ac9f322b563b..78435261b6d00dae389d63f08bc81227194970fd 100644
--- a/appengine/findit/common/deps_parser.py
+++ b/appengine/findit/common/deps_parser.py
@@ -165,12 +165,18 @@ def UpdateDependencyTree(root_dep, target_os_list, deps_loader):
   else:
     target_os_list = [_NormalizeTargetOSName(name) for name in target_os_list]
 
+  # TODO(wrengr): why not just pass the deps_loader to a method on
+  # root_dep which returns the deps_content? What's the point in threading

[stgao, 2016/09/21 23:03:25]

We want root_dep class to be a plain object, and not to deal with http request
or the like.

What's the benefit of moving this over?

[wrengr, 2016/09/28 00:59:49]

Why do we want root_dep to not do http stuff? Even if we do, that doesn't change
the question.
It cleans up the code. Right now, UpdateDependencyTree needs to know all about
the guts of both root_dep and deps_loader, which makes it fragile against
changes to both of them. But UpdateDependencyTree doesn't actually care about
the deps_loader object, we could just as well pass in the deps_content instead.
Or if we really don't want to do that for some reason we could instead say
`deps_content = root_dep.LoadDEPS(deps_loader)`. Either of these changes would
help insulate UpdateDependencyTree against changes to the properties that
root_dep supports. If you're concerned about root_dep not doing any http stuff,
the method can simply delegate back to the deps_loader (aka, use the visitor
pattern):

    def LoadDEPS(self, deps_loader):
        return deps_loader.Load(
            self.deps_repo_url, self.deps_repo_revision, self.deps_file)

Or even better, get rid of the DEPS(Down)loader classes and inline the logic;
this way only the Dependency class really needs to know about its own innards:

    def DownloadDEPS(self, repo):
      content = None
      # Try .DEPS.git instead of DEPS first, for commits during the Git chaos.
      if self.deps_file == 'DEPS' and self.deps_repo_url ==
_CHROMIUM_REPO_MASTER:
        content = repo.GetSource('.DEPS.git', self.deps_repo_revision)

      if content is None:
        content = repo.GetSource(deps_file, self.deps_repo_revision)

      # Like gclient, fall back to raw 'DEPS' when all else fails.
      if content is None and self.deps_file != 'DEPS':
        content = repo.GetSource('DEPS', self.deps_repo_revision)

      if content is None:
        raise Exception(
          'Failed to pull %s file from %s, at revision %s.' % (
            self.deps_file, self.deps_repo_url, self.deps_repo_revision))

      return content

The exact change doesn't matter, what matters is that all these files are
tangled up with one another and consequently hard to maintain. Things like
having the useless DEPSLoader and DEPSDownloader classes, or like
UpdateDependencyTree needing to know all about the guts of root_dep, etc; these
are all just symptoms of the intertwining.

+  # things through like this?
   deps_content = deps_loader.Load(
       root_dep.deps_repo_url, root_dep.deps_repo_revision, root_dep.deps_file)
   deps, deps_os = ParseDEPSContent(deps_content, keys=('deps', 'deps_os'))
 
   all_deps = MergeWithOsDeps(deps, deps_os, target_os_list)
 
+  # TODO(wrengr): why close over root_dep to get at deps_file, but then
+  # force callsers to patch things up after the fact with SetParent? Why
+  # not just call SetParent ourselves?

[stgao, 2016/09/21 23:03:25]

It's good to move that over here. Let's make the change directly in a CL instead
of adding a comment here.

   def _CreateDependency(path, repo_info):
     if not path.endswith('/'):
       path = path + '/'
@@ -189,6 +195,9 @@ def UpdateDependencyTree(root_dep, target_os_list, deps_loader):
       # The dependency is not needed for all the target os.
       continue
 
+    # TODO(wrengr): why are we doing this? i.e., it looks like we're
+    # allocating a dependency.Dependency object and then just dropping it
+    # on the floor. To what end?

[stgao, 2016/09/21 23:03:24]

You meant chromium_deps.py turns the result back to a dict?

     sub_dep = _CreateDependency(path, repo_info)
     sub_dep.SetParent(root_dep)