Rewrited build script for instrumented libraries from bash to python

third_party/instrumented_libraries/download_build_install.py

+#!/usr/bin/python
+# Copyright 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.
+
+# Downloads, builds (with instrumentation) and installs shared libraries.
+
+import argparse
+import os
+import shutil
+import subprocess
+import sys
+
+# Should be a dict from 'sanitizer type' to 'compiler flag'.
+SUPPORTED_SANITIZERS = {"asan": "address"}

[Alexander Potapenko, 2013/11/15 10:32:19]

"Be consistent with your choice of string quote character within a file. Pick '
or " and stick with it."
(http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=String...)

Single quotes are more common in Chromium codebase.

[alextaran1, 2013/11/15 12:00:51]

Done.

+
+
+class ScopedChangeDirectory(object):
+  "Changes current working directory and restores it back automatically."

[Alexander Potapenko, 2013/11/15 10:32:19]

Doc strings should use """.

[alextaran1, 2013/11/15 12:00:51]

Done.

+  def __init__(self, path):
+    self.path = path
+    self.old_path = ""
+
+  def __enter__(self):
+    self.old_path = os.getcwd()
+    os.chdir(self.path)
+
+  def __exit__(self, exc_type, exc_value, traceback):
+    os.chdir(self.old_path)
+
+
+def get_script_absolute_path():
+  return os.path.dirname(os.path.abspath(__file__))
+
+
+def get_library_build_dependencies(library):
+  command = 'apt-get -s build-dep {0} | grep Inst | cut -d " " -f 2'.format(

[Alexander Potapenko, 2013/11/15 10:32:19]

It's more readable to split the string somewhere in the middle, e.g.:
  command = ('apt-get -s build-dep {0} | '
             'grep Inst | cut -d " " -f 2').format(library)

[Alexander Potapenko, 2013/11/15 10:32:19]

Also consider using the % operator instead of the format() method.

[alextaran1, 2013/11/15 12:00:51]

After replacing "format" to "%" wrapping isn't needed anymore :)

+      library)
+  command_result = subprocess.Popen(command, stdout = subprocess.PIPE,

[Alexander Potapenko, 2013/11/15 10:32:19]

"Don't use spaces around the '=' sign when used to indicate a keyword argument
or a default parameter value."

http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Whites...

[alextaran1, 2013/11/15 12:00:51]

Done.

+      shell = True)
+  build_dependencies = [l.strip() for l in command_result.stdout]
+  return build_dependencies
+
+
+def download_build_install(parsed_arguments):
+  sanitizer_flag = SUPPORTED_SANITIZERS[parsed_arguments.sanitizer_type]
+  
+  os.putenv("CFLAGS", "-fsanitize={0} -g -fPIC -w".format(sanitizer_flag))

[Alexander Potapenko, 2013/11/15 10:32:19]

You don't need to change the actual environment of the current process.
Build a dict and pass it to subprocess.* functions as the |env| parameter.

[alextaran1, 2013/11/15 12:00:51]

Done.

+  os.putenv("CXXFLAGS", "-fsanitize={0} -g -fPIC -w".format(sanitizer_flag))
+  # We use XORIGIN as RPATH and after building library replace it to $ORIGIN
+  # The reason: this flag goes through configure script and makefiles
+  # differently for different libraries. So the dollar sign "$" should be
+  # differently escaped. Instead of having problems with that it just
+  # uses XORIGIN to build library and after that replaces it to $ORIGIN
+  # directly in .so file.
+  os.putenv("LDFLAGS", "-Wl,-z,origin -Wl,-R,XORIGIN/.")
+
+  library_directory = "{0}/{1}".format(parsed_arguments.intermediate_directory,
+      parsed_arguments.library)
+  
+  install_prefix = "{0}/{1}/instrumented_libraries/{2}".format(
+      get_script_absolute_path(),
+      parsed_arguments.product_directory,
+      parsed_arguments.sanitizer_type)
+
+  if not os.path.exists(library_directory):
+    os.makedirs(library_directory)
+
+  with ScopedChangeDirectory(library_directory), open(os.devnull, "w") as dev_null:

[Alexander Potapenko, 2013/11/15 10:32:19]

Please wrap this line.

[alextaran1, 2013/11/15 12:00:51]

Done.

+    subprocess.call("apt-get source {0}".format(parsed_arguments.library),
+        stdout = dev_null, stderr = dev_null, shell = True)
+    # There should be only one subdirectory after downloading a package.
+    subdirectories = [d for d in os.listdir(".") if os.path.isdir(d)]
+    if len(subdirectories) != 1:
+      raise Exception("There was not one directory after downloading " \
+          "a package {0}".format(parsed_arguments.library))
+    with ScopedChangeDirectory(subdirectories[0]):

[Alexander Potapenko, 2013/11/15 10:32:19]

You're assuming len(subdirectories) > 0. If this should always be true, update
the condition above to 'len(...) > 1'. Otherwise make sure to check it.

[alextaran1, 2013/11/15 12:00:51]

Done.

+      # Now we are in the package directory.
+      configure_command = "./configure {0} --prefix={1}".format(
+          parsed_arguments.custom_configure_flags, install_prefix)
+      subprocess.call(configure_command, stdout = dev_null, stderr = dev_null,
+          shell = True)
+      subprocess.call("make -j{0}".format(parsed_arguments.jobs),
+          stdout = dev_null, stderr = dev_null, shell = True)
+      subprocess.call("make -j{0} install".format(parsed_arguments.jobs),
+          stdout = dev_null, stderr = dev_null, shell = True)
+  
+  # Touch a txt file to indicate library is installed.
+  open("{0}/{1}.txt".format(install_prefix, parsed_arguments.library), "w") \
+      .close()
+  # Remove downloaded package and generated temporary build files.
+  shutil.rmtree(library_directory)
+
+
+def main():
+  argument_parser = argparse.ArgumentParser(
+      description = "Download, build and install instrumented library")
+  
+  argument_parser.add_argument("-j", "--jobs", type = int, default = 1)
+  argument_parser.add_argument("-l", "--library", required = True)
+  argument_parser.add_argument("-i", "--product-directory", default = ".",
+      help = "Relative path to the directory with chrome binaries")
+  argument_parser.add_argument("-m", "--intermediate-directory", default = ".",
+      help = "Relative path to the directory for temporary build files")
+  argument_parser.add_argument("-c", "--custom-configure-flags", default = "")
+  argument_parser.add_argument("-s", "--sanitizer-type", required = True,
+      choices = SUPPORTED_SANITIZERS.keys())
+  
+  parsed_arguments = argument_parser.parse_args()
+  # Ensure current working directory is this script directory
+  os.chdir(get_script_absolute_path())
+  # Ensure all build dependencies are installed
+  build_dependencies = get_library_build_dependencies(parsed_arguments.library)
+  if len(build_dependencies) != 0:

[Alexander Potapenko, 2013/11/15 10:32:19]

That's just 'if len(build_dependencies)'.
Overall, prefer implicit boolean conversions to explicit ones.

[alextaran1, 2013/11/15 12:00:51]

Done.

+    print >> sys.stderr, "Please, install build-dependencies for {0}".format(
+        parsed_arguments.library)
+    print >> sys.stderr, "One-liner for APT:"
+    print >> sys.stderr, "sudo apt-get -y --no-remove build-dep {0}".format(
+        parsed_arguments.library)
+    sys.exit(1)
+
+  download_build_install(parsed_arguments)
+
+
+if __name__ == "__main__":
+  main()