Page MenuHomePhabricator

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

Authored by ygao on Jan 8 2016, 11:08 AM.

Details

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.

Diff Detail

Event Timeline

ygao updated this revision to Diff 44339.Jan 8 2016, 11:08 AM
ygao retitled this revision from to Add "/dev/console" as a special file name to lit.
ygao updated this object.

/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?

/dev/tty would be appropriate, no?

+1 for /dev/tty

@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 updated this revision to Diff 45440.Jan 20 2016, 1:44 PM

@delcypher,

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

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 added inline comments.
utils/lit/lit/TestRunner.py
251

Why CON: instead of CON?

ygao added a comment.Jan 21 2016, 11:53 AM

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.

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 updated this revision to Diff 45763.Jan 22 2016, 4:09 PM

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?

@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 updated this revision to Diff 45785.Jan 22 2016, 6:12 PM
ygao marked 2 inline comments as done.

@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 edited edge metadata.Jan 25 2016, 6:30 AM

LGTM in terms of doing a reasonable thing on Windows.

rafael accepted this revision.Jan 26 2016, 8:53 AM
rafael added a reviewer: rafael.

LGTM

This revision is now accepted and ready to land.Jan 26 2016, 8:53 AM
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.

@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 retitled this revision from Add "/dev/console" as a special file name to lit to Add "/dev/tty" as a special file name to lit.Jan 26 2016, 5:50 PM
ygao updated this object.
ygao edited edge metadata.
ygao closed this revision.Jan 26 2016, 5:54 PM

Committed rL258898. Thanks!