Bug 1638060 - Standardize interface of `mozfile` classes as `bytes`-based streams r=glandium
authorRicky Stewart <rstewart@mozilla.com>
Fri, 22 May 2020 01:11:29 +0000
changeset 531704 327c6c5acdbcbd884f818c0fa7b507048642291b
parent 531703 8fbcd4668bae62cbba94dfa1657b0d8cce8aa225
child 531705 8783aa871d5e71f52fd8543882a893441c7d3375
push id37442
push userncsoregi@mozilla.com
push dateSat, 23 May 2020 09:21:24 +0000
treeherdermozilla-central@bbcc193fe0f0 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersglandium
bugs1638060, 1602540
milestone78.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 1638060 - Standardize interface of `mozfile` classes as `bytes`-based streams r=glandium At the beginning of the Python 3 migration (circa bug 1602540), we made an update to the interface of `mozpack/files.py` in the direction of aligning with Python 3's built-in `file` support; namely, that opening a file in text mode returns a stream of `str` (text), and that opening a file in binary mode returns a stream of `bytes`. This was deemed to be more trouble than it was worth. This patch undoes all of those changes to the interface in favor of moving back to the Python 2 style, where all files are bytestreams. Differential Revision: https://phabricator.services.mozilla.com/D75424
python/mozbuild/mozbuild/test/backend/test_build.py
python/mozbuild/mozpack/files.py
python/mozbuild/mozpack/packager/__init__.py
python/mozbuild/mozpack/packager/unpack.py
python/mozbuild/mozpack/test/test_files.py
python/mozbuild/mozpack/test/test_packager_formats.py
testing/mozbase/manifestparser/manifestparser/manifestparser.py
toolkit/mozapps/installer/find-dupes.py
toolkit/mozapps/installer/packager.py
--- a/python/mozbuild/mozbuild/test/backend/test_build.py
+++ b/python/mozbuild/mozbuild/test/backend/test_build.py
@@ -2,16 +2,17 @@
 # License, v. 2.0. If a copy of the MPL was not distributed with this
 # file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
 from __future__ import absolute_import, unicode_literals, print_function
 
 import buildconfig
 import os
 import shutil
+import six
 import sys
 import unittest
 import mozpack.path as mozpath
 from contextlib import contextmanager
 from mozunit import main
 from mozbuild.backend import get_backend_class
 from mozbuild.backend.configenvironment import ConfigEnvironment
 from mozbuild.backend.recursivemake import RecursiveMakeBackend
@@ -148,17 +149,17 @@ class TestBuild(unittest.TestCase):
             self.validate(config)
 
     def validate(self, config):
         self.maxDiff = None
         test_path = mozpath.join('$SRCDIR', 'python', 'mozbuild', 'mozbuild',
                                  'test', 'backend', 'data', 'build')
 
         result = {
-            p: f.open(mode='r').read()
+            p: six.ensure_text(f.open().read())
             for p, f in FileFinder(mozpath.join(config.topobjdir, 'dist'))
         }
         self.assertTrue(len(result))
         self.assertEqual(result, {
             'bin/baz.ini': 'baz.ini: FOO is foo\n',
             'bin/child/bar.ini': 'bar.ini\n',
             'bin/child2/foo.css': 'foo.css: FOO is foo\n',
             'bin/child2/qux.ini': 'qux.ini: BAR is not defined\n',
--- a/python/mozbuild/mozpack/files.py
+++ b/python/mozbuild/mozpack/files.py
@@ -1,14 +1,15 @@
 # This Source Code Form is subject to the terms of the Mozilla Public
 # License, v. 2.0. If a copy of the MPL was not distributed with this
 # file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
 from __future__ import absolute_import, print_function, unicode_literals
 
+import codecs
 import errno
 import inspect
 import os
 import platform
 import shutil
 import six
 import stat
 import subprocess
@@ -90,46 +91,45 @@ class Dest(object):
       underlying file.
     - a call to read() after a write() will re-open the underlying file and
       read from it.
     - a call to write() after a read() will re-open the underlying file,
       emptying it, and write to it.
     '''
 
     def __init__(self, path):
+        self.file = None
+        self.mode = None
         self.path = ensure_unicode(path)
-        self.mode = None
 
     @property
     def name(self):
         return self.path
 
-    def read(self, length=-1, mode='rb'):
+    def read(self, length=-1):
         if self.mode != 'r':
-            self.file = _open(self.path, mode)
+            self.file = _open(self.path, mode='rb')
             self.mode = 'r'
         return self.file.read(length)
 
-    def write(self, data, mode='wb'):
+    def write(self, data):
         if self.mode != 'w':
-            self.file = _open(self.path, mode)
+            self.file = _open(self.path, mode='wb')
             self.mode = 'w'
-        if 'b' in mode:
-            to_write = six.ensure_binary(data)
-        else:
-            to_write = six.ensure_text(data)
+        to_write = six.ensure_binary(data)
         return self.file.write(to_write)
 
     def exists(self):
         return os.path.exists(self.path)
 
     def close(self):
         if self.mode:
             self.mode = None
             self.file.close()
+            self.file = None
 
 
 class BaseFile(object):
     '''
     Base interface and helper for file copying. Derived class may implement
     their own copy function, or rely on BaseFile.copy using the open() member
     function and/or the path property.
     '''
@@ -222,17 +222,17 @@ class BaseFile(object):
                 shutil.copystat(self.path, dest.path)
             else:
                 # Ensure the file is always created
                 if not dest.exists():
                     dest.write(b'')
                 shutil.copyfileobj(self.open(), dest)
             return True
 
-        src = self.open('rb')
+        src = self.open()
         copy_content = b''
         while True:
             dest_content = dest.read(32768)
             src_content = src.read(32768)
             copy_content += src_content
             if len(dest_content) == len(src_content) == 0:
                 break
             # If the read content differs between origin and destination,
@@ -241,24 +241,24 @@ class BaseFile(object):
                 six.ensure_binary(src_content)):
                 dest.write(copy_content)
                 shutil.copyfileobj(src, dest)
                 break
         if hasattr(self, 'path') and hasattr(dest, 'path'):
             shutil.copystat(self.path, dest.path)
         return True
 
-    def open(self, mode='rb'):
+    def open(self):
         '''
         Return a file-like object allowing to read() the content of the
         associated file. This is meant to be overloaded in subclasses to return
         a custom file-like object.
         '''
         assert self.path is not None
-        return _open(self.path, mode=mode)
+        return open(self.path, 'rb')
 
     def read(self):
         raise NotImplementedError('BaseFile.read() not implemented. Bug 1170329.')
 
     def size(self):
         """Returns size of the entry.
 
         Derived classes are highly encouraged to override this with a more
@@ -296,17 +296,17 @@ class File(BaseFile):
         if platform.system() == 'Windows':
             return None
         assert self.path is not None
         mode = os.stat(self.path).st_mode
         return self.normalize_mode(mode)
 
     def read(self):
         '''Return the contents of the file.'''
-        with _open(self.path, 'rb') as fh:
+        with open(self.path, 'rb') as fh:
             return fh.read()
 
     def size(self):
         return os.stat(self.path).st_size
 
     def inputs(self):
         return (self.path,)
 
@@ -627,33 +627,29 @@ class PreprocessedFile(BaseFile):
 
 class GeneratedFile(BaseFile):
     '''
     File class for content with no previous existence on the filesystem.
     '''
 
     def __init__(self, content):
         self._content = content
-        self._mode = 'rb'
 
     @property
     def content(self):
-        ensure = (six.ensure_binary if 'b' in self._mode else six.ensure_text)
         if inspect.isfunction(self._content):
-            self._content = ensure(self._content())
-        return ensure(self._content)
+            self._content = self._content()
+        return six.ensure_binary(self._content)
 
     @content.setter
     def content(self, content):
         self._content = content
 
-    def open(self, mode='rb'):
-        self._mode = mode
-        return (BytesIO(self.content) if 'b' in self._mode
-                else six.StringIO(self.content))
+    def open(self):
+        return BytesIO(self.content)
 
     def read(self):
         return self.content
 
     def size(self):
         return len(self.content)
 
     def inputs(self):
@@ -666,17 +662,17 @@ class DeflatedFile(BaseFile):
     extracts the file from the jar archive.
     '''
 
     def __init__(self, file):
         from mozpack.mozjar import JarFileReader
         assert isinstance(file, JarFileReader)
         self.file = file
 
-    def open(self, mode='rb'):
+    def open(self):
         self.file.seek(0)
         return self.file
 
 
 class ExtractedTarFile(GeneratedFile):
     '''
     File class for members of a tar archive. Contents of the underlying file
     are extracted immediately and stored in memory.
@@ -735,27 +731,25 @@ class ManifestFile(BaseFile):
         Remove the given entry from the manifest.
         '''
         assert isinstance(entry, ManifestEntry)
         if isinstance(entry, ManifestInterfaces):
             self._interfaces.remove(entry)
         else:
             self._entries.remove(entry)
 
-    def open(self, mode='rt'):
+    def open(self):
         '''
         Return a file-like object allowing to read() the serialized content of
         the manifest.
         '''
         content = ''.join(
             '%s\n' % e.rebase(self._base)
             for e in chain(self._entries, self._interfaces))
-        if 'b' in mode:
-            return BytesIO(six.ensure_binary(content))
-        return six.StringIO(six.ensure_text(content))
+        return BytesIO(six.ensure_binary(content))
 
     def __iter__(self):
         '''
         Iterate over entries in the manifest file.
         '''
         return chain(self._entries, self._interfaces)
 
     def isempty(self):
@@ -770,53 +764,53 @@ class MinifiedProperties(BaseFile):
     File class for minified properties. This wraps around a BaseFile instance,
     and removes lines starting with a # from its content.
     '''
 
     def __init__(self, file):
         assert isinstance(file, BaseFile)
         self._file = file
 
-    def open(self, mode='r'):
+    def open(self):
         '''
         Return a file-like object allowing to read() the minified content of
         the properties file.
         '''
         content = ''.join(
             l for l in [
-                six.ensure_text(s) for s in self._file.open(mode).readlines()
+                six.ensure_text(s) for s in self._file.open().readlines()
             ] if not l.startswith('#'))
-        if 'b' in mode:
-            return BytesIO(six.ensure_binary(content))
-        return six.StringIO(content)
+        return BytesIO(six.ensure_binary(content))
 
 
 class MinifiedJavaScript(BaseFile):
     '''
     File class for minifying JavaScript files.
     '''
 
     def __init__(self, file, verify_command=None):
         assert isinstance(file, BaseFile)
         self._file = file
         self._verify_command = verify_command
 
-    def open(self, mode='r'):
+    def open(self):
         output = six.StringIO()
-        minify = JavascriptMinify(self._file.open('r'), output, quote_chars="'\"`")
+        minify = JavascriptMinify(codecs.getreader('utf-8')(self._file.open()),
+                                  output, quote_chars="'\"`")
         minify.minify()
         output.seek(0)
+        output_source = six.ensure_binary(output.getvalue())
+        output = BytesIO(output_source)
 
         if not self._verify_command:
             return output
 
-        input_source = self._file.open('r').read()
-        output_source = output.getvalue()
+        input_source = self._file.open().read()
 
-        with NamedTemporaryFile('w+') as fh1, NamedTemporaryFile('w+') as fh2:
+        with NamedTemporaryFile('wb+') as fh1, NamedTemporaryFile('wb+') as fh2:
             fh1.write(input_source)
             fh2.write(output_source)
             fh1.flush()
             fh2.flush()
 
             try:
                 args = list(self._verify_command)
                 args.extend([fh1.name, fh2.name])
@@ -1154,20 +1148,18 @@ class ComposedFinder(BaseFinder):
 
 class MercurialFile(BaseFile):
     """File class for holding data from Mercurial."""
 
     def __init__(self, client, rev, path):
         self._content = client.cat([six.ensure_binary(path)],
                                    rev=six.ensure_binary(rev))
 
-    def open(self, mode='rb'):
-        if 'b' in mode:
-            return BytesIO(six.ensure_binary(self._content))
-        return six.StringIO(six.ensure_text(self._content))
+    def open(self):
+        return BytesIO(six.ensure_binary(self._content))
 
     def read(self):
         return self._content
 
 
 class MercurialRevisionFinder(BaseFinder):
     """A finder that operates on a specific Mercurial revision."""
 
--- a/python/mozbuild/mozpack/packager/__init__.py
+++ b/python/mozbuild/mozpack/packager/__init__.py
@@ -1,33 +1,34 @@
 # This Source Code Form is subject to the terms of the Mozilla Public
 # License, v. 2.0. If a copy of the MPL was not distributed with this
 # file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
 from __future__ import absolute_import, print_function, unicode_literals
 
-from mozbuild.preprocessor import Preprocessor
+import codecs
+from collections import deque
+import json
+import os
 import re
-import os
 import six
 from mozpack.errors import errors
 from mozpack.chrome.manifest import (
     Manifest,
     ManifestBinaryComponent,
     ManifestChrome,
     ManifestInterfaces,
     is_manifest,
     parse_manifest,
 )
 from mozpack.files import (
     ExecutableFile,
 )
 import mozpack.path as mozpath
-from collections import deque
-import json
+from mozbuild.preprocessor import Preprocessor
 
 
 class Component(object):
     '''
     Class that represents a component in a package manifest.
     '''
 
     def __init__(self, name, destdir='', xz_compress=False):
@@ -277,22 +278,22 @@ class SimplePackager(object):
         if is_manifest(path):
             self._add_manifest_file(path, file)
         elif path.endswith('.xpt'):
             self._queue.append(self.formatter.add_interfaces, path, file)
         else:
             self._file_queue.append(self.formatter.add, path, file)
             if mozpath.basename(path) == 'install.rdf':
                 addon = True
-                install_rdf = six.ensure_text(file.open('rt').read())
+                install_rdf = six.ensure_text(file.open().read())
                 if self.UNPACK_ADDON_RE.search(install_rdf):
                     addon = 'unpacked'
                 self._add_addon(mozpath.dirname(path), addon)
             elif mozpath.basename(path) == 'manifest.json':
-                manifest = six.ensure_text(file.open('rt').read())
+                manifest = six.ensure_text(file.open().read())
                 try:
                     parsed = json.loads(manifest)
                 except ValueError:
                     pass
                 if isinstance(parsed, dict) and 'manifest_version' in parsed:
                     self._add_addon(mozpath.dirname(path), True)
 
     def _add_addon(self, path, addon_type):
@@ -316,17 +317,17 @@ class SimplePackager(object):
         '''
         self._manifests.add(path)
         base = ''
         if hasattr(file, 'path'):
             # Find the directory the given path is relative to.
             b = mozpath.normsep(file.path)
             if b.endswith('/' + path) or b == path:
                 base = os.path.normpath(b[:-len(path)])
-        for e in parse_manifest(base, path, file.open('rt')):
+        for e in parse_manifest(base, path, codecs.getreader('utf-8')(file.open())):
             # ManifestResources need to be given after ManifestChrome, so just
             # put all ManifestChrome in a separate queue to make them first.
             if isinstance(e, ManifestChrome):
                 # e.move(e.base) just returns a clone of the entry.
                 self._chrome_queue.append(self.formatter.add_manifest,
                                           e.move(e.base))
             elif not isinstance(e, (Manifest, ManifestInterfaces)):
                 self._queue.append(self.formatter.add_manifest, e.move(e.base))
--- a/python/mozbuild/mozpack/packager/unpack.py
+++ b/python/mozbuild/mozpack/packager/unpack.py
@@ -1,14 +1,15 @@
 # This Source Code Form is subject to the terms of the Mozilla Public
 # License, v. 2.0. If a copy of the MPL was not distributed with this
 # file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
 from __future__ import absolute_import, print_function, unicode_literals
 
+import codecs
 import mozpack.path as mozpath
 from mozpack.files import (
     BaseFinder,
     FileFinder,
     DeflatedFile,
     ManifestFile,
 )
 from mozpack.chrome.manifest import (
@@ -76,17 +77,18 @@ class UnpackFinder(BaseFinder):
                     self._fill_with_jar(p[:-len(self.omnijar) - 1], jar)
                     continue
             # If the file is a manifest, scan its entries for some referencing
             # jar: urls. If there are some, the files contained in the jar they
             # point to, go under a directory named after the jar.
             if is_manifest(p):
                 m = self.files[p] if self.files.contains(p) \
                     else ManifestFile(base)
-                for e in parse_manifest(self.base, p, f.open('rt')):
+                for e in parse_manifest(
+                        self.base, p, codecs.getreader('utf-8')(f.open())):
                     m.add(self._handle_manifest_entry(e, jars))
                 if self.files.contains(p):
                     continue
                 f = m
             # If the file is a packed addon, unpack it under a directory named
             # after the xpi.
             if p.endswith('.xpi') and self._maybe_zip(f):
                 self._fill_with_jar(p[:-4], self._open_jar(p, f))
--- a/python/mozbuild/mozpack/test/test_files.py
+++ b/python/mozbuild/mozpack/test/test_files.py
@@ -809,17 +809,17 @@ class TestManifestFile(TestWithTmpDir):
                                  'toolkit/content/global/'))
         self.assertRaises(
             ValueError,
             f.remove,
             ManifestContent('chrome', 'global', 'toolkit/content/global/')
         )
 
         f.copy(self.tmppath('chrome.manifest'))
-        content = open(self.tmppath('chrome.manifest'), 'rt').read()
+        content = open(self.tmppath('chrome.manifest'), 'rb').read()
         self.assertEqual(content[:42], f.open().read(42))
         self.assertEqual(content, f.open().read())
 
 
 # Compiled typelib for the following IDL:
 #     interface foo;
 #     [scriptable, uuid(5f70da76-519c-4858-b71e-e3c92333e2d6)]
 #     interface bar {
@@ -874,17 +874,17 @@ class TestMinifiedProperties(TestWithTmp
         propLines = [
             '# Comments are removed',
             'foo = bar',
             '',
             '# Another comment',
         ]
         prop = GeneratedFile('\n'.join(propLines))
         self.assertEqual(MinifiedProperties(prop).open().readlines(),
-                         ['foo = bar\n', '\n'])
+                         [b'foo = bar\n', b'\n'])
         open(self.tmppath('prop'), 'w').write('\n'.join(propLines))
         MinifiedProperties(File(self.tmppath('prop'))) \
             .copy(self.tmppath('prop2'))
         self.assertEqual(open(self.tmppath('prop2')).readlines(),
                          ['foo = bar\n', '\n'])
 
 
 class TestMinifiedJavaScript(TestWithTmpDir):
@@ -912,17 +912,17 @@ class TestMinifiedJavaScript(TestWithTmp
             code,
         ]
 
     def test_minified_verify_success(self):
         orig_f = GeneratedFile('\n'.join(self.orig_lines))
         min_f = MinifiedJavaScript(orig_f,
                                    verify_command=self._verify_command('0'))
 
-        mini_lines = min_f.open().readlines()
+        mini_lines = [six.ensure_text(s) for s in min_f.open().readlines()]
         self.assertTrue(mini_lines)
         self.assertTrue(len(mini_lines) < len(self.orig_lines))
 
     def test_minified_verify_failure(self):
         orig_f = GeneratedFile('\n'.join(self.orig_lines))
         errors.out = six.StringIO()
         min_f = MinifiedJavaScript(orig_f,
                                    verify_command=self._verify_command('1'))
--- a/python/mozbuild/mozpack/test/test_packager_formats.py
+++ b/python/mozbuild/mozpack/test/test_packager_formats.py
@@ -311,19 +311,19 @@ def fill_formatter(formatter, contents):
 
 def get_contents(registry, read_all=False, mode='rt'):
     result = {}
     for k, v in registry:
         if isinstance(v, FileRegistry):
             result[k] = get_contents(v)
         elif isinstance(v, ManifestFile) or read_all:
             if 'b' in mode:
-                result[k] = v.open(mode).read()
+                result[k] = v.open().read()
             else:
-                result[k] = v.open(mode).read().splitlines()
+                result[k] = six.ensure_text(v.open().read()).splitlines()
         else:
             result[k] = v
     return result
 
 
 class TestFormatters(TestErrors, unittest.TestCase):
     maxDiff = None
 
--- a/testing/mozbase/manifestparser/manifestparser/manifestparser.py
+++ b/testing/mozbase/manifestparser/manifestparser/manifestparser.py
@@ -1,14 +1,15 @@
 # This Source Code Form is subject to the terms of the Mozilla Public
 # License, v. 2.0. If a copy of the MPL was not distributed with this file,
 # You can obtain one at http://mozilla.org/MPL/2.0/.
 
 from __future__ import absolute_import, print_function
 
+import codecs
 import fnmatch
 import io
 import json
 import os
 import shutil
 import sys
 import types
 
@@ -150,17 +151,17 @@ class ManifestParser(object):
             # during manifest reading, the getcwd() calls that happen
             # with abspath calls will not be meaningful, so absolute
             # paths are required.
             if self.finder:
                 assert os.path.isabs(filename)
             filename = os.path.abspath(filename)
             self.source_files.add(filename)
             if self.finder:
-                fp = self.finder.get(filename).open(mode='r')
+                fp = codecs.getreader('utf-8')(self.finder.get(filename).open())
             else:
                 fp = io.open(filename, encoding='utf-8')
             here = os.path.dirname(filename)
         else:
             fp = filename
             filename = here = None
         defaults['here'] = here
 
--- a/toolkit/mozapps/installer/find-dupes.py
+++ b/toolkit/mozapps/installer/find-dupes.py
@@ -48,18 +48,17 @@ def normalize_path(p):
 
 def find_dupes(source, allowed_dupes, bail=True):
     md5_chunk_size = 1024*10
     allowed_dupes = set(allowed_dupes)
     md5s = OrderedDict()
     for p, f in UnpackFinder(source):
         md5 = hashlib.md5()
         content_size = 0
-        for buf in iter(functools.partial(f.open('rb').read, md5_chunk_size),
-                        b''):
+        for buf in iter(functools.partial(f.open().read, md5_chunk_size), b''):
             md5.update(six.ensure_binary(buf))
             content_size += len(six.ensure_binary(buf))
         m = md5.digest()
         if m not in md5s:
             if isinstance(f, DeflatedFile):
                 compressed = f.file.compressed_size
             else:
                 compressed = content_size
--- a/toolkit/mozapps/installer/packager.py
+++ b/toolkit/mozapps/installer/packager.py
@@ -46,18 +46,17 @@ class RemovedFiles(GeneratedFile):
         GeneratedFile.__init__(self, b'')
 
     def handle_line(self, f):
         f = f.strip()
         if not f:
             return
         if self.copier.contains(f):
             errors.error('Removal of packaged file(s): %s' % f)
-        ensure = six.ensure_binary if 'b' in self._mode else six.ensure_text
-        self.content += ensure(f) + ensure('\n')
+        self.content += six.ensure_binary(f) + b'\n'
 
 
 def split_define(define):
     '''
     Give a VAR[=VAL] string, returns a (VAR, VAL) tuple, where VAL defaults to
     1. Numeric VALs are returned as ints.
     '''
     if '=' in define: