Add priority groups to local paths in the dependency manager.

tools/telemetry/catapult_base/dependency_manager/dependency_info.py

 # Copyright 2015 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.
 
-
 class DependencyInfo(object):
   def __init__(self, dependency, platform, config_file, cs_bucket=None,
                cs_hash=None, download_path=None, cs_remote_path=None,
                version_in_cs=None, unzip_path=None,
                path_within_archive=None, local_paths=None):
     """ Container for the information needed for each dependency/platform pair
     in the dependency_manager.
 
     Args:
         Required:
           dependency: Name of the dependency.
           platform: Name of the platform to be run on.
           config_file: Path to the config_file this information came from. Used
                        for error messages to improve debugging.
 
         Minimum required for downloading from cloud storage:
           cs_bucket: The cloud storage bucket the dependency is located in.
           cs_hash: The hash of the file stored in cloud storage.
           download_path: Where the file should be downloaded to.
           cs_remote_path: Where the file is stored in the cloud storage bucket.
 
         Optional for downloading from cloud storage:
           version: The version of the file stored in cloud storage.
 
         Optional:
           local_paths: A list of paths to search in order for a local file.

[nednguyen, 2015/10/09 16:20:07]

Docstring needs update

[aiolos (Not reviewing), 2015/10/09 16:52:00]

Agreed, but I'm going to wait until the form of the CL is finalized before
updating it.

     """
     # TODO(aiolos): update the above doc string for A) the usage of zip files
     # and B) supporting lists of local_paths to be checked for most recently
     # changed files.
     if not dependency or not platform:
       raise ValueError(
           'Must supply both a dependency and platform to DependencyInfo')
 
     self._dependency = dependency
     self._platform = platform
     self._config_files = [config_file]
-    self._local_paths = local_paths or []
+    self._local_paths = self._ParseLocalPaths(local_paths)

[nednguyen, 2015/10/09 16:20:07]

self._local_paths_groups

[aiolos (Not reviewing), 2015/10/09 16:52:00]

See my response in the comment on _ParseLocalPaths for context.

     self._download_path = download_path
     self._cs_remote_path = cs_remote_path
     self._cs_bucket = cs_bucket
     self._cs_hash = cs_hash
     self._version_in_cs = version_in_cs
-    self.VerifyCloudStorageInfo()
+    self._VerifyCloudStorageInfo()
 
   def Update(self, new_dep_info):
     """Add the information from |new_dep_info| to this instance.
     """
     self._config_files.extend(new_dep_info.config_files)
     if (self.dependency != new_dep_info.dependency or
         self.platform != new_dep_info.platform):
       raise ValueError(
           'Cannot update DependencyInfo with different dependency or platform.'
           'Existing dep: %s, existing platform: %s. New dep: %s, new platform:'
           '%s. Config_files conflicting: %s' % (
               self.dependency, self.platform, new_dep_info.dependency,
               new_dep_info.platform, self.config_files))
     if new_dep_info.has_cs_info:
       if self.has_cs_info:
         raise ValueError(
             'Overriding cloud storage data is not allowed when updating a '
             'DependencyInfo. Conflict in dependency %s on platform %s in '
             'config_files: %s.' % (self.dependency, self.platform,
                                   self.config_files))
       else:
         self._download_path = new_dep_info.download_path
         self._cs_remote_path = new_dep_info.cs_remote_path
         self._cs_bucket = new_dep_info.cs_bucket
         self._cs_hash = new_dep_info.cs_hash
         self._version_in_cs = new_dep_info.version_in_cs
     if new_dep_info.local_paths:
-      for path in new_dep_info.local_paths:
-        if path not in self._local_paths:
-          self._local_paths.append(path)
+      for priority_group in new_dep_info.local_paths:
+        group_list = []
+        for path in priority_group:
+          if not self.IsPathInLocalPaths(path):
+            group_list.append(path)
+        if group_list:
+          self._local_paths.append(group_list)
+
+  def IsPathInLocalPaths(self, path):
+    return any(path in priority_group for priority_group in self._local_paths)
 
   @property
   def dependency(self):
     return self._dependency
 
   @property
   def platform(self):
     return self._platform
 
   @property
   def config_files(self):
     return self._config_files
 
   @property
   def local_paths(self):

[nednguyen, 2015/10/09 16:20:07]

This should be local_paths_groups

[aiolos (Not reviewing), 2015/10/09 16:52:00]

Same.

     return self._local_paths
 
   @property
   def download_path(self):
     return self._download_path
 
   @property
   def cs_remote_path(self):
     return self._cs_remote_path
 
   @property
   def cs_bucket(self):
     return self._cs_bucket
 
   @property
   def cs_hash(self):
     return self._cs_hash
 
   @property
   def version_in_cs(self):
     return self._version_in_cs
 
   @property
   def has_cs_info(self):
-    self.VerifyCloudStorageInfo()
+    self._VerifyCloudStorageInfo()
     return self.cs_hash
 
-  def VerifyCloudStorageInfo(self):
+  @staticmethod
+  def _ParseLocalPaths(local_paths):

[nednguyen, 2015/10/09 16:20:07]

We should restrict the type of local_paths to list of list. Th rule that if this
is a list of path strings, then each path is transformed into a priority group
with single path is surprising.

[aiolos (Not reviewing), 2015/10/09 16:52:00]

I'm not sure why that's surprising, since priority groups are an abstraction
within the dependency manager that keeps the current behavior with flat
local_paths lists. Priority groups are meant to be an optional feature, and
should not force people to use them in their configs if they don't want to. It's
also why I kept the name "local_files" instead of changing it to
"local_file_priority_groups" or similar.

In the json files:

['1', '2', '3'] means files at path 1, 2 and 3 will all be checked in order.
[['1', '2', '3']] means that files at paths 1, 2 and 3 should all be checked and
the most recently changed one should be used.
['1', ['2', '3']] or [['1'], ['2', '3']] mean that the file at path 1 should be
used if it's available, otherwise use path 2 or 3 depending on which one is
newer.


If we changed it, that would mean forcing the json for the first case to look
like this instead, which seems over complicated. (Note that this example still
works with the current implementation, it just isn't required.)

[['1'], ['2'], ['3']]


If we want to force users to always put files in priority groups for Telemetry,
which I think is questionable, we should be doing so at the config level, not
the dependency_info level. That will allow future configs to specify either
always or never using the priority groups if they so desire.

[nednguyen, 2015/10/09 17:04:04]

The fact that priority groups is an optional feature to users is orthogonal to
the issue of naming "local_paths" variable. If I see
"dependency_info.local_files" in code, how would I suppose to expect that the
variable 'local_files' is meant to be a list of list of files? 

You can keep the "local_paths" argument unchanged in the constructor of
DependencyInfo & add documentation explaining the extra meaning. However the
member method "local_paths" of DependencyInfo should be renamed to reflect that
it's not a simple list of paths.
Hmm, this seems reasonable. But can you update the docstring of BaseConfig to
also reflect this?
 

+    if not local_paths:
+      return []
+    list_of_lists = []
+    for element in local_paths:
+      if isinstance(element, basestring):
+        list_of_lists.append([element])
+      else:
+        list_of_lists.append(element)
+    return list_of_lists
+
+  def _VerifyCloudStorageInfo(self):
     """Ensure either all or none of the needed remote information is specified.
     """
     if ((self.cs_bucket or self.cs_remote_path or self.download_path or
           self.cs_hash or self._version_in_cs) and not (self.cs_bucket and
             self.cs_remote_path and self.download_path and self.cs_hash)):
       raise ValueError(
             'Attempted to partially initialize cloud storage data for '
             'dependency: %s, platform: %s, download_path: %s, '
             'cs_remote_path: %s, cs_bucket %s, cs_hash %s, version_in_cs %s' % (
                 self.dependency, self.platform, self.download_path,
                 self.cs_remote_path, self.cs_bucket, self.cs_hash,
                 self._version_in_cs))