Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Find python binary dynamically #671

Merged
merged 2 commits into from
Jun 15, 2020

Conversation

airtower-luna
Copy link
Contributor

@airtower-luna airtower-luna commented Jun 14, 2020

Some systems have a suitable Python interpreter, but not under the expected python binary name. E.g. on Debian(-ish) systems the Python 3 interpreter is called python3. This PR aims to support both.

Description

  • The Makefile now searches for both python3 and python (using which), and uses the first one found.
  • tests/scripts_retention.py uses sys.executable instead of hardcoded 'python'. That should work completely independent of the platform.

Motivation and Context

Fixes #670

Checklist

  • I have read the CONTRIBUTING.md document and my PR follows change requirements therein
  • the changes are also reflected in documentation and code comments
  • all new and existing tests pass (see Travis CI results)
  • test script checklist was followed for new scripts
  • new test script added to tlslite-ng.json and tlslite-ng-random-subset.json
  • new and modified scripts were ran against popular TLS implementations:
    • OpenSSL
    • NSS
    • GnuTLS
  • required version of tlslite-ng updated in requirements.txt and README.md

This change is Reviewable

@airtower-luna
Copy link
Contributor Author

In addition to the problem described in #670 the "server_command" definitions as read from the JSON files also hardcode python as the interpreter. The current patch just replaces that with sys.executable in run_with_json(), would you prefer changing it to something that'd be more obviously a token, like the existing {command}? Would be just a search and replace run to use e.g. {python}. 🙂

Copy link
Member

@tomato42 tomato42 left a comment

Choose a reason for hiding this comment

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

yes, probably search and replace on the json files should be enough

Reviewed 1 of 1 files at r1, 1 of 1 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @airtower-luna)


tests/scripts_retention.py, line 200 at r2 (raw file):

        command = srv_conf["server_command"]
        if command[0] == 'python':
            command[0] = sys.executable

I think using {python} to indicate that it will be replaced is a cleaner solution—principle of least surprise and all that

This works regardless of the name of the installed interpreter, and
has the additional advantage of always using the same interpreter as
used to call the script.

The JSON test configurations now use a "{python}" token instead of
"python" to avoid confusion about it being replaced with
sys.executable at runtime.
@airtower-luna
Copy link
Contributor Author

@tomato42: I've changed the server setup to use a {python} token, using a loop like the {command} token to avoid surprises in case there will ever be command components before the Python interpreter.

Copy link
Member

@tomato42 tomato42 left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 3 files at r3.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@tomato42 tomato42 merged commit 6a36513 into tlsfuzzer:master Jun 15, 2020
@tomato42
Copy link
Member

looks good, thank you!

gnutlsmirror pushed a commit to gnutls/gnutls that referenced this pull request Jun 17, 2020
Tlsfuzzer also assumed the Python interpreter would be called
"python", this update is necessary to get a fixed version (see
tlsfuzzer/tlsfuzzer#671).

Signed-off-by: Fiona Klute <fiona.klute@gmx.de>
jollaitbot pushed a commit to sailfishos-mirror/gnutls that referenced this pull request Aug 31, 2020
Tlsfuzzer also assumed the Python interpreter would be called
"python", this update is necessary to get a fixed version (see
tlsfuzzer/tlsfuzzer#671).

Signed-off-by: Fiona Klute <fiona.klute@gmx.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tests fail if the Python interpreter binary isn't called "python"
2 participants