Patchwork [RFA,3/8] validate_failures.py: pass options.results for clean build case

login
register
mail settings
Submitter Doug Evans
Date Nov. 24, 2012, 10:47 p.m.
Message ID <yjt21ufi7h5d.fsf@ruffy2.mtv.corp.google.com>
Download mbox | patch
Permalink /patch/201482/
State New
Headers show

Comments

Doug Evans - Nov. 24, 2012, 10:47 p.m.
Hi.
This third patch passes options.results to GetSumFiles when fetching the results
for the clean build.
It is useful in my use cases, but I'm not sure it's useful for upstream.
[An alternative is to add another option to specify the results file(s) for the clean
build, but I'm being conservative and not adding an option if I don't have to.]

Ok to check in?

2012-11-24  Doug Evans  <dje@google.com>

	* testsuite-management/validate_failures.py (CompareBuilds): Pass options.results
	to GetSumFiles for clean build.
Diego Novillo - Nov. 25, 2012, 3:40 p.m.
On Sat, Nov 24, 2012 at 5:47 PM, Doug Evans <dje@google.com> wrote:
>
> Hi.
> This third patch passes options.results to GetSumFiles when fetching the
> results
> for the clean build.
> It is useful in my use cases, but I'm not sure it's useful for upstream.
> [An alternative is to add another option to specify the results file(s)
> for the clean
> build, but I'm being conservative and not adding an option if I don't have
> to.]
>
> Ok to check in?
>
> 2012-11-24  Doug Evans  <dje@google.com>
>
>         * testsuite-management/validate_failures.py (CompareBuilds): Pass
> options.results
>         to GetSumFiles for clean build.

I guess I don't understand your use case.  CompareBuilds is for when
you have a clean build  that you want to crawl for failures to use as
your comparison base.  If you have a manifest already, why would you
be using --clean_build?


Diego.
Doug Evans - Nov. 28, 2012, 6:55 p.m.
On Sun, Nov 25, 2012 at 7:40 AM, Diego Novillo <dnovillo@google.com> wrote:
> On Sat, Nov 24, 2012 at 5:47 PM, Doug Evans <dje@google.com> wrote:
>>
>> Hi.
>> This third patch passes options.results to GetSumFiles when fetching the
>> results
>> for the clean build.
>> It is useful in my use cases, but I'm not sure it's useful for upstream.
>> [An alternative is to add another option to specify the results file(s)
>> for the clean
>> build, but I'm being conservative and not adding an option if I don't have
>> to.]
>>
>> Ok to check in?
>>
>> 2012-11-24  Doug Evans  <dje@google.com>
>>
>>         * testsuite-management/validate_failures.py (CompareBuilds): Pass
>> options.results
>>         to GetSumFiles for clean build.
>
> I guess I don't understand your use case.  CompareBuilds is for when
> you have a clean build  that you want to crawl for failures to use as
> your comparison base.  If you have a manifest already, why would you
> be using --clean_build?

In gdb-land, parallel make check runs collect all the subdirectory
.sum files and reconstruct testsuite/${tool}.sum.

There's more than one solution of course: alternatively one could have
gdb stop doing this.  But I'm not sure which is better, and rather
than change gdb I went for changing validate_failure.py (which made
sense regardless of what gdb is doing: use the same .sum files in the
comparison).  Could be missing something of course. :-)
Diego Novillo - Nov. 28, 2012, 8:35 p.m.
On Wed, Nov 28, 2012 at 1:55 PM, Doug Evans <dje@google.com> wrote:

> In gdb-land, parallel make check runs collect all the subdirectory
> .sum files and reconstruct testsuite/${tool}.sum.
>
> There's more than one solution of course: alternatively one could have
> gdb stop doing this.  But I'm not sure which is better, and rather
> than change gdb I went for changing validate_failure.py (which made
> sense regardless of what gdb is doing: use the same .sum files in the
> comparison).  Could be missing something of course. :-)

So, you do have a set of .sum files, right?  You could feed them to
validate_failures.py via --manifest=collected_results_from_gdb.sum.
You do not need to use the --clean_build flag.

What I'm saying is that from a user perspective, it doesn't really
make sense to use --clean_build and --manifest together.  If you have
the --manifest, you do not need to go to --clean_build to look for the
.sum files.

Am I right in understanding that you are intending to call

$ validate_failures.py --clean_build=/path/to/gdb/bld
--manifest=/path/to/gdb.sum ?

If that's not what you are intending, then I am completely lost :)


Diego.
Doug Evans - Nov. 28, 2012, 11:02 p.m.
On Wed, Nov 28, 2012 at 12:35 PM, Diego Novillo <dnovillo@google.com> wrote:
> On Wed, Nov 28, 2012 at 1:55 PM, Doug Evans <dje@google.com> wrote:
>
>> In gdb-land, parallel make check runs collect all the subdirectory
>> .sum files and reconstruct testsuite/${tool}.sum.
>>
>> There's more than one solution of course: alternatively one could have
>> gdb stop doing this.  But I'm not sure which is better, and rather
>> than change gdb I went for changing validate_failure.py (which made
>> sense regardless of what gdb is doing: use the same .sum files in the
>> comparison).  Could be missing something of course. :-)
>
> So, you do have a set of .sum files, right?  You could feed them to
> validate_failures.py via --manifest=collected_results_from_gdb.sum.
> You do not need to use the --clean_build flag.
>
> What I'm saying is that from a user perspective, it doesn't really
> make sense to use --clean_build and --manifest together.  If you have
> the --manifest, you do not need to go to --clean_build to look for the
> .sum files.
>
> Am I right in understanding that you are intending to call
>
> $ validate_failures.py --clean_build=/path/to/gdb/bld
> --manifest=/path/to/gdb.sum ?
>
> If that's not what you are intending, then I am completely lost :)

s/--manifest/--results/

i.e.
$ validate_failures.py --clean_build=/path/to/gdb/bld
--results=/path/to/gdb.sum ...
Diego Novillo - Nov. 28, 2012, 11:35 p.m.
On Wed, Nov 28, 2012 at 6:02 PM, Doug Evans <dje@google.com> wrote:
> On Wed, Nov 28, 2012 at 12:35 PM, Diego Novillo <dnovillo@google.com> wrote:
>>
>> Am I right in understanding that you are intending to call
>>
>> $ validate_failures.py --clean_build=/path/to/gdb/bld
>> --manifest=/path/to/gdb.sum ?
>>
>> If that's not what you are intending, then I am completely lost :)
>
> s/--manifest/--results/
>
> i.e.
> $ validate_failures.py --clean_build=/path/to/gdb/bld
> --results=/path/to/gdb.sum ...

Aha!  OK, yes, that makes sense.  I'm sorry it took me so long to get
this.  At first I was almost convinced that the patch was going to
break my favourite use:

$ validate_failures.py --clean_build=../clean/bld

The patch is fine.


Thanks.  Diego.

Patch

--- validate_failures.py=	2012-11-19 11:47:29.997632548 -0800
+++ validate_failures.py	2012-11-24 13:26:07.727726123 -0800
@@ -373,7 +373,7 @@  def CompareBuilds(options):
   sum_files = GetSumFiles(options.results, options.build_dir)
   actual = GetResults(sum_files)
 
-  clean_sum_files = GetSumFiles(None, options.clean_build)
+  clean_sum_files = GetSumFiles(options.results, options.clean_build)
   clean = GetResults(clean_sum_files)
 
   return PerformComparison(clean, actual, options.ignore_missing_failures)