<div dir="ltr">On Thu, Jan 17, 2013 at 10:59 AM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">On Thu, Jan 17, 2013 at 10:51 AM, Daniel Dunbar <<a href="mailto:daniel@zuster.org">daniel@zuster.org</a>> wrote:<br>

> Note that as far as places to put temporary files, the right place to put<br>
> them is alongside the other test outputs in the test output "sandbox"<br>
> directory.<br>
><br>
> Somewhat orthogonal, but we should also fix up lit to purge those sandboxes<br>
> before it starts a new test run.<br>
<br>
</div>SGTM - though if we are going to go with the plan/features you've<br>
outlined, it might not be so unreasonable to, rather than tunnelling<br>
the log files through the lit output & splitting them back out, do the<br>
work to actually gather those log files directly (it's a bit more work<br>
in the buildbot configuration - enough that I didn't think I could<br>
justify it given Dmitri's original proposal - but seems like it would<br>
simplify this change & leave us roughly where I was discussing earlier<br>
(though your suggestion of generalizing this over all of lit, rather<br>
than just FileCheck is a step beyond what I'd proposed - it sounds<br>
good/right though))<br></blockquote><div><br></div><div style>I'm fine not having the files dumped into the output, but I at least want the buildbot to get the names of the files to collect from the lit output, not just by scanning the folder.</div>
<div style><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
I asked about how to do this in the freenode buildbot channel & they<br>
mentioned that it is possible to name log files dynamically to be<br>
retrieved from the slave - I haven't looked into it in detail because<br>
it did sound a bit more complicated, but should be achievable.<br></blockquote><div><br></div><div style>Yes, it is possible. I can help with the buildbot implementation if need be.</div><div style> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

The drawback to your approach is that we'd have to enable this feature<br>
unconditionally - rather than having the optimization advantage of<br>
only dumping files on failure (see my question earlier in the thread,<br>
Eli's concern that dumping output would be expensive, and Dmitri's<br>
response that we'd only be dumping on failure anyway). Given that it<br>
seems the vast majority of our failures aren't flakey, we could have<br>
lit setup to rerun failures in a "create all the temporary files"<br>
mode, though missing flakes would be unfortunate.<br></blockquote><div><br></div><div style>In my experience, there isn't actually that much performance difference (and sometimes a loss) of using a file instead of a pipe. With all the other stuff going on in the test suite I would be surprised if this added much impact. Note that I personally often write my tests to use temporary files instead of pipes anyway just because I like how the RUN lines look, so its not exactly like we aren't already storing many of these files.</div>
<div style><br></div><div style>There would be some code simplification advantages internally to lit too if it avoided actually using pipes, although I'm not sure I want to go as far as dropping that support.</div><div style>
<br></div><div style>If it is measurable, then my proposal is to do as you say and just rerun the test with the collection enabled. I actually think that lit by default should rerun failing tests so that it can diagnose flaky tests, so this fits in with that.</div>
<div style><br></div><div style> - Daniel</div><div style><br></div><div style><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="HOEnZb"><div class="h5"><br>
><br>
>  - Daniel<br>
><br>
><br>
> On Wed, Jan 16, 2013 at 3:31 PM, Dmitri Gribenko <<a href="mailto:gribozavr@gmail.com">gribozavr@gmail.com</a>><br>
> wrote:<br>
>><br>
>> On Thu, Jan 17, 2013 at 1:23 AM, Evgeniy Stepanov<br>
>> <<a href="mailto:eugeni.stepanov@gmail.com">eugeni.stepanov@gmail.com</a>> wrote:<br>
>> > On Thu, Jan 17, 2013 at 1:19 AM, Dmitri Gribenko <<a href="mailto:gribozavr@gmail.com">gribozavr@gmail.com</a>><br>
>> > wrote:<br>
>> >> On Wed, Jan 16, 2013 at 9:33 PM, Dmitri Gribenko <<a href="mailto:gribozavr@gmail.com">gribozavr@gmail.com</a>><br>
>> >> wrote:<br>
>> >>> On Wed, Jan 16, 2013 at 9:24 PM, Chris Lattner <<a href="mailto:clattner@apple.com">clattner@apple.com</a>><br>
>> >>> wrote:<br>
>> >>>><br>
>> >>>> On Jan 16, 2013, at 10:32 AM, Dmitri Gribenko <<a href="mailto:gribozavr@gmail.com">gribozavr@gmail.com</a>><br>
>> >>>> wrote:<br>
>> >>>><br>
>> >>>>> Hello,<br>
>> >>>>><br>
>> >>>>> When someone breaks a FileCheck-based test on some buildbot,<br>
>> >>>>> sometimes<br>
>> >>>>> it may not be obvious *why* did it fail.  If the failure can not be<br>
>> >>>>> reproduced locally, it can be very hard to fix.<br>
>> >>>>><br>
>> >>>>> I propose adding a "very verbose" mode to FileCheck.  In this mode<br>
>> >>>>> FileCheck will dump the input file in case of failure.  This mode<br>
>> >>>>> will<br>
>> >>>>> be enabled by an environment variable "FILECHECK_VERY_VERBOSE".  If<br>
>> >>>>> we<br>
>> >>>>> chose a command line option, we would have to edit all<br>
>> >>>>> FileCheck-based<br>
>> >>>>> tests to use %FileCheck.<br>
>> >>>><br>
>> >>>> I think that this idea is good, but I'd prefer it be implemented a<br>
>> >>>> different way:<br>
>> >>>><br>
>> >>>>  - Filecheck should take a new flag -dump-input-on-error that causes<br>
>> >>>> it to... dump the input file on error.<br>
>> >>>>  - Lit should be the thing that checks the environment (or perhaps<br>
>> >>>> add a new option to lit), and adds the flag to FileCheck invocations.<br>
>> >>>><br>
>> >>>> I don't like it when the behavior of such a low-level tool like this<br>
>> >>>> changes based on environment variables.  It isn't discoverable in --help.<br>
>> >>>> If for some reason, it is bad for lit to implicitly pass the option, I'd<br>
>> >>>> rather have a standard FILECHECK_COMMANDLINE environment variable, and have<br>
>> >>>> filecheck parse arbitrary options out of it using the<br>
>> >>>> cl::ParseEnvironmentOptions function.<br>
>> >>><br>
>> >>> I agree that a command line option would be better.  But in that case<br>
>> >>> all tests should be updated.  It is not an issue for me -- it is<br>
>> >>> mostly mechanical.  So should I change tests to use %FileCheck?<br>
>> >><br>
>> >> Here's a third attempt.<br>
>> >><br>
>> >> The new behavior is as follows:<br>
>> >><br>
>> >> 1. In case of errors we always dump output to a temporary file and<br>
>> >> print<br>
>> ><br>
>> > Does it mean we get one more file in /tmp every time a test fails, and<br>
>> > it is not cleaned up automatically? I don't think this should happen<br>
>> > in the "default" mode of the tool.<br>
>><br>
>> Well, yes.  David requested that and I agreed that it is a good idea.<br>
>> Are you strongly opposed to it?<br>
>><br>
>> Dmitri<br>
>><br>
>> --<br>
>> main(i,j){for(i=2;;i++){for(j=2;j<i;j++){if(!(i%j)){j=0;break;}}if<br>
>> (j){printf("%d\n",i);}}} /*Dmitri Gribenko <<a href="mailto:gribozavr@gmail.com">gribozavr@gmail.com</a>>*/<br>
>><br>
>> _______________________________________________<br>
>> llvm-commits mailing list<br>
>> <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
>> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
><br>
><br>
><br>
> _______________________________________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
><br>
</div></div></blockquote></div><br></div></div>