[lit] Clean up internal diff's encoding handling

[jdenny]

Summary

As suggested by rnk at D67643#1673043, instead of reading files
multiple times until an appropriate encoding is found, read them once
as binary, and then try to decode what was read.

For Python >= 3.5, don't fail when attempting to decode the
diff_bytes output in order to print it.

Avoid failures for Python 2.7 used on some Windows bots by
transforming diff output with lit.util.to_string before writing it
to stdout.

Finally, add some tests for encoding handling.

[rnk]

lgtm

I'm guessing you've tested with Python 2.7 and 3.5, and that's probably what matters.

Thanks for working on this, and sorry for ever expanding scope of the suggested refactorings.

llvm/utils/lit/lit/builtin_commands/diff.py

36 ↗

(On Diff #223935)

try / except UnicodeDecodeError is an exciting code pattern, but I guess it's the existing behavior. :)

43 ↗

(On Diff #223935)

Extra logging?

63 ↗

(On Diff #223935)

Ditto

[jdenny]

In D68664#1700456, @rnk wrote:

I'm guessing you've tested with Python 2.7 and 3.5, and that's probably what matters.

I have 2.7.15 and 3.6.8, but I assume that's sufficient. With each, I've run check-lit and manually tried diff.py. I still need to try check-all before pushing everything.

Thanks for working on this, and sorry for ever expanding scope of the suggested refactorings.

It all needed to be done. I just ran out of time last month. Thanks for the reviews.

llvm/utils/lit/lit/builtin_commands/diff.py

36 ↗

(On Diff #223935)

I'm not quite sure how to interpret this remark. Indeed, it's the existing behavior. Do you recommend an alternative?

43 ↗

(On Diff #223935)

Hmm. I'm being sloppy. I'll remove. Thanks.

[jdenny]

Removed commented code pointed out during review.

Changed diff.decode(errors="replace") to diff.decode(errors="backslashreplace") so the test suite doesn't fail at sys.stdout.write(diff) when running with python3. The test's commands worked fine when running interactively from a shell prompt, apparently because stdout is then different. Sorry, I must have forgotten to re-run the actual test suite with python3 after making some change here.

In any case, backslashreplace results in output like the following, where \xff represents the bytes that cannot be rendered:

 foo
-bar
+bar\xff\xff
 baz

That seems better than ignore, which just drops them altogether:

 foo
-bar
+bar
 baz

[jdenny]

Rebased onto master so it doesn't depend on D66574 anymore. Thus, it modifies TestRunner.py instead of diff.py.

Added lit.util.to_string calls to transform diff output before writing to stdout. I built in a local Windows 10 to test this. With Python 2.7 and without to_string, I was able to reproduce at least some of the previous bot failures, and to_string fixes them. I also tried Python 3.6, and it works fine for me.

llvm/utils/lit/lit/TestRunner.py

 from __future__ import absolute_import
 import difflib
 import errno
 import functools
 import io
 import itertools
 import getopt
+import locale
 import os, signal, subprocess, sys
 import re
 import stat
 import platform
 import shutil
 import tempfile
 import threading
 
def getDirTree(path, basedir=""):
         for dirname, child_dirs, files in os.walk(os.path.join(basedir, path)):
             for child_dir in child_dirs:
                 child_trees.append(getDirTree(child_dir, dirname))
             for filename in files:
                 child_trees.append((filename, None))
             return path, sorted(child_trees)
 
     def compareTwoFiles(filepaths):
-        compare_bytes = False
-        encoding = None
         filelines = []
         for file in filepaths:
-            try:
-                with open(file, 'r') as f:
-                    filelines.append(f.readlines())
-            except UnicodeDecodeError:
-                try:
-                    with io.open(file, 'r', encoding="utf-8") as f:
-                        filelines.append(f.readlines())
-                    encoding = "utf-8"
-                except:
-                    compare_bytes = True
-
-        if compare_bytes:
-            return compareTwoBinaryFiles(filepaths)
-        else:
-            return compareTwoTextFiles(filepaths, encoding)
-
-    def compareTwoBinaryFiles(filepaths):
-        filelines = []
-        for file in filepaths:
-            with open(file, 'rb') as f:
-                filelines.append(f.readlines())
+            with open(file, 'rb') as file_bin:
+                filelines.append(file_bin.readlines())
+
+            return compareTwoTextFiles(filepaths, filelines,
+                                       locale.getpreferredencoding(False))
+                return compareTwoTextFiles(filepaths, filelines, "utf-8")
+                return compareTwoBinaryFiles(filepaths, filelines)
 
+    def compareTwoBinaryFiles(filepaths, filelines):
         exitCode = 0
         if hasattr(difflib, 'diff_bytes'):
             # python 3.5 or newer
             diffs = difflib.diff_bytes(difflib.unified_diff, filelines[0],
                                        filelines[1], filepaths[0].encode(),
                                        filepaths[1].encode(),
                                        n = num_context_lines)
-            diffs = [diff.decode() for diff in diffs]
+            diffs = [diff.decode(errors="backslashreplace") for diff in diffs]
         else:
             # python 2.7
             func = difflib.unified_diff if unified_diff else difflib.context_diff
             diffs = func(filelines[0], filelines[1], filepaths[0], filepaths[1],
                          n = num_context_lines)
 
         for diff in diffs:
-            stdout.write(diff)
+            stdout.write(to_string(diff))
             exitCode = 1
         return exitCode
 
-    def compareTwoTextFiles(filepaths, encoding):
+    def compareTwoTextFiles(filepaths, filelines_bin, encoding):
         filelines = []
-        for file in filepaths:
-            if encoding is None:
-                with open(file, 'r') as f:
-                    filelines.append(f.readlines())
-            else:
-                with io.open(file, 'r', encoding=encoding) as f:
-                    filelines.append(f.readlines())
+        for lines_bin in filelines_bin:
+            lines = []
+            for line_bin in lines_bin:
+                line = line_bin.decode(encoding=encoding)
+                lines.append(line)
+            filelines.append(lines)
 
         exitCode = 0
         def compose2(f, g):
             return lambda x: f(g(x))
 
         f = lambda x: x
         if strip_trailing_cr:
             f = compose2(lambda line: line.rstrip('\r'), f)
         if ignore_all_space or ignore_space_change:
             ignoreSpace = lambda line, separator: separator.join(line.split())
             ignoreAllSpaceOrSpaceChange = functools.partial(ignoreSpace, separator='' if ignore_all_space else ' ')
             f = compose2(ignoreAllSpaceOrSpaceChange, f)
 
         for idx, lines in enumerate(filelines):
             filelines[idx]= [f(line) for line in lines]
 
         func = difflib.unified_diff if unified_diff else difflib.context_diff
         for diff in func(filelines[0], filelines[1], filepaths[0], filepaths[1],
                          n = num_context_lines):
-            stdout.write(diff)
+            stdout.write(to_string(diff))
             exitCode = 1
         return exitCode
 
     def printDirVsFile(dir_path, file_path):
         if os.path.getsize(file_path):
             msg = "File %s is a directory while file %s is a regular file"
         else:
             msg = "File %s is a directory while file %s is a regular empty file"

llvm/utils/lit/tests/Inputs/shtest-shell/diff-encodings.txt

+# Check that diff falls back to binary mode if it cannot decode a file.
+
+# RUN: diff -u diff-in.bin diff-in.bin
+# RUN: diff -u diff-in.utf16 diff-in.bin && false || true
+# RUN: diff -u diff-in.utf8 diff-in.bin && false || true
+# RUN: diff -u diff-in.bin diff-in.utf8 && false || true
+
+# Fail so lit will print output.
+# RUN: false

llvm/utils/lit/tests/Inputs/shtest-shell/diff-in.utf8

+foo
+bar
+baz

llvm/utils/lit/tests/max-failures.py

 # Check the behavior of --max-failures option.
 #
 # RUN: not %{lit} -j 1 -v %{inputs}/max-failures > %t.out
 # RUN: not %{lit} --max-failures=1 -j 1 -v %{inputs}/max-failures >> %t.out
 # RUN: not %{lit} --max-failures=2 -j 1 -v %{inputs}/max-failures >> %t.out
 # RUN: not %{lit} --max-failures=0 -j 1 -v %{inputs}/max-failures 2>> %t.out
 # RUN: FileCheck < %t.out %s
 #
 # END.
 
-# CHECK: Failing Tests (30)
+# CHECK: Failing Tests (31)
 # CHECK: Failing Tests (1)
 # CHECK: Failing Tests (2)
 # CHECK: error: argument --max-failures: requires positive integer, but found '0'

llvm/utils/lit/tests/shtest-shell.py

 # CHECK: FAIL: shtest-shell :: colon-error.txt
 # CHECK: *** TEST 'shtest-shell :: colon-error.txt' FAILED ***
 # CHECK: $ ":"
 # CHECK: # command stderr:
 # CHECK: Unsupported: ':' cannot be part of a pipeline
 # CHECK: error: command failed with exit status: 127
 # CHECK: ***
 
+
+# CHECK: FAIL: shtest-shell :: diff-encodings.txt
+# CHECK: *** TEST 'shtest-shell :: diff-encodings.txt' FAILED ***
+
+# CHECK: $ "diff" "-u" "diff-in.bin" "diff-in.bin"
+# CHECK-NOT: error
+
+# CHECK: $ "diff" "-u" "diff-in.utf16" "diff-in.bin"
+# CHECK: # command output:
+# CHECK-NEXT: ---
+# CHECK-NEXT: +++
+# CHECK-NEXT: @@
+# CHECK-NEXT: {{^ .f.o.o.$}}
+# CHECK-NEXT: {{^-.b.a.r.$}}
+# CHECK-NEXT: {{^\+.b.a.r..}}
+# CHECK-NEXT: {{^ .b.a.z.$}}
+# CHECK: error: command failed with exit status: 1
+# CHECK: $ "true"
+
+# CHECK: $ "diff" "-u" "diff-in.utf8" "diff-in.bin"
+# CHECK: # command output:
+# CHECK-NEXT: ---
+# CHECK-NEXT: +++
+# CHECK-NEXT: @@
+# CHECK-NEXT: -foo
+# CHECK-NEXT: -bar
+# CHECK-NEXT: -baz
+# CHECK-NEXT: {{^\+.f.o.o.$}}
+# CHECK-NEXT: {{^\+.b.a.r..}}
+# CHECK-NEXT: {{^\+.b.a.z.$}}
+# CHECK: error: command failed with exit status: 1
+# CHECK: $ "true"
+
+# CHECK: $ "diff" "-u" "diff-in.bin" "diff-in.utf8"
+# CHECK: # command output:
+# CHECK-NEXT: ---
+# CHECK-NEXT: +++
+# CHECK-NEXT: @@
+# CHECK-NEXT: {{^\-.f.o.o.$}}
+# CHECK-NEXT: {{^\-.b.a.r..}}
+# CHECK-NEXT: {{^\-.b.a.z.$}}
+# CHECK-NEXT: +foo
+# CHECK-NEXT: +bar
+# CHECK-NEXT: +baz
+# CHECK: error: command failed with exit status: 1
+# CHECK: $ "true"
+
+# CHECK: $ "false"
+
+# CHECK: ***
+
+
 # CHECK: FAIL: shtest-shell :: diff-error-0.txt
 # CHECK: *** TEST 'shtest-shell :: diff-error-0.txt' FAILED ***
 # CHECK: $ "diff" "diff-error-0.txt" "diff-error-0.txt"
 # CHECK: # command stderr:
 # CHECK: Unsupported: 'diff' cannot be part of a pipeline
 # CHECK: error: command failed with exit status: 127
 # CHECK: ***
 

 # CHECK: *** TEST 'shtest-shell :: rm-error-3.txt' FAILED ***
 # CHECK: Exit Code: 1
 # CHECK: ***
 
 # CHECK: PASS: shtest-shell :: rm-unicode-0.txt
 # CHECK: PASS: shtest-shell :: sequencing-0.txt
 # CHECK: XFAIL: shtest-shell :: sequencing-1.txt
 # CHECK: PASS: shtest-shell :: valid-shell.txt
-# CHECK: Failing Tests (30)
+# CHECK: Failing Tests (31)