[libcxx] Use pseudo-terminal in LIT tests so color output from compiler error messages is captured.

[EricWF]

Summary

Change LibcxxTestFormat's execute_command method so it uses a pseudo-terminal when invoking the compiler commands. This means that clang will output color diagnostics and LIT will capture these diagnostics.

[• ddunbar]

But what if Lit isn't running in a device capable of color output? Won't
this add a bunch of ugly ASCII codes?

I think what tools like Ninja do is to also optionally strip the color
codes in that case (
https://github.com/martine/ninja/blob/master/src/util.cc#L384).

• Daniel

[EricWF]

But what if Lit isn't running in a device capable of color output? Won't
this add a bunch of ugly ASCII codes?

Wouldn't clang refrain from outputting color codes to begin with?
Do you know of any way to detect this?

I think what tools like Ninja do is to also optionally strip the color
codes in that case (
https://github.com/martine/ninja/blob/master/src/util.cc#L384).

Not much you can do about that. At least the colored output makes it one step further.

Thanks for the input.

[• ddunbar]

Bearing in mind this isn't a domain I know much about...

[EricWF]

Only use pseudo-terminal if sys.stdout.isatty() is true. Otherwise fall back to the old method.

[silvas]

FWIW, Ninja found that it wasn't tenable to use this approach. Some discussion here: https://github.com/martine/ninja/issues/174

Not sure if any of the concerns apply to lit.

[EricWF]

In D6010#9, @silvas wrote:

FWIW, Ninja found that it wasn't tenable to use this approach. Some discussion here: https://github.com/martine/ninja/issues/174

Not sure if any of the concerns apply to lit.

Their main concern was that providing a pseudo-terminal might confuse some commands like SVN it if tries to spin up an editor.
Since we know exactly what types of command will be run this isn't a concern.

Thanks for the input.

[• ddunbar]

Ok, so on the structure of the patch:

1. It would be nice if this functionality wasn't in the Libcxx test format,

but a utility that was part of lit proper.

2. This: --

+ out = LibcxxTestFormat._read_and_close_pty(out_pty)

+ err = LibcxxTestFormat._read_and_close_pty(err_pty)

is not safe, if a command writes enough output to stderr that the pipe is
full it will block, and then the system will be deadlocked since it will
never finish writing to stdout. Also, you probably shouldn't assume that
all data is returned in a single .read() call (even though that is likely
true). See notes in subprocess module, e.g.,
https://docs.python.org/2/library/subprocess.html#subprocess.Popen.communicate
.

• Daniel

[EricWF]

I'm abandoning this revision because I'm going to try and put this functionality in LIT proper. I have a working implementation that addresses @ddunbar's concerns that I will submit next week.

test/lit.cfg

 import sys
 import tempfile
 import time
+import pty
 
 import lit.Test
 import lit.formats

         self.cpp_flags = list(cpp_flags)
         self.ld_flags = list(ld_flags)
         self.exec_env = dict(exec_env)
-
-    def execute_command(self, command, in_dir=None):
+        if sys.stdout.isatty():
+            self._execute_command_helper = LibcxxTestFormat._execute_command_pty
+        else:
+            self._execute_command_helper = LibcxxTestFormat._execute_command_normal
+
+    @staticmethod
+    def _close_raw_fd(fd):
+        try:
+            os.close(fd)
+        except:
+            pass
+
+    @staticmethod
+    def _read_and_close_pty(p):
+        os.close(p[1])
+        with os.fdopen(p[0], 'r') as f:
+            out = ''
+            try:
+                out = f.read()
+            except IOError:
+                pass
+            return out
+
+    @staticmethod
+    def _execute_command_pty(command, in_dir=None):
+        try:
+            out_pty = tuple(pty.openpty())
+            err_pty = tuple(pty.openpty())
+            kwargs = {
+                'stdin' :subprocess.PIPE,
+                'stdout':out_pty[1],
+                'stderr':err_pty[1],
+                'close_fds':True,
+            }
+            if in_dir:
+                kwargs['cwd'] = in_dir
+            p = subprocess.Popen(command, **kwargs)
+            exitCode = p.wait()
+            out = LibcxxTestFormat._read_and_close_pty(out_pty)
+            err = LibcxxTestFormat._read_and_close_pty(err_pty)
+        except:
+            for fd in out_pty + err_pty:
+                LibcxxTestFormat._close_raw_fd(fd)
+            raise
+        return out,err,exitCode
+
+    @staticmethod
+    def _execute_command_normal(command, in_dir=None):
         kwargs = {
             'stdin' :subprocess.PIPE,
             'stdout':subprocess.PIPE,

         p = subprocess.Popen(command, **kwargs)
         out,err = p.communicate()
         exitCode = p.wait()
+        return out,err,exitCode
 
+    def execute_command(self, command, in_dir=None):
+        out,err,exitCode = self._execute_command_helper(command, in_dir)
         # Detect Ctrl-C in subprocess.
         if exitCode == -signal.SIGINT:
             raise KeyboardInterrupt
-
         return out, err, exitCode
 
     def execute(self, test, lit_config):