Bug 1258539 - [mozharness] Refactor name and arguments of download and unpack methods. r=jlund
☠☠ backed out by ff43d3e4c4fc ☠ ☠
authorHenrik Skupin <mail@hskupin.info>
Tue, 19 Jul 2016 23:00:46 +0200
changeset 307434 8322ffecd9d90d31ff451cd9763fe2cfa5ae09b4
parent 307433 cc2996a53b7107a36433b265318340469cc5eb6f
child 307435 c80456e5e3dd72a65de2f7acaf5be73f3df80144
push id30509
push usercbook@mozilla.com
push dateSun, 31 Jul 2016 15:40:21 +0000
treeherdermozilla-central@221b74c48363 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjlund
bugs1258539
milestone50.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1258539 - [mozharness] Refactor name and arguments of download and unpack methods. r=jlund Given that we have a universal unpack method now do not keep 'unzip' in method names. Also adapt arguments to be better understandable. MozReview-Commit-ID: ClDB5mSVcI2
testing/mozharness/mozharness/base/script.py
testing/mozharness/mozharness/mozilla/testing/firefox_media_tests.py
testing/mozharness/mozharness/mozilla/testing/firefox_ui_tests.py
testing/mozharness/mozharness/mozilla/testing/talos.py
testing/mozharness/mozharness/mozilla/testing/testbase.py
testing/mozharness/mozharness/mozilla/testing/unittest.py
testing/mozharness/scripts/b2g_emulator_unittest.py
testing/mozharness/scripts/desktop_unittest.py
testing/mozharness/scripts/web_platform_tests.py
--- a/testing/mozharness/mozharness/base/script.py
+++ b/testing/mozharness/mozharness/base/script.py
@@ -453,35 +453,38 @@ class ScriptMixin(PlatformMixin):
             kwargs = {"url": url, "file_name": file_name}
 
         return self.retry(
             download_func,
             kwargs=kwargs,
             **retry_args
         )
 
-    def download_unzip(self, url, parent_dir, target_unzip_dirs=None):
-        """Generic method to download and extract a zip file.
+    def download_unpack(self, url, extract_to, extract_dirs=None,
+                        error_level=FATAL):
+        """Generic method to download and extract a compressed file.
 
         The downloaded file will always be saved to the working directory and is not getting
         deleted after extracting.
 
         Args:
             url (str): URL where the file to be downloaded is located.
-            parent_dir (str): directory where the downloaded file will
+            extract_to (str): directory where the downloaded file will
                               be extracted to.
-            target_unzip_dirs (list, optional): directories inside the zip file to extract.
-                                                Defaults to `None`.
+            extract_dirs (list, optional): directories inside the archive to extract.
+                                           Defaults to `None`.
+            error_level (str, optional): log level to use in case an error occurs.
+                                         Defaults to `FATAL`.
 
         """
         dirs = self.query_abs_dirs()
-        zipfile = self.download_file(url, parent_dir=dirs['abs_work_dir'],
-                                     error_level=FATAL)
-
-        self.unpack(zipfile, parent_dir, target_unzip_dirs)
+        archive = self.download_file(url, parent_dir=dirs['abs_work_dir'],
+                                     error_level=error_level)
+        self.unpack(archive, extract_to, extract_dirs=extract_dirs,
+                    error_level=error_level)
 
     def load_json_url(self, url, error_level=None, *args, **kwargs):
         """ Returns a json object from a url (it retries). """
         contents = self._retry_download(
             url=url, error_level=error_level, *args, **kwargs
         )
         return json.loads(contents.read())
 
@@ -1384,28 +1387,24 @@ class ScriptMixin(PlatformMixin):
             try:
                 open(file_name, 'w').close()
             except IOError as e:
                 msg = "I/O error(%s): %s" % (e.errno, e.strerror)
                 self.log(msg, error_level=error_level)
         os.utime(file_name, times)
 
     def unpack(self, filename, extract_to, extract_dirs=None,
-               error_level=ERROR, halt_on_failure=True, fatal_exit_code=2,
-               verbose=False):
-        """
-        This method allows us to extract a file regardless of its extension
+               error_level=ERROR, fatal_exit_code=2, verbose=False):
+        """The method allows to extract a file regardless of its extension.
 
         Args:
             filename (str): filename of the compressed file.
             extract_to (str): where to extract the compressed file.
             extract_dirs (list, optional): directories inside the archive file to extract.
                                            Defaults to `None`.
-            halt_on_failure (bool, optional): whether or not to redefine the
-                                              log level as `FATAL` on errors. Defaults to True.
             fatal_exit_code (int, optional): call `self.fatal` if the return value
               of the command is not in `success_codes`. Defaults to 2.
             verbose (bool, optional): whether or not extracted content should be displayed.
                                       Defaults to False.
 
         Raises:
             IOError: on `filename` file not found.
 
@@ -1414,18 +1413,16 @@ class ScriptMixin(PlatformMixin):
             """Filter entries of the archive based on the specified list of to extract dirs."""
             filter_partial = functools.partial(fnmatch.filter, namelist)
             for entry in itertools.chain(*map(filter_partial, extract_dirs or ['*'])):
                 yield entry
 
         if not os.path.isfile(filename):
             raise IOError('Could not find file to extract: %s' % filename)
 
-        level = FATAL if halt_on_failure else error_level
-
         if zipfile.is_zipfile(filename):
             try:
                 self.info('Using ZipFile to extract {} to {}'.format(filename, extract_to))
                 with zipfile.ZipFile(filename) as bundle:
                     for entry in _filter_entries(bundle.namelist()):
                         if verbose:
                             self.info(' %s' % entry)
                         bundle.extract(entry, path=extract_to)
@@ -1435,33 +1432,33 @@ class ScriptMixin(PlatformMixin):
                         fname = os.path.realpath(os.path.join(extract_to, entry))
                         mode = bundle.getinfo(entry).external_attr >> 16 & 0x1FF
                         # Only set permissions if attributes are available. Otherwise all
                         # permissions will be removed eg. on Windows.
                         if mode:
                             os.chmod(fname, mode)
             except zipfile.BadZipfile as e:
                 self.log('%s (%s)' % (e.message, filename),
-                         level=level, exit_code=fatal_exit_code)
+                         level=error_level, exit_code=fatal_exit_code)
 
         # Bug 1211882 - is_tarfile cannot be trusted for dmg files
         elif tarfile.is_tarfile(filename) and not filename.lower().endswith('.dmg'):
             try:
                 self.info('Using TarFile to extract {} to {}'.format(filename, extract_to))
                 with tarfile.open(filename) as bundle:
                     for entry in _filter_entries(bundle.getnames()):
                         if verbose:
                             self.info(' %s' % entry)
                         bundle.extract(entry, path=extract_to)
             except tarfile.TarError as e:
                 self.log('%s (%s)' % (e.message, filename),
-                         level=level, exit_code=fatal_exit_code)
+                         level=error_level, exit_code=fatal_exit_code)
         else:
             self.log('No extraction method found for: %s' % filename,
-                     level=level, exit_code=fatal_exit_code)
+                     level=error_level, exit_code=fatal_exit_code)
 
 
 def PreScriptRun(func):
     """Decorator for methods that will be called before script execution.
 
     Each method on a BaseScript having this decorator will be called at the
     beginning of BaseScript.run().
 
--- a/testing/mozharness/mozharness/mozilla/testing/firefox_media_tests.py
+++ b/testing/mozharness/mozharness/mozilla/testing/firefox_media_tests.py
@@ -158,25 +158,24 @@ class FirefoxMediaTestsBase(TestingMixin
 
     def download_and_extract(self):
         """Overriding method from TestingMixin for more specific behavior.
 
         We use the test_packages_url command line argument to check where to get the
         harness, puppeteer, and tests from and how to set them up.
 
         """
-        target_unzip_dirs = ['config/*',
-                             'external-media-tests/*',
-                             'marionette/*',
-                             'mozbase/*',
-                             'puppeteer/*',
-                             'tools/wptserve/*',
-                             ]
-        super(FirefoxMediaTestsBase, self).download_and_extract(
-                target_unzip_dirs=target_unzip_dirs)
+        extract_dirs = ['config/*',
+                        'external-media-tests/*',
+                        'marionette/*',
+                        'mozbase/*',
+                        'puppeteer/*',
+                        'tools/wptserve/*',
+                        ]
+        super(FirefoxMediaTestsBase, self).download_and_extract(extract_dirs=extract_dirs)
 
     def query_abs_dirs(self):
         if self.abs_dirs:
             return self.abs_dirs
         abs_dirs = super(FirefoxMediaTestsBase, self).query_abs_dirs()
         dirs = {
             'abs_test_install_dir' : os.path.join(abs_dirs['abs_work_dir'],
                                                     'tests')
--- a/testing/mozharness/mozharness/mozilla/testing/firefox_ui_tests.py
+++ b/testing/mozharness/mozharness/mozilla/testing/firefox_ui_tests.py
@@ -148,24 +148,24 @@ class FirefoxUITests(TestingMixin, VCSTo
 
     def download_and_extract(self):
         """Overriding method from TestingMixin for more specific behavior.
 
         We use the test_packages_url command line argument to check where to get the
         harness, puppeteer, and tests from and how to set them up.
 
         """
-        target_unzip_dirs = ['config/*',
-                             'firefox-ui/*',
-                             'marionette/*',
-                             'mozbase/*',
-                             'puppeteer/*',
-                             'tools/wptserve/*',
-                             ]
-        super(FirefoxUITests, self).download_and_extract(target_unzip_dirs=target_unzip_dirs)
+        extract_dirs = ['config/*',
+                        'firefox-ui/*',
+                        'marionette/*',
+                        'mozbase/*',
+                        'puppeteer/*',
+                        'tools/wptserve/*',
+                        ]
+        super(FirefoxUITests, self).download_and_extract(extract_dirs=extract_dirs)
 
     def query_abs_dirs(self):
         if self.abs_dirs:
             return self.abs_dirs
 
         abs_dirs = super(FirefoxUITests, self).query_abs_dirs()
         abs_tests_install_dir = os.path.join(abs_dirs['abs_work_dir'], 'tests')
 
--- a/testing/mozharness/mozharness/mozilla/testing/talos.py
+++ b/testing/mozharness/mozharness/mozilla/testing/talos.py
@@ -282,23 +282,23 @@ class Talos(TestingMixin, MercurialScrip
         if c.get('run_local'):
             self.talos_path = os.path.dirname(self.talos_json)
 
         src_talos_webdir = os.path.join(self.talos_path, 'talos')
 
         if self.query_pagesets_url():
             self.info("Downloading pageset...")
             src_talos_pageset = os.path.join(src_talos_webdir, 'tests')
-            self.download_unzip(self.pagesets_url, src_talos_pageset)
+            self.download_unpack(self.pagesets_url, src_talos_pageset)
 
     # Action methods. {{{1
     # clobber defined in BaseScript
     # read_buildbot_config defined in BuildbotMixin
 
-    def download_and_extract(self, target_unzip_dirs=None, suite_categories=None):
+    def download_and_extract(self, extract_dirs=None, suite_categories=None):
         return super(Talos, self).download_and_extract(
             suite_categories=['common', 'talos']
         )
 
     def create_virtualenv(self, **kwargs):
         """VirtualenvMixin.create_virtualenv() assuemes we're using
         self.config['virtualenv_modules']. Since we are installing
         talos from its source, we have to wrap that method here."""
--- a/testing/mozharness/mozharness/mozilla/testing/testbase.py
+++ b/testing/mozharness/mozharness/mozilla/testing/testbase.py
@@ -413,17 +413,17 @@ 2. running via buildbot and running the 
                 self.fatal("There was an error reading test package requirements from %s "
                            "requirements: `%s` - error: `%s`" % (source,
                                                                  package_requirements or 'None',
                                                                  err or 'No error'))
         self.info("Using the following test package requirements:\n%s" %
                   pprint.pformat(package_requirements))
         return package_requirements
 
-    def _download_test_packages(self, suite_categories, target_unzip_dirs):
+    def _download_test_packages(self, suite_categories, extract_dirs):
         # Some platforms define more suite categories/names than others.
         # This is a difference in the convention of the configs more than
         # to how these tests are run, so we pave over these differences here.
         aliases = {
             'robocop': 'mochitest',
             'mochitest-chrome': 'mochitest',
             'mochitest-media': 'mochitest',
             'mochitest-plain-clipboard': 'mochitest',
@@ -449,31 +449,31 @@ 2. running via buildbot and running the 
                 # If we don't harness specific requirements, assume the common zip
                 # has everything we need to run tests for this suite.
                 target_packages = package_requirements['common']
 
             self.info("Downloading packages: %s for test suite category: %s" %
                       (target_packages, category))
             for file_name in target_packages:
                 target_dir = test_install_dir
-                unzip_dirs = target_unzip_dirs
+                unpack_dirs = extract_dirs
                 if "jsshell-" in file_name or file_name == "target.jsshell.zip":
                     self.info("Special-casing the jsshell zip file")
-                    unzip_dirs = None
+                    unpack_dirs = None
                     target_dir = dirs['abs_test_bin_dir']
                 url = self.query_build_dir_url(file_name)
-                self.download_unzip(url, target_dir,
-                                     target_unzip_dirs=unzip_dirs)
+                self.download_unpack(url, target_dir,
+                                     extract_dirs=unpack_dirs)
 
-    def _download_test_zip(self, target_unzip_dirs=None):
+    def _download_test_zip(self, extract_dirs=None):
         dirs = self.query_abs_dirs()
         test_install_dir = dirs.get('abs_test_install_dir',
                                     os.path.join(dirs['abs_work_dir'], 'tests'))
-        self.download_unzip(self.test_url, test_install_dir,
-                             target_unzip_dirs=target_unzip_dirs)
+        self.download_unpack(self.test_url, test_install_dir,
+                             extract_dirs=extract_dirs)
 
     def structured_output(self, suite_category):
         """Defines whether structured logging is in use in this configuration. This
         may need to be replaced with data from a different config at the resolution
         of bug 1070041 and related bugs.
         """
         return ('structured_suites' in self.config and
                 suite_category in self.config['structured_suites'])
@@ -510,19 +510,19 @@ 2. running via buildbot and running the 
         if self.config.get('download_symbols') == 'ondemand':
             self.symbols_path = self.symbols_url
             return
         if not self.symbols_path:
             self.symbols_path = os.path.join(dirs['abs_work_dir'], 'symbols')
 
         self.set_buildbot_property("symbols_url", self.symbols_url,
                                    write_to_file=True)
-        self.download_unzip(self.symbols_url, self.symbols_path)
+        self.download_unpack(self.symbols_url, self.symbols_path)
 
-    def download_and_extract(self, target_unzip_dirs=None, suite_categories=None):
+    def download_and_extract(self, extract_dirs=None, suite_categories=None):
         """
         download and extract test zip / download installer
         """
         # Swap plain http for https when we're downloading from ftp
         # See bug 957502 and friends
         from_ = "http://ftp.mozilla.org"
         to_ = "https://ftp-ssl.mozilla.org"
         for attr in 'symbols_url', 'installer_url', 'test_packages_url', 'test_url':
@@ -535,27 +535,27 @@ 2. running via buildbot and running the 
         if 'test_url' in self.config:
             # A user has specified a test_url directly, any test_packages_url will
             # be ignored.
             if self.test_packages_url:
                 self.error('Test data will be downloaded from "%s", the specified test '
                            ' package data at "%s" will be ignored.' %
                            (self.config.get('test_url'), self.test_packages_url))
 
-            self._download_test_zip(target_unzip_dirs)
+            self._download_test_zip(extract_dirs)
         else:
             if not self.test_packages_url:
                 # The caller intends to download harness specific packages, but doesn't know
                 # where the packages manifest is located. This is the case when the
                 # test package manifest isn't set as a buildbot property, which is true
                 # for some self-serve jobs and platforms using parse_make_upload.
                 self.test_packages_url = self.query_prefixed_build_dir_url('.test_packages.json')
 
             suite_categories = suite_categories or ['common']
-            self._download_test_packages(suite_categories, target_unzip_dirs)
+            self._download_test_packages(suite_categories, extract_dirs)
 
         self._download_installer()
         if self.config.get('download_symbols'):
             self._download_and_extract_symbols()
 
     # create_virtualenv is in VirtualenvMixin.
 
     def preflight_install(self):
--- a/testing/mozharness/mozharness/mozilla/testing/unittest.py
+++ b/testing/mozharness/mozharness/mozilla/testing/unittest.py
@@ -230,17 +230,17 @@ class EmulatorMixin(object):
             unzip_cmd = [unzip, '-q', os.path.join(dirs['abs_work_dir'], "emulator.zip")]
             self.run_command(unzip_cmd, cwd=dirs['abs_emulator_dir'], halt_on_failure=True,
                              fatal_exit_code=3)
 
     def install_emulator(self):
         dirs = self.query_abs_dirs()
         self.mkdir_p(dirs['abs_emulator_dir'])
         if self.config.get('emulator_url'):
-            self.download_unzip(self.config['emulator_url'], dirs['abs_emulator_dir'])
+            self.download_unpack(self.config['emulator_url'], dirs['abs_emulator_dir'])
         elif self.config.get('emulator_manifest'):
             manifest_path = self.create_tooltool_manifest(self.config['emulator_manifest'])
             do_unzip = True
             if 'unpack' in self.config['emulator_manifest']:
                 do_unzip = False
             self.install_emulator_from_tooltool(manifest_path, do_unzip)
         elif self.buildbot_config:
             props = self.buildbot_config.get('properties')
--- a/testing/mozharness/scripts/b2g_emulator_unittest.py
+++ b/testing/mozharness/scripts/b2g_emulator_unittest.py
@@ -206,18 +206,17 @@ class B2GEmulatorTest(TestingMixin, VCSM
 
         self.mkdir_p(dirs['abs_emulator_dir'])
         tar = self.query_exe('tar', return_type='list')
         self.run_command(tar + ['zxf', self.installer_path],
                          cwd=dirs['abs_emulator_dir'],
                          error_list=TarErrorList,
                          halt_on_failure=True, fatal_exit_code=3)
 
-        self.download_unzip(self.config['xre_url'],
-                             dirs['abs_xre_dir'])
+        self.download_unpack(self.config['xre_url'], dirs['abs_xre_dir'])
 
         if self.config.get('busybox_url'):
             self.download_file(self.config['busybox_url'],
                                file_name='busybox',
                                parent_dir=dirs['abs_work_dir'])
             self.busybox_path = os.path.join(dirs['abs_work_dir'], 'busybox')
 
     @PreScriptAction('create-virtualenv')
--- a/testing/mozharness/scripts/desktop_unittest.py
+++ b/testing/mozharness/scripts/desktop_unittest.py
@@ -474,30 +474,30 @@ class DesktopUnittest(TestingMixin, Merc
 
     def download_and_extract(self):
         """
         download and extract test zip / download installer
         optimizes which subfolders to extract from tests zip
         """
         c = self.config
 
-        target_unzip_dirs = None
+        extract_dirs = None
         if c['specific_tests_zip_dirs']:
-            target_unzip_dirs = list(c['minimum_tests_zip_dirs'])
+            extract_dirs = list(c['minimum_tests_zip_dirs'])
             for category in c['specific_tests_zip_dirs'].keys():
                 if c['run_all_suites'] or self._query_specified_suites(category) \
                         or 'run-tests' not in self.actions:
-                    target_unzip_dirs.extend(c['specific_tests_zip_dirs'][category])
+                    extract_dirs.extend(c['specific_tests_zip_dirs'][category])
 
         if c.get('run_all_suites'):
             target_categories = SUITE_CATEGORIES
         else:
             target_categories = [cat for cat in SUITE_CATEGORIES
                                  if self._query_specified_suites(cat) is not None]
-        super(DesktopUnittest, self).download_and_extract(target_unzip_dirs=target_unzip_dirs,
+        super(DesktopUnittest, self).download_and_extract(extract_dirs=extract_dirs,
                                                           suite_categories=target_categories)
 
     def stage_files(self):
         for category in SUITE_CATEGORIES:
             suites = self._query_specified_suites(category)
             stage = getattr(self, '_stage_{}'.format(category), None)
             if suites and stage:
                 stage(suites)
--- a/testing/mozharness/scripts/web_platform_tests.py
+++ b/testing/mozharness/scripts/web_platform_tests.py
@@ -161,22 +161,22 @@ class WebPlatformTest(TestingMixin, Merc
                                       str_format_values=str_format_values))
         cmd.extend(self.query_tests_args(try_tests,
                                          str_format_values=str_format_values))
 
         return cmd
 
     def download_and_extract(self):
         super(WebPlatformTest, self).download_and_extract(
-            target_unzip_dirs=["bin/*",
-                               "config/*",
-                               "mozbase/*",
-                               "marionette/*",
-                               "tools/wptserve/*",
-                               "web-platform/*"],
+            extract_dirs=["bin/*",
+                          "config/*",
+                          "mozbase/*",
+                          "marionette/*",
+                          "tools/wptserve/*",
+                          "web-platform/*"],
             suite_categories=["web-platform"])
 
     def run_tests(self):
         dirs = self.query_abs_dirs()
         cmd = self._query_cmd()
 
         parser = StructuredOutputParser(config=self.config,
                                         log_obj=self.log_obj,