This is an archive of the discontinued LLVM Phabricator instance.

[lit] Implement 'cat' command for internal shell
ClosedPublic

Authored by chamalsl on Feb 20 2018, 1:07 AM.

Diff Detail

Event Timeline

chamalsl created this revision.Feb 20 2018, 1:07 AM
chamalsl edited the summary of this revision. (Show Details)Feb 20 2018, 1:08 AM
chamalsl set the repository for this revision to rL LLVM.
rnk added a comment.Feb 20 2018, 8:03 AM

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 updated this revision to Diff 135190.Feb 20 2018, 7:32 PM

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 updated this revision to Diff 135200.Feb 20 2018, 11:18 PM
chamalsl marked an inline comment as done.
chamalsl set the repository for this revision to rL LLVM.

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 updated this revision to Diff 135809.Feb 24 2018, 8:07 PM
chamalsl retitled this revision from [lit] Implement non-pipelined 'cat' command for internal shell to [lit] Implement 'cat' command for internal shell.

Implemented 'cat' command which supports piped output.

rnk added a comment.Feb 27 2018, 10:49 AM

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 updated this revision to Diff 136243.Feb 28 2018, 1:15 AM

Did required changes.

rnk added inline comments.Feb 28 2018, 2:18 PM
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 updated this revision to Diff 136655.Mar 1 2018, 5:40 PM
chamalsl marked 5 inline comments as done.

Did requested changes.

rnk accepted this revision.Mar 1 2018, 5:46 PM

lgtm!

This revision is now accepted and ready to land.Mar 1 2018, 5:46 PM
chamalsl marked 4 inline comments as done.Mar 9 2018, 3:32 AM
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?

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

rnk requested changes to this revision.Mar 9 2018, 3:15 PM

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.

This revision now requires changes to proceed.Mar 9 2018, 3:15 PM
chamalsl updated this revision to Diff 137973.Mar 11 2018, 9:16 PM

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?

I did the requested changes.
Are these modifications OK?

rnk accepted this revision.Mar 23 2018, 2:12 PM

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.

This revision is now accepted and ready to land.Mar 23 2018, 2:12 PM
This revision was automatically updated to reflect the committed changes.
chamalsl updated this revision to Diff 140033.Mar 27 2018, 7:42 PM

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 added a subscriber: asb.Mar 27 2018, 11:58 PM

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 updated this revision to Diff 140050.EditedMar 28 2018, 1:46 AM

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.

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

asb added a comment.Mar 28 2018, 8:11 AM

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 added a comment.Mar 28 2018, 11:24 AM

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