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

appengine/findit/common/deps_parser.py

 # Copyright 2014 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.
 
 """ This is an incomplete implementation of a DEPS file parser.
 
 Now only keys 'vars', 'deps', and 'deps_os' are taken care of.
 
 TODO: support strict mode, 'target_os', 'target_os_only', 'use_relative_paths',
       both forms of recursion, both forms of hooks, 'allowed_hosts', etc.
@@ skipping 147 matching lines @@
   def _NormalizeTargetOSName(target_os):
     os_name = target_os.lower()
     assert os_name in DEPS_OS_CHOICES, 'Target OS "%s" is invalid' % os_name
     return os_name
 
   if 'all' in target_os_list:
     target_os_list = DEPS_OS_CHOICES
   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 + '/'
 
     repo_url = repo_info
     revision = None
     if '@' in repo_info:
       # The dependency is pinned to some revision.
       repo_url, revision = repo_info.split('@')
 
     return dependency.Dependency(
         path, repo_url, revision, root_dep.deps_file)
 
   for path, repo_info in all_deps.iteritems():
     if repo_info is None:
       # 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)
 
     # TODO: go into the next level of dependency if needed.