GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-29854: Print readline version information when running test suite by berkerpeksag · Pull Request #2618 · python/cpython

Conversation

@berkerpeksag

@berkerpeksag berkerpeksag commented Jul 7, 2017

Copy link
Copy Markdown
Member

No description provided.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@berkerpeksag
Print readline version information when running test suite
73eede8
@berkerpeksag
berkerpeksag requested a review from vstinner July 7, 2017 10:48
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Jul 7, 2017
@berkerpeksag berkerpeksag added the trivial label Jul 7, 2017
nirs
nirs reviewed Jul 7, 2017
View reviewed changes

@nirs nirs left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@vstinner

vstinner commented Jul 7, 2017

Copy link
Copy Markdown
Member

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vstinner

vstinner commented Jul 7, 2017

Copy link
Copy Markdown
Member

I prefer to not make regrtest header too big.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@berkerpeksag

berkerpeksag commented Jul 7, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

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 :)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vstinner

vstinner commented Jul 7, 2017

Copy link
Copy Markdown
Member

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vstinner vstinner mentioned this pull request Jul 7, 2017
bpo-29854: test_readline logs versions in verbose mode #2619
Merged
@berkerpeksag
Use subprocess to avoid side-effects
dfbc978
@vstinner

vstinner commented Jul 7, 2017

Copy link
Copy Markdown
Member

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 ;-)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
vstinner
vstinner reviewed Jul 7, 2017
View reviewed changes
Comment thread Lib/test/libregrtest/main.py Outdated
import readline
except ImportError:
readline = None
if readline is not None:

@vstinner vstinner Jul 7, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/libregrtest/main.py Outdated
rl_version_cmd = """if 1:
try:
import readline
except ImportError:

@vstinner vstinner Jul 7, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@berkerpeksag berkerpeksag Jul 7, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/libregrtest/main.py Outdated
print("== encodings: locale=%s, FS=%s"
% (locale.getpreferredencoding(False),
sys.getfilesystemencoding()))
rl_version_cmd = """if 1:

@vstinner vstinner Jul 7, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@berkerpeksag berkerpeksag Jul 7, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean

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

?

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
vstinner
vstinner reviewed Jul 7, 2017
View reviewed changes
Comment thread Lib/test/libregrtest/main.py Outdated
pass
else:
print(f"== readline version: {rl_version}")
print(f"== readline implementation: {rl_implementation}")

@vstinner vstinner Jul 7, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you please write both info on a single line?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@berkerpeksag berkerpeksag Jul 7, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

== readline: 0x0602 (GNU readline)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@vstinner vstinner Jul 7, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@berkerpeksag berkerpeksag Jul 7, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done:

== readline: version 0x0602 (GNU readline)

Thanks!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
nirs
nirs reviewed Jul 7, 2017
View reviewed changes
Comment thread Lib/test/libregrtest/main.py Outdated
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")
"""

@nirs nirs Jul 7, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@berkerpeksag berkerpeksag Jul 7, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
"""

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@berkerpeksag berkerpeksag Jul 7, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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})")
"""

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@nirs nirs Jul 8, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@vstinner 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 vstinner removed the trivial label Jul 7, 2017
@berkerpeksag
Address review comments
4f750a5
@berkerpeksag

berkerpeksag commented Jul 7, 2017

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
nirs
nirs suggested changes Jul 7, 2017
View reviewed changes
Comment thread Lib/test/libregrtest/main.py
import readline
except ImportError:
sys.exit(1)
else:

@nirs nirs Jul 7, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread 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("""

@nirs nirs Jul 7, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The extra indent make this much more clear, thanks!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread 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__

@nirs nirs Jul 7, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread 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()

@nirs nirs Jul 7, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@berkerpeksag

berkerpeksag commented Aug 11, 2017

Copy link
Copy Markdown
Member Author

Superseded by bpo-30871 and PR #3027.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@berkerpeksag berkerpeksag closed this Aug 11, 2017
@asottile

asottile commented Aug 15, 2017

Copy link
Copy Markdown
Contributor

@berkerpeksag do you mean #3075?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@berkerpeksag

berkerpeksag commented Aug 15, 2017

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@berkerpeksag
berkerpeksag deleted the improve-regrtest-header branch August 15, 2017 03:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinner vstinner vstinner left review comments
+1 more reviewer
@nirs nirs nirs requested changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@berkerpeksag @vstinner @asottile @nirs @the-knights-who-say-ni
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.