Add "/dev/tty" as a special file name to lit

[ygao]

Summary

This patch adds "/dev/tty" as a special output file name for lit tests.
If a lit test has a RUN line that includes a redirection to "/dev/tty",
the redirection goes to the special device file corresponding to the
console. It is /dev/tty on UNIX-like systems and "CON" on Windows.

This patch is needed to implement a test like PR25717 (caused by
the size limit of the Windows system call WriteConsole() prior to
Windows 8) where the test only breaks when outputing to the
console and won't fail if using a pipe.

[• rafael]

/dev/tty would be appropriate, no?

When using this a test will print to the console even running check-llvm, no? That is quite unfortunate. Maybe the test should go to the test-suite instead of check-llvm?

[delcypher]

In D16000#325751, @rafael wrote:

/dev/tty would be appropriate, no?

+1 for /dev/tty

[delcypher]

@ygao

This approach looks flawed to me because you have an implicit assumption that does not always hold. It looks like you're assuming that setting r[2] to None (which will happen with your change when the file being redirect to is name /dev/console) will mean you always write to a console.

This assumption is incorrect because when you set stdout/stderr/stdin in the Popen constructor to None it means that the relevant parameter will be inherited from the parent which may not be connected to a console. Put another way your patch will work if lit is run from a console without redirecting its stdout/stderr but if you run lit and redirect its stdout/stderr to files this patch won't do want you want it to.

Is there a file you open under Windows that will point to a console (a quick google suggests 'CON' might be)? If so I think would be better just to do

if r[2] is None:
   if r[0] == '/dev/tty' and os.name == 'nt':
     # Provide direct access to the console to simulate /dev/tty under Windows
     r[2] = open('special_console_file_under_windows', 'r' if index == 0 else 'w')

This is more explicit and the intention is much more clear. The intention is not clear at all from your implementation.

[ygao]

@delcypher,

You are right, and I like your idea of using a special device file on Windows for accessing the console. Thanks!

[• rafael]

The change itself is fine by me, but:

• It should get a review from a windows expert.
• We should agree that we are fine with having a test in check-all that writes to the console (or is the test-suite using lit these days?)

[aaron.ballman]

utils/lit/lit/TestRunner.py

251

Why CON: instead of CON?

[ygao]

We should agree that we are fine with having a test in check-all that writes to
the console (or is the test-suite using lit these days?)

I am not familiar with test-suite and so would appreciate some opinion from an expert in this area regarding
the pros and cons of test-suite vs check-clang. I assume both are run regularly on buildbots.

Looking at the existing test files in test-suite, it seems that it uses cmake or makefile, but not lit.

Here's my concern for moving this test into test-suite: I do not see how to specify a command-line for only
one test. This particular test still needs to output to the console, but modifying the root makefile would risk
making all tests output to the console. On the other hand, I agree that having a test in check-all that writes to
the console does look ugly.

utils/lit/lit/TestRunner.py

251

The trailing colon makes it look like a device, but there is no practical advantage in doing so. I'll remove the colon.

[• rafael]

Here's my concern for moving this test into test-suite: I do not see how to specify a command-line for only
one test. This particular test still needs to output to the console, but modifying the root makefile would risk
making all tests output to the console. On the other hand, I agree that having a test in check-all that writes to
the console does look ugly.

I wonder if we could add a 'REQUIRES: console" and have that be enable
with a command line option. That way it can be off by default and
still enabled in a bot.

Cheers,
Rafael

[ygao]

I wonder if we could add a 'REQUIRES: console" and have that be enable
with a command line option. That way it can be off by default and
still enabled in a bot.

Sure. I can add a lit option like "--enable-console" to control the "console" feature.
I assume that I do not need to add any new check-xx targets to the makefile for
the new feature, but how do I enable this feature on the buildbots?

[delcypher]

@ygao I'm not really a fan of the way you've gone about this.

• There is no need to add a new command line flag for this.
• The "--enable-console" option doesn't have any influence on the code you added to TestRunner.py to write to the console which is misleading.

I don't think you should be modify lit in a special way for this "feature" you want to support. Instead it should be part of your test suite configuration. That way we aren't modifying lit (which supposed to be general purpose tool) for your specific use case.

For example you could have something like this in your lit.cfg file which will only add the "console" feature when passed the right parameter on the command line.

runConsoleTests = int(lit_config.params.get('enable_console','0'))
if runConsoleTests > 0:
  config.available_features.add('console')

so when invoking lit if you want the console feature you would run it like this

$ lit --param enable_console=1 path/to/test/suite

[ygao]

@delcypher,

Thanks a lot! The lit parameter "-param enable_console=1" works for me and it is much cleaner than what I had earlier. I took out those changes.
The change to lit.cfg to use the lit parameter is relevant to the cfe project and I am updating the related diff D15705.

[aaron.ballman]

LGTM in terms of doing a reasonable thing on Windows.

[• rafael]

LGTM

[delcypher]

In D16000#334339, @ygao wrote:

@delcypher,

Thanks a lot! The lit parameter "-param enable_console=1" works for me and it is much cleaner than what I had earlier. I took out those changes.
The change to lit.cfg to use the lit parameter is relevant to the cfe project and I am updating the related diff D15705.

Great. Thanks for taking to time to implement my suggestions.

LGTM.

[delcypher]

@ygao

LGTM.

Oh I forgot to say you should update the summary as it is out of date. It doesn't matter if you commit manually as you can fix it there but if you're using arcanist you could end up with the old summary in your commit message.

[ygao]

Committed rL258898. Thanks!

utils/lit/lit/TestRunner.py

for i,j in enumerate(cmd.commands):
             elif r == (2,):
                 if index != 2:
                     raise InternalShellError(j,"Unsupported redirect on stdout")
                 result = subprocess.PIPE
             else:
                 if r[2] is None:
                     if kAvoidDevNull and r[0] == '/dev/null':
                         r[2] = tempfile.TemporaryFile(mode=r[1])
+                    elif kIsWindows and r[0] == '/dev/tty':
+                        # Simulate /dev/tty on Windows.
+                        # "CON:" is a special filename for the console.
+                        r[2] = open("CON:", r[1])
                     else:
                         # Make sure relative paths are relative to the cwd.
                         redir_filename = os.path.join(cmd_shenv.cwd, r[0])
                         r[2] = open(redir_filename, r[1])
                     # Workaround a Win32 and/or subprocess bug when appending.
                     #
                     # FIXME: Actually, this is probably an instance of PR6753.
                     if r[1] == 'a':