public inbox for bunsen@sourceware.org
 help / color / mirror / Atom feed
* Fwd: Re: [PATCH] Introduce simplified grok_architecture function
@ 2020-09-23 13:21 Serhei Makarov
  2020-09-23 18:14 ` Keith Seitz
  0 siblings, 1 reply; 6+ messages in thread
From: Serhei Makarov @ 2020-09-23 13:21 UTC (permalink / raw)
  To: Bunsen

(should forward to list)

----- Original message -----
From: Serhei Makarov <me@serhei.io>
To: Keith Seitz <keiths@redhat.com>
Subject: Re: [PATCH] Introduce simplified grok_architecture function
Date: Wednesday, September 23, 2020 9:21 AM

On Tue, Sep 22, 2020, at 1:58 PM, Keith Seitz via Bunsen wrote:
> Comments on this apporach?
> 
> Should I move this to the common module and update systemtap/parse_dejagnu.py?
LGTM. You can commit as-is, or add to the common module as you suggested.

Also, I'm going to make another improvement to a different area of the parser that's always been problematic (the code to extract osver). I've thought of a solution that should be more general.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: Fwd: Re: [PATCH] Introduce simplified grok_architecture function
  2020-09-23 13:21 Fwd: Re: [PATCH] Introduce simplified grok_architecture function Serhei Makarov
@ 2020-09-23 18:14 ` Keith Seitz
  2020-09-23 19:15   ` [PATCH v2] " Keith Seitz
  0 siblings, 1 reply; 6+ messages in thread
From: Keith Seitz @ 2020-09-23 18:14 UTC (permalink / raw)
  To: Serhei Makarov, Bunsen

On 9/23/20 6:21 AM, Serhei Makarov wrote:
> (should forward to list)
> 
> ----- Original message -----
> From: Serhei Makarov <me@serhei.io>
> To: Keith Seitz <keiths@redhat.com>
> Subject: Re: [PATCH] Introduce simplified grok_architecture function
> Date: Wednesday, September 23, 2020 9:21 AM
> 
> On Tue, Sep 22, 2020, at 1:58 PM, Keith Seitz via Bunsen wrote:
>> Comments on this apporach?
>>
>> Should I move this to the common module and update systemtap/parse_dejagnu.py?
> LGTM. You can commit as-is, or add to the common module as you suggested.

I did discover one inconsistency between my proposed patch and the current
implementation: native_configuration_map contains a few non-direct mappings
of the architecture name, e.g., "powerpc64le-VENDOR-linux" --> "ppc64le".

I'll adjust the patch and send out a revised copy for review.

> Also, I'm going to make another improvement to a different area of
> the parser that's always been problematic (the code to extract
> osver). I've thought of a solution that should be more general.

Great. I haven't yet looked into that! :-)

Keith


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v2] Introduce simplified grok_architecture function
  2020-09-23 18:14 ` Keith Seitz
@ 2020-09-23 19:15   ` Keith Seitz
  2020-09-23 19:26     ` Serhei Makarov
  0 siblings, 1 reply; 6+ messages in thread
From: Keith Seitz @ 2020-09-23 19:15 UTC (permalink / raw)
  To: me; +Cc: bunsen


This is a rewrite of the previous patch which includes a little more
flexibility about the architecture name mappings listed by DejaGNU.
For example, the previous version did not map "powerpc64" to "ppc64"
as the existing code did.

I've also moved the code to common/parse_dejagnu.py, but I have /not/
yet changed the systemtap version to use this. I'd like to solicit
feedback about this before I attempt to convert that.

How does this look?

Keith

---
Add a simplified and more complete architecture grok'ing function
to handle the result of the "Native configuration is ..." line from
the .log file, taking advantage of the fact that this string will
necessarily be of the form "ARCH-VENDOR-linux".
---
 scripts-master/common/parse_dejagnu.py | 25 +++++++++++++++++++++++++
 scripts-master/gdb/parse_dejagnu.py    | 24 ++++--------------------
 2 files changed, 29 insertions(+), 20 deletions(-)

diff --git a/scripts-master/common/parse_dejagnu.py b/scripts-master/common/parse_dejagnu.py
index 335130f..674d363 100755
--- a/scripts-master/common/parse_dejagnu.py
+++ b/scripts-master/common/parse_dejagnu.py
@@ -21,6 +21,31 @@ import dateutil.parser
 from bunsen import Bunsen, Testrun, Testlog, Cursor
 from bunsen import Testcase
 
+# Standardized architecture name mapping table.  The "Native configuration
+# is " line of a test run is matched against the keys in this table.  If
+# a match is found, the architecture is given by the corresponding value.
+# Keys are regular expressions; values are functions which take the regexp
+# match as parameter and return a string representatin of the architecture.
+standard_architecture_map = {r'powerpc64-(\w+)-linux.*':lambda m: 'ppc64',
+                             r'powerpc64le-(\w+)-linux.*':lambda m: 'ppc64le',
+                             r'armv7l-(\w+)-linux-gnueabihf':lambda m: 'armhf',
+                             r'(\w+)-(\w+)-linux.*':lambda m: m.group(1)}
+
+# Deduce the architecture from TEXT, which must start with the string
+# "Native configuration is ".  If TEXT does not start with this string or
+# the architecture is not deduced, return None.
+
+def grok_architecture(text):
+    arch = None
+    if text.startswith("Native configuration is "):
+        text = text[len("Native configuration is "):-1]
+        for regex in standard_architecture_map:
+            match = re.match(regex, text)
+            if match:
+                arch = standard_architecture_map[regex](match)
+                break
+    return arch
+
 native_configuration_map = {"i686-pc-linux-gnu":"i686",
                             "i686-unknown-linux-gnu":"i686",
                             "x86_64-unknown-linux-gnu":"x86_64",
diff --git a/scripts-master/gdb/parse_dejagnu.py b/scripts-master/gdb/parse_dejagnu.py
index 9013860..75d9cb1 100755
--- a/scripts-master/gdb/parse_dejagnu.py
+++ b/scripts-master/gdb/parse_dejagnu.py
@@ -19,6 +19,7 @@ cmdline_args = [
 
 import sys
 from bunsen import Bunsen, Testrun, Cursor
+from common.parse_dejagnu import grok_architecture
 
 from datetime import datetime
 import dateutil.parser
@@ -32,17 +33,6 @@ import lzma
 
 # === TODO CREATE common.parse_dejagnu AND HARMONIZE WITH SYSTEMTAP ===
 
-native_configuration_map = {"Native configuration is i686-pc-linux-gnu":"i686",
-                            "Native configuration is i686-unknown-linux-gnu":"i686",
-                            "Native configuration is x86_64-unknown-linux-gnu":"x86_64",
-                            "Native configuration is powerpc64-unknown-linux-gnu":"ppc64",
-                            # Older systemtap logs have "Native configuration is /usr/share/dejagnu/libexec/config.guess: unable to guess system type" for ppc64le.
-                            "Native configuration is powerpc64le-unknown-linux-gnu":"ppc64le",
-                            "Native configuration is aarch64-unknown-linux-gnu":"aarch64",
-                            "Native configuration is armv7l-unknown-linux-gnueabihf":"armhf",
-                            "Native configuration is s390x-ibm-linux":"s390x",
-                            "Native configuration is x86_64-pc-linux-gnu":"x86_64", # seen on Ubuntu
-                            }
 
 # TODO Handle other exotic DejaGNU outcome codes if they come up.
 test_outcome_map = {'PASS':'PASS', 'XPASS':'XPASS', 'IPASS':'PASS',
@@ -242,7 +232,7 @@ def annotate_dejagnu_log(testrun, logfile, outcome_lines=[],
     # (2) Parse the logfile and match its segments to the map of testcases.
     i = None # XXX index into testcases
     j = 0 # XXX index into outcome_lines
-    native_configuration_is = None
+    testrun.arch = None
     year_month = None
     gdb_version = None
     running_test = None
@@ -253,8 +243,8 @@ def annotate_dejagnu_log(testrun, logfile, outcome_lines=[],
     for cur in Cursor(logfile, name=os.path.basename(logfile), input_file=f, fast_hack=True):
         line = cur.line
 
-        if line.startswith("Native configuration is"):
-            native_configuration_is = line
+        if testrun.arch is None and line.startswith("Native configuration is"):
+            testrun.arch = grok_architecture(line)
         if (line.startswith("Test Run By") and " on " in line) or (" completed at " in line):
             if line.startswith("Test Run By"):
                 t1 = line.rfind(" on ") + len(" on ")
@@ -339,12 +329,6 @@ def annotate_dejagnu_log(testrun, logfile, outcome_lines=[],
             last_test_cur = Cursor(start=cur); last_test_cur.line_start += 1
     f.close()
 
-    uname_machine = None
-    if uname_machine is None:
-        uname_machine = check_mapping(native_configuration_is,
-                                      native_configuration_map)
-
-    testrun.arch = uname_machine
     # XXX testrun.osver should be extracted from buildbot repo path
     testrun.version = gdb_version
     testrun.year_month = year_month
-- 
2.26.2


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] Introduce simplified grok_architecture function
  2020-09-23 19:15   ` [PATCH v2] " Keith Seitz
@ 2020-09-23 19:26     ` Serhei Makarov
  2020-09-23 19:46       ` Keith Seitz
  0 siblings, 1 reply; 6+ messages in thread
From: Serhei Makarov @ 2020-09-23 19:26 UTC (permalink / raw)
  To: Keith Seitz; +Cc: Bunsen

LGTM, just have one niggle.

On Wed, Sep 23, 2020, at 3:15 PM, Keith Seitz wrote:
> +# Standardized architecture name mapping table.  The "Native 
> configuration
> +# is " line of a test run is matched against the keys in this table.  
> If
> +# a match is found, the architecture is given by the corresponding 
> value.
> +# Keys are regular expressions; values are functions which take the 
> regexp
> +# match as parameter and return a string representatin of the 
> architecture.
> +standard_architecture_map = {r'powerpc64-(\w+)-linux.*':lambda m: 
> 'ppc64',
> +                             r'powerpc64le-(\w+)-linux.*':lambda m: 
> 'ppc64le',
> +                             r'armv7l-(\w+)-linux-gnueabihf':lambda m: 
> 'armhf',
> +                             r'(\w+)-(\w+)-linux.*':lambda m: 
> m.group(1)}
LGTM, I like it.

<snip>
>  # TODO Handle other exotic DejaGNU outcome codes if they come up.
>  test_outcome_map = {'PASS':'PASS', 'XPASS':'XPASS', 'IPASS':'PASS',
> @@ -242,7 +232,7 @@ def annotate_dejagnu_log(testrun, logfile, 
> outcome_lines=[],
>      # (2) Parse the logfile and match its segments to the map of 
> testcases.
>      i = None # XXX index into testcases
>      j = 0 # XXX index into outcome_lines
> -    native_configuration_is = None
> +    testrun.arch = None
<snip>
> +        if testrun.arch is None and line.startswith("Native 
> configuration is"):
> +            testrun.arch = grok_architecture(line)
I would set uname_machine at this point and keep the 'testrun.arch = uname_machine' assignment in the block at the bottom. That way all the properties we plan to set are glanceable in one place. (Note the comment that says we plan to set testrun.osver but don't have the info yet.)

I inherited this code layout from Martin Cermak's logproc code
and I think keeping it intact helps maintainability.

<snip>
> -    uname_machine = None
> -    if uname_machine is None:
> -        uname_machine = check_mapping(native_configuration_is,
> -                                      native_configuration_map)
> -
> -    testrun.arch = uname_machine
>      # XXX testrun.osver should be extracted from buildbot repo path
>      testrun.version = gdb_version
>      testrun.year_month = year_month
> -- 

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] Introduce simplified grok_architecture function
  2020-09-23 19:26     ` Serhei Makarov
@ 2020-09-23 19:46       ` Keith Seitz
  2020-09-23 22:48         ` Serhei Makarov
  0 siblings, 1 reply; 6+ messages in thread
From: Keith Seitz @ 2020-09-23 19:46 UTC (permalink / raw)
  To: Serhei Makarov; +Cc: Bunsen

On 9/23/20 12:26 PM, Serhei Makarov wrote:
>> -    native_configuration_is = None
>> +    testrun.arch = None
> <snip>
>> +        if testrun.arch is None and line.startswith("Native 
>> configuration is"):
>> +            testrun.arch = grok_architecture(line)

> I would set uname_machine at this point and keep the 'testrun.arch =
> uname_machine' assignment in the block at the bottom.

I've reverted those hunks (more or less) to maintain that style 
and pushed the resulting patch.

Thanks!

Keith


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] Introduce simplified grok_architecture function
  2020-09-23 19:46       ` Keith Seitz
@ 2020-09-23 22:48         ` Serhei Makarov
  0 siblings, 0 replies; 6+ messages in thread
From: Serhei Makarov @ 2020-09-23 22:48 UTC (permalink / raw)
  To: Keith Seitz; +Cc: Bunsen

On Wed, Sep 23, 2020, at 3:46 PM, Keith Seitz wrote:
> I've reverted those hunks (more or less) to maintain that style 
> and pushed the resulting patch.
Thanks! That makes the third contributor to the project.
(Martin Cermak also contributed bugfixes.)

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2020-09-23 22:49 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-23 13:21 Fwd: Re: [PATCH] Introduce simplified grok_architecture function Serhei Makarov
2020-09-23 18:14 ` Keith Seitz
2020-09-23 19:15   ` [PATCH v2] " Keith Seitz
2020-09-23 19:26     ` Serhei Makarov
2020-09-23 19:46       ` Keith Seitz
2020-09-23 22:48         ` Serhei Makarov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).