[lit] Implement 'cat' command for internal shell

[chamalsl]

Summary

LLVM Bug Id : 36449

[rnk]

With the pipelining restriction, this doesn't seem very useful. Typically this is used to send outputs to FileCheck, although we could rewrite most of those uses to stdin redirects (<).

utils/lit/lit/TestRunner.py

585 ↗

(On Diff #135016)

Rather than building up a potentially large string in Python memory, I'd prefer to just say stdout.write(fileToCat.read()). It's probably clearer, too.

[chamalsl]

It is bit difficult to implement a pipelined version of 'cat' command, without some restructuring of _executeShCmd method in TestRunner.py. So is it ok if I keep the non-pipelined version?

Did the change to use stdout.write(fileToCat.read()) instead of string concatenation.

[chamalsl]

Fixed 2 more issues.

1. Print output of 'cat' command to command output when output is not redirected.
2. TestRunner.py crashed when a non existent file is passed to 'cat' command. Fixed this bug and added a test case.

[chamalsl]

Implemented 'cat' command which supports piped output.

[rnk]

Neat!

utils/lit/lit/TestRunner.py

785 ↗

(On Diff #135809)

Aside from the kAvoidDevNull and kIsWindows constants, this file mostly uses foo_bar underscore style. Please match that style. is_builtin_cmd seems consistent.

813 ↗

(On Diff #135809)

You need to move the logic below that treats args as a list above this call to quote_windows_command, which flattens it into an appropriately quoted string on Windows.

816 ↗

(On Diff #135809)

You don't need tempArgs and tempExecutable.

817 ↗

(On Diff #135809)

Move this logic out of the try / except.

utils/lit/lit/builtin_commands/cat.py

3

Please wrap this code in def main(): and use the if __name__ == '__main__': main() pattern. I know it's boiler-plate, but running Python code at global scope has gotchas and these things tend to grow and get copy-pasted.

[chamalsl]

Did required changes.

[rnk]

utils/lit/lit/TestRunner.py

748 ↗

(On Diff #136243)

I'd make this a set, since that matches the API we're using.

785 ↗

(On Diff #136243)

You can simplify all this code like this:

    is_builtin_cmd = args[0] in builtin_cmds
    if not is_builtin_cmd:
        # For paths relative to cwd, use the cwd of the shell environment.
        if args[0].startswith('.'):
...

Make all the code that sets executable and diagnoses missing commands only run when we're not dealing with a builtin command.

Leave executable as None, and you won't have to have the ternary below in the Popen call.

810 ↗

(On Diff #136243)

There's no need to test bools for equality with other bools. if is_builtin_cmd: is simpler, here and below.

811 ↗

(On Diff #136243)

This should be indented just 4 spaces

[chamalsl]

Did requested changes.

[rnk]

lgtm!

[chamalsl]

This comment was removed by chamalsl.

utils/lit/lit/TestRunner.py

816 ↗

(On Diff #135809)

Did the change. But is it OK to modify original 'args' list instead of a temporary list?
Can there be another place that needs the original 'args' list?

[chamalsl]

I do not have commit access.
Can you please commit this change-set.

[rnk]

I applied this locally and found two issues in the LLVM test suite. First, we have to open files in binary mode. That's easy to fix:

diff --git a/llvm/utils/lit/lit/builtin_commands/cat.py b/llvm/utils/lit/lit/builtin_commands/cat.py
index ef239612555..b0bbd369552 100644
--- a/llvm/utils/lit/lit/builtin_commands/cat.py
+++ b/llvm/utils/lit/lit/builtin_commands/cat.py
@@ -10,7 +10,7 @@ def main(argv):

     for filename in filenames:
         try:
-            fileToCat = open(filename,"r")
+            fileToCat = open(filename, "rb")
             sys.stdout.write(fileToCat.read())
             sys.stdout.flush()
             fileToCat.close()

Second, we actually use the -v flag for cat in llvm/test/Object/archive-format.test. It looks like:

If an archive has an object with no symbols, the linker and some other
tools on some versions of Solaris will abort operations if there is no
symbol table.  Create such an object, put it into an archive, and check to
see that there is an empty symbol table.
RUN: mkdir -p %t
RUN: yaml2obj %S/Inputs/solaris-nosymbols.yaml > %t/foo.o
RUN: llvm-ar rs %t/foo.a %t/foo.o
RUN: cat -v %t/foo.a | FileCheck -strict-whitespace --check-prefix=SOLARIS %s
SOLARIS:      !<arch>
SOLARIS-NEXT: /               0           0     0     0       8         `
SOLARIS-NEXT: ^@^@^@^@^@^@^@^@foo.o/

We could mark the test REQUIRES: shell, but our goal here is to reduce those, not increase them.

[chamalsl]

Did requested changes.
A newly added test requires binary file cat_nonprinting.bin.
Please advise how I can upload a binary file to this site?

[chamalsl]

I did the requested changes.
Are these modifications OK?

[rnk]

lgtm

The nonprinting character test fails because I don't have the nonprinting character file. If you send it to me over email, I can commit it.

[chamalsl]

This is a speculative fix for test failiure on ArchLinux , since I am unable to recreate the exact python environment on my Ubuntu machine. According to email sent by Alex Bradbury, on ArchLinux default python version is set to Python 3. But lit.py is run using /usr/bin/python2.7.

So I used 'sys.executable' instead of 'python' to execute built-in commands. So I am hoping 'sys.executable' will point to python2.7 since lit.py is run using python2.7.

[asb]

Could you please post the change as a separate revision, as a diff vs what was committed?

I think the correct fix would be to alter the code so it works in both Python 2 and Python 3, given that lit is compatible with Python 3 otherwise.

Should this builtin be implemented so that stdout is re-opened as binary? Won't undesired CRLF translation happen on Windows with the current implementation? (Honest question - I could be wrong!)

[chamalsl]

Modified cat.py file to work with python3 and python2.

Alex, I could not understand your question regarding CRLF on windows.
Please check whether new modifications prevent the issue you mentioned.

[chamalsl]

Alex, Now I understand the CRLF on Windows issue you raised. I will provide a fix for it.
Thanks.

[asb]

Could you please make a new review with your proposed fix(es) so we can continue discussion there? It's much easier to keep track of things that way. Typically one phabricator review == one commit.

[rnk]

+1 to new review and +1 to reopening stdout in binary to deal with the Py3k Unicode dumpster fire.

utils/lit/lit/builtin_commands/cat.py

 import getopt
 import sys
 try:
     from StringIO import StringIO
 except ImportError:
     from io import StringIO
 
 def convertToCaretAndMNotation(data):
    newdata = StringIO()
    for char in data:
        intval = ord(char)
for option, value in options:
             show_nonprinting = True;
 
     for filename in filenames:
         try:
             fileToCat = open(filename,"rb")
             contents = fileToCat.read()
             if show_nonprinting:
                 contents = convertToCaretAndMNotation(contents)
-            sys.stdout.write(contents)
+            sys.stdout.write(contents.decode("utf-8"))
             sys.stdout.flush()
             fileToCat.close()
         except IOError as error:
             sys.stderr.write(str(error))
             sys.exit(1)
 
 if __name__ == "__main__":
     main(sys.argv)