Split RmTreeAndMkDir into two separate functions (refactoring).

tools/auto_bisect/bisect_perf_regression.py

 #!/usr/bin/env python
 # Copyright (c) 2013 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.
 
 """Performance Test Bisect Tool
 
 This script bisects a series of changelists using binary search. It starts at
 a bad revision where a performance metric has regressed, and asks for a last
 known-good revision. It will then binary search across this revision range by
@@ skipping 149 matching lines @@
 BUILD_RESULT_FAIL = 1
 BUILD_RESULT_SKIPPED = 2
 
 # Maximum time in seconds to wait after posting build request to the try server.
 # TODO: Change these values based on the actual time taken by buildbots on
 # the try server.
 MAX_MAC_BUILD_TIME = 14400
 MAX_WIN_BUILD_TIME = 14400
 MAX_LINUX_BUILD_TIME = 14400
 
-# The confidence percentage at which confidence can be consider "high".
+# The percentage at which confidence is considered high.
 HIGH_CONFIDENCE = 95
 
 # Patch template to add a new file, DEPS.sha under src folder.
 # This file contains SHA1 value of the DEPS changes made while bisecting
 # dependency repositories. This patch send along with DEPS patch to try server.
 # When a build requested is posted with a patch, bisect builders on try server,
 # once build is produced, it reads SHA value from this file and appends it
 # to build archive filename.
 DEPS_SHA_PATCH = """diff --git DEPS.sha DEPS.sha
 new file mode 100644
@@ skipping 1061 matching lines @@
     Returns:
       Path to backup or restored location as string. otherwise None if it fails.
     """
     build_dir = os.path.abspath(
         builder.GetBuildOutputDirectory(self.opts, self.src_cwd))
     source_dir = os.path.join(build_dir, build_type)
     destination_dir = os.path.join(build_dir, '%s.bak' % build_type)
     if restore:
       source_dir, destination_dir = destination_dir, source_dir
     if os.path.exists(source_dir):
-      RmTreeAndMkDir(destination_dir, skip_makedir=True)
+      RemoveDirectoryTree(destination_dir)
       shutil.move(source_dir, destination_dir)
       return destination_dir
     return None
 
   def GetBuildArchiveForRevision(self, revision, gs_bucket, target_arch,
                                  patch_sha, out_dir):
     """Checks and downloads build archive for a given revision.
 
     Checks for build archive with Git hash or SVN revision. If either of the
     file exists, then downloads the archive file.
@@ skipping 19 matching lines @@
         # Source archive file path on cloud storage using SVN revision.
         source_file = GetRemoteBuildPath(
             commit_position, self.opts.target_platform, target_arch, patch_sha)
         return FetchFromCloudStorage(gs_bucket, source_file, out_dir)
     return downloaded_archive
 
   def DownloadCurrentBuild(self, revision, depot, build_type='Release'):
     """Downloads the build archive for the given revision.
 
     Args:
-      revision: The Git revision to download or build.
-      build_type: Target build type ('Release', 'Debug', 'Release_x64' etc.)
-      patch: A DEPS patch (used while bisecting 3rd party repositories).
+      revision: The git revision to download or build.
+      depot: The name of a dependency repository. Should be in DEPOT_NAMES.
+      build_type: Target build type, e.g. Release', 'Debug', 'Release_x64' etc.
 
     Returns:
       True if download succeeds, otherwise False.
     """
     patch = None
     patch_sha = None
     if depot != 'chromium':
       # Create a DEPS patch with new revision for dependency repository.
       revision, patch = self.CreateDEPSPatch(depot, revision)
 
     if patch:
       # Get the SHA of the DEPS changes patch.
       patch_sha = GetSHA1HexDigest(patch)
 
       # Update the DEPS changes patch with a patch to create a new file named
       # 'DEPS.sha' and add patch_sha evaluated above to it.
       patch = '%s\n%s' % (patch, DEPS_SHA_PATCH % {'deps_sha': patch_sha})
 
-    # Get Build output directory
-    abs_build_dir = os.path.abspath(
-        builder.GetBuildOutputDirectory(self.opts, self.src_cwd))
+    # Get build output directory.
+    build_dir = builder.GetBuildOutputDirectory(self.opts, self.src_cwd)
+    abs_build_dir = os.path.abspath(build_dir)
 
     fetch_build_func = lambda: self.GetBuildArchiveForRevision(
       revision, self.opts.gs_bucket, self.opts.target_arch,
       patch_sha, abs_build_dir)
 
     # Downloaded archive file path, downloads build archive for given revision.
     downloaded_file = fetch_build_func()
 
     # When build archive doesn't exists, post a build request to tryserver
     # and wait for the build to be produced.
     if not downloaded_file:
       downloaded_file = self.PostBuildRequestAndWait(
           revision, fetch_build=fetch_build_func, patch=patch)
       if not downloaded_file:
         return False
 
     # Generic name for the archive, created when archive file is extracted.
     output_dir = os.path.join(
         abs_build_dir, GetZipFileName(target_arch=self.opts.target_arch))
+
     # Unzip build archive directory.
     try:
-      RmTreeAndMkDir(output_dir, skip_makedir=True)
+      RemoveDirectoryTree(output_dir)
       self.BackupOrRestoreOutputDirectory(restore=False)
       # Build output directory based on target(e.g. out/Release, out/Debug).
       target_build_output_dir = os.path.join(abs_build_dir, build_type)
       ExtractZip(downloaded_file, abs_build_dir)
       if not os.path.exists(output_dir):
         # Due to recipe changes, the builds extract folder contains
         # out/Release instead of full-build-<platform>/Release.
         if os.path.exists(os.path.join(abs_build_dir, 'out', build_type)):
           output_dir = os.path.join(abs_build_dir, 'out', build_type)
         else:
           raise IOError('Missing extracted folder %s ' % output_dir)
 
       print 'Moving build from %s to %s' % (
           output_dir, target_build_output_dir)
       shutil.move(output_dir, target_build_output_dir)
       return True
     except Exception as e:
       print 'Something went wrong while extracting archive file: %s' % e
       self.BackupOrRestoreOutputDirectory(restore=True)
       # Cleanup any leftovers from unzipping.
       if os.path.exists(output_dir):
-        RmTreeAndMkDir(output_dir, skip_makedir=True)
+        RemoveDirectoryTree(output_dir)
     finally:
       # Delete downloaded archive
       if os.path.exists(downloaded_file):
         os.remove(downloaded_file)
     return False
 
   def PostBuildRequestAndWait(self, git_revision, fetch_build, patch=None):
     """POSTs the build request job to the try server instance.
 
     A try job build request is posted to tryserver.chromium.perf master,
@@ skipping 1512 matching lines @@
     opts: The options parsed from the command line.
 
   Returns:
     True if the platform and build system are supported.
   """
   # Haven't tested the script out on any other platforms yet.
   supported = ['posix', 'nt']
   return os.name in supported
 
 
-def RmTreeAndMkDir(path_to_dir, skip_makedir=False):
-  """Removes the directory tree specified, and then creates an empty
-  directory in the same location (if not specified to skip).
+def RemakeDirectoryTree(path_to_dir):
+  """Removes a directory tree and replaces it with an empty one.
 
-  Args:
-    path_to_dir: Path to the directory tree.
-    skip_makedir: Whether to skip creating empty directory, default is False.
+  Returns True if successful, False otherwise.
+  """
+  if not RemoveDirectoryTree(path_to_dir):
+    return False
+  return MaybeMakeDirectory(path_to_dir)
 
-  Returns:
-    True if successful, False if an error occurred.
-  """
+
+def RemoveDirectoryTree(path_to_dir):
+  """Removes a directory tree. Returns True if successful or False otherwise."""
   try:
     if os.path.exists(path_to_dir):
       shutil.rmtree(path_to_dir)
   except OSError, e:
     if e.errno != errno.ENOENT:
       return False
-
-  if not skip_makedir:
-    return MaybeMakeDirectory(path_to_dir)
-
   return True
 
 
 def RemoveBuildFiles(build_type):
   """Removes build files from previous runs."""
-  if RmTreeAndMkDir(os.path.join('out', build_type)):
-    if RmTreeAndMkDir(os.path.join('build', build_type)):
+  if RemakeDirectoryTree(os.path.join('out', build_type)):
+    if RemakeDirectoryTree(os.path.join('build', build_type)):

[RobertoCN, 2014/10/14 18:31:34]

Wouldn't an 'and' look better here as opposed to the nested if blocks?

[qyearsley, 2014/10/14 18:36:26]

IMO, "and" would definitely be better here if the two conditions were a bit
shorter, but if the condition takes up two lines, it's not necessarily more
readable than the nested "if".

What about:

out_dir = os.path.join('out', build_type)
build_dir = os.path.join('build', build_type)
return RemakeDirectoryTree(out_dir) and RemakeDirectoryTree(build_dir)

       return True
   return False
 
 
 class BisectOptions(object):
   """Options to be used when running bisection."""
   def __init__(self):
     super(BisectOptions, self).__init__()
 
     self.target_platform = 'chromium'
@@ skipping 327 matching lines @@
       # bugs. If you change this, please update the perf dashboard as well.
       bisect_utils.OutputAnnotationStepStart('Results')
     print 'Error: %s' % e.message
     if opts.output_buildbot_annotations:
       bisect_utils.OutputAnnotationStepClosed()
   return 1
 
 
 if __name__ == '__main__':
   sys.exit(main())