[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

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

828

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

853

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.

857

You don't need tempArgs and tempExecutable.

858

Move this logic out of the try / except.

utils/lit/lit/builtin_commands/cat.py

3 ↗

(On Diff #135809)

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

793

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

828

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.

849

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

850

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

857

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/TestRunner.py

for path in args:
                     os.chmod(path,
                              stat.S_IMODE(os.stat(path).st_mode) | stat.S_IWRITE)
                 os.remove(path)
         except OSError as err:
             stderr.write("Error: 'rm' command failed, %s" % str(err))
             exitCode = 1
     return ShellCommandResult(cmd, "", stderr.getvalue(), exitCode, False)
 
+def executeBuiltinCat(cmd, cmd_shenv):
+    """
+       executeBuiltinCat - Prints contents of a file or merge and print
+       contents of several files.
+    """
+    opened_files = []
+    stdin, stdout, stderr = processRedirects(cmd, subprocess.PIPE, cmd_shenv,
+                                             opened_files)
+    if stdin != subprocess.PIPE or stderr != subprocess.PIPE:
+        raise InternalShellError(
+                cmd, "stdin and stderr redirects not supported for cat")
+
+    isRedirected = True
+    if stdout == subprocess.PIPE:
+        stdout = StringIO()
+        isRedirected = False
+
+    args = expand_glob_expressions(cmd.args, cmd_shenv.cwd)[1:]
+    exitCode = 0
+    errMsg = ""
+    try:
+        for filename in args:
+            fileToCat = open(filename,"r")
+            stdout.write(fileToCat.read())
+            fileToCat.close();
+    except IOError as error:
+        exitCode = 1
+        errMsg = str(error);
+
+    for (name, mode, f, path) in opened_files:
+        f.close()
+
+    contents = "";
+    if (isRedirected == False):
+        contents = stdout.getvalue();
+    return ShellCommandResult(cmd, contents, errMsg, exitCode, False)
+
 def processRedirects(cmd, stdin_source, cmd_shenv, opened_files):
     """Return the standard fds for cmd after applying redirects
 
     Returns the three standard file descriptors for the new child process.  Each
     fd may be an open, writable file object or a sentinel value from the
     subprocess module.
     """
 
def _executeShCmd(cmd, shenv, results, timeoutHelper):
     if cmd.commands[0].args[0] == 'rm':
         if len(cmd.commands) != 1:
             raise InternalShellError(cmd.commands[0], "Unsupported: 'rm' "
                                      "cannot be part of a pipeline")
         cmdResult = executeBuiltinRm(cmd.commands[0], shenv)
         results.append(cmdResult)
         return cmdResult.exitCode
 
+    if cmd.commands[0].args[0] == 'cat':
+        if len(cmd.commands) != 1:
+            raise InternalShellError(cmd.commands[0], "Unsupported: 'cat' "
+                                     "cannot be part of a pipeline")
+        cmdResult = executeBuiltinCat(cmd.commands[0], shenv)
+        results.append(cmdResult)
+        return cmdResult.exitCode
+
     procs = []
     default_stdin = subprocess.PIPE
     stderrTempFiles = []
     opened_files = []
     named_temp_files = []
     # To avoid deadlock, we use a single stderr stream for piped
     # output. This is null until we have seen some output using
     # stderr.
     for i,j in enumerate(cmd.commands):
         # Reference the global environment by default.
         cmd_shenv = shenv
         if j.args[0] == 'env':
             # Create a copy of the global environment and modify it for this one
             # command. There might be multiple envs in a pipeline:
for i,j in enumerate(cmd.commands):
             # FIXME: This is slow, but so is deadlock.
             if stderr == subprocess.PIPE and j != cmd.commands[-1]:
                 stderr = tempfile.TemporaryFile(mode='w+b')
                 stderrTempFiles.append((i, stderr))
 
         # Resolve the executable path ourselves.
         args = list(j.args)
         executable = None
         # For paths relative to cwd, use the cwd of the shell environment.
         if args[0].startswith('.'):
             exe_in_cwd = os.path.join(cmd_shenv.cwd, args[0])
             if os.path.isfile(exe_in_cwd):
                 executable = exe_in_cwd
         if not executable:
             executable = lit.util.which(args[0], cmd_shenv.env['PATH'])
         if not executable:
             raise InternalShellError(j, '%r: command not found' % j.args[0])
 
         # Replace uses of /dev/null with temporary files.
         if kAvoidDevNull:
             for i,arg in enumerate(args):
                 if arg == "/dev/null":
                     f = tempfile.NamedTemporaryFile(delete=False)
                     f.close()
                     named_temp_files.append(f.name)
                     args[i] = f.name
 
         # Expand all glob expressions
         args = expand_glob_expressions(args, cmd_shenv.cwd)
 
         # On Windows, do our own command line quoting for better compatibility
         # with some core utility distributions.
         if kIsWindows:
             args = quote_windows_command(args)
 
         try:
             procs.append(subprocess.Popen(args, cwd=cmd_shenv.cwd,
                                           executable = executable,
                                           stdin = stdin,
                                           stdout = stdout,
                                           stderr = stderr,
                                           env = cmd_shenv.env,
                                           close_fds = kUseCloseFDs))
             # Let the helper know about this process
             timeoutHelper.addProcess(procs[-1])
         except OSError as e:
             raise InternalShellError(j, 'Could not create process ({}) due to {}'.format(executable, e))

utils/lit/tests/Inputs/shtest-shell/cat-error-0.txt

+# Check error on a unsupported cat (cannot be part of a pipeline).
+#
+# RUN: cat cat-error-0.txt | echo Output

utils/lit/tests/Inputs/shtest-shell/cat-error-1.txt

+# Check error on a unsupported cat (Unable to find input file).
+#
+# RUN: cat temp1.txt

utils/lit/tests/Inputs/shtest-shell/valid-shell.txt

 # RUN: diff -u %t.stdout %t1.stdout
 # RUN: echo "hello-2" > %t1.stdout
 # RUN: diff %t.stdout %t1.stdout || true
 #
 # RUN: mkdir -p %T/dir1 %T/dir2
 # RUN: cd %T/dir1 && echo "hello" > temp1.txt
 # RUN: cd %T/dir2 && echo "hello" > temp2.txt
 # RUN: diff temp2.txt ../dir1/temp1.txt
+#
+# Check cat command with single file.
+#
+# RUN: rm -rf %T/testCat
+# RUN: mkdir -p %T/testCat
+# RUN: echo "abcdefgh" > %T/testCat/temp.write
+# RUN: cat %T/testCat/temp.write > %T/testCat/tempcat.write
+# RUN: "%{python}" %S/check_path.py file %T/testCat/tempcat.write > %T/testCat/path.out
+# RUN: FileCheck --check-prefix=FILE-EXISTS < %T/testCat/path.out %s
+# RUN: FileCheck --check-prefix=CAT_OUTPUT < %T/testCat/tempcat.write %s
+# FILE-EXISTS: True
+# CAT_OUTPUT: abcdefgh
+#
+# Check cat command with multiple files.
+#
+# RUN: rm -rf %T/testCat
+# RUN: mkdir -p %T/testCat
+# RUN: echo "abcdefgh" > %T/testCat/temp1.write
+# RUN: echo "efghijkl" > %T/testCat/temp2.write
+# RUN: echo "mnopqrst" > %T/testCat/temp3.write
+# RUN: cat %T/testCat/temp1.write %T/testCat/temp2.write %T/testCat/temp3.write > %T/testCat/tempmulticat.write
+# RUN: "%{python}" %S/check_path.py file %T/testCat/tempmulticat.write > %T/testCat/path.out
+# RUN: FileCheck --check-prefix=MULTI-FILE-EXISTS < %T/testCat/path.out %s
+# RUN: FileCheck --check-prefix=MULTI-CAT_OUTPUT < %T/testCat/tempmulticat.write %s
+# MULTI-FILE-EXISTS: True
+# MULTI-CAT_OUTPUT: abcdefgh
+# MULTI-CAT_OUTPUT-NEXT: efghijkl
+# MULTI-CAT_OUTPUT-NEXT: mnopqrst

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 (24)
+# CHECK: Failing Tests (26)
 # CHECK: Failing Tests (1)
 # CHECK: Failing Tests (2)
 # CHECK: error: Setting --max-failures to 0 does not have any effect.

utils/lit/tests/shtest-shell.py

 # Check the internal shell handling component of the ShTest format.
 #
 # RUN: not %{lit} -j 1 -v %{inputs}/shtest-shell > %t.out
 # FIXME: Temporarily dump test output so we can debug failing tests on
 # buildbots.
 # RUN: cat %t.out
 # RUN: FileCheck --input-file %t.out %s
 #
 # END.
 
 # CHECK: -- Testing:
 
+# CHECK: FAIL: shtest-shell :: cat-error-0.txt
+# CHECK: *** TEST 'shtest-shell :: cat-error-0.txt' FAILED ***
+# CHECK: $ "cat" "cat-error-0.txt"
+# CHECK: # command stderr:
+# CHECK: Unsupported: 'cat' cannot be part of a pipeline
+# CHECK: error: command failed with exit status: 127
+# CHECK: ***
+
+# CHECK: FAIL: shtest-shell :: cat-error-1.txt
+# CHECK: *** TEST 'shtest-shell :: cat-error-1.txt' FAILED ***
+# CHECK: $ "cat" "temp1.txt"
+# CHECK: # command stderr:
+# CHECK: [Errno 2] No such file or directory: 'temp1.txt'
+# CHECK: error: command failed with exit status: 1
+# 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: FAIL: shtest-shell :: rm-error-3.txt
 # CHECK: *** TEST 'shtest-shell :: rm-error-3.txt' FAILED ***
 # CHECK: Exit Code: 1
 # CHECK: ***
 
 # CHECK: PASS: shtest-shell :: sequencing-0.txt
 # CHECK: XFAIL: shtest-shell :: sequencing-1.txt
 # CHECK: PASS: shtest-shell :: valid-shell.txt
-# CHECK: Failing Tests (24)
+# CHECK: Failing Tests (26)