[NaCl SDK] Refactor sdk_update*.

native_client_sdk/src/build_tools/sdk_tools/sdk_update_main.py

Index: native_client_sdk/src/build_tools/sdk_tools/sdk_update_main.py
diff --git a/native_client_sdk/src/build_tools/sdk_tools/sdk_update_main.py b/native_client_sdk/src/build_tools/sdk_tools/sdk_update_main.py
index c16af4a7ba5e8fe41563492e77ec427b42eb5d20..048433e5de3b7bf9fc6ae07dcda8ec3c6595ea87 100755
--- a/native_client_sdk/src/build_tools/sdk_tools/sdk_update_main.py
+++ b/native_client_sdk/src/build_tools/sdk_tools/sdk_update_main.py
@@ -3,760 +3,369 @@
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
-'''A simple tool to update the Native Client SDK to the latest version'''
+# CMD code copied from git_cl.py in depot_tools.
 
+import config
 import cStringIO
-import cygtar
+import download
 import json
-import manifest_util
+import logging
 import optparse
 import os
-from sdk_update_common import RenameDir, RemoveDir, Error
-import shutil
-import subprocess
+import re
+import sdk_update_common
+from sdk_update_common import Error
 import sys
-import tempfile
-# when pylint runs the third_party module is the one from depot_tools
-# pylint: disable=E0611
-from third_party import fancy_urllib
 import urllib2
-import urlparse
 
-# pylint: disable=C0301
-
-#------------------------------------------------------------------------------
-# Constants
+SCRIPT_DIR = os.path.dirname(os.path.abspath(__file__))
+PARENT_DIR = os.path.dirname(SCRIPT_DIR)
+
+try:
+  import manifest_util
+except ImportError:
+  sys.path.append(os.path.dirname(SCRIPT_DIR))
+  import manifest_util

[Sam Clegg, 2012/10/22 22:55:30]

Why not just always add to sys.path and avoid the try block?

[binji, 2012/10/23 00:14:09]

Done.

+
+try:
+  # Unused import cygtar; its nicer to import here (and use in the commands)
+  # than to find it in every command that needs it.
+  # pylint: disable=W0611
+  import cygtar
+except ImportError:
+  # Try to find this in the Chromium repo.
+  sys.path.append(os.path.abspath(
+      os.path.join(PARENT_DIR, '..', '..', '..', 'native_client', 'build')))
+  import cygtar

[Sam Clegg, 2012/10/22 22:55:30]

Yuck.. is this so the script can work in SCM as well as when packaged?

Could the import not happen at the top of commands/__init__.py?

[binji, 2012/10/23 00:14:09]

It seems that only update is using cygtar anyway, I just moved the import there.

+
+# Import late so each command script can find our imports
+# pylint thinks this is commands from the python library.
+import commands
+
+# Module commands has no member... (this is because pylint thinks commands is
+# from the python standard library).
+# pylint: disable=E1101
 
 # This revision number is autogenerated from the Chrome revision.
 REVISION = '{REVISION}'
 
-GLOBAL_HELP = '''Usage: naclsdk [options] command [command_options]
-
-naclsdk is a simple utility that updates the Native Client (NaCl)
-Software Developer's Kit (SDK).  Each component is kept as a 'bundle' that
-this utility can download as as subdirectory into the SDK.
-
-Commands:
-  help [command] - Get either general or command-specific help
-  info - Displays information about a bundle
-  list - Lists the available bundles
-  update/install - Updates/installs bundles in the SDK
-  sources - Manage external package sources
-
-Example Usage:
-  naclsdk info pepper_canary
-  naclsdk list
-  naclsdk update --force pepper_17
-  naclsdk install recommended
-  naclsdk help update
-  naclsdk sources --list'''
-
+GSTORE_URL = 'https://commondatastorage.googleapis.com/nativeclient-mirror'
 CONFIG_FILENAME = 'naclsdk_config.json'
 MANIFEST_FILENAME = 'naclsdk_manifest2.json'
-SDK_TOOLS = 'sdk_tools'  # the name for this tools directory
-USER_DATA_DIR = 'sdk_cache'
-
-HTTP_CONTENT_LENGTH = 'Content-Length'  # HTTP Header field for content length
-
-
-#------------------------------------------------------------------------------
-# General Utilities
-
-
-_debug_mode = False
-_quiet_mode = False
-
-
-def DebugPrint(msg):
-  '''Display a message to stderr if debug printing is enabled
-
-  Note: This function appends a newline to the end of the string
+DEFAULT_SDK_ROOT = os.path.abspath(PARENT_DIR)
+USER_DATA_DIR = os.path.join(DEFAULT_SDK_ROOT, 'sdk_cache')
 
-  Args:
-    msg: A string to send to stderr in debug mode'''
-  if _debug_mode:
-    sys.stderr.write("%s\n" % msg)
-    sys.stderr.flush()
 
+def usage(more):
+  def hook(fn):
+    fn.usage_more = more
+    return fn
+  return hook
 
-def InfoPrint(msg):
-  '''Display an informational message to stdout if not in quiet mode
 
-  Note: This function appends a newline to the end of the string
+def hide(fn):
+  fn.hide = True
+  return fn
 
-  Args:
-    mgs: A string to send to stdio when not in quiet mode'''
-  if not _quiet_mode:
-    sys.stdout.write("%s\n" % msg)
-    sys.stdout.flush()
 
-
-def WarningPrint(msg):
-  '''Display an informational message to stderr.
-
-  Note: This function appends a newline to the end of the string
-
-  Args:
-    mgs: A string to send to stderr.'''
-  sys.stderr.write("WARNING: %s\n" % msg)
-  sys.stderr.flush()
-
-
-def UrlOpen(url):
-  request = fancy_urllib.FancyRequest(url)
-  ca_certs = os.path.join(os.path.dirname(os.path.abspath(__file__)),
-                          'cacerts.txt')
-  request.set_ssl_info(ca_certs=ca_certs)
-  url_opener = urllib2.build_opener(
-      fancy_urllib.FancyProxyHandler(),
-      fancy_urllib.FancyRedirectHandler(),
-      fancy_urllib.FancyHTTPSHandler())
-  return url_opener.open(request)
-
-def ExtractInstaller(installer, outdir):
-  '''Extract the SDK installer into a given directory
-
-  If the outdir already exists, then this function deletes it
-
-  Args:
-    installer: full path of the SDK installer
-    outdir: output directory where to extract the installer
-
-  Raises:
-    CalledProcessError - if the extract operation fails'''
-  RemoveDir(outdir)
-
-  if os.path.splitext(installer)[1] == '.exe':
-    # If the installer has extension 'exe', assume it's a Windows NSIS-style
-    # installer that handles silent (/S) and relocated (/D) installs.
-    command = [installer, '/S', '/D=%s' % outdir]
-    subprocess.check_call(command)
-  else:
-    os.mkdir(outdir)
-    tar_file = None
-    curpath = os.getcwd()
-    try:
-      tar_file = cygtar.CygTar(installer, 'r', verbose=True)
-      if outdir:
-        os.chdir(outdir)
-      tar_file.Extract()
-    finally:
-      if tar_file:
-        tar_file.Close()
-      os.chdir(curpath)
-
-
-class ProgressFunction(object):
-  '''Create a progress function for a file with a given size'''
-
-  def __init__(self, file_size=0):
-    '''Constructor
-
-    Args:
-      file_size: number of bytes in file.  0 indicates unknown'''
-    self.dots = 0
-    self.file_size = int(file_size)
-
-  def GetProgressFunction(self):
-    '''Returns a progress function based on a known file size'''
-    def ShowKnownProgress(progress):
-      if progress == 0:
-        sys.stdout.write('|%s|\n' % ('=' * 48))
-      else:
-        new_dots = progress * 50 / self.file_size - self.dots
-        sys.stdout.write('.' * new_dots)
-        self.dots += new_dots
-        if progress == self.file_size:
-          sys.stdout.write('\n')
-      sys.stdout.flush()
-
-    return ShowKnownProgress
-
-
-def DownloadArchiveToFile(archive, dest_path):
-  '''Download the archive's data to a file at dest_path.
-
-  As a side effect, computes the sha1 hash and data size, both returned as a
-  tuple. Raises an Error if the url can't be opened, or an IOError exception if
-  dest_path can't be opened.
-
-  Args:
-    dest_path: Path for the file that will receive the data.
-  Return:
-    A tuple (sha1, size) with the sha1 hash and data size respectively.'''
-  sha1 = None
-  size = 0
-  with open(dest_path, 'wb') as to_stream:
-    from_stream = None
-    try:
-      from_stream = UrlOpen(archive.url)
-    except urllib2.URLError:
-      raise Error('Cannot open "%s" for archive %s' %
-          (archive.url, archive.host_os))
-    try:
-      content_length = int(from_stream.info()[HTTP_CONTENT_LENGTH])
-      progress_function = ProgressFunction(content_length).GetProgressFunction()
-      InfoPrint('Downloading %s' % archive.url)
-      sha1, size = manifest_util.DownloadAndComputeHash(
-          from_stream,
-          to_stream=to_stream,
-          progress_func=progress_function)
-      if size != content_length:
-        raise Error('Download size mismatch for %s.\n'
-                    'Expected %s bytes but got %s' %
-                    (archive.url, content_length, size))
-    finally:
-      if from_stream:
-        from_stream.close()
-  return sha1, size
-
-
-def LoadFromFile(path, obj):
-  '''Returns a manifest loaded from the JSON file at |path|.
-
-  If the path does not exist or is invalid, returns unmodified object.'''
-  methodlist = [m for m in dir(obj) if callable(getattr(obj, m))]
-  if 'LoadDataFromString' not in methodlist:
-    return obj
+def LoadConfig(raise_on_error=False):
+  path = os.path.join(USER_DATA_DIR, CONFIG_FILENAME)
   if not os.path.exists(path):
-    return obj
-
-  with open(path, 'r') as f:
-    json_string = f.read()
-  if not json_string:
-    return obj
-
-  obj.LoadDataFromString(json_string)
-  return obj
+    return config.Config()
 
-
-def LoadManifestFromURLs(urls):
-  '''Returns a manifest loaded from |urls|, merged into one manifest.'''
-  manifest = manifest_util.SDKManifest()
-  for url in urls:
+  try:
     try:
-      url_stream = UrlOpen(url)
-    except urllib2.URLError as e:
-      raise Error('Unable to open %s. [%s]' % (url, e))
-
-    manifest_stream = cStringIO.StringIO()
-    manifest_util.DownloadAndComputeHash(url_stream, manifest_stream)
-    temp_manifest = manifest_util.SDKManifest()
-    temp_manifest.LoadDataFromString(manifest_stream.getvalue())
+      return config.Config(json.loads(open(path).read()))
+    except IOError as e:
+      raise Error('Unable to read config from "%s".\n  %s' % (path, e))
+    except Exception as e:
+      raise Error('Parsing config file from "%s" failed.\n  %s' % (path, e))
+  except Error as e:
+    if raise_on_error:
+      raise
+    else:
+      logging.warn(str(e))
+  return config.Config()
 
-    manifest.MergeManifest(temp_manifest)
 
-  def BundleFilter(bundle):
-    # Only add this bundle if it's supported on this platform.
-    return bundle.GetHostOSArchive()
+def WriteConfig(cfg):
+  path = os.path.join(USER_DATA_DIR, CONFIG_FILENAME)
+  try:
+    sdk_update_common.MakeDirs(USER_DATA_DIR)
+  except Exception as e:
+    raise Error('Unable to create directory "%s".\n  %s' % (USER_DATA_DIR, e))
 
-  manifest.FilterBundles(BundleFilter)
-  return manifest
+  try:
+    open(path, 'w').write(cfg.ToJson())
+  except IOError as e:
+    raise Error('Unable to write config to "%s".\n  %s' % (path, e))
+  except Exception as e:
+    raise Error('Json encoding error writing config "%s".\n  %s' % (path, e))

[Sam Clegg, 2012/10/22 22:55:30]

Maybe use two different try blocks here.   Generate json first, then write it to
a file.

Is it worth catching IOError here?  Is it likely that the file is not writable?

We should probably always be using the the "with open() as.." pattern for file
I/O.

[binji, 2012/10/23 00:14:09]

Done.

 
 
-def WriteToFile(path, obj):
-  '''Write |manifest| to a JSON file at |path|.'''
-  methodlist = [m for m in dir(obj) if callable(getattr(obj, m))]
-  if 'GetDataAsString' not in methodlist:
-    raise Error('Unable to write object to file')
-  json_string = obj.GetDataAsString()
-
-  # Write the JSON data to a temp file.
-  temp_file_name = None
-  # TODO(dspringer): Use file locks here so that multiple sdk_updates can
-  # run at the same time.
-  with tempfile.NamedTemporaryFile(mode='w', delete=False) as f:
-    f.write(json_string)
-    temp_file_name = f.name
-  # Move the temp file to the actual file.
-  if os.path.exists(path):
-    os.remove(path)
-  shutil.move(temp_file_name, path)
-
-
-class SDKConfig(object):
-  '''This class contains utilities for manipulating an SDK config
-  '''
-
-  def __init__(self):
-    '''Create a new SDKConfig object with default contents'''
-    self._data = {
-        'sources': [],
-        }
-
-  def AddSource(self, string):
-    '''Add a source file to load packages from.
-
-    Args:
-      string: a URL to an external package manifest file.'''
-    # For now whitelist only the following location for external sources:
-    # https://commondatastorage.googleapis.com/nativeclient-mirror/nacl/nacl_sdk
-    (scheme, host, path, _, _, _) = urlparse.urlparse(string)
-    if (host != 'commondatastorage.googleapis.com' or
-        scheme != 'https' or
-        not path.startswith('/nativeclient-mirror/nacl/nacl_sdk')):
-      WarningPrint('Only whitelisted sources from '
-                   '\'https://commondatastorage.googleapis.com/nativeclient-'
-                   'mirror/nacl/nacl_sdk\' are currently allowed.')
-      return
-    if string in self._data['sources']:
-      WarningPrint('source \''+string+'\' already exists in config.')
-      return
+def LoadLocalManifest(raise_on_error=False):
+  path = os.path.join(USER_DATA_DIR, MANIFEST_FILENAME)
+  manifest = manifest_util.SDKManifest()
+  try:
     try:
-      UrlOpen(string)
-    except urllib2.URLError:
-      WarningPrint('Unable to fetch manifest URL \'%s\'. Exiting...' % string)
-      return
-
-    self._data['sources'].append(string)
-    InfoPrint('source \''+string+'\' added to config.')
-
-  def RemoveSource(self, string):
-    '''Remove a source file to load packages from.
-
-    Args:
-      string: a URL to an external SDK manifest file.'''
-    if string not in self._data['sources']:
-      WarningPrint('source \''+string+'\' doesn\'t exist in config.')
-    else:
-      self._data['sources'].remove(string)
-      InfoPrint('source \''+string+'\' removed from config.')
-
-  def RemoveAllSources(self):
-    if len(self.GetSources()) == 0:
-      InfoPrint('There are no external sources to remove.')
-    # Copy the list because RemoveSource modifies the underlying list
-    sources = list(self.GetSources())
-    for source in sources:
-      self.RemoveSource(source)
-
-
-  def ListSources(self):
-    '''List all external sources in config.'''
-    if len(self._data['sources']):
-      InfoPrint('Installed sources:')
-      for s in self._data['sources']:
-        InfoPrint('  '+s)
+      manifest_string = open(path).read()
+      manifest.LoadDataFromString(manifest_string)
+    except IOError as e:
+      raise Error('Unable to read manifest from "%s".\n  %s' % (path, e))
+    except Exception as e:
+      raise Error('Parsing local manifest "%s" failed.\n  %s' % (path, e))
+  except Error as e:
+    if raise_on_error:
+      raise
     else:
-      InfoPrint('No external sources installed')
-
-  def GetSources(self):
-    '''Return a list of external sources'''
-    return self._data['sources']
-
-  def LoadDataFromString(self, string):
-    ''' Load a JSON config string. Raises an exception if string
-        is not well-formed JSON.
-
-    Args:
-      string: a JSON-formatted string containing the previous config'''
-    self._data = json.loads(string)
-
-
-  def GetDataAsString(self):
-    '''Returns the current JSON manifest object, pretty-printed'''
-    pretty_string = json.dumps(self._data, sort_keys=False, indent=2)
-    # json.dumps sometimes returns trailing whitespace and does not put
-    # a newline at the end.  This code fixes these problems.
-    pretty_lines = pretty_string.split('\n')
-    return '\n'.join([line.rstrip() for line in pretty_lines]) + '\n'
+      logging.warn(str(e))
+  return manifest
 
 
-#------------------------------------------------------------------------------
-# Commands
+def WriteLocalManifest(manifest):
+  path = os.path.join(USER_DATA_DIR, MANIFEST_FILENAME)
+  try:
+    sdk_update_common.MakeDirs(USER_DATA_DIR)
+  except Exception as e:
+    raise Error('Unable to create directory "%s".\n  %s' % (USER_DATA_DIR, e))
 
+  try:
+    manifest_json = manifest.GetDataAsString()
+    open(path, 'w').write(manifest_json)
+  except IOError as e:
+    logging.error('Unable to write manifest to "%s".\n  %s' % (path, e))
+  except Exception as e:
+    logging.error('Error encoding manifest "%s" to JSON.\n  %s' % (path, e))

[Sam Clegg, 2012/10/22 22:55:30]

same as above.

Why are you logging here rather than raising Error for the
user to see?

[binji, 2012/10/23 00:14:09]

Ah, it was because I was calling this from the finally block, but when this
function raised, it was killing the original raiser -- the better solution is to
handle that below.

 
-def Info(options, argv, config):
-  '''Usage: %prof [global_options] info [options] bundle_names...
 
-  Displays information about a SDK bundle.'''
+def LoadRemoteManifest(url):
+  manifest = manifest_util.SDKManifest()
+  url_stream = None
+  try:
+    manifest_stream = cStringIO.StringIO()
+    url_stream = download.UrlOpen(url)
+    download.DownloadAndComputeHash(url_stream, manifest_stream)
+  except urllib2.URLError as e:
+    raise Error('Unable to read remote manifest from URL "%s".\n  %s' % (
+        url, e))
+  finally:
+    if url_stream:
+      url_stream.close()
 
-  DebugPrint("Running List command with: %s, %s" %(options, argv))
+  try:
+    manifest.LoadDataFromString(manifest_stream.getvalue())
+    return manifest
+  except manifest_util.Error as e:
+    raise Error('Parsing remote manifest from URL "%s" failed.\n  %s' % (
+        url, e,))
 
-  parser = optparse.OptionParser(usage=Info.__doc__)
-  (_, args) = parser.parse_args(argv)
 
-  if not args:
-    parser.print_help()
-    return
-
-  manifest = LoadManifestFromURLs([options.manifest_url] + config.GetSources())
-  valid_bundles = [bundle.name for bundle in manifest.GetBundles()]
-  valid_args = set(args) & set(valid_bundles)
-  invalid_args = set(args) - valid_args
-  if invalid_args:
-    InfoPrint('Unknown bundle(s): %s\n' % (', '.join(invalid_args)))
-
-  for bundle_name in args:
-    if bundle_name not in valid_args:
-      continue
-
-    bundle = manifest.GetBundle(bundle_name)
-
-    InfoPrint('%s' % bundle.name)
-    for key, value in bundle.iteritems():
-      if key == manifest_util.ARCHIVES_KEY:
-        archive = bundle.GetHostOSArchive()
-        InfoPrint('  Archive:')
-        for archive_key, archive_value in archive.iteritems():
-          InfoPrint('    %s: %s' % (archive_key, archive_value))
-      elif key not in (manifest_util.ARCHIVES_KEY, manifest_util.NAME_KEY):
-        InfoPrint('  %s: %s' % (key, value))
-    InfoPrint('')
-
-
-def List(options, argv, config):
-  '''Usage: %prog [global_options] list [options]
-
-  Lists the available SDK bundles that are available for download.'''
-
-  def PrintBundle(local_bundle, bundle, needs_update, display_revisions):
-    installed = local_bundle is not None
-    # If bundle is None, there is no longer a remote bundle with this name.
-    if bundle is None:
-      bundle = local_bundle
-
-    if display_revisions:
-      if needs_update:
-        revision = ' (r%s -> r%s)' % (local_bundle.revision, bundle.revision)
-      else:
-        revision = ' (r%s)' % (bundle.revision,)
-    else:
-      revision = ''
+def LoadCombinedRemoteManifest(default_manifest_url, cfg):
+  manifest = LoadRemoteManifest(default_manifest_url)
+  for source in cfg.sources:
+    manifest.MergeManifest(LoadRemoteManifest(source))
+  return manifest
 
-    InfoPrint('  %s%s %s (%s)%s' % (
-      'I' if installed else ' ',
-      '*' if needs_update else ' ',
-      bundle.name,
-      bundle.stability,
-      revision))
 
+# Commands #####################################################################
 
-  DebugPrint("Running List command with: %s, %s" %(options, argv))
 
-  parser = optparse.OptionParser(usage=List.__doc__)
-  parser.add_option(
-      '-r', '--revision', dest='revision',
-      default=False, action='store_true',
-      help='display revision numbers')
-  (list_options, _) = parser.parse_args(argv)
-
-  manifest = LoadManifestFromURLs([options.manifest_url] + config.GetSources())
-  manifest_path = os.path.join(options.user_data_dir, options.manifest_filename)
-  local_manifest = LoadFromFile(manifest_path, manifest_util.SDKManifest())
-
-  any_bundles_need_update = False
-  InfoPrint('Bundles:')
-  InfoPrint(' I: installed\n *: update available\n')
-  for bundle in manifest.GetBundles():
-    local_bundle = local_manifest.GetBundle(bundle.name)
-    needs_update = local_bundle and local_manifest.BundleNeedsUpdate(bundle)
-    if needs_update:
-      any_bundles_need_update = True
-
-    PrintBundle(local_bundle, bundle, needs_update, list_options.revision)
-
-  if not any_bundles_need_update:
-    InfoPrint('\nAll installed bundles are up-to-date.')
-
-  local_only_bundles = set([b.name for b in local_manifest.GetBundles()])
-  local_only_bundles -= set([b.name for b in manifest.GetBundles()])
-  if local_only_bundles:
-    InfoPrint('\nBundles installed locally that are not available remotely:')
-    for bundle_name in local_only_bundles:
-      local_bundle = local_manifest.GetBundle(bundle_name)
-      PrintBundle(local_bundle, None, False, list_options.revision)
-
-
-def Update(options, argv, config):
-  '''Usage: %prog [global_options] update [options] [target]
-
-  Updates the Native Client SDK to a specified version.  By default, this
-  command updates all the recommended components.  The update process works
-  like this:
-    1. Fetch the manifest from the mirror.
-    2. Load manifest from USER_DATA_DIR - if there is no local manifest file,
-       make an empty manifest object.
-    3. Update each the bundle:
-      for bundle in bundles:
-        # Compare bundle versions & revisions.
-        # Test if local version.revision < mirror OR local doesn't exist.
-        if local_manifest < mirror_manifest:
-          update(bundle)
-          update local_manifest with mirror_manifest for bundle
-          write manifest to disk.  Use locks.
-        else:
-          InfoPrint('bundle is up-to-date')
-
-  Targets:
-    recommended: (default) Install/Update all recommended components
-    all:         Install/Update all available components
-    bundle_name: Install/Update only the given bundle
-  '''
-  DebugPrint("Running Update command with: %s, %s" % (options, argv))
-  ALL = 'all'  # Update all bundles
-  RECOMMENDED = 'recommended'  # Only update the bundles with recommended=yes
-
-  parser = optparse.OptionParser(usage=Update.__doc__)
+@usage('<bundle names...>')
+def CMDinfo(parser, args):
+  """display information about a bundle"""
+  options, args = parser.parse_args(args)
+  if len(args) == 0:
+    parser.error('No bundles given')
+    return 0
+  cfg = LoadConfig()
+  remote_manifest = LoadCombinedRemoteManifest(options.manifest_url, cfg)
+  commands.Info(remote_manifest, args)
+  return 0
+
+
+def CMDlist(parser, args):
+  """list all available bundles"""
+  parser.add_option('-r', '--revision', action='store_true',
+                    help='display revision numbers')
+  options, args = parser.parse_args(args)
+  if args:
+    parser.error('Unsupported argument(s): %s' % ', '.join(args))
+  local_manifest = LoadLocalManifest()
+  cfg = LoadConfig()
+  remote_manifest = LoadCombinedRemoteManifest(options.manifest_url, cfg)
+  commands.List(remote_manifest, local_manifest, options.revision)
+  return 0
+
+
+@usage('<bundle names...>')
+def CMDupdate(parser, args):
+  """update a bundle in the SDK to the latest version"""
   parser.add_option(
-      '-F', '--force', dest='force',
-      default=False, action='store_true',
+      '-F', '--force', action='store_true',
       help='Force updating existing components that already exist')
-  (update_options, args) = parser.parse_args(argv)
+  options, args = parser.parse_args(args)
+  local_manifest = LoadLocalManifest()
+  cfg = LoadConfig()
+  remote_manifest = LoadCombinedRemoteManifest(options.manifest_url, cfg)
 
-  if len(args) == 0:
-    args = [RECOMMENDED]
-
-  manifest = LoadManifestFromURLs([options.manifest_url] + config.GetSources())
-  bundles = manifest.GetBundles()
-  local_manifest_path = os.path.join(options.user_data_dir,
-                                     options.manifest_filename)
-  local_manifest = LoadFromFile(local_manifest_path,
-                                manifest_util.SDKManifest())
-
-  # Validate the arg list against the available bundle names.  Raises an
-  # error if any invalid bundle names or args are detected.
-  valid_args = set([ALL, RECOMMENDED] + [bundle.name for bundle in bundles])
-  bad_args = set(args) - valid_args
-  if len(bad_args) > 0:
-    raise Error("Unrecognized bundle name or argument: '%s'" %
-                ', '.join(bad_args))
-
-  if SDK_TOOLS in args and not options.update_sdk_tools:
-    # We only want sdk_tools to be updated by sdk_update.py. If the user
-    # tries to update directly, we just ignore the request.
-    InfoPrint('Updating sdk_tools happens automatically.\n'
-        'Ignoring manual update request.')
-    args.remove(SDK_TOOLS)
-
-  for bundle in bundles:
-    bundle_path = os.path.join(options.sdk_root_dir, bundle.name)
-    bundle_update_path = '%s_update' % bundle_path
-    if not (bundle.name in args or
-            ALL in args or (RECOMMENDED in args and
-                            bundle[RECOMMENDED] == 'yes')):
-      continue
-
-    if bundle.name == SDK_TOOLS and not options.update_sdk_tools:
-      continue
-
-    def UpdateBundle():
-      '''Helper to install a bundle'''
-      archive = bundle.GetHostOSArchive()
-      (_, _, path, _, _, _) = urlparse.urlparse(archive['url'])
-      dest_filename = os.path.join(options.user_data_dir, path.split('/')[-1])
-      sha1, size = DownloadArchiveToFile(archive, dest_filename)
-      if sha1 != archive.GetChecksum():
-        raise Error("SHA1 checksum mismatch on '%s'.  Expected %s but got %s" %
-                    (bundle.name, archive.GetChecksum(), sha1))
-      if size != archive.size:
-        raise Error("Size mismatch on Archive.  Expected %s but got %s bytes" %
-                    (archive.size, size))
-      InfoPrint('Updating bundle %s to version %s, revision %s' % (
-                (bundle.name, bundle.version, bundle.revision)))
-      ExtractInstaller(dest_filename, bundle_update_path)
-      if bundle.name != SDK_TOOLS:
-        repath = bundle.get('repath', None)
-        if repath:
-          bundle_move_path = os.path.join(bundle_update_path, repath)
-        else:
-          bundle_move_path = bundle_update_path
-        RenameDir(bundle_move_path, bundle_path)
-        if os.path.exists(bundle_update_path):
-          RemoveDir(bundle_update_path)
-      os.remove(dest_filename)
-      local_manifest.MergeBundle(bundle)
-      WriteToFile(local_manifest_path, local_manifest)
-    # Test revision numbers, update the bundle accordingly.
-    # TODO(dspringer): The local file should be refreshed from disk each
-    # iteration thought this loop so that multiple sdk_updates can run at the
-    # same time.
-    if local_manifest.BundleNeedsUpdate(bundle):
-      if (not update_options.force and os.path.exists(bundle_path) and
-          bundle.name != SDK_TOOLS):
-        WarningPrint('%s already exists, but has an update available.\n'
-                     'Run update with the --force option to overwrite the '
-                     'existing directory.\nWarning: This will overwrite any '
-                     'modifications you have made within this directory.'
-                     % bundle.name)
-      else:
-        UpdateBundle()
-    else:
-      InfoPrint('%s is already up-to-date.' % bundle.name)
-
-def Sources(options, argv, config):
-  '''Usage: %prog [global_options] sources [options] [--list,--add URL,--remove URL]
-
-  Manage additional package sources.  URL should point to a valid package
-  manifest file for download.
-  '''
-  DebugPrint("Running Sources command with: %s, %s" % (options, argv))
+  if not args:
+    args = [commands.RECOMMENDED]
 
-  parser = optparse.OptionParser(usage=Sources.__doc__)
-  parser.add_option(
-      '-a', '--add', dest='url_to_add',
-      default=None,
-      help='Add additional package source')
+  try:
+    delegate = commands.RealUpdateDelegate(USER_DATA_DIR, DEFAULT_SDK_ROOT)
+    commands.Update(delegate, remote_manifest, local_manifest, args,
+                    options.force)
+  finally:
+    # Always write out the local manifest, we may have successfully updated one
+    # or more bundles before failing.
+    WriteLocalManifest(local_manifest)
+  return 0
+
+
+def CMDinstall(parser, args):
+  """install a bundle in the SDK"""
+  # For now, forward to CMDupdate. We may want different behavior for this
+  # in the future, though...
+  return CMDupdate(parser, args)
+
+
+def CMDsources(parser, args):
+  """manage external package sources"""
+  parser.add_option('-a', '--add', dest='url_to_add',
+                    help='Add an additional package source')
   parser.add_option(
       '-r', '--remove', dest='url_to_remove',
-      default=None,
       help='Remove package source (use \'all\' for all additional sources)')
-  parser.add_option(
-      '-l', '--list', dest='do_list',
-      default=False, action='store_true',
-      help='List additional package sources')
-  source_options, _ = parser.parse_args(argv)
+  parser.add_option('-l', '--list', dest='do_list', action='store_true',
+                    help='List additional package sources')
+  options, args = parser.parse_args(args)
 
+  cfg = LoadConfig(True)
   write_config = False
-  if source_options.url_to_add:
-    config.AddSource(source_options.url_to_add)
+  if options.url_to_add:
+    commands.AddSource(cfg, options.url_to_add)
     write_config = True
-  elif source_options.url_to_remove:
-    if source_options.url_to_remove == 'all':
-      config.RemoveAllSources()
-    else:
-      config.RemoveSource(source_options.url_to_remove)
+  elif options.url_to_remove:
+    commands.RemoveSource(cfg, options.url_to_remove)
     write_config = True
-  elif source_options.do_list:
-    config.ListSources()
+  elif options.do_list:
+    commands.ListSources(cfg)
   else:
     parser.print_help()
 
   if write_config:
-    WriteToFile(os.path.join(options.user_data_dir, options.config_filename),
-                config)
+    WriteConfig(cfg)
 
-#------------------------------------------------------------------------------
-# Command-line interface
+  return 0
 
 
-def main(argv):
-  '''Main entry for the sdk_update utility'''
-  parser = optparse.OptionParser(usage=GLOBAL_HELP, add_help_option=False)
-  DEFAULT_SDK_ROOT = os.path.dirname(os.path.dirname(os.path.abspath(__file__)))
+def CMDversion(parser, args):
+  """display version information"""
+  _, _ = parser.parse_args(args)
+  print "Native Client SDK Updater, version r%s" % (REVISION,)

[Sam Clegg, 2012/10/22 22:55:30]

no need to pass tuple here.   Just "% REVISION"

[binji, 2012/10/23 00:14:09]

It's habit for me, I always make sure to pass a tuple to % for one element,
because if that element is a tuple, it will be expanded. But yes, not necessary
here.

+  return 0
 
-  # Manually add help options so we can ignore it when auto-updating.
-  parser.add_option(
-      '-h', '--help', dest='help', action='store_true',
-      help='show this help message and exit')
-  parser.add_option(
-      '-U', '--manifest-url', dest='manifest_url',
-      default='https://commondatastorage.googleapis.com/nativeclient-mirror/'
-              'nacl/nacl_sdk/%s' % MANIFEST_FILENAME,
-      help='override the default URL for the NaCl manifest file')
-  parser.add_option(
-      '-d', '--debug', dest='debug',
-      default=False, action='store_true',
-      help='enable displaying debug information to stderr')
-  parser.add_option(
-      '-q', '--quiet', dest='quiet',
-      default=False, action='store_true',
-      help='suppress displaying informational prints to stdout')
-  parser.add_option(
-      '-u', '--user-data-dir', dest='user_data_dir',
-      # TODO(mball): the default should probably be in something like
-      # ~/.naclsdk (linux), or ~/Library/Application Support/NaClSDK (mac),
-      # or %HOMEPATH%\Application Data\NaClSDK (i.e., %APPDATA% on windows)
-      default=os.path.join(DEFAULT_SDK_ROOT, USER_DATA_DIR),
-      help="specify location of NaCl SDK's data directory")
-  parser.add_option(
-      '-s', '--sdk-root-dir', dest='sdk_root_dir',
-      default=DEFAULT_SDK_ROOT,
-      help="location where the SDK bundles are installed")
-  parser.add_option(
-      '-v', '--version', dest='show_version',
-      action='store_true',
-      help='show version information and exit')
-  parser.add_option(
-      '-m', '--manifest', dest='manifest_filename',
-      default=MANIFEST_FILENAME,
-      help="name of local manifest file relative to user-data-dir")
-  parser.add_option(
-      '-c', '--config', dest='config_filename',
-      default=CONFIG_FILENAME,
-      help="name of the local config file relative to user-data-dir")
-  parser.add_option(
-      '--update-sdk-tools', dest='update_sdk_tools',
-      default=False, action='store_true')
-
-
-  COMMANDS = {
-      'info': Info,
-      'list': List,
-      'update': Update,
-      'install': Update,
-      'sources': Sources,
-      }
-
-  # Separate global options from command-specific options
-  global_argv = argv
-  command_argv = []
-  for index, arg in enumerate(argv):
-    if arg in COMMANDS:
-      global_argv = argv[:index]
-      command_argv = argv[index:]
-      break
-
-  (options, args) = parser.parse_args(global_argv)
-  args += command_argv
-
-  global _debug_mode, _quiet_mode
-  _debug_mode = options.debug
-  _quiet_mode = options.quiet
-
-  def PrintHelpAndExit(unused_options=None, unused_args=None):
-    parser.print_help()
-    exit(1)
 
-  if options.update_sdk_tools:
-    # Ignore all other commands, and just update the sdk tools.
-    args = ['update', 'sdk_tools']
-    # Leave the rest of the options alone -- they may be needed to update
-    # correctly.
-    options.show_version = False
-    options.sdk_root_dir = DEFAULT_SDK_ROOT
+def CMDhelp(parser, args):
+  """print list of commands or help for a specific command"""
+  _, args = parser.parse_args(args)
+  if len(args) == 1:
+    return main(args + ['--help'])
+  parser.print_help()
+  return 0
 
-  if options.show_version:
-    print "Native Client SDK Updater, version r%s" % (REVISION,)
-    exit(0)
 
+def Command(name):
+  return getattr(sys.modules[__name__], 'CMD' + name, None)

[Sam Clegg, 2012/10/22 22:55:30]

Hmm... I think globals() might be nicer here.  I really don't like using
sys.modules like this.. but I'm open to change.

[binji, 2012/10/23 00:14:09]

Haha, this is stolen from git_cl.py...
Done.

 
-  if not args:
-    print "Need to supply a command"
-    PrintHelpAndExit()
-
-  if options.help:
-    PrintHelpAndExit()
-
-  def DefaultHandler(unused_options=None, unused_args=None, unused_config=None):
-    print "Unknown Command: %s" % args[0]
-    PrintHelpAndExit()
-
-  def InvokeCommand(args):
-    command = COMMANDS.get(args[0], DefaultHandler)
-    # Load the config file before running commands
-    config = LoadFromFile(os.path.join(options.user_data_dir,
-                                       options.config_filename),
-                          SDKConfig())
-    command(options, args[1:], config)
-
-  if args[0] == 'help':
-    if len(args) == 1:
-      PrintHelpAndExit()
-    else:
-      InvokeCommand([args[1], '-h'])
+
+def GenUsage(parser, command):
+  """Modify an OptParse object with the function's documentation."""
+  obj = Command(command)
+  more = getattr(obj, 'usage_more', '')
+  if command == 'help':
+    command = '<command>'
   else:
-    # Make sure the user_data_dir exists.
-    if not os.path.exists(options.user_data_dir):
-      os.makedirs(options.user_data_dir)
-    InvokeCommand(args)
+    # OptParser.description prefer nicely non-formatted strings.
+    parser.description = re.sub('[\r\n ]{2,}', ' ', obj.__doc__)
+  parser.set_usage('usage: %%prog %s [options] %s' % (command, more))
+
+
+def UpdateSDKTools(options, args):
+  """update the sdk_tools bundle"""
+
+  local_manifest = LoadLocalManifest()
+  cfg = LoadConfig()
+  remote_manifest = LoadCombinedRemoteManifest(options.manifest_url, cfg)
+
+  try:
+    delegate = commands.RealUpdateDelegate(USER_DATA_DIR, DEFAULT_SDK_ROOT)
+    commands.UpdateBundleIfNeeded(delegate, remote_manifest, local_manifest,
+                                  commands.SDK_TOOLS, force=True)
+  finally:
+    # Always write out the local manifest, we may have successfully updated one
+    # or more bundles before failing.
+    WriteLocalManifest(local_manifest)
+  return 0
+
+
+def main(argv):
+  # Get all commands...
+  cmds = [fn[3:] for fn in dir(sys.modules[__name__]) if fn.startswith('CMD')]
+  # Remove hidden commands...
+  cmds = filter(lambda fn: not getattr(Command(fn), 'hide', 0), cmds)
+  # Format for CMDhelp usage.
+  CMDhelp.usage_more = ('\n\nCommands are:\n' + '\n'.join([
+      '  %-10s %s' % (fn, Command(fn).__doc__.split('\n')[0].strip())
+      for fn in cmds]))
+
+  # Create the option parse and add --verbose support.
+  parser = optparse.OptionParser()
+  parser.add_option(
+      '-v', '--verbose', action='count', default=0,
+      help='Use 2 times for more debugging info')
+  parser.add_option('-U', '--manifest-url', dest='manifest_url',
+      default=GSTORE_URL + '/nacl/nacl_sdk/' + MANIFEST_FILENAME,
+      metavar='URL', help='override the default URL for the NaCl manifest file')
+  parser.add_option('--update-sdk-tools', action='store_true',
+                    dest='update_sdk_tools', help=optparse.SUPPRESS_HELP)
+
+  old_parser_args = parser.parse_args
+  def Parse(args):
+    options, args = old_parser_args(args)
+    if options.verbose >= 2:
+      loglevel = logging.DEBUG
+    elif options.verbose:
+      loglevel = logging.INFO
+    else:
+      loglevel = logging.WARNING
+
+    fmt = '%(levelname)s:%(message)s'
+    logging.basicConfig(stream=sys.stdout, level=loglevel, format=fmt)
+
+    # If --update-sdk-tools is passed, circumvent any other command running.
+    if options.update_sdk_tools:
+      UpdateSDKTools(options, args)
+      sys.exit(1)
+
+    return options, args
+  parser.parse_args = Parse
+
+  if argv:
+    command = Command(argv[0])
+    if command:
+      # "fix" the usage and the description now that we know the subcommand.
+      GenUsage(parser, argv[0])
+      return command(parser, argv[1:])
 
-  return 0  # Success
+  # Not a known command. Default to help.
+  GenUsage(parser, 'help')
+  return CMDhelp(parser, argv)
 
 
 if __name__ == '__main__':
   try:
     sys.exit(main(sys.argv[1:]))
-  except Error as error:
-    print "Error: %s" % error
+  except Error as e:
+    logging.error(str(e))

[Sam Clegg, 2012/10/22 22:55:30]

Does this basically just print the error?  Or does it decorate in some way?

[binji, 2012/10/23 00:14:09]

it adds 'ERROR:', see fmt = '%(levelname)s:%(message)s' above.

     sys.exit(1)