[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

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

43

Extra logging?

63

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

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

43

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/builtin_commands/diff.py

 import difflib
 import functools
 import getopt
+import locale
 import os
 import sys
 
 class DiffFlags():
     def __init__(self):
         self.ignore_all_space = False
         self.ignore_space_change = False
         self.unified_diff = False
         self.recursive_diff = False
         self.strip_trailing_cr = False
 
 def getDirTree(path, basedir=""):
     # Tree is a tuple of form (dirname, child_trees).
     # An empty dir has child_trees = [], a file has child_trees = None.
     child_trees = []
     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(flags, 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(flags, filepaths)
-    else:
-        return compareTwoTextFiles(flags, filepaths, encoding)
-
-def compareTwoBinaryFiles(flags, 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(flags, filepaths, filelines,
+                                   locale.getpreferredencoding(False))
+            return compareTwoTextFiles(flags, filepaths, filelines, "utf-8")
+            return compareTwoBinaryFiles(flags, filepaths, filelines)
 
+def compareTwoBinaryFiles(flags, filepaths, filelines):
+    #sys.stderr.write("Trying as binary....\n")
     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())
-        diffs = [diff.decode() for diff in diffs]
+        diffs = [diff.decode(errors="replace") for diff in diffs]
     else:
         # python 2.7
         if flags.unified_diff:
             func = difflib.unified_diff
         else:
             func = difflib.context_diff
         diffs = func(filelines[0], filelines[1], filepaths[0], filepaths[1])
 
     for diff in diffs:
         sys.stdout.write(diff)
         exitCode = 1
     return exitCode
 
-def compareTwoTextFiles(flags, filepaths, encoding):
+def compareTwoTextFiles(flags, filepaths, filelines_bin, encoding):
+    #sys.stderr.write("Trying with encoding {}....\n".format(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 flags.strip_trailing_cr:
         f = compose2(lambda line: line.rstrip('\r'), f)

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 (27)
+# CHECK: Failing Tests (28)
 # CHECK: Failing Tests (1)
 # CHECK: Failing Tests (2)
 # CHECK: error: Option '--max-failures' requires positive integer

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-1.txt
 # CHECK: *** TEST 'shtest-shell :: diff-error-1.txt' FAILED ***
 # CHECK: $ "diff" "-B" "temp1.txt" "temp2.txt"
 # CHECK: # command stderr:
 # CHECK: Unsupported: 'diff': option -B not recognized
 # CHECK: error: command failed with exit status: 1
 # 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 (27)
+# CHECK: Failing Tests (28)