Chrome updater test framework

install_test/chrome_installer.py

Index: install_test/chrome_installer.py
===================================================================
--- install_test/chrome_installer.py	(revision 0)
+++ install_test/chrome_installer.py	(revision 0)
@@ -0,0 +1,286 @@
+#!/usr/bin/python
+# Copyright (c) 2012 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.
+
+"""Provides an interface for installing Chrome."""
+
+import _winreg
+import ctypes
+import httplib
+import logging
+import os
+import platform
+import shutil
+import socket
+import subprocess
+import tempfile
+import urllib
+
+
+_CSIDL_COMMON_APPDATA = 0x1C

[kkania, 2012/07/23 16:46:23]

how about move these 2 to class Installation?

[nkang, 2012/07/25 23:39:21]

Moved both these variables to Installation or ChromeInstallation, as it is now
called.

+_CSIDL_PROGRAM_FILESX86 = 0x2A
+_PLATFORM = platform.system().lower()

[kkania, 2012/07/23 16:46:23]

I see there's a few checks in this file that check if we're on windows. I
thought these scripts were only for windows. If so, these checks seem a bit
unnecessary.

[nkang, 2012/07/25 23:39:21]

I added this check because I was told that ultimately we will support other
platforms, as well. You certainly don't want to import ctypes or windll when on
other platforms, as it will invoke an error. But since this script will only run
on Windows for the time being, I will go ahead and remove the check. We can
always add it later on, if need be. 

[kkania, 2012/07/25 23:59:38]

There's a few other places you need to update that assert or check this. Remove
this var when you're done.

[nkang, 2012/08/02 21:05:42]

Got rid of the _PLATFORM variable and removed all references to it. The script
will now run under the assumption that the platform its running on is Windows,
since all the platform checks have now been removed.

+
+# Import only on Windows. On other platforms it will invoke a ValueError. 
+if _PLATFORM == 'windows':
+  from ctypes import wintypes, windll
+
+
+class InstallationType:
+  """Defines the Chrome installation types."""
+  SYSTEM = 0
+  USER = 1
+
+
+def _RemoveDuplicates(args):
+  """Removes duplicates options from a list.
+
+  Args:
+    args: A list containing the options.
+
+  Returns:
+    A string without any duplicate options.
+  """
+  return ((lambda arg: arg if arg else '')
+          (' '.join(map(lambda opt: opt, list(frozenset(args.split(',')))))))
+
+
+def Install(installer_path, chrome_type, build, options='', clean=True):  
+  """Installs the specified Chrome build."""

[kkania, 2012/07/23 16:46:23]

Document the args and return

[nkang, 2012/07/25 23:39:21]

Done.

+  def DoPreliminaryChecks(chrome_install):    
+    """Validates the test parameters and Chrome version."""
+    assert(_PLATFORM == 'windows' and os.path.isfile(installer_path))
+    # Chrome installation type specified by the user.
+    install_type = ('system-level' in options and InstallationType.SYSTEM
+                    or InstallationType.USER)     
+    cur_build = chrome_install.GetChromeVersion()
+    # Chrome already installed, make sure new build can be installed over it.
+    if cur_build:            
+      current_type = chrome_install.GetChromeInstallationType()      
+      if(current_type == InstallationType.SYSTEM and install_type ==
+         InstallationType.USER):
+        raise RuntimeError('System level Chrome exists, aborting user level '
+                           'installation.')
+      # Installing a build that's older than the currently installed build.
+      elif current_type == install_type:                
+        if cur_build >= build:
+          raise RuntimeError('Please specify a newer version of Chrome.')
+        
+  chrome_install = Installation(chrome_type)
+  DoPreliminaryChecks(chrome_install)
+  options += ' --install --do-not-launch-chrome'
+  options = _RemoveDuplicates(options)

[kkania, 2012/07/23 16:46:23]

what's an example of options that might want to be passed in?    Why would there
be duplicates? Do duplicates actually cause a problem?

[nkang, 2012/07/25 23:39:21]

Example of an option that needs to be passed in is --system-level, which
specifies a system-level installation. Duplicates can occur either due to user
error, or if the user unknowingly passes an option that's already included in
the "UninstallArguments" string in the registry. The latter, as the name
implies, only applies to uninstall. The uninstall arguments string is obtained
from the registry and contains command line args to be passed to the installer.
When uninstalling a chrome build, we get the uninstall arguments from the
registry, per Anantha's request, and combine it with any user specified command
arguments; this is the scenario that's most likely to produce duplicate options.
As for whether or not duplicates cause any problems, I am not entirely sure. I
am inclined to believe that they do not cause any problems and are simply
ignored, but I cannot say conclusively whether they are or are not. So
eliminating duplicates is more of a sanity check for our own peace of mind. I
think we should just leave it; it certainly can't hurt. But if you feel strongly
that we should remove it, I am willing to reconsider. Let me know.

[kkania, 2012/07/25 23:59:38]

Try running the installer/uninstaller manually and see if duplicate args cause
problems. If the installer uses the same command line parser as chrome (which it
probably does), this shouldn't be an issue. I'd go ahead and remove the
duplicate check.

[nkang, 2012/08/02 21:05:42]

Checked with Prudhvi regarding duplicate options, since he does a lot of
installer testing. According to him, it makes no difference if there are
duplicates in the command line options. All command line options passed to the
installer are saved in the registry under the 'UninstallArguments' key. If those
options contain duplicates, they are filtered before the options are entered
into the registry. In other words, no duplicates are saved in the registry. So
it seems that the duplicate removal task that chrome_installer was doing, is
already being done by Chrome. Went ahead and removed the _RemoveDuplicates
method and any references to it.

+  logging.log(logging.INFO, 'Launching Chrome installer...')
+  cmd = '%s %s' % (installer_path, options)
+  ret = subprocess.Popen(cmd, shell=True).wait()   
+  if ret == 0:
+    logging.log(logging.INFO, 'Installation complete.')
+    return chrome_install
+  logging.log(logging.ERROR, 'Installation failed.')

[kkania, 2012/07/23 16:46:23]

How about throw an error here instead, which includes the exit code

[nkang, 2012/07/25 23:39:21]

Got rid of the logging.log message and the return value. If installation fails,
a RuntimeError exception is raised, which gets propagated to InstallTest.
InstallTest, which previously raised this exception, would not have to do so
anymore.

+  return None
+
+  
+class Installation(object):
+  """Provides pertinent information about the installed Chrome version.
+
+  The type of Chrome version must be passed as an argument to the constructor, 
+  (i.e. - user or system level).
+  """
+
+  HKEY_LOCAL = (r'SOFTWARE\Wow6432Node\Google\Update\ClientState'

[kkania, 2012/07/23 16:46:23]

make these private?

[nkang, 2012/07/25 23:39:21]

Made both of them private. I had initially held off on making them private
because I wanted the names to adhere to Windows' naming convention. Therefore,
HKEY_USER fit the bill better than _HKEY_USER. But I went ahead and changed the
names, so they're now private.

+                '\{8A69D345-D564-463C-AFF1-A69D9E530F96}')
+  HKEY_USER  = HKEY_LOCAL.replace('\\Wow6432Node', '')                
+  
+  def __init__(self, install_type):
+    assert(install_type == InstallationType.SYSTEM or
+           install_type == InstallationType.USER)
+    self._type = install_type 
+    self._key_type = self._GetKeyType(self._type)
+
+  def _OpenKey(self, key_type, key_name, key_access=_winreg.KEY_READ):
+    """Opens a registry key and returns the key handle.
+
+    Args:
+      key_type: The type of key HKLM or HKCU.
+      key_name: Name of the key.
+      key_access: Type of access required. Be default its read only.
+
+    Returns:
+      Key handle if successful, otherwise None.
+    """
+    try:
+      key = _winreg.OpenKey(key_type, key_name, 0, key_access)
+      return key
+    except _winreg.error:
+      return None

[kkania, 2012/07/23 16:46:23]

when could this occur? Should we throw an exception instead?

[nkang, 2012/07/25 23:39:21]

This could occur for a number of reasons. It could occur if the key_name is
wrong (i.e., doesn't exist or misspelled), if the key_type is wrong (user
instead of system or vice versa), or if the caller doesn't have sufficient
privileges. I think we should leave this method as is. I think we should return
None instead of raising. Returning a handle if successful and None or zero on
failure is behavior that is more inline with the Windows APIs used for
manipulating the registry.

[kkania, 2012/07/25 23:59:38]

I don't think it's too important that we try to follow the windows API. I think
trying to access a nonexistent or key that you don't have privileges for should
count as an error, and it would better to just have the exception be raised from
_winreg. I would just remove this function and use _winreg directly. The only
thing it really does for us is add a 0 as the third parameter.

[nkang, 2012/08/02 21:05:42]

The reason I wrote this method was because I wanted to handle the
_winreg.error/OSError exception that occurs when the key cannot be opened for
whatever reason. But I didn't want every method that called _winreg.OpenKey to
have to have its own try/except block, which would've been redundant. Plus,
there's one method called 'GetType', which determines the Chrome installation
type, that absolutely needs to handle this exception. 'GetType' first tries to
check the user level installation by opening the Chrome key in HKCU. If that
fails, it catches the exception, and in the except block, it tries to open the
HKLM key. If that raises an exception as well, it handles the exception and
return None as the installation type, which indicates that no Chrome versions
are present on this machine. That is why I wrote this method, but if you want
_winreg to just raise the exception then there's really no need for this method.
So I am going to remove ChromeInstallation._OpenKey. Wherever it is called,
we'll now use _winreg.OpenKey instead. As a result, I will also have to
drastically change the 'GetType' method.

+
+  def _GetWinLocalFolder(self, ftype=_CSIDL_COMMON_APPDATA):
+    """Returns full path of the 'Local' folder on Windows.
+
+    Args:
+      ftype: Location to look up, which could vary based on installation type.
+
+    Returns:
+      A String representing the folder path if successful, otherwise an empty
+      string.
+    """
+    if _PLATFORM != 'windows':      
+      return ''    
+    SHGetFolderPathW = windll.shell32.SHGetFolderPathW
+    SHGetFolderPathW.argtypes = [wintypes.HWND,
+                                 ctypes.c_int,
+                                 wintypes.HANDLE,
+                                 wintypes.DWORD,
+                                 wintypes.LPCWSTR]
+    path_buf = wintypes.create_unicode_buffer(wintypes.MAX_PATH)
+    result = SHGetFolderPathW(0, ftype, 0, 0, path_buf)    
+    return str(path_buf.value)
+
+  def _GetKeyType(self, install_type):
+    """Determines the registry key to use based on installation type."""
+    if install_type == InstallationType.SYSTEM:      
+      return _winreg.HKEY_LOCAL_MACHINE
+    elif install_type == InstallationType.USER:      
+      return _winreg.HKEY_CURRENT_USER    
+  
+  def _GetKeyName(self, install_type, replace=True):
+    """Returns Chrome's registry key name based on installation type."""

[kkania, 2012/07/23 16:46:23]

The replace arg here is a bit strange. Can you find a cleaner way to do this?

[nkang, 2012/07/25 23:39:21]

Changed the replace arg with a 'value' arg. If the value specified is 'pv',
which stands for Product Version and represents the current Chrome version, then
ClientState in key name is replaced with Clients (since 'pv' is in
HKEY_CURRENT_USER\Software\Google\Update\Clients\{8A69D345-D564-463c-AFF1-A69D9E530F96}).
If not, which would be the case for 'UninstallArguments' or 'UninstallString',
it returns the key name as is. The key name itself is determined by the type of
installation. If user, it uses self._HKEY_USER, and for system level
installation, it uses self._HKEY_LOCAL.

[kkania, 2012/07/25 23:59:38]

I see. I think this function is still confusing. See if you can rework it
somehow to make it more understandable. Also, why do you have to pass in the
install type? Can we just use self._type?

[nkang, 2012/08/02 21:05:42]

Got rid of this method altogether. Per our conversation, created a new class
called ChromeRegistryKeys that will be used for accessing and manipulating
Chrome related registry keys. All registry related methods from
ChromeInstallation were moved to this class. In addition, several new methods
were created that, among other things, generate the appropriate registry key
name based on the installation type and the value the user wishes to look up. To
identify Chrome's various different registry values - such as 'pv',
'UninstallArguments', 'UninstallString', etc. - a new enumerator class was
created. The enumerator class contains only three members, each uses a numerical
code to identify the aforementioned Chrome registry values.

+    name = (install_type == InstallationType.SYSTEM and self.HKEY_LOCAL or
+            self.HKEY_USER)
+    return (replace and name.replace('ClientState', 'Clients') or name)
+
+  def _DeleteChromeRegEntries(self):
+    """Deletes chrome registry settings."""    
+    if self._type == InstallationType.USER:
+      p_key = self.HKEY_USER[: self.HKEY_USER.rfind('\\')]      
+      key_name = self.HKEY_USER[self.HKEY_USER.rfind('\\') + 1 :]
+      _type = _winreg.HKEY_CURRENT_USER    
+    elif self._type == InstallationType.SYSTEM:
+      p_key = self.HKEY_LOCAL[: self.HKEY_LOCAL.rfind('\\')]
+      key_name = self.HKEY_LOCAL[self.HKEY_LOCAL.rfind('\\') + 1 :]
+      _type = _winreg.HKEY_LOCAL_MACHINE    
+    try:
+      key = self._OpenKey(_type, p_key, _winreg.KEY_ALL_ACCESS)      
+      _winreg.DeleteKey(key, key_name)
+      key.Close()
+      return 0      
+    except _winreg.error, err:
+      logging.log(logging.ERROR, 'Could not delete registry entries: %s' % err)      
+      return -1
+
+  def _GetValueFromRegistry(self, reg_type, key, subkey):
+    """Gets value of the specified subkey from the registry.
+
+    Args:
+      reg_type: Type of key to use HKLM or HKCU.
+      key: Name of the key.
+      subkey: Name of the subkey whose value will be returned.
+
+    Returns:
+      A string representing the subkey value if successful, otherwise None.      
+    """    
+    hkey = self._OpenKey(reg_type, key)
+    if hkey:
+      try:
+        value = str(_winreg.QueryValueEx(hkey, subkey)[0])        
+      except _winreg.error:
+        value = None
+      _winreg.CloseKey(hkey)
+      return value
+    return None
+
+  def _GetUninstallString(self):
+    """Returns Chrome uninstall string from the registry."""
+    key_name = self._GetKeyName(self._type, False)    
+    return self._GetValueFromRegistry(self._key_type, key_name,
+                                      'UninstallString')
+
+  def _GetUninstallArguments(self):
+    """Returns Chrome uninstall arguments from the registry."""
+    key_name = self._GetKeyName(self._type, False)
+    return self._GetValueFromRegistry(self._key_type, key_name,
+                                      'UninstallArguments')
+
+  def GetChromeVersion(self):
+    """Returns the installed version of Chrome."""
+    key_name = self._GetKeyName(self._type, True)
+    return self._GetValueFromRegistry(self._key_type, key_name, 'pv')
+
+  def _LaunchInstaller(self, installer_path, options):
+    """Launches the Chrome installer.
+
+    Args:
+      installer_path: Path where the installer is located.
+      options: Any additional options to be used for installation.
+    """
+    cmd = '%s %s' % (installer_path, options)
+    try:
+      subprocess.Popen(cmd, shell=True).wait()
+    except OSError, err:
+      raise err
+
+  def _Delete(self, _path):

[kkania, 2012/07/23 16:46:23]

i don't think this is used

[nkang, 2012/07/25 23:39:21]

Yes, you are right. This method is currently not being used. The reason for that
is, we had decided that for the time being we're not going to delete the Chrome
build folders we create in the temp directory. But the plan was that down the
line we will start deleting them, which is why I wrote this method. But since
we're not using it at the moment, I went ahead and excluded it from the code. I
moved it to a utility module I wrote that contains similar methods. This is fine
for the time being, but ultimately I think it would be a good idea to move the
'_Delete' method to pyauto_utils.py. That way we can import and use it as
needed.

+    """Deletes a file or folder."""
+    try:
+      (lambda p: shutil.rmtree(p) if os.path.isdir(p) else os.remove(p))(_path)
+      return 0
+    except(OSError, IOError, TypeError), err:      
+      return -1
+    
+  def GetChromeExePath(self):
+    """Returns Chrome binary location based on installation type."""
+    chrome_path = ''
+    if self._type == InstallationType.USER:
+      folder_id = _CSIDL_COMMON_APPDATA
+    elif self._type == InstallationType.SYSTEM:
+      folder_id = _CSIDL_PROGRAM_FILESX86
+    if _PLATFORM == 'windows':
+      chrome_path = os.path.join(self._GetWinLocalFolder(folder_id), 'Google',
+                                 'Chrome', 'Application', 'chrome.exe')
+    return chrome_path
+
+  def GetChromeInstallationType(self):

[kkania, 2012/07/23 16:46:23]

a lot of functions in this class have Chrome in the name, which I think is
redundant. I would just drop the Chrome part of the name. If you want, you can
rename the class name to ChromeInstallation.  Also, I would drop the
Installation part from this name too, since that is implied by the class name. 
So this should just be GetType

[nkang, 2012/07/25 23:39:21]

Changed the class name from Installation to ChromeInstallation. The following
methods were renamed from _DeleteChromeRegSettings, GetChromeVersion,
GetChromeExePath, GetChromeInstallationType, and UninstallChrome to
_DeleteRegistrySettings, GetVersion, GetPath, GetType, and Uninstall,
respectively. Updated chrome_installer.py and install_test.py to reflect this
change.

+    """Determines Chrome installation type."""
+    reg_type = _winreg.HKEY_CURRENT_USER
+    key_name = self.HKEY_USER.replace('ClientState', 'Clients')
+    key = self._OpenKey(reg_type, key_name)    
+    if not key:      
+      reg_type = _winreg.HKEY_LOCAL_MACHINE
+      key_name = self.HKEY_LOCAL.replace('ClientState', 'Clients')
+      key = self._OpenKey(reg_type, key_name)
+      if not key:        
+        return None    
+    version = self._GetValueFromRegistry(reg_type, key_name, 'pv')                                  
+    key.Close()    
+    if version:      
+      return (reg_type == _winreg.HKEY_CURRENT_USER and
+              InstallationType.USER or InstallationType.SYSTEM)
+    return None
+  
+  def UninstallChrome(self):
+    """Uninstalls Chrome."""
+    if not self.GetChromeVersion():
+      raise RuntimeError('No Chrome version found on this system.')
+    chrome_path = self.GetChromeExePath()
+    install_type = self.GetChromeInstallationType()
+    reg_opts = self._GetUninstallArguments()
+    uninstall_str = self._GetUninstallString()
+    options = '%s --force-uninstall' % (reg_opts)
+    if self._type == InstallationType.SYSTEM:
+      options += ' --system-level'    
+    if not os.path.exists(chrome_path):
+      logging.log(logging.ERROR, 'Could not find chrome, aborting uninstall.')

[nkang, 2012/07/25 23:39:21]

Since we're now raising an exception instead of returning False below, I also
went ahead and changed this return value. This method no longer returns True or
False; it simply raises on error or not if everything goes smoothly. Updated
InstallTest accordingly.

+      return False
+    logging.log(logging.INFO, 'Launching Chrome installer...')
+    ret = self._LaunchInstaller(uninstall_str, options)    

[kkania, 2012/07/23 16:46:23]

this function doesn't look like it returns anything

[nkang, 2012/07/25 23:39:21]

LaunchInstaller did at one point return a value, but it was later changed. So it
now raises instead of returning, but I guess where this method was being called
was left unchanged. Went ahead and removed the 'ret ='.

+    if not os.path.exists(chrome_path):
+      logging.log(logging.INFO, 'Chrome was uninstalled successfully...')      
+      # TODO: add clean option like ChromeInstaller

[kkania, 2012/07/23 16:46:23]

remove this TODO

[nkang, 2012/07/25 23:39:21]

TODO: Remove the comment. Done!

+      logging.log(logging.INFO, 'Deleting registry entries...')        
+      self._DeleteChromeRegEntries()
+      logging.log(logging.INFO, 'Uninstall complete.')      
+      return True
+    else:
+      logging.log(logging.ERROR, 'Uninstall failed.')      

[kkania, 2012/07/23 16:46:23]

how about throw error instead

[nkang, 2012/07/25 23:39:21]

This method now raises a RuntimeError exception. Updated InstallTest
accordingly.

+    return False
Property changes on: install_test\chrome_installer.py
___________________________________________________________________
Added: svn:eol-style
   + LF