[LLVMdev] [llvm-commits] [PATCH] A "very verbose" mode for FileCheck

[David Blaikie]

On Thu, Jan 17, 2013 at 11:19 AM, Daniel Dunbar <daniel at zuster.org> wrote:
> On Thu, Jan 17, 2013 at 10:59 AM, David Blaikie <dblaikie at gmail.com> wrote:
>>
>> On Thu, Jan 17, 2013 at 10:51 AM, Daniel Dunbar <daniel at zuster.org> wrote:
>> > Note that as far as places to put temporary files, the right place to
>> > put
>> > them is alongside the other test outputs in the test output "sandbox"
>> > directory.
>> >
>> > Somewhat orthogonal, but we should also fix up lit to purge those
>> > sandboxes
>> > before it starts a new test run.
>>
>> SGTM - though if we are going to go with the plan/features you've
>> outlined, it might not be so unreasonable to, rather than tunnelling
>> the log files through the lit output & splitting them back out, do the
>> work to actually gather those log files directly (it's a bit more work
>> in the buildbot configuration - enough that I didn't think I could
>> justify it given Dmitri's original proposal - but seems like it would
>> simplify this change & leave us roughly where I was discussing earlier
>> (though your suggestion of generalizing this over all of lit, rather
>> than just FileCheck is a step beyond what I'd proposed - it sounds
>> good/right though))
>
>
> 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.

While I'd tend to agree - is there a particular reason you're not in
favor of collecting from the folder?

(one reason I can think of is that it might mean we'd lose (or have to
work harder to maintain) the association between test files and
temporary files - but we do name the temp files in a structured way
relative to the test file name, so that doesn't seem terribly hard (&
we'll have to use a similar naming structure for the output file names
on the buildbot anyway, to make it easier to pick out the output files
associated with a failure)

>> I asked about how to do this in the freenode buildbot channel & they
>> mentioned that it is possible to name log files dynamically to be
>> retrieved from the slave - I haven't looked into it in detail because
>> it did sound a bit more complicated, but should be achievable.
>
>
> Yes, it is possible. I can help with the buildbot implementation if need be.

Yep, mightn't hurt.

>> The drawback to your approach is that we'd have to enable this feature
>> unconditionally - rather than having the optimization advantage of
>> only dumping files on failure (see my question earlier in the thread,
>> Eli's concern that dumping output would be expensive, and Dmitri's
>> response that we'd only be dumping on failure anyway). Given that it
>> seems the vast majority of our failures aren't flakey, we could have
>> lit setup to rerun failures in a "create all the temporary files"
>> mode, though missing flakes would be unfortunate.
>
>
> 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.

Fair enough.

Eli - any counterargument/other views on this issue? (since you'd
mentioned some concern previously)

> 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.
>
> 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.

SGTM

>
>  - Daniel
>
>
>>
>> >
>> >  - Daniel
>> >
>> >
>> > On Wed, Jan 16, 2013 at 3:31 PM, Dmitri Gribenko <gribozavr at gmail.com>
>> > wrote:
>> >>
>> >> On Thu, Jan 17, 2013 at 1:23 AM, Evgeniy Stepanov
>> >> <eugeni.stepanov at gmail.com> wrote:
>> >> > On Thu, Jan 17, 2013 at 1:19 AM, Dmitri Gribenko
>> >> > <gribozavr at gmail.com>
>> >> > wrote:
>> >> >> On Wed, Jan 16, 2013 at 9:33 PM, Dmitri Gribenko
>> >> >> <gribozavr at gmail.com>
>> >> >> wrote:
>> >> >>> On Wed, Jan 16, 2013 at 9:24 PM, Chris Lattner <clattner at apple.com>
>> >> >>> wrote:
>> >> >>>>
>> >> >>>> On Jan 16, 2013, at 10:32 AM, Dmitri Gribenko
>> >> >>>> <gribozavr at gmail.com>
>> >> >>>> wrote:
>> >> >>>>
>> >> >>>>> Hello,
>> >> >>>>>
>> >> >>>>> When someone breaks a FileCheck-based test on some buildbot,
>> >> >>>>> sometimes
>> >> >>>>> it may not be obvious *why* did it fail.  If the failure can not
>> >> >>>>> be
>> >> >>>>> reproduced locally, it can be very hard to fix.
>> >> >>>>>
>> >> >>>>> I propose adding a "very verbose" mode to FileCheck.  In this
>> >> >>>>> mode
>> >> >>>>> FileCheck will dump the input file in case of failure.  This mode
>> >> >>>>> will
>> >> >>>>> be enabled by an environment variable "FILECHECK_VERY_VERBOSE".
>> >> >>>>> If
>> >> >>>>> we
>> >> >>>>> chose a command line option, we would have to edit all
>> >> >>>>> FileCheck-based
>> >> >>>>> tests to use %FileCheck.
>> >> >>>>
>> >> >>>> I think that this idea is good, but I'd prefer it be implemented a
>> >> >>>> different way:
>> >> >>>>
>> >> >>>>  - Filecheck should take a new flag -dump-input-on-error that
>> >> >>>> causes
>> >> >>>> it to... dump the input file on error.
>> >> >>>>  - Lit should be the thing that checks the environment (or perhaps
>> >> >>>> add a new option to lit), and adds the flag to FileCheck
>> >> >>>> invocations.
>> >> >>>>
>> >> >>>> I don't like it when the behavior of such a low-level tool like
>> >> >>>> this
>> >> >>>> changes based on environment variables.  It isn't discoverable in
>> >> >>>> --help.
>> >> >>>> If for some reason, it is bad for lit to implicitly pass the
>> >> >>>> option, I'd
>> >> >>>> rather have a standard FILECHECK_COMMANDLINE environment variable,
>> >> >>>> and have
>> >> >>>> filecheck parse arbitrary options out of it using the
>> >> >>>> cl::ParseEnvironmentOptions function.
>> >> >>>
>> >> >>> I agree that a command line option would be better.  But in that
>> >> >>> case
>> >> >>> all tests should be updated.  It is not an issue for me -- it is
>> >> >>> mostly mechanical.  So should I change tests to use %FileCheck?
>> >> >>
>> >> >> Here's a third attempt.
>> >> >>
>> >> >> The new behavior is as follows:
>> >> >>
>> >> >> 1. In case of errors we always dump output to a temporary file and
>> >> >> print
>> >> >
>> >> > Does it mean we get one more file in /tmp every time a test fails,
>> >> > and
>> >> > it is not cleaned up automatically? I don't think this should happen
>> >> > in the "default" mode of the tool.
>> >>
>> >> Well, yes.  David requested that and I agreed that it is a good idea.
>> >> Are you strongly opposed to it?
>> >>
>> >> Dmitri
>> >>
>> >> --
>> >> main(i,j){for(i=2;;i++){for(j=2;j<i;j++){if(!(i%j)){j=0;break;}}if
>> >> (j){printf("%d\n",i);}}} /*Dmitri Gribenko <gribozavr at gmail.com>*/
>> >>
>> >> _______________________________________________
>> >> llvm-commits mailing list
>> >> llvm-commits at cs.uiuc.edu
>> >> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>> >
>> >
>> >
>> > _______________________________________________
>> > llvm-commits mailing list
>> > llvm-commits at cs.uiuc.edu
>> > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>> >
>
>