[PATCH] validate_failures.py: also ignore .git

[PATCH] validate_failures.py: also ignore .git

[Bernhard Reutner-Fischer]

contrib/ChangeLog:

2012-12-01  Bernhard Reutner-Fischer  <aldot@gcc.gnu.org>

	* testsuite-management/validate_failures.py
	(IsInterestingResult): Only strip line a second time if we did split.
	Rephrase return statement while at it.
	(CollectSumFiles): Also ignore .git directory.

Signed-off-by: Bernhard Reutner-Fischer <rep.dot.nop@gmail.com>
---
 contrib/testsuite-management/validate_failures.py |   12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/contrib/testsuite-management/validate_failures.py b/contrib/testsuite-management/validate_failures.py
index 7844cb0..280fd23 100755
--- a/contrib/testsuite-management/validate_failures.py
+++ b/contrib/testsuite-management/validate_failures.py
@@ -194,11 +194,8 @@ def IsInterestingResult(line):
     return False
   if '|' in line:
     (_, line) = line.split('|', 1)
-  line = line.strip()
-  for result in _VALID_TEST_RESULTS:
-    if line.startswith(result):
-      return True
-  return False
+    line = line.strip()
+  return any(line.startswith(result) for result in _VALID_TEST_RESULTS)
 
 
 def ParseSummary(sum_fname):
@@ -240,8 +237,9 @@ def GetManifest(manifest_path):
 def CollectSumFiles(builddir):
   sum_files = []
   for root, dirs, files in os.walk(builddir):
-    if '.svn' in dirs:
-      dirs.remove('.svn')
+    for ignored in ('.svn', '.git'):
+      if ignored in dirs:
+        dirs.remove(ignored)
     for fname in files:
       if fname.endswith('.sum'):
         sum_files.append(os.path.join(root, fname))
-- 
1.7.10.4

Re: [PATCH] validate_failures.py: also ignore .git

[Diego Novillo]

On Tue, Dec 4, 2012 at 4:24 AM, Bernhard Reutner-Fischer
<rep.dot.nop@gmail.com> wrote:
> contrib/ChangeLog:
>
> 2012-12-01  Bernhard Reutner-Fischer  <aldot@gcc.gnu.org>
>
>         * testsuite-management/validate_failures.py
>         (IsInterestingResult): Only strip line a second time if we did split.
>         Rephrase return statement while at it.
>         (CollectSumFiles): Also ignore .git directory.

OK.


Diego.

Re: [PATCH] validate_failures.py: also ignore .git

[Bernhard Reutner-Fischer]

On Tue, Dec 04, 2012 at 08:53:50AM -0500, Diego Novillo wrote:
>On Tue, Dec 4, 2012 at 4:24 AM, Bernhard Reutner-Fischer
><rep.dot.nop@gmail.com> wrote:
>> contrib/ChangeLog:
>>
>> 2012-12-01  Bernhard Reutner-Fischer  <aldot@gcc.gnu.org>
>>
>>         * testsuite-management/validate_failures.py
>>         (IsInterestingResult): Only strip line a second time if we did split.
>>         Rephrase return statement while at it.
>>         (CollectSumFiles): Also ignore .git directory.
>
>OK.

applied to trunk as r194182
thanks!

[PATCH] validate_failures.py: Fix performance regression

[Bernhard Reutner-Fischer]

2012-12-06  Bernhard Reutner-Fischer  <aldot@gcc.gnu.org>

	* testsuite-management/validate_failures.py
	(IsInterestingResult): Fix performance regression

---

Rephrasing the return statement was not a good idea, it regressed
tremendously ;)

==> LOG-before <==
real	0m22.696s
user	0m19.953s
sys	0m2.696s

==> LOG-r194182 <==
real	0m43.527s
user	0m38.346s
sys	0m5.092s

==> LOG-fixed <==
real	0m12.302s
user	0m10.841s
sys	0m1.432s

Sorry for that, OK for trunk?

Signed-off-by: Bernhard Reutner-Fischer <rep.dot.nop@gmail.com>
---
 contrib/testsuite-management/validate_failures.py |    9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/contrib/testsuite-management/validate_failures.py b/contrib/testsuite-management/validate_failures.py
index ec51de9..00a1527 100755
--- a/contrib/testsuite-management/validate_failures.py
+++ b/contrib/testsuite-management/validate_failures.py
@@ -62,6 +62,7 @@ import sys
 
 # Handled test results.
 _VALID_TEST_RESULTS = [ 'FAIL', 'UNRESOLVED', 'XPASS', 'ERROR' ]
+_VALID_TEST_RESULTS_REX = re.compile("%s" % "|".join(_VALID_TEST_RESULTS))
 
 # Subdirectory of srcdir in which to find the manifest file.
 _MANIFEST_SUBDIR = 'contrib/testsuite-management'
@@ -202,7 +203,7 @@ def ValidBuildDirectory(builddir, target):
 
 def IsComment(line):
   """Return True if line is a comment."""
-  return line.startswith('#')
+  return bool(re.matches("#", line))
 
 
 def IsInterestingResult(line):
@@ -210,12 +211,12 @@ def IsInterestingResult(line):
   if '|' in line:
     (_, line) = line.split('|', 1)
     line = line.strip()
-  return any(line.startswith(result) for result in _VALID_TEST_RESULTS)
+  return bool(_VALID_TEST_RESULTS_REX.match(line))
 
 
 def IsInclude(line):
   """Return True if line is an include of another file."""
-  return line.startswith("@include ")
+  return bool(re.matches("@include ", line))
 
 
 def GetIncludeFile(line, includer):
@@ -227,7 +228,7 @@ def GetIncludeFile(line, includer):
 
 def IsNegativeResult(line):
   """Return True if line should be removed from the expected results."""
-  return line.startswith("@remove ")
+  return bool(re.matches("@remove ", line))
 
 
 def GetNegativeResult(line):
-- 
1.7.10.4

Re: [PATCH] validate_failures.py: Fix performance regression

[Diego Novillo]

On Thu, Dec 6, 2012 at 1:12 PM, Bernhard Reutner-Fischer
<rep.dot.nop@gmail.com> wrote:

>  def IsComment(line):
>    """Return True if line is a comment."""
> -  return line.startswith('#')
> +  return bool(re.matches("#", line))

startswith() is a better match here.

>  def IsInclude(line):
>    """Return True if line is an include of another file."""
> -  return line.startswith("@include ")
> +  return bool(re.matches("@include ", line))

Likewise.

>  def IsNegativeResult(line):
>    """Return True if line should be removed from the expected results."""
> -  return line.startswith("@remove ")
> +  return bool(re.matches("@remove ", line))

Likewise.


OK with those changes.


Diego.

Re: [PATCH] validate_failures.py: Fix performance regression

[Bernhard Reutner-Fischer]

On Fri, Dec 07, 2012 at 10:31:57AM -0500, Diego Novillo wrote:
>On Thu, Dec 6, 2012 at 1:12 PM, Bernhard Reutner-Fischer
><rep.dot.nop@gmail.com> wrote:

@@ -210,12 +211,12 @@ def IsInterestingResult(line):
   if '|' in line:
     (_, line) = line.split('|', 1)
     line = line.strip()
-  return any(line.startswith(result) for result in _VALID_TEST_RESULTS)
+  return bool(_VALID_TEST_RESULTS_REX.match(line))

I wonder why we care about '|' at all? Can you give an example where
this is of relevance?

Just asking because IIUC you throw away the beginning of the line until
the first pipe and try to match the remainder. Did you mean to do this
the other way round, throwing away the pipe and remainder instead?

I suspect that this function should just be
def IsInterestingResult(line):
  """Return True if line is one of the summary lines we care about."""
  return bool(_VALID_TEST_RESULTS_REX.match(line))

or, if there ever is a pipe in an interesting result
def IsInterestingResult(line):
  """Return True if line is one of the summary lines we care about."""
  if bool(_VALID_TEST_RESULTS_REX.match(line)):
    if '|' in line:
      (line, _) = line.split('|', 1)
      line = line.strip()
    return True
  return False

>
>>  def IsComment(line):
>>    """Return True if line is a comment."""
>> -  return line.startswith('#')
>> +  return bool(re.matches("#", line))
>
>startswith() is a better match here.
>
>>  def IsInclude(line):
>>    """Return True if line is an include of another file."""
>> -  return line.startswith("@include ")
>> +  return bool(re.matches("@include ", line))
>
>Likewise.
>
>>  def IsNegativeResult(line):
>>    """Return True if line should be removed from the expected results."""
>> -  return line.startswith("@remove ")
>> +  return bool(re.matches("@remove ", line))
>
>Likewise.

I dropped these 3.

TIA for clarification,
>
>
>OK with those changes.
>
>
>Diego.

Re: [PATCH] validate_failures.py: Fix performance regression

[Diego Novillo]

On Thu, Dec 13, 2012 at 10:12 AM, Bernhard Reutner-Fischer
<rep.dot.nop@gmail.com> wrote:
> On Fri, Dec 07, 2012 at 10:31:57AM -0500, Diego Novillo wrote:
>>On Thu, Dec 6, 2012 at 1:12 PM, Bernhard Reutner-Fischer
>><rep.dot.nop@gmail.com> wrote:
>
> @@ -210,12 +211,12 @@ def IsInterestingResult(line):
>    if '|' in line:
>      (_, line) = line.split('|', 1)
>      line = line.strip()
> -  return any(line.startswith(result) for result in _VALID_TEST_RESULTS)
> +  return bool(_VALID_TEST_RESULTS_REX.match(line))
>
> I wonder why we care about '|' at all? Can you give an example where
> this is of relevance?

That's for the attributes.  See the syntax at the top of the file.
One can add an attribute to a test.  For instance, mark it 'flaky' so
it's always ignored.  Or you can add an expiration date, to ignore it
until that timer elapses.

> or, if there ever is a pipe in an interesting result
> def IsInterestingResult(line):
>   """Return True if line is one of the summary lines we care about."""
>   if bool(_VALID_TEST_RESULTS_REX.match(line)):
>     if '|' in line:
>       (line, _) = line.split('|', 1)
>       line = line.strip()
>     return True
>   return False

*shrug*

Performance is not really a problem with this script.



Diego.

Re: [PATCH] validate_failures.py: Fix performance regression

[Bernhard Reutner-Fischer]

On Fri, Dec 07, 2012 at 10:31:57AM -0500, Diego Novillo wrote:
>On Thu, Dec 6, 2012 at 1:12 PM, Bernhard Reutner-Fischer
><rep.dot.nop@gmail.com> wrote:
>
>>  def IsComment(line):
>>    """Return True if line is a comment."""
>> -  return line.startswith('#')
>> +  return bool(re.matches("#", line))
>
>startswith() is a better match here.
>
>>  def IsInclude(line):
>>    """Return True if line is an include of another file."""
>> -  return line.startswith("@include ")
>> +  return bool(re.matches("@include ", line))
>
>Likewise.
>
>>  def IsNegativeResult(line):
>>    """Return True if line should be removed from the expected results."""
>> -  return line.startswith("@remove ")
>> +  return bool(re.matches("@remove ", line))
>
>Likewise.
>
>
>OK with those changes.

Applied as r195811

thanks,