<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Thu, Jan 17, 2013 at 12:11 PM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br>
<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 11:19 AM, Daniel Dunbar <<a href="mailto:daniel@zuster.org">daniel@zuster.org</a>> wrote:<br>

> On Thu, Jan 17, 2013 at 10:59 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
>><br>
>> 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<br>
>> > 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<br>
>> > sandboxes<br>
>> > before it starts a new test run.<br>
>><br>
>> 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>
><br>
><br>
> I'm fine not having the files dumped into the output, but I at least want<br>
> the buildbot to get the names of the files to collect from the lit output,<br>
> not just by scanning the folder.<br>
<br>
</div>While I'd tend to agree - is there a particular reason you're not in<br>
favor of collecting from the folder?<br></blockquote><div><br></div><div style>Just explicit vs implicit, I want the test output to be explicit about what files are related and the buildbot only to pick up things it was explicitly told about.</div>
<div style><br></div><div style>Also, I don't want an implicit hidden dependency between the buildbot runners "knowing" how lit lays out its test results.</div><div style><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

(one reason I can think of is that it might mean we'd lose (or have to<br>
work harder to maintain) the association between test files and<br>
temporary files - but we do name the temp files in a structured way<br>
relative to the test file name, so that doesn't seem terribly hard (&<br>
we'll have to use a similar naming structure for the output file names<br>
on the buildbot anyway, to make it easier to pick out the output files<br>
associated with a failure)<br>
<div class="im"><br>
>> 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>
><br>
><br>
> Yes, it is possible. I can help with the buildbot implementation if need be.<br>
<br>
</div>Yep, mightn't hurt.<br>
<div class="im"><br>
>> 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>
><br>
><br>
> In my experience, there isn't actually that much performance difference (and<br>
> sometimes a loss) of using a file instead of a pipe. With all the other<br>
> stuff going on in the test suite I would be surprised if this added much<br>
> impact. Note that I personally often write my tests to use temporary files<br>
> instead of pipes anyway just because I like how the RUN lines look, so its<br>
> not exactly like we aren't already storing many of these files.<br>
<br>
</div>Fair enough.<br>
<br>
Eli - any counterargument/other views on this issue? (since you'd<br>
mentioned some concern previously)<br>
<div class="im"><br>
> There would be some code simplification advantages internally to lit too if<br>
> it avoided actually using pipes, although I'm not sure I want to go as far<br>
> as dropping that support.<br>
><br>
> If it is measurable, then my proposal is to do as you say and just rerun the<br>
> test with the collection enabled. I actually think that lit by default<br>
> should rerun failing tests so that it can diagnose flaky tests, so this fits<br>
> in with that.<br>
<br>
</div>SGTM</blockquote><div> </div><div style>Cool!</div><div style><br></div><div style> - Daniel</div><div style> </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>
>  - Daniel<br>
><br>
><br>
>><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<br>
>> >> > <<a href="mailto:gribozavr@gmail.com">gribozavr@gmail.com</a>><br>
>> >> > wrote:<br>
>> >> >> On Wed, Jan 16, 2013 at 9:33 PM, Dmitri Gribenko<br>
>> >> >> <<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<br>
>> >> >>>> <<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<br>
>> >> >>>>> be<br>
>> >> >>>>> reproduced locally, it can be very hard to fix.<br>
>> >> >>>>><br>
>> >> >>>>> I propose adding a "very verbose" mode to FileCheck.  In this<br>
>> >> >>>>> 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".<br>
>> >> >>>>> 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<br>
>> >> >>>> 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<br>
>> >> >>>> invocations.<br>
>> >> >>>><br>
>> >> >>>> I don't like it when the behavior of such a low-level tool like<br>
>> >> >>>> this<br>
>> >> >>>> changes based on environment variables.  It isn't discoverable in<br>
>> >> >>>> --help.<br>
>> >> >>>> If for some reason, it is bad for lit to implicitly pass the<br>
>> >> >>>> option, I'd<br>
>> >> >>>> rather have a standard FILECHECK_COMMANDLINE environment variable,<br>
>> >> >>>> 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<br>
>> >> >>> 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,<br>
>> >> > 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>
><br>
><br>
</div></div></blockquote></div><br></div></div>