[lit] Implement `not` as a builtin in the Lit internal shell

[ldionne]

Summary

Instead of using LLVM's not.cpp, implement not in the internal Lit
shell as a builtin. This allows creating test suites that use the
internal Lit shell without depending on llvm/utils/not, which in
turn depends on the LLVM support libraries.

This work is done in the context of trying to move the libc++ test
suite to the internal Lit shell -- the libc++ test suite currently
doesn't depend on the LLVM support libraries, and it would be ideal
to keep it that way.

[ldionne]

I'm not entirely satisfied with this patch -- if folks think we should implement not as an in-process builtin, let me know and I can look into that. It's also annoying that we're treating not differently from the other out-of-process builtins after this patch, but I couldn't find another way to do it.

[delcypher]

I'm not entirely satisfied with this patch -- if folks think we should implement not as an in-process builtin, let me know and I can look into that.

I guess the advantage here would be avoiding spawning another process? A downside might mean it's harder to test the builtin implementation unless we write the implementation to be testable. I hadn't looked at the builtin code in lit before. It's interesting that it already tries to parse the --crash arg to not.

Shouldn't we have some tests to test your implementation of the not builtin?

llvm/utils/lit/lit/builtin_commands/not.py

65

You might want to use sys.exit() here instead of exit(). exit() is meant for use in interactive shells.

[delcypher]

llvm/utils/lit/lit/TestRunner.py

657

Nit: I don't like having args.pop(0) + ".py" here. I think it makes the code harder to read due to args.pop(0) both returning a value but also having a side-effect.

llvm/utils/lit/lit/builtin_commands/not.py

34

Are you missing a \n here?

[ldionne]

In D89493#2334002, @delcypher wrote:

I'm not entirely satisfied with this patch -- if folks think we should implement not as an in-process builtin, let me know and I can look into that.

I guess the advantage here would be avoiding spawning another process? A downside might mean it's harder to test the builtin implementation unless we write the implementation to be testable. I hadn't looked at the builtin code in lit before. It's interesting that it already tries to parse the --crash arg to not.

Shouldn't we have some tests to test your implementation of the not builtin?

The implementation is already tested -- the shtest-not tests are running in the internal shell, and are testing that very implementation. As it stands, it mostly means that the original not.cpp in LLVM's utilities is the thing that's not tested anymore, I think.

[ldionne]

Address Dan's comments

[ldionne]

Rebase onto master.

[ldionne]

Rebase onto main

[delcypher]

LGTM other than my nits.

I'm adding @yln as reviewer just in case he has thoughts on this change.

llvm/utils/lit/lit/builtin_commands/not.py

16

Minor nit: echo probably isn't a good example here as it's usually a shell builtin so it's not something that this script can actually invoke.

33

Do we need to call sys.stderr.flush() here? I recently ran into a problem with sys.stdout where I needed to do this to ensure it gets written out but I'm not sure about stderr.

[dexonsmith]

llvm/utils/lit/lit/builtin_commands/not.py

33

I don't know for sure about python, but usually stdout is buffered and stderr isn't.

[ldionne]

Superseded by https://reviews.llvm.org/D98859

llvm/utils/lit/lit/TestRunner.py

def _executeShCmd(cmd, shenv, results, timeoutHelper):
     assert isinstance(cmd, ShUtil.Pipeline)
 
     procs = []
     proc_not_counts = []
     default_stdin = subprocess.PIPE
     stderrTempFiles = []
     opened_files = []
     named_temp_files = []
-    builtin_commands = set(['cat', 'diff'])
+    builtin_commands = set(['cat', 'diff', 'not'])
     builtin_commands_dir = os.path.join(os.path.dirname(os.path.abspath(__file__)), "builtin_commands")
     inproc_builtins = {'cd': executeBuiltinCd,
                        'export': executeBuiltinExport,
                        'echo': executeBuiltinEcho,
                        'mkdir': executeBuiltinMkdir,
                        'rm': executeBuiltinRm,
                        ':': executeBuiltinColon}
     # To avoid deadlock, we use a single stderr stream for piped
for i,j in enumerate(cmd.commands):
                 #   env FOO=1 %{another_env_plus_cmd} | FileCheck %s
                 if cmd_shenv is shenv:
                     cmd_shenv = ShellEnvironment(shenv.cwd, shenv.env)
                 args = updateEnv(cmd_shenv, args)
                 if not args:
                     raise InternalShellError(j, "Error: 'env' requires a"
                                                 " subcommand")
             elif args[0] == 'not':
-                not_args.append(args.pop(0))
+                cmd_shenv.env['PYTHONPATH'] = os.path.dirname(os.path.abspath(__file__))
+                not_args.append(sys.executable)
+                not_args.append(os.path.join(builtin_commands_dir, args[0] + ".py"))
+                args.pop(0)
                 not_count += 1
                 if args and args[0] == '--crash':
                     not_args.append(args.pop(0))
                     not_crash = True
                 if not args:
                     raise InternalShellError(j, "Error: 'not' requires a"
                                                 " subcommand")
             elif args[0] == '!':
def processLineToFixedPoint(ln):
         if processed != ln:
             raise ValueError("Recursive substitution of '%s' did not complete "
                              "in the provided recursion limit (%s)" % \
                              (origLine, recursion_limit))
 
         return processed
 
     process = processLine if recursion_limit is None else processLineToFixedPoint
-    
     return [unescape(process(ln)) for ln in script]
 
 
 class ParserKind(object):
     """
     An enumeration representing the style of an integrated test keyword or
     command.
 

llvm/utils/lit/lit/builtin_commands/not.py

+#===----------------------------------------------------------------------===##
+#
+# Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+# See https://llvm.org/LICENSE.txt for license information.
+# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+#
+#===----------------------------------------------------------------------===##
+
+import subprocess
+import sys
+
+"""
+Inverts the return code of a command.
+
+For example:
+    $ not.py echo hello
+    $ echo $?
+    1
+"""
+
+def find_program(prog):
+    """Find a program in a way similar to `which`.
+       Returns the path to the program if found, and None otherwise.
+    """
+    # Allow for import errors on distutils.spawn
+    found = None
+    try:
+        import distutils.spawn
+        found = distutils.spawn.find_executable(prog)
+    except:
+        pass
+    if found is None:
+        sys.stderr.write('Failed to find program {}\n'.format(prog))
+    return found
+
+
+def main(argv):
+    if len(argv) > 0 and argv[0] == '--crash':
+        crash = True
+        argv.pop(0)
+    else:
+        crash = False
+
+    if not argv: # No command provided
+        return 1
+
+    if find_program(argv[0]) is None:
+        return 1
+
+    result = subprocess.call(argv)
+
+    # The program crashed
+    if result < 0:
+        return 0 if crash else 1
+
+    # The program didn't crash, but we were expecting a crash
+    if crash:
+        return 1
+
+    # The program didn't crash, and we were not expecting a crash
+    return result == 0
+
+if __name__ == '__main__':
+    sys.exit(main(sys.argv[1:]))