bpo-29854: Print readline version information when running test suite #2618

berkerpeksag · 2017-07-07T10:48:05Z

No description provided.

nirs

👍

vstinner · 2017-07-07T12:02:51Z

I would prefer to dump the curses version in test_readline, as done by test_tcl or test_gdb in verbose mode. When a test fails on buildbots, the test is run again in verbose mode.

vstinner · 2017-07-07T12:07:15Z

I prefer to not make regrtest header too big.

berkerpeksag · 2017-07-07T12:24:02Z

Why would we want to print curses version in test_readline? Unless I'm missing something, curses doesn't have anything to do with bpo-29854.

tcl and gdb are much less common dependencies than readline. readline is probably available on all Unix platforms and I think it makes sense to easily see which version is installed on the host.

I prefer to not make regrtest header too big.

Sorry, I don't have time for this kind of bikeshedding. We don't design a production level UI for end users and it's already hard to read outputs of regrrest and Buildbot. I prefer not to spend too much time to find which version of readline installed at http://buildbot.python.org/all/builders/x86%20Tiger%203.x/builds/908/steps/test/logs/stdio Plus, that header has already prints less useful information so I don't think two additional lines would set the world on fire :)

vstinner · 2017-07-07T12:56:19Z

Why would we want to print curses version in test_readline? Unless I'm missing something, curses doesn't have anything to do with bpo-29854.

Oh sorry, I'm always confused between readline and curses, they are the same thing for me, sorry :-)

By the way, you should mention bpo-29854 in your PR title. It helps later to get the rationale of a change ;-)

tcl and gdb are much less common dependencies than readline. readline is probably available on all Unix platforms and I think it makes sense to easily see which version is installed on the host.

Another issue with importing readline early is that importing a module has side effect, especially readline. See for example http://bugs.python.org/issue19884 and TestReadline.test_init(): this test still fails on our RHEL buildbot, so importing readline still have an annoying side effect of writing a string into stdout.

I would prefer to limit imports at regrtest startup, to make it more deterministic.

I prefer not to spend too much time to find which version of readline installed at http://buildbot.python.org/all/builders/x86%20Tiger%203.x/builds/908/steps/test/logs/stdio

Would you mind to review my PR #2619? When it will be merged, I can retrieve the libreadline version for you and put it in the issue.

vstinner · 2017-07-07T14:03:00Z

We discuss this PR on IRC with @berkerpeksag. If imports are done are done in subprocess to prevent side effects, I'm ok to get such change merged ;-)

vstinner · 2017-07-07T14:03:58Z

Lib/test/libregrtest/main.py

+            import readline
+        except ImportError:
+            readline = None
+        if readline is not None:


this line just can be an "else:" no?

vstinner · 2017-07-07T14:04:15Z

Lib/test/libregrtest/main.py

+        rl_version_cmd = """if 1:
+        try:
+            import readline
+        except ImportError:


Maybe use sys.exit(1) to not try to decode empty stdout in the parent process.

Good point!

vstinner · 2017-07-07T14:05:16Z

Lib/test/libregrtest/main.py

@@ -432,6 +433,26 @@ def display_header(self):
        print("== encodings: locale=%s, FS=%s"
              % (locale.getpreferredencoding(False),
                 sys.getfilesystemencoding()))
+        rl_version_cmd = """if 1:


You may put the textwrap.dedent() directly here. But do you need "if 1:" in that case?

Do you mean

rl_version_cmd = textwrap.dedent(""" ... """)

?

And, you're right, we don't need if 1:, thanks!

vstinner · 2017-07-07T14:06:00Z

Lib/test/libregrtest/main.py

+                pass
+            else:
+                print(f"== readline version: {rl_version}")
+                print(f"== readline implementation: {rl_implementation}")


can you please write both info on a single line?

Sure, what do you thing about the following format?:

== readline: 0x0602 (GNU readline)

I like it. Maybe just add version somewhere? "== readline: version 0x0602 (GNU readline)"

Done:

== readline: version 0x0602 (GNU readline)

Thanks!

nirs · 2017-07-07T14:08:14Z

Lib/test/libregrtest/main.py

            is_editline = readline.__doc__ and "libedit" in readline.__doc__
-            print("== readline implementation:", "editline" if is_editline else "GNU readline")
+            print("editline" if is_editline else "GNU readline")
+        """


It is very hard to read this code like this. The other program that you run should be indented differently from the other code.

Also the code to return the info should be in separate function, and here we can just print the values.

So what you're suggesting is similar to the following snippet, right?

rl_version_cmd = """ def get_rl_info(): # all code is here return version, implementation version, implementation = get_rl_info() print(version) print(implementation) """

Or:

rl_version_cmd = """ def get_rl_info(): # all code is here return version, implementation version, implementation = get_rl_info() print("== readline: version {version} ({implementation})") """

More like:

def readline_info(): script = """ import json import readline is_editline = readline.__doc__ and "libedit" in readline.__doc__ info = {"version": readline._READLINE_VERSION, "implementation": "editline" if is_editline else "GNU readline"} print(json.dumps(info)) """ c = subprocess.run([sys.executable, "-c", script], stdout=subprocess.PIPE, stderr=subprocess.PIPE) if c.returncode == 0: return json.loads(c.stdout.decode())

And use readline_info() in display_header(), or in any other context as needed.

berkerpeksag · 2017-07-07T14:59:24Z

I've addressed all review comments (and hopefully I got @nirs' comment right) Thanks!

nirs · 2017-07-07T16:33:50Z

Lib/test/libregrtest/main.py

-                pass
+                import readline
+            except ImportError:
+                sys.exit(1)
            else:


We can remove the try/else/exit, just import readline:

import readline

If import fails, we exit with exit code 1 anyway, no special code needed.

nirs · 2017-07-07T16:34:13Z

Lib/test/libregrtest/main.py

-        rl_output = subprocess.run([sys.executable, "-c", textwrap.dedent(rl_version_cmd)],
-                                   stdout=subprocess.PIPE, stderr=subprocess.PIPE)
-        if rl_output.returncode == 0:
+        rl_version_cmd = textwrap.dedent("""


The extra indent make this much more clear, thanks!

nirs · 2017-07-07T16:37:07Z

Lib/test/libregrtest/main.py

-                print(f"== readline version: {rl_version}")
-                print(f"== readline implementation: {rl_implementation}")
+                def get_rl_info():
+                    is_editline = readline.__doc__ and "libedit" in readline.__doc__


This is now the third place using this horrible doc check. I think we should eliminate this, now, or at least leave a todo and clean up later. Ideally we should have exactly one is_editline() function in cpython.

nirs · 2017-07-07T16:37:58Z

Lib/test/libregrtest/main.py

+                def get_rl_info():
+                    is_editline = readline.__doc__ and "libedit" in readline.__doc__
+                    return f"{readline._READLINE_VERSION:#06x}", "editline" if is_editline else "GNU readline"
+                rl_version, rl_implementation = get_rl_info()


I think we should print the same values ptinted now in test_readline.setupModule

berkerpeksag · 2017-08-11T22:29:51Z

Superseded by bpo-30871 and PR #3027.

asottile · 2017-08-15T02:12:26Z

@berkerpeksag do you mean #3075?

berkerpeksag · 2017-08-15T03:57:21Z

@asottile yes that's what I meant! :) Good catch, thanks.

Print readline version information when running test suite

73eede8

berkerpeksag requested a review from vstinner July 7, 2017 10:48

the-knights-who-say-ni added the CLA signed label Jul 7, 2017

berkerpeksag added the trivial label Jul 7, 2017

nirs reviewed Jul 7, 2017

View reviewed changes

vstinner mentioned this pull request Jul 7, 2017

bpo-29854: test_readline logs versions in verbose mode #2619

Merged

Use subprocess to avoid side-effects

dfbc978

vstinner reviewed Jul 7, 2017

View reviewed changes

nirs reviewed Jul 7, 2017

View reviewed changes

vstinner changed the title ~~Print readline version information when running test suite~~ bpo-29854: Print readline version information when running test suite Jul 7, 2017

vstinner removed the trivial label Jul 7, 2017

Address review comments

4f750a5

nirs suggested changes Jul 7, 2017

View reviewed changes

berkerpeksag closed this Aug 11, 2017

berkerpeksag deleted the improve-regrtest-header branch August 15, 2017 03:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-29854: Print readline version information when running test suite #2618

bpo-29854: Print readline version information when running test suite #2618

berkerpeksag commented Jul 7, 2017

nirs left a comment

vstinner commented Jul 7, 2017

vstinner commented Jul 7, 2017

berkerpeksag commented Jul 7, 2017 •

edited

vstinner commented Jul 7, 2017

vstinner commented Jul 7, 2017

vstinner Jul 7, 2017

vstinner Jul 7, 2017

berkerpeksag Jul 7, 2017

vstinner Jul 7, 2017

berkerpeksag Jul 7, 2017

vstinner Jul 7, 2017

berkerpeksag Jul 7, 2017

vstinner Jul 7, 2017

berkerpeksag Jul 7, 2017

nirs Jul 7, 2017

berkerpeksag Jul 7, 2017

berkerpeksag Jul 7, 2017

nirs Jul 8, 2017 •

edited

berkerpeksag commented Jul 7, 2017

nirs Jul 7, 2017

nirs Jul 7, 2017

nirs Jul 7, 2017

nirs Jul 7, 2017

berkerpeksag commented Aug 11, 2017

asottile commented Aug 15, 2017

berkerpeksag commented Aug 15, 2017

bpo-29854: Print readline version information when running test suite #2618

bpo-29854: Print readline version information when running test suite #2618

Conversation

berkerpeksag commented Jul 7, 2017

nirs left a comment

Choose a reason for hiding this comment

vstinner commented Jul 7, 2017

vstinner commented Jul 7, 2017

berkerpeksag commented Jul 7, 2017 • edited

vstinner commented Jul 7, 2017

vstinner commented Jul 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nirs Jul 8, 2017 • edited

Choose a reason for hiding this comment

berkerpeksag commented Jul 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

berkerpeksag commented Aug 11, 2017

asottile commented Aug 15, 2017

berkerpeksag commented Aug 15, 2017

berkerpeksag commented Jul 7, 2017 •

edited

nirs Jul 8, 2017 •

edited