Message ID | Pine.LNX.4.64.1403131502030.7091@digraph.polyomino.org.uk |
---|---|
State | Committed |
Headers |
Return-Path: <x14307373@homiemail-mx20.g.dreamhost.com> X-Original-To: siddhesh@wilcox.dreamhost.com Delivered-To: siddhesh@wilcox.dreamhost.com Received: from homiemail-mx20.g.dreamhost.com (caibbdcaaahc.dreamhost.com [208.113.200.72]) by wilcox.dreamhost.com (Postfix) with ESMTP id 59BEF3600D9 for <siddhesh@wilcox.dreamhost.com>; Thu, 13 Mar 2014 08:07:29 -0700 (PDT) Received: by homiemail-mx20.g.dreamhost.com (Postfix, from userid 14307373) id EADF8407D9A75; Thu, 13 Mar 2014 08:07:28 -0700 (PDT) X-Original-To: glibc@patchwork.siddhesh.in Delivered-To: x14307373@homiemail-mx20.g.dreamhost.com Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by homiemail-mx20.g.dreamhost.com (Postfix) with ESMTPS id 8A215407DB807 for <glibc@patchwork.siddhesh.in>; Thu, 13 Mar 2014 08:07:28 -0700 (PDT) DomainKey-Signature: a=rsa-sha1; c=nofws; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:date:from:to:cc:subject:in-reply-to:message-id :references:mime-version:content-type; q=dns; s=default; b=TEszK A8DWTFEwD/bdbVWVHpjhM2WZwv7akhZox2NMYibsJ3bA1BjZQEBoCLjX8XC3mK/4 IqO8lXNN/AYGeFKvXnj1fRI9xQD2DVn3w3Ui/KEbyojAQYT0DGGsTn9VaUCy8Gul sKsjXHnjnmsptRCgJKyFXzDNVdFiRbhChqy8yQ= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:date:from:to:cc:subject:in-reply-to:message-id :references:mime-version:content-type; s=default; bh=1jjFYJ3gyOw rPLnaXxQiXlUWYv8=; b=FbHbQGV4QshwjuoYPBwvgBJPGLSW+5pBPU850o9Vmk/ 0du88N5sCOf6atyHWBqAHDCaz8VX1pFaF1072Zy0HHAI6L16h29S+YBbENnOy7y4 o7/jHI46uVCpyteeHoV74B6Kv1onno4Fn5X3qLCrAV5brwb1Dky04f57wsQXHttQ = Received: (qmail 21710 invoked by alias); 13 Mar 2014 15:07:12 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <libc-alpha.sourceware.org> List-Unsubscribe: <mailto:libc-alpha-unsubscribe-glibc=patchwork.siddhesh.in@sourceware.org> List-Subscribe: <mailto:libc-alpha-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 21652 invoked by uid 89); 13 Mar 2014 15:07:11 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.0 required=5.0 tests=AWL, BAYES_00 autolearn=ham version=3.3.2 X-HELO: relay1.mentorg.com Date: Thu, 13 Mar 2014 15:07:03 +0000 From: "Joseph S. Myers" <joseph@codesourcery.com> To: Carlos O'Donell <carlos@redhat.com> CC: <libc-alpha@sourceware.org> Subject: Re: Do not terminate default test runs on test failure In-Reply-To: <53215053.4090008@redhat.com> Message-ID: <Pine.LNX.4.64.1403131502030.7091@digraph.polyomino.org.uk> References: <Pine.LNX.4.64.1403071741020.6302@digraph.polyomino.org.uk> <53215053.4090008@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset="US-ASCII" X-DH-Original-To: glibc@patchwork.siddhesh.in |
Commit Message
Joseph Myers
March 13, 2014, 3:07 p.m. UTC
On Thu, 13 Mar 2014, Carlos O'Donell wrote: > I would prefer you split this into two patches, a minimal patch > to support running without `-k' and another for all the other > changes, but I'm not opposed to one patch for this change. > The first patch would change nothing, but add the variable, > and the second patch changes the default and includes the summary > changes. I'm not clear on what you're proposing in light of Roland's and Brooks's comments about exit status (which require reworking the documentation parts of the patch anyway). > > diff --git a/INSTALL b/INSTALL > > index bfa692d..bcb53b8 100644 > > --- a/INSTALL > > +++ b/INSTALL > > @@ -154,7 +154,7 @@ will be used, and CFLAGS sets optimization options for the compiler. > > > > If you only specify `--host', `configure' will prepare for a > > native compile but use what you specify instead of guessing what > > - your system is. This is most useful to change the CPU submodel. > > + your system is. This is most useful to change the CPU submodel. > > OK. Split into another patch? I've committed the regeneration below as obvious. > > @@ -324,10 +324,20 @@ tests: $(tests-special) > > $(..)scripts/merge-test-results.sh -t $(objpfx) subdir-tests.sum \ > > $(sort $(subdirs) .) \ > > > $(objpfx)tests.sum > > + @grep '^ERROR:' $(objpfx)tests.sum || true > > + @grep '^FAIL:' $(objpfx)tests.sum || true > > + @echo "Summary of test results:" > > + @sed 's/:.*//' < $(objpfx)tests.sum | sort | uniq -c > > + @if grep -q '^ERROR:' $(objpfx)tests.sum; then exit 1; fi > > xtests: > > $(..)scripts/merge-test-results.sh -t $(objpfx) subdir-xtests.sum \ > > $(sort $(subdirs)) \ > > > $(objpfx)xtests.sum > > + @grep '^ERROR:' $(objpfx)xtests.sum || true > > + @grep '^FAIL:' $(objpfx)xtests.sum || true > > + @echo "Summary of test results for extra tests:" > > + @sed 's/:.*//' < $(objpfx)xtests.sum | sort | uniq -c > > + @if grep -q '^ERROR:' $(objpfx)xtests.sum; then exit 1; fi > > OK. Split into another patch? stop-on-test-failure=n can't work correctly in exit status terms without the "if grep". And an exit with error status would be very confusing without some message immediately before giving the reason for the error. So I don't think it makes sense to add the variable without this logic that is needed for it to work reasonably. > > +* Running the testsuite no longer terminates as soon as a test fails. > > + Instead, a file tests.sum (xtests.sum from "make xcheck") is generated, > > + with PASS or FAIL lines for individual tests. A summary of the results is > > + printed, including a list of failing lists, and "make check" exits with > > + error status only if test results for a subdirectory are completely > > + missing, or if a test failed to compile. "make check > > + stop-on-test-failure=y" may be used to keep the old behavior. > > + > > OK. Split into another patch? If a patch does something worth mentioning in NEWS, it's best for the NEWS entry to go in as part of the same patch. commit acd6e389122b39c1d4b37e739ef4e6676e706ce5 Author: Joseph Myers <joseph@codesourcery.com> Date: Thu Mar 13 15:01:15 2014 +0000 Regenerate INSTALL.
Comments
On 03/13/2014 11:07 AM, Joseph S. Myers wrote: > On Thu, 13 Mar 2014, Carlos O'Donell wrote: > >> I would prefer you split this into two patches, a minimal patch >> to support running without `-k' and another for all the other >> changes, but I'm not opposed to one patch for this change. >> The first patch would change nothing, but add the variable, >> and the second patch changes the default and includes the summary >> changes. > > I'm not clear on what you're proposing in light of Roland's and Brooks's > comments about exit status (which require reworking the documentation > parts of the patch anyway). I reviewed this independent of Roland and Brooks' reviews. I leave it up to you to decide what to do with the final merger of recommendations. My suggestion is simply that you could split this into two patches, one that adds the bits required to support running without terminating the the test runs when one fails, and another to add the documentation and change the default. I suggest this only because I may wish to backport some of this to earlier source that I support, but perhaps without the documentation or change in defaults e.g. backport the functionality but don't expose it for users. >>> diff --git a/INSTALL b/INSTALL >>> index bfa692d..bcb53b8 100644 >>> --- a/INSTALL >>> +++ b/INSTALL >>> @@ -154,7 +154,7 @@ will be used, and CFLAGS sets optimization options for the compiler. >>> >>> If you only specify `--host', `configure' will prepare for a >>> native compile but use what you specify instead of guessing what >>> - your system is. This is most useful to change the CPU submodel. >>> + your system is. This is most useful to change the CPU submodel. >> >> OK. Split into another patch? > > I've committed the regeneration below as obvious. Perfect. Thanks. >>> @@ -324,10 +324,20 @@ tests: $(tests-special) >>> $(..)scripts/merge-test-results.sh -t $(objpfx) subdir-tests.sum \ >>> $(sort $(subdirs) .) \ >>> > $(objpfx)tests.sum >>> + @grep '^ERROR:' $(objpfx)tests.sum || true >>> + @grep '^FAIL:' $(objpfx)tests.sum || true >>> + @echo "Summary of test results:" >>> + @sed 's/:.*//' < $(objpfx)tests.sum | sort | uniq -c >>> + @if grep -q '^ERROR:' $(objpfx)tests.sum; then exit 1; fi >>> xtests: >>> $(..)scripts/merge-test-results.sh -t $(objpfx) subdir-xtests.sum \ >>> $(sort $(subdirs)) \ >>> > $(objpfx)xtests.sum >>> + @grep '^ERROR:' $(objpfx)xtests.sum || true >>> + @grep '^FAIL:' $(objpfx)xtests.sum || true >>> + @echo "Summary of test results for extra tests:" >>> + @sed 's/:.*//' < $(objpfx)xtests.sum | sort | uniq -c >>> + @if grep -q '^ERROR:' $(objpfx)xtests.sum; then exit 1; fi >> >> OK. Split into another patch? > > stop-on-test-failure=n can't work correctly in exit status terms without > the "if grep". And an exit with error status would be very confusing > without some message immediately before giving the reason for the error. > So I don't think it makes sense to add the variable without this logic > that is needed for it to work reasonably. That is a reasonable position to take, which is why I was asking if you thought this should be split into a second patch. >>> +* Running the testsuite no longer terminates as soon as a test fails. >>> + Instead, a file tests.sum (xtests.sum from "make xcheck") is generated, >>> + with PASS or FAIL lines for individual tests. A summary of the results is >>> + printed, including a list of failing lists, and "make check" exits with >>> + error status only if test results for a subdirectory are completely >>> + missing, or if a test failed to compile. "make check >>> + stop-on-test-failure=y" may be used to keep the old behavior. >>> + >> >> OK. Split into another patch? > > If a patch does something worth mentioning in NEWS, it's best for the NEWS > entry to go in as part of the same patch. Agreed. > commit acd6e389122b39c1d4b37e739ef4e6676e706ce5 > Author: Joseph Myers <joseph@codesourcery.com> > Date: Thu Mar 13 15:01:15 2014 +0000 > > Regenerate INSTALL. > > diff --git a/ChangeLog b/ChangeLog > index 6f0a932..792fdb8 100644 > --- a/ChangeLog > +++ b/ChangeLog > @@ -1,3 +1,7 @@ > +2014-03-13 Joseph Myers <joseph@codesourcery.com> > + > + * INSTALL: Regenerated. > + > 2014-03-13 Will Newton <will.newton@linaro.org> > > * manual/setjmp.texi (System V contexts): Improve > diff --git a/INSTALL b/INSTALL > index bfa692d..f845940 100644 > --- a/INSTALL > +++ b/INSTALL > @@ -154,7 +154,7 @@ will be used, and CFLAGS sets optimization options for the compiler. > > If you only specify `--host', `configure' will prepare for a > native compile but use what you specify instead of guessing what > - your system is. This is most useful to change the CPU submodel. > + your system is. This is most useful to change the CPU submodel. > For example, if `configure' guesses your machine as > `i686-pc-linux-gnu' but you want to compile a library for 586es, > give `--host=i586-pc-linux-gnu' or just `--host=i586-linux' and add > Thanks. Cheers, Carlos.
diff --git a/ChangeLog b/ChangeLog index 6f0a932..792fdb8 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,7 @@ +2014-03-13 Joseph Myers <joseph@codesourcery.com> + + * INSTALL: Regenerated. + 2014-03-13 Will Newton <will.newton@linaro.org> * manual/setjmp.texi (System V contexts): Improve diff --git a/INSTALL b/INSTALL index bfa692d..f845940 100644 --- a/INSTALL +++ b/INSTALL @@ -154,7 +154,7 @@ will be used, and CFLAGS sets optimization options for the compiler. If you only specify `--host', `configure' will prepare for a native compile but use what you specify instead of guessing what - your system is. This is most useful to change the CPU submodel. + your system is. This is most useful to change the CPU submodel. For example, if `configure' guesses your machine as `i686-pc-linux-gnu' but you want to compile a library for 586es, give `--host=i586-pc-linux-gnu' or just `--host=i586-linux' and add