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

@@ -7,6 +7,7 @@ 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
@@ -153,7 +154,7 @@ class TestBuild(unittest.TestCase):
                                  '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))

python/mozbuild/mozpack/files.py

@@ -4,6 +4,7 @@
 
 from __future__ import absolute_import, print_function, unicode_literals
 
+import codecs
 import errno
 import inspect
 import os
@@ -95,27 +96,25 @@ class Dest(object):
     '''
 
     def __init__(self, path):
-        self.path = ensure_unicode(path)
+        self.file = None
         self.mode = None
+        self.path = ensure_unicode(path)
 
     @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):
@@ -125,6 +124,7 @@ class Dest(object):
         if self.mode:
             self.mode = None
             self.file.close()
+            self.file = None
 
 
 class BaseFile(object):
@@ -227,7 +227,7 @@ class BaseFile(object):
                 shutil.copyfileobj(self.open(), dest)
             return True
 
-        src = self.open('rb')
+        src = self.open()
         copy_content = b''
         while True:
             dest_content = dest.read(32768)
@@ -246,14 +246,14 @@ class BaseFile(object):
             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.')
@@ -301,7 +301,7 @@ class File(BaseFile):
 
     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):
@@ -632,23 +632,19 @@ class GeneratedFile(BaseFile):
 
     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
@@ -671,7 +667,7 @@ class DeflatedFile(BaseFile):
         assert isinstance(file, JarFileReader)
         self.file = file
 
-    def open(self, mode='rb'):
+    def open(self):
         self.file.seek(0)
         return self.file
 
@@ -740,7 +736,7 @@ class ManifestFile(BaseFile):
         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.
@@ -748,9 +744,7 @@ class ManifestFile(BaseFile):
         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):
         '''
@@ -775,18 +769,16 @@ class MinifiedProperties(BaseFile):
         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):
@@ -799,19 +791,21 @@ class MinifiedJavaScript(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()
@@ -1159,10 +1153,8 @@ class MercurialFile(BaseFile):
         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

python/mozbuild/mozpack/packager/__init__.py

@@ -4,9 +4,11 @@
 
 from __future__ import absolute_import, print_function, unicode_literals
 
-from mozbuild.preprocessor import Preprocessor
-import re
+import codecs
+from collections import deque
+import json
 import os
+import re
 import six
 from mozpack.errors import errors
 from mozpack.chrome.manifest import (
@@ -21,8 +23,7 @@ from mozpack.files import (
     ExecutableFile,
 )
 import mozpack.path as mozpath
-from collections import deque
-import json
+from mozbuild.preprocessor import Preprocessor
 
 
 class Component(object):
@@ -282,12 +283,12 @@ class SimplePackager(object):
             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:
@@ -321,7 +322,7 @@ class SimplePackager(object):
             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):

python/mozbuild/mozpack/packager/unpack.py

@@ -4,6 +4,7 @@
 
 from __future__ import absolute_import, print_function, unicode_literals
 
+import codecs
 import mozpack.path as mozpath
 from mozpack.files import (
     BaseFinder,
@@ -81,7 +82,8 @@ class UnpackFinder(BaseFinder):
             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

python/mozbuild/mozpack/test/test_files.py

@@ -814,7 +814,7 @@ class TestManifestFile(TestWithTmpDir):
         )
 
         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())
 
@@ -879,7 +879,7 @@ class TestMinifiedProperties(TestWithTmpDir):
         ]
         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'))
@@ -917,7 +917,7 @@ class TestMinifiedJavaScript(TestWithTmpDir):
         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))
 

python/mozbuild/mozpack/test/test_packager_formats.py

@@ -316,9 +316,9 @@ def get_contents(registry, read_all=False, mode='rt'):
             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

testing/mozbase/manifestparser/manifestparser/manifestparser.py

@@ -4,6 +4,7 @@
 
 from __future__ import absolute_import, print_function
 
+import codecs
 import fnmatch
 import io
 import json
@@ -155,7 +156,7 @@ class ManifestParser(object):
             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)

toolkit/mozapps/installer/find-dupes.py

@@ -53,8 +53,7 @@ def find_dupes(source, allowed_dupes, bail=True):
     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()

toolkit/mozapps/installer/packager.py

@@ -51,8 +51,7 @@ class RemovedFiles(GeneratedFile):
             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):