Make get_contents() return bytes (Python3)

Currently if we end up having `Node` that is generic `Entry` and it doesn't exist when you call `get_contents` on it will give you empty string.
However in all other cases we always return `bytes` objects.
This subtle difference can break places that search for things in `get_contents` when running with Python3.

This led me to untangling str vs bytes vs unicode confusion going on
when migrating from Python2 and Python3.

I assumed (not entirely sure if correctly) that when outputting text in Python3
we really want to operate on utf-8 encoding not ascii.
So I went through places that immediately came at me (usually due to failing
tests).
Unfortunately it seems I had to adjust some tests themselves.

I introduced env.EncodeText which in Python2 just passes through (I am not sure
if that's what we want to end up doing) and converts string to utf-8 encoded data.
Ideally I think all tools writing text to a file should be writing binary
files and converting all text using env.EncodeText.
It seems environment level granularity to configure encoding seems good enough,
but potentially in future we could have per node encoding (I really can't think
of scenario where that would be required).

Minor thing is it also includes fix to some tests:

 - Few where using different PATH when checking if test can be run vs
   PATH used during actual SCons invocation (thus it decided it can run test
   because it could find an executable, but failed test itself
   because that could not find it)

 - Fixed TestCmd to work with Python3.6 when trying to create non-ascii filename
   (without change it simply raises exception "could not encode as ascii")

Author: Krzysztof Trzcinski

QMTest/TestCmd.py

@@ -310,6 +310,10 @@
 IS_PY3 = sys.version_info[0] == 3
 IS_WINDOWS = sys.platform == 'win32'
 
+if sys.version_info >= (3, 0, 0):
+    fsencode = lambda name: name.encode('utf-8')
+else:
+    fsencode = lambda name: name
 
 class null(object):
     pass
@@ -1881,7 +1885,7 @@ def do_chmod(fname):
                     do_chmod(os.path.join(dirpath, name))
             do_chmod(top)
 
-    def write(self, file, content, mode='wb'):
+    def write(self, filename, content, mode='wb'):
         """Writes the specified content text (second argument) to the
         specified file name (first argument).  The file name may be
         a list, in which case the elements are concatenated with the
@@ -1890,10 +1894,10 @@ def write(self, file, content, mode='wb'):
         exist.  The I/O mode for the file may be specified; it must
         begin with a 'w'.  The default is 'wb' (binary write).
         """
-        file = self.canonicalize(file)
+        filename = self.canonicalize(filename)
         if mode[0] != 'w':
             raise ValueError("mode must begin with 'w'")
-        with open(file, mode) as f:
+        with open(fsencode(filename), mode) as f:
             try:
                 f.write(content)
             except TypeError as e:

runtest.py

@@ -72,7 +72,7 @@
 #                       This is (will be) used for reporting results back
 #                       to a central SCons test monitoring infrastructure.
 #
-#       --exclude-list file 
+#       --exclude-list file
 #                       list of tests to exclude in the current selection of test
 #                       mostly meant to easily exclude tests from -a option
 #
@@ -99,6 +99,9 @@
     from Queue import Queue
 import subprocess
 
+if sys.version_info > (3, 0, 0):
+    import codecs
+    sys.stdout = codecs.getwriter('utf8')(sys.stdout.buffer)
 
 cwd = os.getcwd()
 
@@ -736,7 +739,7 @@ def find_py(directory):
     tests.extend(unittests)
     tests.extend(endtests)
     tests.sort()
-    
+
 if not tests:
     sys.stderr.write(usagestr + """
 runtest.py:  No tests were found.
@@ -932,4 +935,4 @@ def run(self):
 # tab-width:4
 # indent-tabs-mode:nil
 # End:
-# vim: set expandtab tabstop=4 shiftwidth=4:
+# vim: set expandtab tabstop=4 shiftwidth=4:

src/engine/SCons/Action.py

@@ -626,20 +626,9 @@ def default_batch_key(self, env, target, source):
             SCons.Util.AddMethod(self, batch_key, 'batch_key')
 
     def print_cmd_line(self, s, target, source, env):
-        """
-        In python 3, and in some of our tests, sys.stdout is
-        a String io object, and it takes unicode strings only
-        In other cases it's a regular Python 2.x file object
-        which takes strings (bytes), and if you pass those a
-        unicode object they try to decode with 'ascii' codec
-        which fails if the cmd line has any hi-bit-set chars.
-        This code assumes s is a regular string, but should
-        work if it's unicode too.
-        """
-        try:
-            sys.stdout.write(s + u"\n")
-        except UnicodeDecodeError:
-            sys.stdout.write(s + "\n")
+        print(s.__class__)
+        sys.stdout.write(s)
+        sys.stdout.write("\n")
 
     def __call__(self, target, source, env,
                                exitstatfunc=_null,

src/engine/SCons/Environment.py

@@ -60,6 +60,13 @@
 import SCons.Util
 import SCons.Warnings
 
+if sys.version_info >= (3, 0, 0):
+    def _encode_text(text):
+        return text.encode('utf-8')
+else:
+    def _encode_text(text):
+        return text
+
 class _Null(object):
     pass
 
@@ -1216,6 +1223,18 @@ def _canonicalize(self, path):
             path = str(self.fs.Dir(path))
         return path
 
+    def EncodeText(self, text):
+        """ This is meant to be used when writing files.
+        It is especially important in Python3 where it always tries to encode
+        things in text mode.
+        However it seems there is no "generic" way to set up default
+        encoding to be "utf-8".
+
+        This is simply meant as one place to define default encoding
+        for all text files generated by tools.
+        """
+        return _encode_text(text)
+
     def AppendENVPath(self, name, newpath, envname = 'ENV',
                       sep = os.pathsep, delete_existing=1):
         """Append path elements to the path 'name' in the 'ENV'

src/engine/SCons/Node/FS.py

@@ -1051,6 +1051,10 @@ def get_subst_proxy(self):
 # the method of the FS class.
 _classEntry = Entry
 
+if sys.version_info >= (3, 0, 0):
+    fsencode = os.fsencode
+else:
+    fsencode = lambda name: name
 
 class LocalFS(object):
 
@@ -1106,6 +1110,8 @@ def open(self, path):
         return open(path)
     def unlink(self, path):
         return os.unlink(path)
+    def fsencode(self, name):
+        return fsencode(name)
 
     if hasattr(os, 'symlink'):
         def islink(self, path):
@@ -1824,10 +1830,9 @@ def scanner_key(self):
         """A directory does not get scanned."""
         return None
 
-    def get_text_contents(self):
-        """We already emit things in text, so just return the binary
-        version."""
-        return self.get_contents()
+    def get_text_contents(self, encoding='utf-8'):
+        """Return decoded version of contents."""
+        return self.get_contents().decode(encoding)
 
     def get_contents(self):
         """Return content signatures and names of all our children

src/engine/SCons/Node/FSTests.py

@@ -120,6 +120,15 @@ def setUp(self):
     def tearDown(self):
         os.chdir(self.save_cwd)
 
+def assert_contents_invariants(contents):
+    assert "".encode('ascii') in contents, contents
+
+    if sys.version_info >= (3,0,0):
+        expected = bytes
+    else:
+        expected = str
+    assert isinstance(contents, expected), 'get_contents has to return decodable item ({0}) got ({1}) `{2}`'.format(expected, contents.__class__, repr(contents))
+
 class VariantDirTestCase(unittest.TestCase):
     def runTest(self):
         """Test variant dir functionality"""
@@ -335,6 +344,7 @@ def link_func(target, source, env, link_made=link_made):
         f10 = fs.File('build/var1/asourcefile')
         assert f10.exists()
         assert f10.get_contents() == bytearray('stuff','utf-8'), f10.get_contents()
+        assert_contents_invariants(f10.get_contents())
 
         f11 = fs.File('src/file11')
         t, m = f11.alter_targets()
@@ -1307,6 +1317,7 @@ class DummyNode(object):
         test.write("binary_file", "Foo\x1aBar")
         f1 = fs.File(test.workpath("binary_file"))
         assert f1.get_contents() == bytearray("Foo\x1aBar",'utf-8'), f1.get_contents()
+        assert_contents_invariants(f1.get_contents())
 
         # This tests to make sure we can decode UTF-8 text files.
         test_string = u"Foo\x1aBar"
@@ -1376,13 +1387,15 @@ def nonexistent(method, s):
         # test Entry.get_contents()
         e = fs.Entry('does_not_exist')
         c = e.get_contents()
-        assert c == "", c
+        assert_contents_invariants(c)
+        assert c == b"", c
         assert e.__class__ == SCons.Node.FS.Entry
 
         test.write("file", "file\n")
         try:
             e = fs.Entry('file')
             c = e.get_contents()
+            assert_contents_invariants(c)
             assert c == bytearray("file\n",'utf-8'), c
             assert e.__class__ == SCons.Node.FS.File
         finally:
@@ -1406,26 +1419,22 @@ def nonexistent(method, s):
         test.subdir("dir")
         e = fs.Entry('dir')
         c = e.get_contents()
-        assert c == "", c
+        assert_contents_invariants(c)
+        assert c == b"", c
         assert e.__class__ == SCons.Node.FS.Dir
 
         c = e.get_text_contents()
-        try:
-            eval('assert c == u"", c')
-        except SyntaxError:
-            assert c == ""
+        assert c == '', c
 
         if sys.platform != 'win32' and hasattr(os, 'symlink'):
             os.symlink('nonexistent', test.workpath('dangling_symlink'))
             e = fs.Entry('dangling_symlink')
             c = e.get_contents()
+            assert_contents_invariants(c)
             assert e.__class__ == SCons.Node.FS.Entry, e.__class__
-            assert c == "", c
+            assert c == b"", repr(c)
             c = e.get_text_contents()
-            try:
-                eval('assert c == u"", c')
-            except SyntaxError:
-                assert c == "", c
+            assert c == "", repr(c)
 
         test.write("tstamp", "tstamp\n")
         try:
@@ -2013,14 +2022,17 @@ def test_get_contents(self):
         e = self.fs.Dir(os.path.join('d', 'empty'))
         s = self.fs.Dir(os.path.join('d', 'sub'))
 
-        files = d.get_contents().split('\n')
+        files = d.get_contents().decode('utf-8').split('\n')
+
+        assert_contents_invariants(d.get_contents())
+        assert_contents_invariants(e.get_contents())
 
-        assert e.get_contents() == '', e.get_contents()
+        assert e.get_contents() == b'', e.get_contents()
         assert e.get_text_contents() == '', e.get_text_contents()
-        assert e.get_csig()+" empty" == files[0], files
-        assert f.get_csig()+" f" == files[1], files
-        assert g.get_csig()+" g" == files[2], files
-        assert s.get_csig()+" sub" == files[3], files
+        assert e.get_csig() + " empty" == files[0], files
+        assert f.get_csig() + " f" == files[1], files
+        assert g.get_csig() + " g" == files[2], files
+        assert s.get_csig() + " sub" == files[3], files
 
     def test_implicit_re_scans(self):
         """Test that adding entries causes a directory to be re-scanned
@@ -2351,14 +2363,17 @@ def test_runTest(self):
 
         e3d = fs.Entry('e3d')
         e3d.get_contents()
+        assert_contents_invariants(e3d.get_contents())
         assert e3d.__class__ is SCons.Node.FS.Dir, e3d.__class__
 
         e3f = fs.Entry('e3f')
         e3f.get_contents()
+        assert_contents_invariants(e3f.get_contents())
         assert e3f.__class__ is SCons.Node.FS.File, e3f.__class__
 
         e3n = fs.Entry('e3n')
         e3n.get_contents()
+        assert_contents_invariants(e3n.get_contents())
         assert e3n.__class__ is SCons.Node.FS.Entry, e3n.__class__
 
         test.subdir('e4d')
@@ -3113,6 +3128,7 @@ def test_get_contents(self):
         test.write(["rep3", "contents"], "Con\x1aTents\n")
         try:
             c = fs.File("contents").get_contents()
+            assert_contents_invariants(c)
             assert c == bytearray("Con\x1aTents\n",'utf-8'), "got '%s'" % c
         finally:
             test.unlink(["rep3", "contents"])

src/engine/SCons/Node/NodeTests.py

@@ -273,7 +273,7 @@ class TestNodeInfo(SCons.Node.NodeInfoBase):
 
         f = ni1.format()
         assert f == ['x', 'y', 'z'], f
- 
+
         field_list = ['xxx', 'zzz', 'aaa']
 
         f = ni1.format(field_list)
@@ -586,7 +586,7 @@ def test_get_actions(self):
     def test_get_csig(self):
         """Test generic content signature calculation
         """
-        
+
         class TestNodeInfo(SCons.Node.NodeInfoBase):
             __slots__ = ('csig',)
         try:
@@ -625,7 +625,7 @@ class TestNodeInfo(SCons.Node.NodeInfoBase):
             __slots__ = ('csig',)
         SCons.Node.Node.NodeInfo = TestNodeInfo
         node = SCons.Node.Node()
- 
+
         binfo = node.get_binfo()
         assert isinstance(binfo, SCons.Node.BuildInfoBase), binfo
 

src/engine/SCons/Node/__init__.py

@@ -197,7 +197,7 @@ def get_contents_entry(node):
         # string so calls to get_contents() in emitters and the
         # like (e.g. in qt.py) don't have to disambiguate by hand
         # or catch the exception.
-        return ''
+        return b''
     else:
         return _get_contents_map[node._func_get_contents](node)
 
@@ -207,7 +207,7 @@ def get_contents_dir(node):
     contents = []
     for n in sorted(node.children(), key=lambda t: t.name):
         contents.append('%s %s\n' % (n.get_csig(), n.name))
-    return ''.join(contents)
+    return node.fs.fsencode(''.join(contents))
 
 def get_contents_file(node):
     if not node.rexists():