Add text file support for z/OS back in

[perry-ca]

Support for auto conversion of text files was added to cat.py in #90128. That was accidently removed in #204711 while removing some old win32 code. This adds the z/OS support back in.

[llvmorg-github-actions]

@llvm/pr-subscribers-testing-tools

Author: Sean Perry (perry-ca)

Changes

Support for auto conversion of text files was added to cat.py in #90128. That was accidently removed in #204711 while removing some old win32 code. This adds the z/OS support back in.

---

Full diff: https://github.com/llvm/llvm-project/pull/209259.diff

1 Files Affected:

• (modified) llvm/utils/lit/lit/builtin_commands/cat.py (+20-7)

diff --git a/llvm/utils/lit/lit/builtin_commands/cat.py b/llvm/utils/lit/lit/builtin_commands/cat.py
index fc7d1229bc674..826cf94b1cb8e 100644
--- a/llvm/utils/lit/lit/builtin_commands/cat.py
+++ b/llvm/utils/lit/lit/builtin_commands/cat.py
@@ -65,18 +65,31 @@ def run(argv, stdin, stdout, stderr, cwd):
 
     for filename in filenames:
         path = filename
-        contents = None
         if not os.path.isabs(path):
             path = os.path.join(cwd, path)
+
+        contents = None
+        is_text = False
         try:
-            with open(path, "rb") as fileToCat:
-                contents = fileToCat.read()
-        except IOError as error:
-            error.filename = filename
-            stderr.write(str(error).encode())
-            return 1
+            fileToCat = open(filename, "r")
+            contents = fileToCat.read()
+            is_text = True
+        except:
+            pass
+
+        if contents is None:
+            try:
+                with open(path, "rb") as fileToCat:
+                    contents = fileToCat.read()
+            except IOError as error:
+                error.filename = filename
+                stderr.write(str(error).encode())
+                return 1
+
         if show_nonprinting:
             contents = convertToCaretAndMNotation(contents)
+        elif is_text:
+            contents = contents.encode()
         stdout.write(contents)
     return 0
 

[boomanaiden154]

I'm guessing there might be non-negligible overhead from the decoding/encoding that we now do by default. I'm wondering if it's better to special case this just for platform.system() == "OS/390".

[boomanaiden154]

Why can we not use a context manager (with) here?

[perry-ca]

The code was like this since May 2024. I don't see this adding any new overhead. But I can add that in.

I should change filename to path too.

[github-actions]

🪟 Windows x64 Test Results

• 138506 tests passed
• 3536 tests skipped

✅ The build succeeded and all tests passed.

[ilovepi]

LGTM.

[boomanaiden154]

LGTM minus nit

[boomanaiden154]

I think we can avoid is_text entirely if we just do this where we currently set is_text = True.

[perry-ca]

I'll have to dig out my notes, but I believe the --show-nonprinting option fails if you do that. It looks like this would double encode the text (once at the file read and then again inside of the convert function). That would produce gibberish.

Still good?

[boomanaiden154]

It's unclear to me how this proposal would cause a semantic difference from what the code does now. When we set is_text, we're reading in text node, so .read() does not read an encoded form and we still only encode once.